From 329c258bc8b0a7a3bd21b0ec88d1a7960f5f5b03 Mon Sep 17 00:00:00 2001 From: Viktor Voytovych Date: Tue, 3 Nov 2026 11:45:36 +0200 Subject: [PATCH] fix: Let a context settle when its network's DNS is off The project reconciler stamped the internal-dns-region annotation and then, because the network's DNS is off, cleared it again with the rest of the intent. Each write is an event on the context it reconciles, so it never settled. Decide that DNS is off before the region is stamped. --- internal/controller/internaldns_controller.go | 18 +-- .../controller/internaldns_controller_test.go | 119 ++++++++++++++++++ 2 files changed, 130 insertions(+), 7 deletions(-) diff --git a/internal/controller/internaldns_controller.go b/internal/controller/internaldns_controller.go index c483cbd4..fbd0c28c 100644 --- a/internal/controller/internaldns_controller.go +++ b/internal/controller/internaldns_controller.go @@ -144,6 +144,17 @@ func (r *InternalDNSProjectReconciler) reconcile( return ctrl.Result{}, nil } + // Decided before the region is stamped: disable clears the region with the + // rest of the intent, and a stamp it then clears is two writes whose events + // run this reconcile again. + network := &networkingv1alpha.Network{} + if err := cl.Get(ctx, client.ObjectKey{Namespace: key.Namespace, Name: networkContext.Spec.Network.Name}, network); err != nil { + return ctrl.Result{}, err + } + if network.Spec.DNS.Mode == networkingv1alpha.NetworkDNSModeDisabled { + return ctrl.Result{}, r.disable(ctx, cl, networkContext, network) + } + region, err := r.region(ctx, cl, networkContext) if err != nil { return ctrl.Result{}, err @@ -169,13 +180,6 @@ func (r *InternalDNSProjectReconciler) reconcile( } } - network := &networkingv1alpha.Network{} - if err := cl.Get(ctx, client.ObjectKey{Namespace: key.Namespace, Name: networkContext.Spec.Network.Name}, network); err != nil { - return ctrl.Result{}, err - } - if network.Spec.DNS.Mode == networkingv1alpha.NetworkDNSModeDisabled { - return ctrl.Result{}, r.disable(ctx, cl, networkContext, network) - } if region == "" { return ctrl.Result{RequeueAfter: internalDNSRetryInterval}, nil } diff --git a/internal/controller/internaldns_controller_test.go b/internal/controller/internaldns_controller_test.go index 15a72fa1..47cf1bef 100644 --- a/internal/controller/internaldns_controller_test.go +++ b/internal/controller/internaldns_controller_test.go @@ -17,6 +17,7 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" @@ -691,3 +692,121 @@ func TestInternalDNSReadinessExplainsUnreadyContexts(t *testing.T) { } } } + +// A context of a network with DNS turned off must settle. In a location with an +// internal-DNS region the reconciler once stamped the region and then cleared it +// with the rest of the intent, and each write was an event on the context itself. +func TestInternalDNSProjectSettlesWhenDNSIsOff(t *testing.T) { + t.Parallel() + wasOn := testNetworkContext("was-on", "context-1") + wasOn.Annotations = testDNSIntent(time.Time{}) + wasOn.Finalizers = []string{internalDNSFinalizer} + cases := map[string]struct { + networkContext *networkingv1alpha.NetworkContext + region string + }{ + "in a location with a region": {networkContext: testNetworkContext("clean", "context-1"), region: "us-central"}, + "in a location without a region": {networkContext: testNetworkContext("clean", "context-1")}, + "after DNS was on": {networkContext: wasOn, region: "us-central"}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newDNSOffFixture(t, tc.networkContext, tc.region) + key := client.ObjectKeyFromObject(tc.networkContext) + + f.reconcile(t, ctx, key) + f.writes = 0 + f.reconcile(t, ctx, key) + if f.writes != 0 { + t.Fatalf("the second reconcile wrote the context %d times, want none", f.writes) + } + f.assertDNSOff(t, ctx, key, len(tc.networkContext.Annotations) > 0) + }) + } +} + +// dnsOffFixture holds a network with DNS turned off, its location, one context +// and the regional access that context used, and counts the context's writes. +type dnsOffFixture struct { + cl client.Client + access *unstructured.Unstructured + writes int +} + +func newDNSOffFixture(t *testing.T, networkContext *networkingv1alpha.NetworkContext, region string) *dnsOffFixture { + t.Helper() + scheme := runtime.NewScheme() + if err := networkingv1alpha.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := locationsv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + scheme.AddKnownTypeWithName(internalDNSContextGVK, &unstructured.Unstructured{}) + scheme.AddKnownTypeWithName(internalDNSAccessGVK, &unstructured.Unstructured{}) + network := &networkingv1alpha.Network{ + ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "blue", UID: "network-uid"}, + Spec: networkingv1alpha.NetworkSpec{DNS: networkingv1alpha.NetworkDNSPolicy{ + Mode: networkingv1alpha.NetworkDNSModeDisabled, + }}, + } + location := &locationsv1alpha1.Location{ObjectMeta: metav1.ObjectMeta{Name: "ord-1"}} + if region != "" { + location.Spec.Topology = map[string]string{internalDNSRegionTopologyKey: region} + } + f := &dnsOffFixture{access: newInternalDNSObject(internalDNSAccessGVK, "tenant", internalDNSRegionalName("resolver", "us-central"))} + f.access.SetAnnotations(map[string]string{internalDNSOwnerNetworkUIDAnnotation: "network-uid"}) + f.cl = interceptor.NewClient(fake.NewClientBuilder().WithScheme(scheme). + WithObjects(network, location, networkContext.DeepCopy(), f.access.DeepCopy()).Build(), interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + f.count(obj) + return c.Patch(ctx, obj, patch, opts...) + }, + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + f.count(obj) + return c.Update(ctx, obj, opts...) + }, + }) + return f +} + +func (f *dnsOffFixture) count(obj client.Object) { + if _, ok := obj.(*networkingv1alpha.NetworkContext); ok { + f.writes++ + } +} + +func (f *dnsOffFixture) reconcile(t *testing.T, ctx context.Context, key client.ObjectKey) { + t.Helper() + r := &InternalDNSProjectReconciler{ + Config: config.InternalDNSConfig{Frontend: "fd53::53", ServicePrefix: "fd70:100::/64"}, + Projects: fixedProjectIdentity("project-uid"), + } + if _, err := r.reconcile(ctx, f.cl, "project", key); err != nil { + t.Fatal(err) + } +} + +// assertDNSOff fails unless the context carries no DNS intent and no internal +// DNS finalizer, and, when it had used one, its regional access is gone. +func (f *dnsOffFixture) assertDNSOff(t *testing.T, ctx context.Context, key client.ObjectKey, usedAccess bool) { + t.Helper() + current := &networkingv1alpha.NetworkContext{} + if err := f.cl.Get(ctx, key, current); err != nil { + t.Fatal(err) + } + for _, annotation := range internalDNSIntentAnnotations() { + if _, ok := current.Annotations[annotation]; ok { + t.Errorf("annotation %s stayed on a context whose DNS is off", annotation) + } + } + if slices.Contains(current.Finalizers, internalDNSFinalizer) { + t.Error("the internal DNS finalizer stayed on a context whose DNS is off") + } + err := f.cl.Get(ctx, client.ObjectKeyFromObject(f.access), f.access.DeepCopy()) + if usedAccess && !apierrors.IsNotFound(err) { + t.Errorf("the regional access the context used survived DNS turning off: %v", err) + } +}