Label network interface claims so a network service can select them

internal/controller/networkinterfaceclaim.go

@@ -207,3 +207,44 @@ func networkIPProjection(address string) string {
 	}
 	return prefix.Addr().String()
 }
+
+// desiredNetworkInterfaceClaimLabels returns the well-known labels compute
+// stamps on the claims it creates, so a networking NetworkService can select
+// claim membership by label without a consumer labelling anything first.
+//
+// The keys are the ones already stamped on every Instance, reused rather than
+// reinvented, and are sourced from the deployment that drove the instance. A key
+// whose source is empty is omitted rather than stamped blank, so a selector on
+// it matches nothing instead of matching every unset claim.
+func desiredNetworkInterfaceClaimLabels(
+	deployment *computev1alpha.WorkloadDeployment,
+	instance *computev1alpha.Instance,
+) map[string]string {
+	candidates := map[string]string{
+		computev1alpha.WorkloadNameLabel:  deployment.Spec.WorkloadRef.Name,
+		computev1alpha.PlacementNameLabel: deployment.Spec.PlacementName,
+		computev1alpha.CityCodeLabel:      deployment.Spec.CityCode,
+		computev1alpha.InstanceIndexLabel: instance.Labels[computev1alpha.InstanceIndexLabel],
+	}
+
+	labels := make(map[string]string, len(candidates))
+	for key, value := range candidates {
+		if value != "" {
+			labels[key] = value
+		}
+	}
+	return labels
+}
+
+// networkInterfaceClaimLabelsStale reports whether any label compute owns is
+// absent from or differs on a live claim. Only the keys compute sets are
+// considered: networking stamps its own keys on the claim, and a key compute
+// does not set is none of its business.
+func networkInterfaceClaimLabelsStale(current, desired map[string]string) bool {
+	for key, value := range desired {
+		if current[key] != value {
+			return true
+		}
+	}
+	return false
+}

internal/controller/networkinterfaceclaim_controller_test.go

@@ -4,6 +4,8 @@ package controller
 
 import (
 	"context"
+	"maps"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -23,6 +25,8 @@ const (
 	claimTestNamespace  = "ns-aabbccdd-0000-1111-2222-333344445555"
 	claimTestDeployment = "claim-test-wd"
 	claimTestNetwork    = "default"
+	claimTestWorkload   = "claim-test-workload"
+	claimTestPlacement  = "claim-test-placement"
 
 	// claimTestClass and the addresses below mirror what NSO publishes on a
 	// bound claim: a class-allocated external address, and an interface address
@@ -55,12 +59,19 @@ func newClaimTestDeployment() *computev1alpha.WorkloadDeployment {
 			UID:       "claim-test-wd-uid",
 		},
 		Spec: computev1alpha.WorkloadDeploymentSpec{
-			CityCode:    wdControllerTestCityCode,
-			WorkloadRef: computev1alpha.WorkloadReference{Name: "claim-test-workload"},
+			CityCode:      wdControllerTestCityCode,
+			PlacementName: claimTestPlacement,
+			WorkloadRef:   computev1alpha.WorkloadReference{Name: claimTestWorkload},
 		},
 	}
 }
 
+// instanceIndexFromName recovers the ordinal the instance-control strategy
+// encodes in an instance name, which is what it also stamps as the index label.
+func instanceIndexFromName(name string) string {
+	return name[strings.LastIndex(name, "-")+1:]
+}
+
 // newClaimTestInstance builds an instance with the given interfaces and the
 // scheduling gates the instance-control strategy stamps at creation.
 func newClaimTestInstance(name string, interfaces ...computev1alpha.InstanceNetworkInterface) *computev1alpha.Instance {
@@ -69,6 +80,9 @@ func newClaimTestInstance(name string, interfaces ...computev1alpha.InstanceNetw
 			Name:              name,
 			Namespace:         claimTestNamespace,
 			CreationTimestamp: metav1.Now(),
+			Labels: map[string]string{
+				computev1alpha.InstanceIndexLabel: instanceIndexFromName(name),
+			},
 			OwnerReferences: []metav1.OwnerReference{{
 				APIVersion: computev1alpha.GroupVersion.String(),
 				Kind:       kindWorkloadDeployment,
@@ -571,3 +585,185 @@ func TestNetworkGateHeldUntilPrepared(t *testing.T) {
 		})
 	}
 }
+
+// TestDesiredNetworkInterfaceClaimLabels covers where each well-known key is
+// sourced from, and that a key with no source is omitted rather than blank.
+func TestDesiredNetworkInterfaceClaimLabels(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name       string
+		deployment func(*computev1alpha.WorkloadDeployment)
+		instance   func(*computev1alpha.Instance)
+		want       map[string]string
+	}{
+		{
+			name: "every key sourced from the deployment and instance",
+			want: map[string]string{
+				computev1alpha.WorkloadNameLabel:  claimTestWorkload,
+				computev1alpha.PlacementNameLabel: claimTestPlacement,
+				computev1alpha.CityCodeLabel:      wdControllerTestCityCode,
+				computev1alpha.InstanceIndexLabel: "0",
+			},
+		},
+		{
+			name: "unset placement is omitted",
+			deployment: func(d *computev1alpha.WorkloadDeployment) {
+				d.Spec.PlacementName = ""
+			},
+			want: map[string]string{
+				computev1alpha.WorkloadNameLabel:  claimTestWorkload,
+				computev1alpha.CityCodeLabel:      wdControllerTestCityCode,
+				computev1alpha.InstanceIndexLabel: "0",
+			},
+		},
+		{
+			name: "instance without an index label is omitted",
+			instance: func(i *computev1alpha.Instance) {
+				delete(i.Labels, computev1alpha.InstanceIndexLabel)
+			},
+			want: map[string]string{
+				computev1alpha.WorkloadNameLabel:  claimTestWorkload,
+				computev1alpha.PlacementNameLabel: claimTestPlacement,
+				computev1alpha.CityCodeLabel:      wdControllerTestCityCode,
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			deployment := newClaimTestDeployment()
+			if tc.deployment != nil {
+				tc.deployment(deployment)
+			}
+			instance := newClaimTestInstance(claimTestDeployment + "-0")
+			if tc.instance != nil {
+				tc.instance(instance)
+			}
+
+			assert.Equal(t, tc.want, desiredNetworkInterfaceClaimLabels(deployment, instance))
+		})
+	}
+}
+
+// TestReconcileNetworkInterfaceClaims_Labels covers the two paths a claim
+// becomes selectable by: stamped at creation, and backfilled onto a claim that
+// predates the labels or drifted from them. The immutable spec must survive the
+// backfill untouched.
+func TestReconcileNetworkInterfaceClaims_Labels(t *testing.T) {
+	t.Parallel()
+
+	wantLabels := map[string]string{
+		computev1alpha.WorkloadNameLabel:  claimTestWorkload,
+		computev1alpha.PlacementNameLabel: claimTestPlacement,
+		computev1alpha.CityCodeLabel:      wdControllerTestCityCode,
+		computev1alpha.InstanceIndexLabel: "0",
+	}
+
+	// A spec no reconcile pass would derive, so any write to it is visible.
+	existingSpec := networkingv1alpha.NetworkInterfaceClaimSpec{
+		Network:       networkingv1alpha.LocalNetworkRef{Name: "network-from-an-older-request"},
+		InterfaceName: defaultInterfaceName,
+		IPFamilies:    []networkingv1alpha.IPFamily{networkingv1alpha.IPv6Protocol},
+		ReclaimPolicy: networkingv1alpha.NetworkInterfaceReclaimPolicyRetain,
+	}
+
+	testCases := []struct {
+		name          string
+		existing      map[string]string
+		wantExtra     map[string]string
+		expectExisted bool
+	}{
+		{
+			name: "created claim carries the labels",
+		},
+		{
+			name:          "claim created before the labels is backfilled",
+			existing:      nil,
+			expectExisted: true,
+		},
+		{
+			name: "foreign labels survive the backfill",
+			existing: map[string]string{
+				"networking.datumapis.com/location": "us-central-1",
+			},
+			wantExtra: map[string]string{
+				"networking.datumapis.com/location": "us-central-1",
+			},
+			expectExisted: true,
+		},
+		{
+			name: "stale value is corrected",
+			existing: map[string]string{
+				computev1alpha.WorkloadNameLabel:  "a-workload-renamed-since",
+				computev1alpha.PlacementNameLabel: claimTestPlacement,
+				computev1alpha.CityCodeLabel:      wdControllerTestCityCode,
+				computev1alpha.InstanceIndexLabel: "0",
+			},
+			expectExisted: true,
+		},
+		{
+			name:          "labels already correct",
+			existing:      wantLabels,
+			expectExisted: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			deployment := newClaimTestDeployment()
+			instance := newClaimTestInstance(claimTestDeployment+"-0",
+				computev1alpha.InstanceNetworkInterface{
+					Network: networkingv1alpha.NetworkRef{Name: claimTestNetwork},
+					Name:    defaultInterfaceName,
+				},
+			)
+
+			builder := fake.NewClientBuilder().
+				WithScheme(newClaimTestScheme()).
+				WithObjects(deployment, instance)
+
+			if tc.expectExisted {
+				existing := &networkingv1alpha.NetworkInterfaceClaim{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      instance.Name + "-eth0",
+						Namespace: claimTestNamespace,
+						Labels:    maps.Clone(tc.existing),
+					},
+					Spec: existingSpec,
+				}
+				builder = builder.WithObjects(existing)
+			}
+
+			cl := builder.Build()
+
+			r := &WorkloadDeploymentReconciler{NetworkingEnabled: true}
+			_, err := r.reconcileNetworkInterfaceClaims(context.Background(), cl, deployment,
+				[]computev1alpha.Instance{*instance})
+			require.NoError(t, err)
+
+			var claim networkingv1alpha.NetworkInterfaceClaim
+			require.NoError(t, cl.Get(context.Background(), client.ObjectKey{
+				Namespace: claimTestNamespace,
+				Name:      instance.Name + "-eth0",
+			}, &claim))
+
+			want := maps.Clone(wantLabels)
+			for k, v := range tc.wantExtra {
+				want[k] = v
+			}
+			assert.Equal(t, want, claim.Labels)
+
+			if tc.expectExisted {
+				assert.Equal(t, existingSpec, claim.Spec,
+					"only labels may be patched; the claim spec is immutable")
+			} else {
+				assert.Equal(t, claimTestNetwork, claim.Spec.Network.Name)
+			}
+		})
+	}
+}

internal/controller/workloaddeployment_controller.go

@@ -777,10 +777,12 @@ func (r *WorkloadDeploymentReconciler) reconcileNetworkInterfaceClaims(
 // ensureNetworkInterfaceClaim creates the claim for one instance interface if it
 // is absent, and returns the claim either way.
 //
-// An existing claim is never updated: almost every field of a claim spec is
+// An existing claim's spec is never updated: almost every field of it is
 // immutable, because the addresses were allocated against it. A changed
 // interface request is expressed by replacing the instance, which replaces the
-// claim with it.
+// claim with it. Labels are the exception — they are mutable, and a claim
+// created before compute stamped them would otherwise never become selectable,
+// so they are patched onto what is already there.
 func (r *WorkloadDeploymentReconciler) ensureNetworkInterfaceClaim(
 	ctx context.Context,
 	c client.Client,
@@ -794,8 +796,13 @@ func (r *WorkloadDeploymentReconciler) ensureNetworkInterfaceClaim(
 		Name:      networkInterfaceClaimName(instance.Name, instanceInterfaceName(networkInterface)),
 	}
 
+	labels := desiredNetworkInterfaceClaimLabels(deployment, instance)
+
 	err := c.Get(ctx, key, claim)
 	if err == nil {
+		if err := r.backfillNetworkInterfaceClaimLabels(ctx, c, claim, labels); err != nil {
+			return nil, err
+		}
 		return claim, nil
 	}
 	if !apierrors.IsNotFound(err) {
@@ -806,6 +813,7 @@ func (r *WorkloadDeploymentReconciler) ensureNetworkInterfaceClaim(
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: key.Namespace,
 			Name:      key.Name,
+			Labels:    labels,
 		},
 		Spec: desiredNetworkInterfaceClaimSpec(networkInterface),
 	}
@@ -823,6 +831,9 @@ func (r *WorkloadDeploymentReconciler) ensureNetworkInterfaceClaim(
 			if getErr := c.Get(ctx, key, claim); getErr != nil {
 				return nil, fmt.Errorf("failed fetching network interface claim: %w", getErr)
 			}
+			if err := r.backfillNetworkInterfaceClaimLabels(ctx, c, claim, labels); err != nil {
+				return nil, err
+			}
 			return claim, nil
 		}
 		return nil, fmt.Errorf("failed creating network interface claim: %w", err)
@@ -833,6 +844,41 @@ func (r *WorkloadDeploymentReconciler) ensureNetworkInterfaceClaim(
 	return claim, nil
 }
 
+// backfillNetworkInterfaceClaimLabels brings the labels compute owns up to date
+// on a claim that already exists, leaving everything else on the object alone.
+//
+// The patch is computed from a copy of the live claim and only ever adds the
+// keys compute sets, so networking's own labels on the same object survive it
+// and the two controllers do not write over each other. Nothing but metadata is
+// in the patch, which is what keeps the immutable spec untouched.
+func (r *WorkloadDeploymentReconciler) backfillNetworkInterfaceClaimLabels(
+	ctx context.Context,
+	c client.Client,
+	claim *networkingv1alpha.NetworkInterfaceClaim,
+	desired map[string]string,
+) error {
+	if !networkInterfaceClaimLabelsStale(claim.Labels, desired) {
+		return nil
+	}
+
+	patch := client.MergeFrom(claim.DeepCopy())
+	if claim.Labels == nil {
+		claim.Labels = map[string]string{}
+	}
+	for key, value := range desired {
+		claim.Labels[key] = value
+	}
+
+	if err := c.Patch(ctx, claim, patch); err != nil {
+		return fmt.Errorf("failed labelling network interface claim %s/%s: %w",
+			claim.Namespace, claim.Name, err)
+	}
+
+	log.FromContext(ctx).Info("backfilled network interface claim labels", "claim", claim.Name)
+
+	return nil
+}
+
 func (r *WorkloadDeploymentReconciler) Finalize(_ context.Context, _ client.Object) (finalizer.Result, error) {
 	// Instance cascade is handled by Kubernetes GC via owner references set at
 	// Instance creation time. No explicit deletion is needed here.