[barney-s]

Fixes #12500

Root Cause Analysis

When a NetworkConnectivityInternalRange resource was created with spec.allocationOptions or spec.targetCIDRRange, subsequent reconcile loops immediately failed with:
googleapi: Error 400: Unsupported update mask path: "allocation_options": invalid argument

The issue was caused by:

1. convertAPIToProto Deserialization Failure:

   • convertAPIToProto in pkg/controller/direct/networkconnectivity/utils.go used standard json.Unmarshal(j, &v) instead of protojson.Unmarshal.
   • The Go client library api.InternalRange serializes JSON with camelCase tags (allocationOptions, prefixLength, targetCidrRange), whereas protobuf-generated structs have snake_case tags (allocation_options, prefix_length, target_cidr_range).
   • Consequently, json.Unmarshal failed to deserialize these fields into a.actual, leaving a.actual.AllocationOptions, a.actual.TargetCidrRange, and a.actual.PrefixLength unpopulated (nil/0).

2. Spurious Update Mask Diff on Create-Only / Immutable Fields:

   • In Update() (networkconnectivityinternalrange_controller.go), the controller diffed create-only/immutable fields (allocationOptions, targetCidrRange, ipCidrRange, peering, usage) against a.actual.
   • Because a.actual lacked these fields, !reflect.DeepEqual(...) evaluated to true on every single reconcile loop, appending "allocationOptions" and "targetCidrRange" to the update mask and issuing a PATCH request.
   • GCP's Patch API does not support allocation_options or target_cidr_range in the update mask, rejecting the request with Error 400.

3. Missing Re-Reconciliation Check in E2E Tests:

   • NetworkConnectivityInternalRange was listed on the exemption list in tests/e2e/ratcheting.go, bypassing the steady-state re-reconciliation check during E2E tests.

Changes Made

• Fixed convertAPIToProto (pkg/controller/direct/networkconnectivity/utils.go) to use protojson.UnmarshalOptions{DiscardUnknown: true} for proper proto unmarshalling.
• Scoped Update() (pkg/controller/direct/networkconnectivity/networkconnectivityinternalrange_controller.go) to only diff mutable fields (description, labels, prefixLength).
• Enabled Re-Reconciliation Testing (tests/e2e/ratcheting.go) by removing NetworkConnectivityInternalRange from the exemption list.
• Added Test Fixture (pkg/test/resourcefixture/testdata/basic/networkconnectivity/v1alpha1/networkconnectivityinternalrange/networkconnectivityinternalrange-firstsmallestfitting) reproducing the customer configuration (allocationStrategy: FIRST_SMALLEST_FITTING and targetCIDRRange).
• Updated Golden Files for existing internal range fixtures.

Fix continuous reconciliation loop and 400 error on NetworkConnectivityInternalRange when auto-allocation fields are set.

[barney-s]

Field Mutability Analysis for InternalRange

For reference and clarity on the controller's Update() diff logic, here is the breakdown of all fields in NetworkConnectivityInternalRangeSpec and the GCP InternalRange resource:

KRM Spec Field	GCP Proto Field	Status in GCP	Supported in updateMask?	Notes / API Semantics
description	description	Mutable	✅ Yes (description)	Can be updated at any time.
labels	labels	Mutable	✅ Yes (labels)	Standard GCP resource labels.
prefixLength	prefix_length	Mutable	✅ Yes (prefixLength)	Used to resize IPv4 reservations ("Can also be used during updates to change the range size").
allocationOptions	allocation_options	Mutable	✅ Yes (allocationOptions)	Mutable for RANDOM_FIRST_N_AVAILABLE strategy (e.g. updating firstAvailableRangesLookupSize).
overlaps	overlaps	Mutable	✅ Yes (overlaps)	Allows updating overlap permissions (ROUTE_OVERLAP, OVERLAP_EXISTING_SUBNET_RANGE).
networkRef	network	Immutable	❌ No	GCP docs explicitly specify: "Immutable. The URL or resource ID of the network in which to reserve the internal range."
ipCIDRRange	ip_cidr_range	Immutable	❌ No	The allocated IP range cannot be changed in-place. Modifying IP address requires recreating the range (resizing is done via prefixLength).
targetCIDRRange	target_cidr_range	Immutable / Create-Only	❌ No	Search space used only during initial auto-allocation. Not part of the live patch mask.
usage	usage	Immutable	❌ No	Core purpose (FOR_VPC, EXTERNAL_TO_VPC, FOR_MIGRATION). Cannot be altered after creation.
peering	peering	Immutable	❌ No	Peering mode (FOR_SELF, FOR_PEER, NOT_SHARED). Cannot be altered after creation.
migration	migration	Immutable	❌ No	Only used during create when usage: FOR_MIGRATION.
projectRef / location	Parent path	Immutable	❌ No	Resource identity hierarchy.

The direct controller's Update() diff check covers all 5 mutable fields (description, labels, prefixLength, allocationOptions, overlaps) and excludes the immutable / create-only fields from the update mask.

[maqiuyujoyce]

I don't think this is the correct logic. We should not hide the users' invalid changes by dropping them. We should pass whatever the users want to change to the API so that the API decides whether it is acceptable or not.

And in a no-change re-reconciliation, there shouldn't be any diffs found.

[maqiuyujoyce]

Created #12620

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ldanielmadariaga

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ldanielmadariaga]

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