From d756e60144cfdb017cd4651cfed3966d865afeff Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Thu, 27 Aug 2026 13:47:16 -0500 Subject: [PATCH 1/2] feat: take a VPC identifier from the network A VPC drew its own random identifier per location, so the two locations of one network were two unrelated networks on the fabric: different VRF names, different Route Targets, no route import between them. The identity that makes them one network is already allocated centrally and carried to each cell, and nothing read it. The VPC now derives its identifier from that identity, through the existing identifier package so the reserved-value guards and the base62 width that keeps a kernel interface name inside fifteen characters still apply. Key changes: - Read the NetworkFabricIdentity for the context's network and encode spec.identity as the VPC identifier - Wait up to five minutes from the VPC's own creation timestamp for an identity that has not propagated yet, since the identifier is immutable and a fallback taken early is permanent - Fall back to the previous random draw past that window, so a network that will never have an identity still gets a working VPC - Report the wait as Ready=False with reason AwaitingFabricIdentity - Watch NetworkFabricIdentity so an arriving identity is taken at once --- .../controller/networkcontext_controller.go | 140 ++++++++++- .../networkcontext_controller_test.go | 220 ++++++++++++++++++ internal/identifier/identifier.go | 10 + internal/identifier/identifier_test.go | 20 ++ 4 files changed, 389 insertions(+), 1 deletion(-) diff --git a/internal/controller/networkcontext_controller.go b/internal/controller/networkcontext_controller.go index bddb95f..993ac82 100644 --- a/internal/controller/networkcontext_controller.go +++ b/internal/controller/networkcontext_controller.go @@ -23,18 +23,30 @@ import ( "slices" "time" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/reconcile" cloudv1alpha1 "go.datum.net/cloud/api/v1alpha1" "go.datum.net/cloud/internal/identifier" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" ) +const ( + // fabricIdentityGracePeriod bounds how long a new VPC waits for the identity + // the fabric knows its network by before falling back to drawing its own. + fabricIdentityGracePeriod = 5 * time.Minute + + // fabricIdentityPollInterval is how often a VPC still waiting looks again. + fabricIdentityPollInterval = 10 * time.Second +) + // NetworkContextReconciler gives a network's presence in one location its // data-plane identity: one VPC per NetworkContext, carrying the base62 VPC // identifier the whole galactic fabric keys on. @@ -44,6 +56,7 @@ type NetworkContextReconciler struct { } // +kubebuilder:rbac:groups=networking.datumapis.com,resources=networkcontexts,verbs=get;list;watch +// +kubebuilder:rbac:groups=cloud.datumapis.com,resources=networkfabricidentities,verbs=get;list;watch // +kubebuilder:rbac:groups=networking.datumapis.com,resources=subnets,verbs=get;list;watch // +kubebuilder:rbac:groups=cloud.datumapis.com,resources=vpcs,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=cloud.datumapis.com,resources=vpcs/status,verbs=get;update;patch @@ -82,11 +95,19 @@ func (r *NetworkContextReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, fmt.Errorf("reconcile VPC %s: %w", vpc.Name, err) } + // Everything below is conditioned on the VPC not having an identifier yet. A + // VPC that already has one keeps it: the VRF device is named from it and the + // Route Target derives from it, so renumbering a live VPC would rename its + // interface and change its routes under running traffic. if vpc.Status.VPC == "" { - allocated, err := r.allocateVPCIdentifier(ctx) + allocated, waiting, err := r.vpcIdentifier(ctx, &networkContext, vpc) if err != nil { return ctrl.Result{}, err } + if waiting { + return ctrl.Result{RequeueAfter: fabricIdentityPollInterval}, + r.markAwaitingFabricIdentity(ctx, vpc) + } vpc.Status.VPC = allocated } vpc.Status.ObservedGeneration = vpc.Generation @@ -104,6 +125,98 @@ func (r *NetworkContextReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, nil } +// vpcIdentifier resolves the identifier this VPC carries for the rest of its +// life, and reports whether it is still worth waiting for a better answer. +// +// The identity the fabric knows a network by is allocated centrally, once for +// the whole network, and carried to each cell the network reaches. Deriving the +// VPC identifier from it is what makes two locations of one network the same +// network on the fabric; drawing a random value per location, which is what +// this used to do unconditionally, made them two. +// +// The identity may not have landed in this cell yet when the VPC is first +// reconciled, and the identifier is immutable once written, so a fallback taken +// too eagerly is permanent. The wait is bounded rather than indefinite, because +// a network that will never have an identity — one predating the allocator, or +// one whose central allocation is stuck — has to end up with a working VPC +// rather than none at all. Past the grace period the old random draw still +// happens and nothing regresses. +// +// The window is measured from the VPC's own creation timestamp, so it survives +// a controller restart or a change of leader rather than resetting each time. +func (r *NetworkContextReconciler) vpcIdentifier( + ctx context.Context, + networkContext *networkingv1alpha.NetworkContext, + vpc *cloudv1alpha1.VPC, +) (string, bool, error) { + identity, found, err := r.fabricIdentity(ctx, networkContext) + if err != nil { + return "", false, err + } + if found { + encoded, err := identifier.VPCBase62(uint64(identity)) + if err != nil { + return "", false, fmt.Errorf("encode fabric identity %d for VPC %s: %w", identity, vpc.Name, err) + } + return encoded, false, nil + } + + // An age that cannot be read reads as new. The fallback is permanent, so the + // only safe way to be wrong about it is to wait longer. + if vpc.CreationTimestamp.IsZero() || + time.Since(vpc.CreationTimestamp.Time) < fabricIdentityGracePeriod { + return "", true, nil + } + + allocated, err := r.allocateVPCIdentifier(ctx) + return allocated, false, err +} + +// fabricIdentity reads the identity carried to this cell for the context's +// network. It is one object per network, named after the network, in the +// network's namespace. A missing one is an ordinary answer, not a failure. +func (r *NetworkContextReconciler) fabricIdentity( + ctx context.Context, networkContext *networkingv1alpha.NetworkContext, +) (int64, bool, error) { + networkName := networkContext.Spec.Network.Name + if networkName == "" { + return 0, false, nil + } + + var identity cloudv1alpha1.NetworkFabricIdentity + key := client.ObjectKey{Namespace: networkContext.Namespace, Name: networkName} + if err := r.Get(ctx, key, &identity); err != nil { + if apierrors.IsNotFound(err) { + return 0, false, nil + } + return 0, false, fmt.Errorf("read the fabric identity for network %q: %w", networkName, err) + } + if identity.Spec.Identity == 0 { + return 0, false, nil + } + return identity.Spec.Identity, true, nil +} + +// markAwaitingFabricIdentity says out loud that the VPC has no identifier yet +// and why, so a network stuck waiting on a central allocation is visible as +// that rather than as a VPC that silently never became ready. +func (r *NetworkContextReconciler) markAwaitingFabricIdentity( + ctx context.Context, vpc *cloudv1alpha1.VPC, +) error { + vpc.Status.ObservedGeneration = vpc.Generation + meta.SetStatusCondition(&vpc.Status.Conditions, metav1.Condition{ + Type: cloudv1alpha1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: "AwaitingFabricIdentity", + Message: "Waiting for the identity the fabric knows this network by", + ObservedGeneration: vpc.Generation, + }) + if err := r.Status().Update(ctx, vpc); err != nil { + return fmt.Errorf("update VPC %s status: %w", vpc.Name, err) + } + return nil +} + // networksForContext collects the CIDRs IPAM allocated for this location. func (r *NetworkContextReconciler) networksForContext( ctx context.Context, networkContext *networkingv1alpha.NetworkContext, @@ -174,6 +287,31 @@ func (r *NetworkContextReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&networkingv1alpha.NetworkContext{}). Owns(&cloudv1alpha1.VPC{}). + Watches(&cloudv1alpha1.NetworkFabricIdentity{}, + handler.EnqueueRequestsFromMapFunc(r.contextsForFabricIdentity)). Named("networkcontext"). Complete(r) } + +// contextsForFabricIdentity wakes a network's presences the moment its identity +// reaches this cell, so a VPC waiting on one takes it immediately instead of +// sitting out its poll interval. +func (r *NetworkContextReconciler) contextsForFabricIdentity( + ctx context.Context, object client.Object, +) []reconcile.Request { + var contexts networkingv1alpha.NetworkContextList + if err := r.List(ctx, &contexts, client.InNamespace(object.GetNamespace())); err != nil { + return nil + } + + requests := make([]reconcile.Request, 0, len(contexts.Items)) + for i := range contexts.Items { + if contexts.Items[i].Spec.Network.Name != object.GetName() { + continue + } + requests = append(requests, reconcile.Request{ + NamespacedName: client.ObjectKeyFromObject(&contexts.Items[i]), + }) + } + return requests +} diff --git a/internal/controller/networkcontext_controller_test.go b/internal/controller/networkcontext_controller_test.go index 5734222..3dec2aa 100644 --- a/internal/controller/networkcontext_controller_test.go +++ b/internal/controller/networkcontext_controller_test.go @@ -18,8 +18,19 @@ along with this program. If not, see . package controller import ( + "context" "testing" + "time" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + cloudv1alpha1 "go.datum.net/cloud/api/v1alpha1" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" ) @@ -50,3 +61,212 @@ func TestSubnetRangePrefersStatusAndFallsBackToSpec(t *testing.T) { t.Fatal("an unallocated subnet should yield nothing") } } + +const ( + vpcTestNamespace = "ns-project" + vpcTestNetwork = "prod" + vpcTestLocation = "us-central-1" + + // The identity the fabric knows the test network by, and the identifier a + // VPC derives from it. + vpcTestIdentity = 16 + vpcTestIdentifier = "g" +) + +type vpcFixture struct { + t *testing.T + ctx context.Context + client client.Client + reconciler *NetworkContextReconciler +} + +// newVPCFixture stands up one network present at one location, with the subnet +// that gives the VPC an address space to be created for. +func newVPCFixture(t *testing.T, objects ...client.Object) *vpcFixture { + t.Helper() + + scheme := runtime.NewScheme() + if err := networkingv1alpha.AddToScheme(scheme); err != nil { + t.Fatalf("build the networking scheme: %v", err) + } + if err := cloudv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("build the cloud scheme: %v", err) + } + + presence := &networkingv1alpha.NetworkContext{} + presence.Namespace = vpcTestNamespace + presence.Name = vpcTestNetwork + "-" + vpcTestLocation + presence.Spec.Network = networkingv1alpha.LocalNetworkRef{Name: vpcTestNetwork} + presence.Spec.Location = networkingv1alpha.LocationReference{Name: vpcTestLocation} + + subnet := &networkingv1alpha.Subnet{} + subnet.Namespace = vpcTestNamespace + subnet.Name = presence.Name + subnet.Spec.NetworkContext = networkingv1alpha.LocalNetworkContextRef{Name: presence.Name} + subnet.Spec.StartAddress = "fd00::" + subnet.Spec.PrefixLength = 48 + + all := append([]client.Object{presence, subnet}, objects...) + fakeClient := fake.NewClientBuilder().WithScheme(scheme). + WithStatusSubresource(&cloudv1alpha1.VPC{}). + WithObjects(all...).Build() + + return &vpcFixture{ + t: t, + ctx: context.Background(), + client: fakeClient, + reconciler: &NetworkContextReconciler{Client: fakeClient, Scheme: scheme}, + } +} + +func (f *vpcFixture) reconcile() ctrl.Result { + f.t.Helper() + result, err := f.reconciler.Reconcile(f.ctx, ctrl.Request{NamespacedName: types.NamespacedName{ + Namespace: vpcTestNamespace, + Name: vpcTestNetwork + "-" + vpcTestLocation, + }}) + if err != nil { + f.t.Fatalf("reconcile: %v", err) + } + return result +} + +func (f *vpcFixture) vpc() *cloudv1alpha1.VPC { + f.t.Helper() + vpc := &cloudv1alpha1.VPC{} + key := types.NamespacedName{Namespace: vpcTestNamespace, Name: vpcTestNetwork + "-" + vpcTestLocation} + if err := f.client.Get(f.ctx, key, vpc); err != nil { + f.t.Fatalf("read the VPC: %v", err) + } + return vpc +} + +func fabricIdentityObject() *cloudv1alpha1.NetworkFabricIdentity { + object := &cloudv1alpha1.NetworkFabricIdentity{} + object.Namespace = vpcTestNamespace + object.Name = vpcTestNetwork + object.Spec.Identity = vpcTestIdentity + object.Spec.NetworkRef = cloudv1alpha1.NetworkFabricIdentityNetworkRef{Name: vpcTestNetwork} + return object +} + +// The whole point: every location of one network reaches the same identifier, +// because every one of them reads the same centrally allocated identity. +func TestVPCTakesItsIdentifierFromTheNetworksFabricIdentity(t *testing.T) { + fixture := newVPCFixture(t, fabricIdentityObject()) + + fixture.reconcile() + + vpc := fixture.vpc() + if vpc.Status.VPC != vpcTestIdentifier { + t.Fatalf("VPC identifier: got %q, want %q", vpc.Status.VPC, vpcTestIdentifier) + } + if condition := meta.FindStatusCondition(vpc.Status.Conditions, cloudv1alpha1.ConditionTypeReady); condition == nil || + condition.Status != metav1.ConditionTrue { + t.Fatalf("a VPC with an identifier should be ready, got %+v", condition) + } +} + +// A network whose identity has not reached this cell yet waits rather than +// drawing a value it could never give back: the identifier is immutable, so a +// fallback taken early is permanent. +func TestVPCWaitsForAnIdentityThatHasNotArrived(t *testing.T) { + fixture := newVPCFixture(t) + + result := fixture.reconcile() + + if result.RequeueAfter == 0 { + t.Fatal("a VPC waiting on an identity should ask to be looked at again") + } + vpc := fixture.vpc() + if vpc.Status.VPC != "" { + t.Fatalf("no identifier should be written while waiting, got %q", vpc.Status.VPC) + } + condition := meta.FindStatusCondition(vpc.Status.Conditions, cloudv1alpha1.ConditionTypeReady) + if condition == nil || condition.Status != metav1.ConditionFalse || + condition.Reason != "AwaitingFabricIdentity" { + t.Fatalf("the wait should be visible on the VPC, got %+v", condition) + } +} + +// The identity landing is what ends the wait, and the identifier that follows +// is the derived one rather than a random draw. +func TestVPCTakesTheIdentityOnceItArrives(t *testing.T) { + fixture := newVPCFixture(t) + + fixture.reconcile() + if got := fixture.vpc().Status.VPC; got != "" { + t.Fatalf("no identifier should be written while waiting, got %q", got) + } + + if err := fixture.client.Create(fixture.ctx, fabricIdentityObject()); err != nil { + t.Fatalf("publish the identity: %v", err) + } + fixture.reconcile() + + if got := fixture.vpc().Status.VPC; got != vpcTestIdentifier { + t.Fatalf("VPC identifier: got %q, want %q", got, vpcTestIdentifier) + } +} + +// Not every network has an identity yet, and one that never gets one still has +// to end up with a working VPC. Past the grace period the old random draw +// happens exactly as it did before. +func TestVPCFallsBackToARandomIdentifierAfterTheGracePeriod(t *testing.T) { + stale := &cloudv1alpha1.VPC{} + stale.Namespace = vpcTestNamespace + stale.Name = vpcTestNetwork + "-" + vpcTestLocation + stale.CreationTimestamp = metav1.NewTime(time.Now().Add(-2 * fabricIdentityGracePeriod)) + + fixture := newVPCFixture(t, stale) + + result := fixture.reconcile() + + if result.RequeueAfter != 0 { + t.Fatal("a VPC past its grace period should stop waiting") + } + vpc := fixture.vpc() + if vpc.Status.VPC == "" { + t.Fatal("a VPC past its grace period should get a random identifier") + } + if condition := meta.FindStatusCondition(vpc.Status.Conditions, cloudv1alpha1.ConditionTypeReady); condition == nil || + condition.Status != metav1.ConditionTrue { + t.Fatalf("a VPC with an identifier should be ready, got %+v", condition) + } +} + +// Renumbering a live VPC would rename its VRF device and change its Route +// Target under running traffic, so an identifier already written stays written +// even when it disagrees with the identity that later arrived. +func TestVPCKeepsAnIdentifierItAlreadyHas(t *testing.T) { + existing := &cloudv1alpha1.VPC{} + existing.Namespace = vpcTestNamespace + existing.Name = vpcTestNetwork + "-" + vpcTestLocation + existing.Status.VPC = "R2POk4jT" + + fixture := newVPCFixture(t, existing, fabricIdentityObject()) + + fixture.reconcile() + + if got := fixture.vpc().Status.VPC; got != "R2POk4jT" { + t.Fatalf("an allocated VPC must keep its identifier, got %q", got) + } +} + +// An identity reaching the cell has to wake the presences waiting on it, or a +// VPC would sit out its poll interval for a value already there. +func TestFabricIdentityWakesTheNetworksPresences(t *testing.T) { + fixture := newVPCFixture(t) + + requests := fixture.reconciler.contextsForFabricIdentity(fixture.ctx, fabricIdentityObject()) + + if len(requests) != 1 || requests[0].Name != vpcTestNetwork+"-"+vpcTestLocation { + t.Fatalf("the network's presence should be enqueued, got %v", requests) + } + + other := fabricIdentityObject() + other.Name = "unrelated" + if requests := fixture.reconciler.contextsForFabricIdentity(fixture.ctx, other); len(requests) != 0 { + t.Fatalf("another network's identity should enqueue nothing, got %v", requests) + } +} diff --git a/internal/identifier/identifier.go b/internal/identifier/identifier.go index 5360aa8..a766228 100644 --- a/internal/identifier/identifier.go +++ b/internal/identifier/identifier.go @@ -80,6 +80,16 @@ func Base62ToHex(value string) (string, error) { return baseconv.Convert(value, baseconv.Digits62, baseconv.DigitsHex) } +// VPCBase62 renders a known VPC identifier in base62, applying the same +// reserved-value guards and width as a drawn one. +func VPCBase62(value uint64) (string, error) { + hex, err := Hex(value, MaxVPC) + if err != nil { + return "", err + } + return HexToBase62(hex) +} + // RandomVPCBase62 returns a random VPC identifier in base62. func RandomVPCBase62() (string, error) { hex, err := RandomVPC() diff --git a/internal/identifier/identifier_test.go b/internal/identifier/identifier_test.go index 938633b..f577dd6 100644 --- a/internal/identifier/identifier_test.go +++ b/internal/identifier/identifier_test.go @@ -73,3 +73,23 @@ func trimLeadingZeros(value string) string { } return "0" } + +func TestVPCBase62RejectsReservedValuesAndFitsItsSegment(t *testing.T) { + for _, value := range []uint64{0, MaxVPC, MaxVPC + 1} { + if _, err := VPCBase62(value); err == nil { + t.Errorf("VPCBase62(%d): got no error, want one", value) + } + } + + // The widest fabric identity is 32 bits, well inside the 48 the VPC + // identifier holds, so nothing an identity carries can overflow the segment. + for _, value := range []uint64{1, 16, 1 << 31, 1<<32 - 1} { + encoded, err := VPCBase62(value) + if err != nil { + t.Fatalf("VPCBase62(%d): %v", value, err) + } + if len(encoded) > 9 { + t.Errorf("VPCBase62(%d) = %q exceeds nine base62 characters", value, encoded) + } + } +} From 8074f0b7714e702fd2188c06293ebc749bf02a86 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Thu, 27 Aug 2026 14:10:42 -0500 Subject: [PATCH 2/2] refactor: make a VPC wait for its network's identity Falling back to a random identifier when the fabric identity has not arrived is worse than having no VPC. The identifier is immutable, so the fallback is permanent, and the extension server refuses to bind a VRF for any Envoy cluster whose members span more than one VPC. A network whose locations disagree therefore serves no traffic at all, including through healthy members, where a network still waiting recovers the instant its identity lands. Key changes: - Remove the grace period, the poll interval and the random fallback: a VPC with no identity writes no identifier and waits - Rely on the NetworkFabricIdentity watch alone; the informer lists before it watches, so an identity present at startup is seen rather than missed - Delete allocateVPCIdentifier, RandomVPCBase62 and RandomVPC, which the fabric identity leaves with no callers --- .../controller/networkcontext_controller.go | 108 ++++-------------- .../networkcontext_controller_test.go | 40 +------ internal/identifier/identifier.go | 18 +-- internal/identifier/identifier_test.go | 12 +- 4 files changed, 30 insertions(+), 148 deletions(-) diff --git a/internal/controller/networkcontext_controller.go b/internal/controller/networkcontext_controller.go index 993ac82..77bed0b 100644 --- a/internal/controller/networkcontext_controller.go +++ b/internal/controller/networkcontext_controller.go @@ -38,15 +38,6 @@ import ( networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" ) -const ( - // fabricIdentityGracePeriod bounds how long a new VPC waits for the identity - // the fabric knows its network by before falling back to drawing its own. - fabricIdentityGracePeriod = 5 * time.Minute - - // fabricIdentityPollInterval is how often a VPC still waiting looks again. - fabricIdentityPollInterval = 10 * time.Second -) - // NetworkContextReconciler gives a network's presence in one location its // data-plane identity: one VPC per NetworkContext, carrying the base62 VPC // identifier the whole galactic fabric keys on. @@ -100,15 +91,18 @@ func (r *NetworkContextReconciler) Reconcile(ctx context.Context, req ctrl.Reque // Route Target derives from it, so renumbering a live VPC would rename its // interface and change its routes under running traffic. if vpc.Status.VPC == "" { - allocated, waiting, err := r.vpcIdentifier(ctx, &networkContext, vpc) + identity, found, err := r.fabricIdentity(ctx, &networkContext) if err != nil { return ctrl.Result{}, err } - if waiting { - return ctrl.Result{RequeueAfter: fabricIdentityPollInterval}, - r.markAwaitingFabricIdentity(ctx, vpc) + if !found { + return ctrl.Result{}, r.markAwaitingFabricIdentity(ctx, vpc) + } + encoded, err := identifier.VPCBase62(uint64(identity)) + if err != nil { + return ctrl.Result{}, fmt.Errorf("encode fabric identity %d for VPC %s: %w", identity, vpc.Name, err) } - vpc.Status.VPC = allocated + vpc.Status.VPC = encoded } vpc.Status.ObservedGeneration = vpc.Generation meta.SetStatusCondition(&vpc.Status.Conditions, metav1.Condition{ @@ -125,56 +119,21 @@ func (r *NetworkContextReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, nil } -// vpcIdentifier resolves the identifier this VPC carries for the rest of its -// life, and reports whether it is still worth waiting for a better answer. -// -// The identity the fabric knows a network by is allocated centrally, once for -// the whole network, and carried to each cell the network reaches. Deriving the -// VPC identifier from it is what makes two locations of one network the same -// network on the fabric; drawing a random value per location, which is what -// this used to do unconditionally, made them two. -// -// The identity may not have landed in this cell yet when the VPC is first -// reconciled, and the identifier is immutable once written, so a fallback taken -// too eagerly is permanent. The wait is bounded rather than indefinite, because -// a network that will never have an identity — one predating the allocator, or -// one whose central allocation is stuck — has to end up with a working VPC -// rather than none at all. Past the grace period the old random draw still -// happens and nothing regresses. -// -// The window is measured from the VPC's own creation timestamp, so it survives -// a controller restart or a change of leader rather than resetting each time. -func (r *NetworkContextReconciler) vpcIdentifier( - ctx context.Context, - networkContext *networkingv1alpha.NetworkContext, - vpc *cloudv1alpha1.VPC, -) (string, bool, error) { - identity, found, err := r.fabricIdentity(ctx, networkContext) - if err != nil { - return "", false, err - } - if found { - encoded, err := identifier.VPCBase62(uint64(identity)) - if err != nil { - return "", false, fmt.Errorf("encode fabric identity %d for VPC %s: %w", identity, vpc.Name, err) - } - return encoded, false, nil - } - - // An age that cannot be read reads as new. The fallback is permanent, so the - // only safe way to be wrong about it is to wait longer. - if vpc.CreationTimestamp.IsZero() || - time.Since(vpc.CreationTimestamp.Time) < fabricIdentityGracePeriod { - return "", true, nil - } - - allocated, err := r.allocateVPCIdentifier(ctx) - return allocated, false, err -} - // fabricIdentity reads the identity carried to this cell for the context's // network. It is one object per network, named after the network, in the // network's namespace. A missing one is an ordinary answer, not a failure. +// +// The identity is allocated centrally, once for the whole network, and carried +// to each cell the network reaches. Deriving the VPC identifier from it is what +// makes two locations of one network the same network on the fabric; drawing a +// random value per location, which is what this used to do, made them two. +// +// A VPC whose identity has not arrived waits, and there is deliberately nothing +// else it can do. The identifier is immutable, so a random value taken while +// waiting is permanent, and a network whose locations then disagree is worse +// than one with no VPC at all: a NetworkService spanning them binds no VRF and +// fails every request, including through healthy members. Waiting ends the +// moment the identity lands. A random draw never ends. func (r *NetworkContextReconciler) fabricIdentity( ctx context.Context, networkContext *networkingv1alpha.NetworkContext, ) (int64, bool, error) { @@ -255,33 +214,6 @@ func subnetRange(subnet *networkingv1alpha.Subnet) (string, int32, bool) { return "", 0, false } -// allocateVPCIdentifier draws a random 48-bit identifier not already in use. -// A single leader-elected controller is the only writer, so a list plus a -// collision check serializes correctly. -func (r *NetworkContextReconciler) allocateVPCIdentifier(ctx context.Context) (string, error) { - var vpcs cloudv1alpha1.VPCList - if err := r.List(ctx, &vpcs); err != nil { - return "", fmt.Errorf("list VPCs: %w", err) - } - used := make(map[string]struct{}, len(vpcs.Items)) - for _, vpc := range vpcs.Items { - if vpc.Status.VPC != "" { - used[vpc.Status.VPC] = struct{}{} - } - } - - for range maxIdentifierAttempts { - candidate, err := identifier.RandomVPCBase62() - if err != nil { - return "", err - } - if _, taken := used[candidate]; !taken { - return candidate, nil - } - } - return "", fmt.Errorf("no unused VPC identifier found after %d attempts", maxIdentifierAttempts) -} - // SetupWithManager registers the reconciler with the manager. func (r *NetworkContextReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). diff --git a/internal/controller/networkcontext_controller_test.go b/internal/controller/networkcontext_controller_test.go index 3dec2aa..36e24e2 100644 --- a/internal/controller/networkcontext_controller_test.go +++ b/internal/controller/networkcontext_controller_test.go @@ -20,7 +20,6 @@ package controller import ( "context" "testing" - "time" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -167,17 +166,16 @@ func TestVPCTakesItsIdentifierFromTheNetworksFabricIdentity(t *testing.T) { } } -// A network whose identity has not reached this cell yet waits rather than -// drawing a value it could never give back: the identifier is immutable, so a -// fallback taken early is permanent. +// A network whose identity has not reached this cell yet waits, and writes +// nothing. The identifier is immutable, so a value taken while waiting is +// permanent, and locations that disagree bind no VRF at all. func TestVPCWaitsForAnIdentityThatHasNotArrived(t *testing.T) { fixture := newVPCFixture(t) - result := fixture.reconcile() - - if result.RequeueAfter == 0 { - t.Fatal("a VPC waiting on an identity should ask to be looked at again") + if result := fixture.reconcile(); result.RequeueAfter != 0 { + t.Fatalf("the identity's arrival is the trigger, not a poll, got %v", result) } + vpc := fixture.vpc() if vpc.Status.VPC != "" { t.Fatalf("no identifier should be written while waiting, got %q", vpc.Status.VPC) @@ -209,32 +207,6 @@ func TestVPCTakesTheIdentityOnceItArrives(t *testing.T) { } } -// Not every network has an identity yet, and one that never gets one still has -// to end up with a working VPC. Past the grace period the old random draw -// happens exactly as it did before. -func TestVPCFallsBackToARandomIdentifierAfterTheGracePeriod(t *testing.T) { - stale := &cloudv1alpha1.VPC{} - stale.Namespace = vpcTestNamespace - stale.Name = vpcTestNetwork + "-" + vpcTestLocation - stale.CreationTimestamp = metav1.NewTime(time.Now().Add(-2 * fabricIdentityGracePeriod)) - - fixture := newVPCFixture(t, stale) - - result := fixture.reconcile() - - if result.RequeueAfter != 0 { - t.Fatal("a VPC past its grace period should stop waiting") - } - vpc := fixture.vpc() - if vpc.Status.VPC == "" { - t.Fatal("a VPC past its grace period should get a random identifier") - } - if condition := meta.FindStatusCondition(vpc.Status.Conditions, cloudv1alpha1.ConditionTypeReady); condition == nil || - condition.Status != metav1.ConditionTrue { - t.Fatalf("a VPC with an identifier should be ready, got %+v", condition) - } -} - // Renumbering a live VPC would rename its VRF device and change its Route // Target under running traffic, so an identifier already written stays written // even when it disagrees with the identity that later arrived. diff --git a/internal/identifier/identifier.go b/internal/identifier/identifier.go index a766228..657d22d 100644 --- a/internal/identifier/identifier.go +++ b/internal/identifier/identifier.go @@ -60,11 +60,6 @@ func Random(max uint64) (string, error) { return Hex(n.Uint64()+1, max) } -// RandomVPC returns a random 48-bit VPC identifier in hex. -func RandomVPC() (string, error) { - return Random(MaxVPC) -} - // RandomVPCAttachment returns a random 16-bit attachment identifier in hex. func RandomVPCAttachment() (string, error) { return Random(MaxVPCAttachment) @@ -80,8 +75,8 @@ func Base62ToHex(value string) (string, error) { return baseconv.Convert(value, baseconv.Digits62, baseconv.DigitsHex) } -// VPCBase62 renders a known VPC identifier in base62, applying the same -// reserved-value guards and width as a drawn one. +// VPCBase62 renders a VPC identifier in base62, with the reserved-value guards +// and the width that keeps a kernel interface name inside fifteen characters. func VPCBase62(value uint64) (string, error) { hex, err := Hex(value, MaxVPC) if err != nil { @@ -90,15 +85,6 @@ func VPCBase62(value uint64) (string, error) { return HexToBase62(hex) } -// RandomVPCBase62 returns a random VPC identifier in base62. -func RandomVPCBase62() (string, error) { - hex, err := RandomVPC() - if err != nil { - return "", err - } - return HexToBase62(hex) -} - // RandomVPCAttachmentBase62 returns a random attachment identifier in base62. func RandomVPCAttachmentBase62() (string, error) { hex, err := RandomVPCAttachment() diff --git a/internal/identifier/identifier_test.go b/internal/identifier/identifier_test.go index f577dd6..5e7cda3 100644 --- a/internal/identifier/identifier_test.go +++ b/internal/identifier/identifier_test.go @@ -29,14 +29,6 @@ func TestHexRejectsReservedValues(t *testing.T) { func TestRandomIdentifiersFitTheirInterfaceNameSegment(t *testing.T) { for range 200 { - vpc, err := RandomVPCBase62() - if err != nil { - t.Fatalf("RandomVPCBase62: %v", err) - } - if len(vpc) > 9 { - t.Errorf("VPC identifier %q exceeds nine base62 characters", vpc) - } - attachment, err := RandomVPCAttachmentBase62() if err != nil { t.Fatalf("RandomVPCAttachmentBase62: %v", err) @@ -48,9 +40,9 @@ func TestRandomIdentifiersFitTheirInterfaceNameSegment(t *testing.T) { } func TestBase62RoundTrip(t *testing.T) { - hex, err := RandomVPC() + hex, err := Random(MaxVPC) if err != nil { - t.Fatalf("RandomVPC: %v", err) + t.Fatalf("Random(MaxVPC): %v", err) } base62, err := HexToBase62(hex) if err != nil {