fix: Collect a projected interface before its name is reused

internal/controller/networkinterface_projection.go

@@ -6,8 +6,10 @@ import (
 	"context"
 	"fmt"
 	"maps"
+	"time"
 
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -73,13 +75,81 @@ func isProjection(iface *networkingv1alpha.NetworkInterface) bool {
 	return iface.Labels[networkingv1alpha.NetworkInterfaceProjectionLabel] == "true"
 }
 
-// writeProjection converges a copy onto what the source says. The copy is
-// overwritten in full on every pass, so an edit to it never survives and never
-// reaches the source: no controller reads a copy to decide anything.
+// projectionSlotRetry paces a writer that found the name it publishes under
+// still held. A teardown takes one reconcile of the controller holding it, so
+// what is waited on here is measured in seconds.
+const projectionSlotRetry = 5 * time.Second
+
+// projectionSlotHeld reports that the copy occupying a name is still being torn
+// down, so the name is not free to write yet.
+//
+// A copy is collected while the interface it was published from is still held,
+// and the name is free once that has finished. A writer that arrives mid
+// teardown waits for it rather than writing through what is going.
+type projectionSlotHeld struct {
+	key client.ObjectKey
+}
+
+func (e *projectionSlotHeld) Error() string {
+	return fmt.Sprintf("the interface copy at %s has not finished being torn down", e.key)
+}
+
+// inspectProjectionSlot reports the copy occupying a name.
+func inspectProjectionSlot(
+	ctx context.Context,
+	cl client.Client,
+	key client.ObjectKey,
+) (*networkingv1alpha.NetworkInterface, error) {
+	var occupant networkingv1alpha.NetworkInterface
+	if err := cl.Get(ctx, key, &occupant); err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil, nil
+		}
+		return nil, fmt.Errorf("failed reading the interface copy: %w", err)
+	}
+
+	// Nothing here owns an interface somebody else made, and an interface that is
+	// not a copy is the only thing in this namespace that could be holding the
+	// name for a reason.
+	if !isProjection(&occupant) {
+		return nil, fmt.Errorf(
+			"an interface that is not a published copy already holds %s, refusing to write over it", key)
+	}
+
+	return &occupant, nil
+}
+
+// applyProjection converges a copy onto what the source says.
 //
 // A label the source has dropped is removed from the copy rather than left in
 // place. A copy is selected by its labels, so one the source no longer carries
 // is a member of a service the interface has left.
+func applyProjection(
+	copied *networkingv1alpha.NetworkInterface,
+	desired *networkingv1alpha.NetworkInterface,
+	extraLabels map[string]string,
+	owner func(*networkingv1alpha.NetworkInterface) error,
+) error {
+	wanted := map[string]string{}
+	maps.Copy(wanted, desired.Labels)
+	maps.Copy(wanted, extraLabels)
+
+	copied.Labels, _ = replaceOwnedLabels(copied.Labels, wanted, platformLabelPrefixes, nil)
+	copied.Spec = *desired.Spec.DeepCopy()
+	if owner != nil {
+		return owner(copied)
+	}
+	return nil
+}
+
+// writeProjection converges a copy onto what the source says. The copy is
+// overwritten in full on every pass, so an edit to it never survives and never
+// reaches the source: no controller reads a copy to decide anything.
+//
+// The name an interface is published under is one a replacement reuses. Nothing
+// here empties the name first, because the copy that held it is collected while
+// the interface it was published from is still there to say where it went, and a
+// replacement that arrives before that has finished waits for it.
 func writeProjection(
 	ctx context.Context,
 	cl client.Client,
@@ -88,21 +158,23 @@ func writeProjection(
 	extraLabels map[string]string,
 	owner func(*networkingv1alpha.NetworkInterface) error,
 ) error {
-	copied := &networkingv1alpha.NetworkInterface{
-		ObjectMeta: metav1.ObjectMeta{Name: desired.Name, Namespace: namespace},
+	key := client.ObjectKey{Namespace: namespace, Name: desired.Name}
+
+	occupant, err := inspectProjectionSlot(ctx, cl, key)
+	if err != nil {
+		return err
+	}
+	// A copy on its way out is not something to write through. What is left of it
+	// goes when its teardown finishes, and the name is free then.
+	if occupant != nil && !occupant.DeletionTimestamp.IsZero() {
+		return &projectionSlotHeld{key: key}
 	}
 
+	copied := &networkingv1alpha.NetworkInterface{
+		ObjectMeta: metav1.ObjectMeta{Name: key.Name, Namespace: key.Namespace},
+	}
 	if _, err := controllerutil.CreateOrUpdate(ctx, cl, copied, func() error {
-		wanted := map[string]string{}
-		maps.Copy(wanted, desired.Labels)
-		maps.Copy(wanted, extraLabels)
-
-		copied.Labels, _ = replaceOwnedLabels(copied.Labels, wanted, platformLabelPrefixes, nil)
-		copied.Spec = *desired.Spec.DeepCopy()
-		if owner != nil {
-			return owner(copied)
-		}
-		return nil
+		return applyProjection(copied, desired, extraLabels, owner)
 	}); err != nil {
 		return fmt.Errorf("failed writing the interface copy: %w", err)
 	}
@@ -119,9 +191,9 @@ func writeProjection(
 	return nil
 }
 
-// collectProjection removes a copy, and only ever a copy. A copy holds no
-// finalizer and nothing real depends on it, so removing one cannot strand an
-// address or hold a namespace open.
+// collectProjection removes a copy, and only ever a copy. Nothing real depends
+// on a copy and a copy in a project control plane holds no finalizer, so
+// removing one cannot strand an address or hold a namespace open.
 func collectProjection(ctx context.Context, cl client.Client, key client.ObjectKey) error {
 	var copied networkingv1alpha.NetworkInterface
 	if err := cl.Get(ctx, key, &copied); err != nil {

internal/controller/networkinterface_projection_gc.go

@@ -20,11 +20,16 @@ import (
 // NetworkInterfaceProjectionGCReconciler removes an interface copy from a
 // project control plane once nothing is published behind it.
 //
-// It is the projector's other half, and it exists because the projector is
-// keyed on the published copy: an object that has gone carries no labels, so
-// nothing on the hub can say which project a vanished copy belonged to. Keying
-// on the copy instead answers that from the copy itself, and it also brings a
-// copy back that somebody deleted by hand.
+// It is the projector's backstop rather than its cleanup path: the projector
+// holds a published interface until its copy is collected, so the ordinary
+// deletion never reaches here. What does reach here is a copy that outlived a
+// deletion nothing was there to hold, and a copy somebody deleted by hand, both
+// of which are answered from the copy itself.
+//
+// It asks only whether anything is published behind a copy. It deliberately does
+// not compare what the copy was published from: it reads the hub through a cache
+// that lags, and a copy that had just been replaced would look superseded to a
+// lagging read, so identity is the writer's question and presence is this one's.
 type NetworkInterfaceProjectionGCReconciler struct {
 	Projects ProjectClusterResolver