diff --git a/internal/controller/networkinterfaceclaim.go b/internal/controller/networkinterfaceclaim.go index 22c62a94..7648a970 100644 --- a/internal/controller/networkinterfaceclaim.go +++ b/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 +} diff --git a/internal/controller/networkinterfaceclaim_controller_test.go b/internal/controller/networkinterfaceclaim_controller_test.go index b48da930..98d74d1d 100644 --- a/internal/controller/networkinterfaceclaim_controller_test.go +++ b/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) + } + }) + } +} diff --git a/internal/controller/workloaddeployment_controller.go b/internal/controller/workloaddeployment_controller.go index 308b3700..06bc90b8 100644 --- a/internal/controller/workloaddeployment_controller.go +++ b/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.