fix(memorystore): prevent infinite reconciliation drift loop by aligning connections list length

[ldanielmadariaga]

Why is this change necessary?

During E2E testing of network connectivity topologies, we observed that MemorystoreInstance controllers can fall into an infinite reconciliation loop due to false drift detection on the endpoints[i].connections list.

Specifically:

• In the KRM spec (desired), a user defines the target VPC network(s) under spec.endpoints[i].connections (typically 1 connection item representing the intended PSC network target).
• Under the hood, GCP automatically allocates and returns multiple server-generated connection entries per target network (e.g., one connection per shard or node attachment).
• Because the GCP API returns an actual connections list that is longer than the user's desired list, tags.DiffForTopLevelFields flags the length mismatch as a drift. KCC repeatedly sends update requests attempting to shrink the connections list, which GCP ignores, resulting in an infinite reconciliation loop.

What does this change do?

1. Normalizes Connections list length before diffing (pkg/controller/direct/memorystore/memorystoreinstance_controller.go):
   Truncates the server-generated extra Connections in maskedActual to match len(desiredEndpoint.Connections) before comparison (actualEndpoint.Connections = actualEndpoint.Connections[:len(desiredEndpoint.Connections)]).
   • Why truncating is correct and does not cause data loss: Truncation only applies to the temporary maskedActual copy used for drift comparison against the user's spec. It does not discard any user-configured target connections, nor does it affect the full observed state reported in status.endpoints. It ensures we compare exactly the user-declared intent (desired) against the corresponding actual connection items without failing on extra server-created replica entries.
2. Prevents partial status overwrites (pkg/controller/direct/directbase/operations.go):
   Updates directbase/operations.go to prevent partial status updates from overwriting existing status fields (such as conditions or externalRef) with nil.
3. Sorts HTTP logs in E2E tests:
   Aligns/sorts HTTP log entries for the MemorystoreInstance test fixtures to prevent test flakiness.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ldanielmadariaga. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[gemmahou]

Not a blocker of this PR.

This seems another use case for the issue I've experienced: The API converts the desired state into a value that we cannot easily simulate on our end, unlike predictable conversions we already handle(e.g., mapping project ID to number or altering string/enum casing, ordering, etc).

I am leaning toward a more generic solution that deserves a design discussion: storing a hash(last applied desired state, last applied actual state) in new field "ObservedState.LastApplied". We can then compare the current hash(desired state, actual state) against this stored hash to detect diffs. We should document the specific fields requiring this logic so we don't hash all fields unnecessarily.

cc @maqiuyujoyce

[ldanielmadariaga]

Yeah this makes sense, a nice thing in this case is that we do store the received values in status so we shouldn't lose this data

[maqiuyujoyce]

I think this is a different use case? The reason we want observedState to store the actual value for comparison is that the service-returned value and the user-provided value are different. And for this use case, the service still return the same values.

[maqiuyujoyce]

Nit: Looping through connections made me think different connections will generate different keys. Might be more readable if the loop is hidden by a function name.

[maqiuyujoyce]

I think all connections in a single endpoint group belong to the same VPC network, and they all have the same network key. So this loop ends up writing the exact same key over and over again. actualSet will contain exactly one unique key (e.g., {"my-project/my-vpc": true}). Right?

[maqiuyujoyce]

And we already matched the parent desiredEndpoint and actualEndpoint using this network key k in the caller function (alignEndpointsWithDesired), we already know that "my-project/my-vpc" is the key for both. Thus, this lookup will always succeed. Right?

I guess my question is how we identify the desired connections is a subset of the actual connections here.

[maqiuyujoyce]

I think the isConnectionSubset check is not necessary.

[ldanielmadariaga]

This will check that all connections in desired are present in actual connections. If a desired connection is not present it can't be a subset.

The top level matching where we use the connection as keys is mostly an artifact of endpoints only having the connections field and thus not having a better field to key by and although it may end up guaranteeing some correctness it's best not to depend on the logic.

The order here should be:

1. Match endpoints what alignEndpointsWithDesired is trying to do (in a poor way due to keying limitation)
2. Check if desired endpoints connections are a subset of actual

[maqiuyujoyce]

Thanks for the explanation!

One minor observation on the updated connectionsMatch function:

Since the KRM schema for PscAutoConnection only defines projectRef and networkRef in the Spec, output-only fields like PscConnectionId, IpAddress, ForwardingRule, ServiceAttachment, and Port will never be populated on the desired connection object.

As a result, the checks for those output-only fields in connectionsMatch will always be skipped (evaluating to true). In practice, the matching is based solely on ProjectId and Network.

That's why I suggested we remove the isConnectionSubset function previously.

To make the code cleaner and prevent future confusion about what is actually being compared, I suggest we simplify connectionsMatch to only check the fields that can actually be set in the KRM spec (i.e., ProjectId and Network / projectRef and networkRef). What do you think?