Merge pull request 'Sort render inputs so config is deterministic (stop restart loop)' (#11) from benvin/deterministic-render into main

Reviewed-on: #11

internal/bind/render.go

@@ -28,9 +28,28 @@ type RenderInput struct {
 // cluster. Both variants are shipped in the ConfigMap; the entrypoint selects
 // one based on the pod ordinal.
 func RenderNamedConf(in RenderInput) (primary string, secondary string) {
+	// client.List returns cache-ordered (non-deterministic) results, so sort
+	// every input slice before rendering. Otherwise the rendered config
+	// reshuffles between reconciles, churning the ConfigMap — and with the
+	// pod-template config hash that means an endless rolling restart.
+	sortInput(&in)
 	return render(in, true), render(in, false)
 }
 
+// sortInput orders every list rendered into named.conf deterministically.
+func sortInput(in *RenderInput) {
+	sort.Slice(in.ACLs, func(i, j int) bool { return in.ACLs[i].Name < in.ACLs[j].Name })
+	sort.Slice(in.Views, func(i, j int) bool {
+		if in.Views[i].Spec.Order != in.Views[j].Spec.Order {
+			return in.Views[i].Spec.Order < in.Views[j].Spec.Order
+		}
+		return in.Views[i].Name < in.Views[j].Name
+	})
+	sort.Slice(in.Forwards, func(i, j int) bool { return in.Forwards[i].Spec.ZoneName < in.Forwards[j].Spec.ZoneName })
+	sort.Slice(in.Policies, func(i, j int) bool { return in.Policies[i].Spec.ZoneName < in.Policies[j].Spec.ZoneName })
+	sort.Slice(in.DNSSECPolicies, func(i, j int) bool { return in.DNSSECPolicies[i].Name < in.DNSSECPolicies[j].Name })
+}
+
 func render(in RenderInput, isPrimary bool) string {
 	c := in.Cluster
 	var b strings.Builder
@@ -230,15 +249,40 @@ func responsePolicyClause(policies []bindv1alpha1.BindPolicy, indent string) str
 	return b.String()
 }
 
+// transferPrimaries returns the primaries list secondaries use to AXFR the
+// catalog (and, by inheritance, its member zones), each annotated with the
+// catalog transfer TSIG key. The primary requires key-authenticated transfers
+// (allow-transfer { key ... }), so an unkeyed primaries list is REFUSED.
+func transferPrimaries(in RenderInput) []string {
+	primaries := in.Catalog.Spec.DefaultPrimaries
+	if len(primaries) == 0 && in.PrimaryAddress != "" {
+		primaries = []string{in.PrimaryAddress}
+	}
+	key := in.Catalog.Spec.TransferKeyRef
+	if key == "" {
+		return primaries
+	}
+	out := make([]string, 0, len(primaries))
+	for _, p := range primaries {
+		p = strings.TrimSpace(strings.TrimRight(p, ";"))
+		if p == "" {
+			continue
+		}
+		if strings.Contains(p, " key ") {
+			out = append(out, p)
+		} else {
+			out = append(out, fmt.Sprintf("%s key \"%s\"", p, key))
+		}
+	}
+	return out
+}
+
 func catalogZonesClause(in RenderInput, isPrimary bool, indent string) string {
 	// Only secondaries consume the catalog to auto-provision member zones.
 	if in.Catalog == nil || isPrimary {
 		return ""
 	}
-	primaries := in.Catalog.Spec.DefaultPrimaries
-	if len(primaries) == 0 && in.PrimaryAddress != "" {
-		primaries = []string{in.PrimaryAddress}
-	}
+	primaries := transferPrimaries(in)
 	if len(primaries) == 0 {
 		return ""
 	}
@@ -259,10 +303,7 @@ func renderCatalogZoneDecl(in RenderInput, isPrimary bool, indent string) string
 	}
 	cat := in.Catalog
 	file := CatalogFilePath(cat.Spec.ZoneName)
-	primaries := cat.Spec.DefaultPrimaries
-	if len(primaries) == 0 && in.PrimaryAddress != "" {
-		primaries = []string{in.PrimaryAddress}
-	}
+	primaries := transferPrimaries(in)
 	if len(primaries) == 0 {
 		// Primary IP not known yet; omit the secondary catalog zone rather than
 		// emit an invalid empty primaries list. A Pod-triggered reconcile renders

internal/bind/render_test.go

@@ -80,6 +80,24 @@ func TestRenderCatalogUsesPrimaryIP(t *testing.T) {
 	}
 }
 
+func TestRenderCatalogPrimariesCarryTransferKey(t *testing.T) {
+	// When the catalog declares a transfer key, secondaries must present it in
+	// both the catalog-zones default-primaries and the secondary catalog zone,
+	// or the key-authenticated primary REFUSES the AXFR.
+	in := RenderInput{
+		Cluster:        newCluster(bindv1alpha1.ModeAuthoritative),
+		Catalog:        &bindv1alpha1.BindCatalogZone{Spec: bindv1alpha1.BindCatalogZoneSpec{ZoneName: "catalog.internal", TransferKeyRef: "transfer-key"}},
+		PrimaryAddress: "10.43.0.5",
+	}
+	_, secondary := RenderNamedConf(in)
+	if !strings.Contains(secondary, `default-primaries { 10.43.0.5 key "transfer-key"; }`) {
+		t.Fatalf("catalog-zones default-primaries must carry the transfer key:\n%s", secondary)
+	}
+	if !strings.Contains(secondary, `primaries { 10.43.0.5 key "transfer-key"; }`) {
+		t.Fatalf("secondary catalog zone primaries must carry the transfer key:\n%s", secondary)
+	}
+}
+
 func TestRenderForwardZoneInView(t *testing.T) {
 	rec := true
 	in := RenderInput{
@@ -128,3 +146,34 @@ func TestCatalogHashStable(t *testing.T) {
 		t.Fatalf("expected 40-char hex sha1, got %d: %s", len(h1), h1)
 	}
 }
+
+func TestRenderDeterministicWithShuffledForwards(t *testing.T) {
+	// client.List order is non-deterministic; the render must not depend on
+	// input order, or the ConfigMap churns and (with the config hash) the
+	// StatefulSet rolls forever.
+	mkFwd := func(zone, fwd string) bindv1alpha1.BindZone {
+		return bindv1alpha1.BindZone{
+			ObjectMeta: metav1.ObjectMeta{Name: zone},
+			Spec: bindv1alpha1.BindZoneSpec{
+				ClusterRef: "r", Type: bindv1alpha1.ZoneForward,
+				ZoneName: zone, Forwarders: []string{fwd},
+			},
+		}
+	}
+	base := RenderInput{Cluster: newCluster(bindv1alpha1.ModeResolver)}
+	orderA := base
+	orderA.Forwards = []bindv1alpha1.BindZone{
+		mkFwd("unkin.net", "198.18.200.6"), mkFwd("consul", "198.18.19.14"),
+		mkFwd("k8s.syd1.au.unkin.net", "198.18.200.8"), mkFwd("13.18.198.in-addr.arpa", "198.18.200.6"),
+	}
+	orderB := base
+	orderB.Forwards = []bindv1alpha1.BindZone{
+		mkFwd("13.18.198.in-addr.arpa", "198.18.200.6"), mkFwd("k8s.syd1.au.unkin.net", "198.18.200.8"),
+		mkFwd("consul", "198.18.19.14"), mkFwd("unkin.net", "198.18.200.6"),
+	}
+	pa, _ := RenderNamedConf(orderA)
+	pb, _ := RenderNamedConf(orderB)
+	if pa != pb {
+		t.Fatalf("render must be independent of forward-zone input order:\n--- A ---\n%s\n--- B ---\n%s", pa, pb)
+	}
+}

internal/controller/bindcluster_controller.go

@@ -1,7 +1,10 @@
 package controller
 
 import (
+	"bytes"
 	"context"
+	"crypto/sha256"
+	"encoding/hex"
 	"fmt"
 	"sort"
 	"strings"
@@ -168,10 +171,11 @@ func (r *BindClusterReconciler) reconcileKeysSecret(ctx context.Context, c *bind
 }
 
 func (r *BindClusterReconciler) reconcileConfigMap(ctx context.Context, c *bindv1alpha1.BindCluster) error {
-	// BIND primaries/default-primaries need the primary's IP address, not a DNS
-	// name, so render with pod-0's current IP (empty until it is scheduled; the
-	// Pod watch re-renders when it appears or changes).
-	in := bind.RenderInput{Cluster: c, PrimaryAddress: primaryPodIP(ctx, r.Client, c)}
+	// BIND primaries/default-primaries need an IP address, not a DNS name. Use
+	// the stable primary Service ClusterIP so secondaries keep transferring
+	// across primary pod restarts (falls back to the pod IP when no primary
+	// Service exists; the Pod/Service watches re-render when it changes).
+	in := bind.RenderInput{Cluster: c, PrimaryAddress: primaryTransferAddress(ctx, r.Client, c)}
 
 	var acls bindv1alpha1.BindACLList
 	if err := r.List(ctx, &acls, client.InNamespace(c.Namespace)); err == nil {
@@ -350,6 +354,12 @@ func (r *BindClusterReconciler) reconcileStatefulSet(ctx context.Context, c *bin
 		}}},
 	}
 
+	// Pods copy config from the projected volume into an emptyDir once at
+	// startup; a ConfigMap/keys change never reaches a running pod (rndc
+	// reconfig re-reads the stale startup copy). Stamp a hash of the projected
+	// config onto the pod template so a config change rolls the StatefulSet,
+	// which is the only way the change takes effect. The operator owns the
+	// template, so this restart is operator-driven and not reverted.
 	sts := &appsv1.StatefulSet{
 		ObjectMeta: metav1.ObjectMeta{Name: c.Name, Namespace: c.Namespace, Labels: labels},
 		Spec: appsv1.StatefulSetSpec{
@@ -357,7 +367,7 @@ func (r *BindClusterReconciler) reconcileStatefulSet(ctx context.Context, c *bin
 			Replicas:    &replicas,
 			Selector:    &metav1.LabelSelector{MatchLabels: labels},
 			Template: corev1.PodTemplateSpec{
-				ObjectMeta: metav1.ObjectMeta{Labels: labels},
+				ObjectMeta: metav1.ObjectMeta{Labels: labels, Annotations: map[string]string{configHashAnnotation: r.configHash(ctx, c)}},
 				Spec: corev1.PodSpec{
 					NodeSelector: c.Spec.NodeSelector,
 					Tolerations:  c.Spec.Tolerations,
@@ -425,6 +435,45 @@ func (r *BindClusterReconciler) reconcileStatefulSet(ctx context.Context, c *bin
 	return &existing, nil
 }
 
+// configHashAnnotation carries a hash of the projected config on the pod
+// template; changing it triggers a rolling restart so pods pick up new config.
+const configHashAnnotation = "bind.unkin.net/config-hash"
+
+// configHash returns a deterministic hash of the config projected into the pods
+// (the rendered ConfigMap and the keys.conf Secret). It is read after those are
+// reconciled, so it reflects the current desired config. A stable hash means no
+// spurious restarts; any config or TSIG-key change flips it and rolls the pods.
+func (r *BindClusterReconciler) configHash(ctx context.Context, c *bindv1alpha1.BindCluster) string {
+	var buf bytes.Buffer
+	var cm corev1.ConfigMap
+	if err := r.Get(ctx, types.NamespacedName{Namespace: c.Namespace, Name: configMapName(c.Name)}, &cm); err == nil {
+		for _, k := range sortedKeys(cm.Data) {
+			fmt.Fprintf(&buf, "%s\x00%s\x00", k, cm.Data[k])
+		}
+	}
+	var keys corev1.Secret
+	if err := r.Get(ctx, types.NamespacedName{Namespace: c.Namespace, Name: keysSecretName(c.Name)}, &keys); err == nil {
+		data := make(map[string]string, len(keys.Data))
+		for k, v := range keys.Data {
+			data[k] = string(v)
+		}
+		for _, k := range sortedKeys(data) {
+			fmt.Fprintf(&buf, "%s\x00%s\x00", k, data[k])
+		}
+	}
+	sum := sha256.Sum256(buf.Bytes())
+	return hex.EncodeToString(sum[:])
+}
+
+func sortedKeys(m map[string]string) []string {
+	keys := make([]string, 0, len(m))
+	for k := range m {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	return keys
+}
+
 func (r *BindClusterReconciler) reloadReadyPods(ctx context.Context, c *bindv1alpha1.BindCluster) {
 	if r.Exec == nil {
 		return

internal/controller/bindzone_controller.go

@@ -80,7 +80,7 @@ func (r *BindZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		return r.setPhase(ctx, &zone, "Pending", "PrimaryNotReady", "waiting for cluster primary to be ready")
 	}
 
-	zoneConfig, err := r.buildZoneConfig(ctx, &zone)
+	zoneConfig, err := r.buildZoneConfig(ctx, &zone, r.zoneTransferKeyRef(ctx, &zone, cluster))
 	if err != nil {
 		return r.setPhase(ctx, &zone, "Error", "ConfigError", err.Error())
 	}
@@ -133,7 +133,9 @@ func (r *BindZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 }
 
 // buildZoneConfig renders the inner clause passed to rndc addzone/modzone.
-func (r *BindZoneReconciler) buildZoneConfig(ctx context.Context, zone *bindv1alpha1.BindZone) (string, error) {
+// transferKey, when set, is the catalog transfer TSIG key name; catalog member
+// primary zones must allow AXFR with it so secondaries can pull them.
+func (r *BindZoneReconciler) buildZoneConfig(ctx context.Context, zone *bindv1alpha1.BindZone, transferKey string) (string, error) {
 	zType := zone.Spec.Type
 	if zType == "" {
 		zType = bindv1alpha1.ZonePrimary
@@ -145,8 +147,12 @@ func (r *BindZoneReconciler) buildZoneConfig(ctx context.Context, zone *bindv1al
 		if zone.Spec.DynamicUpdate && zone.Spec.UpdateKeyRef != "" {
 			parts = append(parts, fmt.Sprintf("allow-update { key \"%s\"; }", updateKeyName(ctx, r.Client, zone)))
 		}
-		if len(zone.Spec.AllowTransfer) > 0 {
+		switch {
+		case len(zone.Spec.AllowTransfer) > 0:
 			parts = append(parts, fmt.Sprintf("allow-transfer { %s }", matchListInline(zone.Spec.AllowTransfer)))
+		case transferKey != "":
+			// Catalog member: permit key-authenticated AXFR from secondaries.
+			parts = append(parts, fmt.Sprintf("allow-transfer { key \"%s\"; }", transferKey))
 		}
 		if zone.Spec.DNSSECPolicyRef != "" {
 			parts = append(parts, fmt.Sprintf("dnssec-policy \"%s\"", zone.Spec.DNSSECPolicyRef), "inline-signing yes")
@@ -202,6 +208,26 @@ func (r *BindZoneReconciler) deregisterCatalog(ctx context.Context, zone *bindv1
 	_ = r.Exec.RemoveCatalogMember(ctx, zone.Namespace, primaryPod, catalog.Spec.ZoneName, zone.Spec.ZoneName, creds)
 }
 
+// zoneTransferKeyRef returns the catalog transfer TSIG key name that a catalog
+// member primary zone must allow AXFR with, so secondaries (which present that
+// key) can pull it. Returns "" for non-member zones, non-primary zones, or when
+// the cluster has no catalog.
+func (r *BindZoneReconciler) zoneTransferKeyRef(ctx context.Context, zone *bindv1alpha1.BindZone, cluster *bindv1alpha1.BindCluster) string {
+	if !isPrimaryType(zone.Spec.Type) || !catalogEnabled(zone) {
+		return ""
+	}
+	var catalogs bindv1alpha1.BindCatalogZoneList
+	if err := r.List(ctx, &catalogs, client.InNamespace(zone.Namespace)); err != nil {
+		return ""
+	}
+	for i := range catalogs.Items {
+		if catalogs.Items[i].Spec.ClusterRef == cluster.Name {
+			return catalogs.Items[i].Spec.TransferKeyRef
+		}
+	}
+	return ""
+}
+
 func (r *BindZoneReconciler) catalogFor(ctx context.Context, zone *bindv1alpha1.BindZone, cluster *bindv1alpha1.BindCluster) (*bindv1alpha1.BindCatalogZone, bind.TSIGCreds, bool) {
 	var catalogs bindv1alpha1.BindCatalogZoneList
 	if err := r.List(ctx, &catalogs, client.InNamespace(zone.Namespace)); err != nil {

internal/controller/config_hash_test.go

@@ -0,0 +1,64 @@
+package controller
+
+import (
+	"context"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	bindv1alpha1 "git.unkin.net/unkin/bind-operator/api/v1alpha1"
+)
+
+func TestConfigHashStableAndSensitive(t *testing.T) {
+	scheme := runtime.NewScheme()
+	if err := clientgoscheme.AddToScheme(scheme); err != nil {
+		t.Fatal(err)
+	}
+	if err := bindv1alpha1.AddToScheme(scheme); err != nil {
+		t.Fatal(err)
+	}
+	cluster := &bindv1alpha1.BindCluster{ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "ns"}}
+	cm := &corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{Name: configMapName("c"), Namespace: "ns"},
+		Data:       map[string]string{"named.conf.secondary": "options { recursion yes; };"},
+	}
+	keys := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{Name: keysSecretName("c"), Namespace: "ns"},
+		Data:       map[string][]byte{"keys.conf": []byte("key x {};")},
+	}
+	c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cm, keys).Build()
+	r := &BindClusterReconciler{Client: c, Scheme: scheme}
+	ctx := context.Background()
+
+	h1 := r.configHash(ctx, cluster)
+	if h1 == "" {
+		t.Fatal("hash should not be empty when config exists")
+	}
+	// Stable across calls when nothing changes.
+	if h2 := r.configHash(ctx, cluster); h2 != h1 {
+		t.Fatalf("hash not stable: %s != %s", h1, h2)
+	}
+
+	// A config change flips the hash (this is what rolls the StatefulSet).
+	cm.Data["named.conf.secondary"] = "options { recursion yes; validate-except { unkin.net; }; };"
+	if err := c.Update(ctx, cm); err != nil {
+		t.Fatal(err)
+	}
+	if h3 := r.configHash(ctx, cluster); h3 == h1 {
+		t.Fatal("hash must change when the ConfigMap changes")
+	}
+
+	// A TSIG key (keys.conf) change also flips it.
+	afterCM := r.configHash(ctx, cluster)
+	keys.Data["keys.conf"] = []byte("key x { algorithm hmac-sha256; };")
+	if err := c.Update(ctx, keys); err != nil {
+		t.Fatal(err)
+	}
+	if h4 := r.configHash(ctx, cluster); h4 == afterCM {
+		t.Fatal("hash must change when keys.conf changes")
+	}
+}

internal/controller/helpers.go

@@ -108,6 +108,24 @@ func primaryPodIP(ctx context.Context, c client.Client, cluster *bindv1alpha1.Bi
 	return pod.Status.PodIP
 }
 
+// primaryTransferAddress returns the address secondaries use to reach the
+// primary for catalog and zone AXFR. It prefers the primary Service ClusterIP,
+// which is stable across primary pod restarts (the pod IP is not: it changes on
+// every restart, leaving secondaries pointed at a dead address). It falls back
+// to the primary pod IP when no primary Service is configured or its ClusterIP
+// is not yet assigned.
+func primaryTransferAddress(ctx context.Context, c client.Client, cluster *bindv1alpha1.BindCluster) string {
+	if cluster.Spec.PrimaryService != nil {
+		var svc corev1.Service
+		if err := c.Get(ctx, client.ObjectKey{Namespace: cluster.Namespace, Name: primaryServiceName(cluster.Name)}, &svc); err == nil {
+			if ip := svc.Spec.ClusterIP; ip != "" && ip != corev1.ClusterIPNone {
+				return ip
+			}
+		}
+	}
+	return primaryPodIP(ctx, c, cluster)
+}
+
 // resolveTSIG reads the material of a BindTSIGKey into TSIG credentials.
 func resolveTSIG(ctx context.Context, c client.Client, namespace, keyRef string) (bind.TSIGCreds, error) {
 	var creds bind.TSIGCreds