[ipam/azure] Avoid redundant CiliumNode status writes

pkg/azure/ipam/node.go

@@ -44,17 +44,21 @@ func (n *Node) UpdatedNode(obj *v2.CiliumNode) {
 // PopulateStatusFields fills in the status field of the CiliumNode custom
 // resource with Azure specific information
 func (n *Node) PopulateStatusFields(k8sObj *v2.CiliumNode) {
-	k8sObj.Status.Azure.Interfaces = []types.AzureInterface{}
+	interfaces := []types.AzureInterface{}
 
 	n.manager.mutex.RLock()
 	defer n.manager.mutex.RUnlock()
 	n.manager.instances.ForeachInterface(n.node.InstanceID(), func(instanceID, interfaceID string, interfaceObj ipamTypes.InterfaceRevision) error {
 		iface, ok := interfaceObj.Resource.(*types.AzureInterface)
 		if ok {
-			k8sObj.Status.Azure.Interfaces = append(k8sObj.Status.Azure.Interfaces, *(iface.DeepCopy()))
+			interfaces = append(interfaces, *(iface.DeepCopy()))
 		}
 		return nil
 	})
+
+	// ForeachInterface iterates a Go map, so order is randomized per call;
+	// SetInterfaces sorts by ID for a stable slice.
+	k8sObj.Status.Azure.SetInterfaces(interfaces)
 }
 
 // PrepareIPRelease prepares the release of IPs

pkg/azure/ipam/node_test.go

@@ -6,12 +6,133 @@ package ipam
 import (
 	"testing"
 
+	"github.com/cilium/hive/hivetest"
 	"github.com/stretchr/testify/require"
 
+	apimock "github.com/cilium/cilium/pkg/azure/api/mock"
 	"github.com/cilium/cilium/pkg/azure/types"
+	ipamTypes "github.com/cilium/cilium/pkg/ipam/types"
+	v2 "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2"
 )
 
 func TestGetMaximumAllocatableIPv4(t *testing.T) {
 	n := &Node{}
 	require.Equal(t, types.InterfaceAddressLimit, n.GetMaximumAllocatableIPv4())
 }
+
+// TestPopulateStatusFieldsDeterministicOrder ensures that repeated calls to
+// PopulateStatusFields produce a byte-for-byte identical (DeepEqual)
+// Status.Azure.Interfaces slice, sorted by interface ID, regardless of the
+// randomized map-iteration order of ForeachInterface. Without this, the
+// operator's DeepEqual-based write-skip gate cannot suppress no-op
+// CiliumNode /status writes, reintroducing the per-tick spurious write bug
+// this test guards against.
+func TestPopulateStatusFieldsDeterministicOrder(t *testing.T) {
+	api := apimock.NewAPI(nil, nil)
+	require.NotNil(t, api)
+
+	mngr := NewInstancesManager(hivetest.Logger(t), api)
+	require.NotNil(t, mngr)
+
+	instances := ipamTypes.NewInstanceMap()
+	for _, id := range []string{"intf-c", "intf-a", "intf-b"} {
+		iface := &types.AzureInterface{
+			SecurityGroup: "sg-" + id,
+		}
+		iface.SetID(id)
+		instances.Update("i-1", ipamTypes.InterfaceRevision{
+			Resource: iface.DeepCopy(),
+		})
+	}
+	api.UpdateInstances(instances)
+	require.False(t, mngr.Resync(t.Context()).IsZero())
+
+	node := &Node{
+		node:    fakeNodeActions{instanceID: "i-1"},
+		manager: mngr,
+	}
+
+	var results [][]types.AzureInterface
+	for i := 0; i < 10; i++ {
+		k8sObj := &v2.CiliumNode{}
+		node.PopulateStatusFields(k8sObj)
+		results = append(results, k8sObj.Status.Azure.Interfaces)
+	}
+
+	for i, ifaces := range results {
+		require.Len(t, ifaces, 3, "iteration %d", i)
+		require.True(t, ifaces[0].ID < ifaces[1].ID && ifaces[1].ID < ifaces[2].ID,
+			"iteration %d: interfaces not sorted by ID: %v", i, []string{ifaces[0].ID, ifaces[1].ID, ifaces[2].ID})
+		if i > 0 {
+			require.Equal(t, results[0], ifaces, "iteration %d produced a different order than iteration 0", i)
+		}
+	}
+}
+
+// TestPopulateStatusFieldsDeepEqualAcrossShuffledOrders ties the ordering
+// fix directly to the actual write-skip gate it exists to fix: the
+// operator's origNode.Status.DeepEqual(&node.Status) call in
+// operator/cmd/cilium_node.go. TestPopulateStatusFieldsDeterministicOrder
+// only checks slice order via reflect-based require.Equal on hand-built
+// interfaces with empty vmssName/vmID/resourceGroup; this test instead uses
+// real VMSS-style resource IDs (so those unexported fields are actually
+// populated by SetID/extractIDs, not empty) and asserts NodeStatus.DeepEqual
+// -- the real consumer -- reports no difference across repeated,
+// independently-randomized calls to PopulateStatusFields.
+func TestPopulateStatusFieldsDeepEqualAcrossShuffledOrders(t *testing.T) {
+	api := apimock.NewAPI(nil, nil)
+	require.NotNil(t, api)
+
+	mngr := NewInstancesManager(hivetest.Logger(t), api)
+	require.NotNil(t, mngr)
+
+	instances := ipamTypes.NewInstanceMap()
+	resourceIDs := []string{
+		"/subscriptions/xxx/resourceGroups/rg1/providers/Microsoft.Compute/virtualMachineScaleSets/vmss1/virtualMachines/0/networkInterfaces/intf-a",
+		"/subscriptions/xxx/resourceGroups/rg1/providers/Microsoft.Compute/virtualMachineScaleSets/vmss1/virtualMachines/1/networkInterfaces/intf-b",
+		"/subscriptions/xxx/resourceGroups/rg1/providers/Microsoft.Compute/virtualMachineScaleSets/vmss1/virtualMachines/2/networkInterfaces/intf-c",
+	}
+	for _, id := range resourceIDs {
+		iface := &types.AzureInterface{CIDR: "10.0.0.0/24"}
+		iface.SetID(id)
+		instances.Update("i-1", ipamTypes.InterfaceRevision{
+			Resource: iface.DeepCopy(),
+		})
+	}
+	api.UpdateInstances(instances)
+	require.False(t, mngr.Resync(t.Context()).IsZero())
+
+	node := &Node{
+		node:    fakeNodeActions{instanceID: "i-1"},
+		manager: mngr,
+	}
+
+	var reference *v2.CiliumNode
+	for i := 0; i < 20; i++ {
+		k8sObj := &v2.CiliumNode{}
+		node.PopulateStatusFields(k8sObj)
+		require.Len(t, k8sObj.Status.Azure.Interfaces, len(resourceIDs), "iteration %d", i)
+		for _, iface := range k8sObj.Status.Azure.Interfaces {
+			// Confirm the unexported, json:"-" fields are actually
+			// populated (non-empty) for this test, unlike
+			// TestPopulateStatusFieldsDeterministicOrder's bare interfaces.
+			require.NotEmpty(t, iface.GetVMScaleSetName(), "iteration %d", i)
+			require.NotEmpty(t, iface.GetVMID(), "iteration %d", i)
+			require.NotEmpty(t, iface.GetResourceGroup(), "iteration %d", i)
+		}
+		if reference == nil {
+			reference = k8sObj
+			continue
+		}
+		require.True(t, reference.Status.DeepEqual(&k8sObj.Status),
+			"iteration %d: NodeStatus.DeepEqual reported a difference despite identical underlying Azure interfaces", i)
+	}
+}
+
+type fakeNodeActions struct {
+	instanceID string
+}
+
+func (f fakeNodeActions) InstanceID() string {
+	return f.instanceID
+}

pkg/azure/types/types.go

@@ -4,6 +4,7 @@
 package types
 
 import (
+	"slices"
 	"strings"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
@@ -59,6 +60,23 @@ type AzureStatus struct {
 	Interfaces []AzureInterface `json:"interfaces,omitempty"`
 }
 
+// SetInterfaces replaces Interfaces with a copy of ifaces sorted
+// deterministically by ID.
+//
+// Interfaces must be kept sorted by ID: the generated AzureStatus.DeepEqual
+// compares the slice index-by-index, and the operator's status-update path
+// relies on that DeepEqual to skip no-op CiliumNode /status writes. Any code
+// that populates Interfaces from a non-deterministically ordered source (e.g.
+// Go map iteration) MUST use this method rather than assigning the field
+// directly, to preserve that invariant.
+func (s *AzureStatus) SetInterfaces(ifaces []AzureInterface) {
+	sorted := slices.Clone(ifaces)
+	slices.SortFunc(sorted, func(a, b AzureInterface) int {
+		return strings.Compare(a.ID, b.ID)
+	})
+	s.Interfaces = sorted
+}
+
 // AzureAddress is an IP address assigned to an AzureInterface
 type AzureAddress struct {
 	// IP is the ip address of the address
@@ -121,14 +139,19 @@ type AzureInterface struct {
 	// +optional
 	CIDR string `json:"cidr,omitempty"`
 
-	// vmssName is the name of the virtual machine scale set. This field is
-	// set by extractIDs()
+	// vmssName is set by extractIDs() and is never serialized (json:"-"), so it
+	// is excluded from DeepEqual to avoid spurious diffs against apiserver copies.
+	// +deepequal-gen=false
 	vmssName string `json:"-"`
 
 	// vmID is the ID of the virtual machine
+	//
+	// +deepequal-gen=false
 	vmID string `json:"-"`
 
 	// resourceGroup is the resource group the interface belongs to
+	//
+	// +deepequal-gen=false
 	resourceGroup string `json:"-"`
 }
 

pkg/azure/types/types_test.go

@@ -51,6 +51,52 @@ func TestForeachAddresses(t *testing.T) {
 	require.Equal(t, 2, interfaces)
 }
 
+// TestAzureInterfaceDeepEqualIgnoresUnexportedFields locks in that
+// AzureInterface.DeepEqual ignores the unexported, non-serialized
+// (json:"-") vmssName/vmID/resourceGroup fields, while still comparing all
+// exported fields. This is the exact write-suppression behavior the
+// +deepequal-gen=false markers on those fields exist to provide: a
+// freshly-populated in-memory AzureInterface (with vmssName/vmID/
+// resourceGroup set by SetID) must compare equal to a copy that round-
+// tripped through the apiserver (where those fields are always empty,
+// since they are never serialized).
+func TestAzureInterfaceDeepEqualIgnoresUnexportedFields(t *testing.T) {
+	resourceID := "/subscriptions/xxx/resourceGroups/rg1/providers/Microsoft.Compute/virtualMachineScaleSets/vmss1/virtualMachines/0/networkInterfaces/vmss1"
+	base := &AzureInterface{Name: "eth0", MAC: "aa:bb:cc:dd:ee:ff", CIDR: "10.0.0.0/24"}
+	base.SetID(resourceID)
+	require.NotEmpty(t, base.GetResourceGroup())
+	require.NotEmpty(t, base.GetVMID())
+	require.NotEmpty(t, base.GetVMScaleSetName())
+
+	// apiserverRoundTripped simulates the same interface as fetched back
+	// from the apiserver: identical exported fields (including ID, which
+	// IS serialized), but the unexported vmssName/vmID/resourceGroup are
+	// zero-valued because those json:"-" fields are never serialized.
+	apiserverRoundTripped := &AzureInterface{ID: resourceID, Name: "eth0", MAC: "aa:bb:cc:dd:ee:ff", CIDR: "10.0.0.0/24"}
+
+	require.True(t, base.DeepEqual(apiserverRoundTripped),
+		"AzureInterface.DeepEqual must ignore vmssName/vmID/resourceGroup so identical exported fields compare equal")
+	require.True(t, apiserverRoundTripped.DeepEqual(base))
+
+	tests := []struct {
+		name   string
+		mutate func(*AzureInterface)
+	}{
+		{"ID differs", func(a *AzureInterface) { a.ID = "intf-2" }},
+		{"Name differs", func(a *AzureInterface) { a.Name = "eth1" }},
+		{"MAC differs", func(a *AzureInterface) { a.MAC = "ff:ee:dd:cc:bb:aa" }},
+		{"CIDR differs", func(a *AzureInterface) { a.CIDR = "10.0.1.0/24" }},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			other := apiserverRoundTripped.DeepCopy()
+			tt.mutate(other)
+			require.False(t, base.DeepEqual(other),
+				"AzureInterface.DeepEqual must still detect differences in exported field: %s", tt.name)
+		})
+	}
+}
+
 func TestExtractIDs(t *testing.T) {
 	tests := []struct {
 		name             string