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) + } +}