atenet/dns: publish the router's IPv6 ClusterIP as an AAAA

cmd/atenet/internal/dns/README.md

@@ -10,7 +10,6 @@ Cluster resources:
 
 * Deployment `ate-system:dns`. Label: app=dns
 * Service `ate-system:dns`.
-* ConfigMap `ate-system:dns`.
 
 These are defined in manifests/ate-install/atenet-dns.yaml.
 
@@ -20,16 +19,53 @@ These are defined in manifests/ate-install/atenet-dns.yaml.
   * Deployment `ate-system:dns`.
   * Service `ate-system:dns` pointing to the Deployment.
 
-ConfigMap `ate-system:dns`:
+`corefile.go` renders the zone below; the controller writes it to
+`--corefile-path` on an emptyDir shared with the CoreDNS container and signals
+a reload. The excerpt is illustrative — `corefile.go` is authoritative, and
+`TestMakeCoreFile` pins the exact rendering for each family combination.
 
 ```
-# Match any 'A' query for an actor name + atespace pattern under actors.resources.substrate.ate.dev
+# Answer any 'A' query for an actor name + atespace pattern under actors.resources.substrate.ate.dev
     template IN A actors.resources.substrate.ate.dev {
         match "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.actors\\.resources\\.substrate\\.ate\\.dev\\.$"
-        answer "{{ .Name }} 60 IN A <router service address>"
+        answer "{{ .Name }} 60 IN A <router service IPv4 ClusterIP>"
+        fallthrough
+    }
+# The same for 'AAAA', when the router Service has an IPv6 ClusterIP.
+    template IN AAAA actors.resources.substrate.ate.dev {
+        match "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.actors\\.resources\\.substrate\\.ate\\.dev\\.$"
+        answer "{{ .Name }} 60 IN AAAA <router service IPv6 ClusterIP>"
+        fallthrough
+    }
+# NODATA for a well-formed actor name on any other qtype (HTTPS, SRV, ...), and
+# for the family the router has no ClusterIP in.
+    template ANY ANY actors.resources.substrate.ate.dev {
+        match "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.actors\\.resources\\.substrate\\.ate\\.dev\\.$"
+        rcode NOERROR
+        authority "{{ .Zone }} 60 IN SOA ns.dns.{{ .Zone }} hostmaster.{{ .Zone }} (1 60 60 60 60)"
+        fallthrough
+    }
+# Terminal catch-all: NXDOMAIN for anything else in the zone.
+    template ANY ANY actors.resources.substrate.ate.dev {
+        rcode NXDOMAIN
+        authority "{{ .Zone }} 60 IN SOA ns.dns.{{ .Zone }} hostmaster.{{ .Zone }} (1 60 60 60 60)"
     }
 ```
 
+An address block is emitted only for a family the atenet-router Service
+actually has a ClusterIP in, which on any cluster where `ipFamilyPolicy` is
+unset means exactly one of the two. That is not tidiness: the `answer` line is a
+literal RR, so an `IN A` carrying an IPv6 address parses fine as a Corefile and
+then fails `dns.NewRR` on every query, SERVFAILing the whole zone. Leaving the
+family out hands it to the NODATA block instead, which is the right answer for a
+name with no address of that type.
+
+The last two blocks keep the zone from ever answering SERVFAIL, which musl libc
+maps to `EAI_AGAIN` — sinking the paired A query with it — and which cannot be
+cached negatively. The `fallthrough` on every block that carries a `match` is
+load-bearing: the template plugin walks past a class or qtype mismatch on its
+own, but a regex miss returns SERVFAIL immediately unless the block declares it.
+
 ## Integration
 
 * CoreDNS: Update CoreDNS ConfigMap to add the stub resolver.

cmd/atenet/internal/dns/corefile.go

@@ -22,15 +22,31 @@ import (
 	"github.com/agent-substrate/substrate/internal/resources"
 )
 
-// corefileTemplate is a Sprintf template for the CoreDNS configuration.
-var corefileTemplate string
+const (
+	fallthroughDirective = "  fallthrough"
+	soaDirective         = `  authority "{{ .Zone }} 60 IN SOA ns.dns.{{ .Zone }} hostmaster.{{ .Zone }} (1 60 60 60 60)"`
+)
 
-func init() {
-	corefileTemplate = buildTemplate()
-}
+// generatedAt stamps the rendered Corefile once per process, and must not be
+// recomputed per call: reconcileCoreDNSConfig decides whether to rewrite the
+// file and signal CoreDNS by comparing the render against what is on disk, so a
+// moving timestamp would reload the server on every tick of the reconcile loop.
+var generatedAt = time.Now()
 
-func buildTemplate() string {
-	// Build up the corefileTemplate programmatically to make it easier to understand.
+// makeCoreFile renders the actor zone for the router Service's ClusterIPs.
+//
+// A family gets an address template only when the router actually has an
+// address in it. That is not an optimization: an address template is a literal
+// RR, so emitting `IN A <v6 address>` on a v6-only cluster produces a Corefile
+// that loads clean and then fails dns.NewRR on every query, turning the whole
+// zone into SERVFAIL. Omitting the block instead leaves the family to the
+// NODATA template below, which is the correct answer for a name with no address
+// of that type.
+//
+// Either argument may be empty, and on any cluster where ipFamilyPolicy is
+// unset exactly one of them will be.
+func makeCoreFile(routerV4, routerV6 string) string {
+	// Build up the Corefile programmatically to make it easier to understand.
 	var directives []string
 	// Plugins to enable.
 	directives = append(directives, "log")
@@ -41,24 +57,54 @@ func buildTemplate() string {
 
 	// Construct match pattern for <ActorName>.<atespace>.<dnsDomain>. Both the
 	// actor name and the atespace are DNS-1123 labels (same regex).
-	directives = append(directives, fmt.Sprintf("template IN A %s {", resources.ActorDNSSuffix))
 	// Escape the suffix's dots so they match literally; the final \. matches the FQDN's trailing dot.
 	escapedSuffix := strings.ReplaceAll(resources.ActorDNSSuffix, ".", `\.`)
-	directives = append(directives, fmt.Sprintf(`  match "^%s\.%s\.%s\.$"`, resources.ResourceNameRegexPattern, resources.ResourceNameRegexPattern, escapedSuffix))
-	// Note the %s -- this will be filled with the router IP.
-	directives = append(directives, `  answer "{{ .Name }} 60 IN A %s"`)
+	actorMatch := fmt.Sprintf(`  match "^%s\.%s\.%s\.$"`, resources.ResourceNameRegexPattern, resources.ResourceNameRegexPattern, escapedSuffix)
+
+	if routerV4 != "" {
+		directives = append(directives, addressTemplate("A", routerV4, actorMatch)...)
+	}
+	if routerV6 != "" {
+		directives = append(directives, addressTemplate("AAAA", routerV6, actorMatch)...)
+	}
+
+	// Valid actor names return NOERROR (NODATA) for the qtypes not answered
+	// above, which includes the family the router has no address in.
+	directives = append(directives, fmt.Sprintf("template ANY ANY %s {", resources.ActorDNSSuffix))
+	directives = append(directives, actorMatch)
+	directives = append(directives, "  rcode NOERROR")
+	directives = append(directives, soaDirective)
+	directives = append(directives, fallthroughDirective)
+	directives = append(directives, "}")
+
+	// Returns rcode NXDOMAIN (Non-Existent Domain) for any query that did not
+	// match the valid actor regex in the previous blocks.
+	// TODO(#922): answer empty non-terminals with NODATA.
+	directives = append(directives, fmt.Sprintf("template ANY ANY %s {", resources.ActorDNSSuffix))
+	directives = append(directives, "  rcode NXDOMAIN")
+	directives = append(directives, soaDirective)
 	directives = append(directives, "}")
 
-	// Generate the template.
+	// Generate the Corefile.
 	b := strings.Builder{}
-	fmt.Fprintf(&b, "# Generated at %s\n", time.Now())
+	fmt.Fprintf(&b, "# Generated at %s\n", generatedAt)
 	fmt.Fprintf(&b, "%s:53 {\n  ", resources.ActorDNSSuffix)
 	fmt.Fprint(&b, strings.Join(directives, "\n  "))
 	fmt.Fprint(&b, "\n}\n")
 
 	return b.String()
 }
 
-func makeCoreFile(routerIP string) string {
-	return fmt.Sprintf(corefileTemplate, routerIP)
+// addressTemplate returns the template block that answers qtype ("A" or "AAAA")
+// for actor names with addr. addr is interpolated into an RR verbatim, so it
+// must already be known to be an address of that family -- see
+// ipfamily.ClusterIPsByFamily, which is where callers get it.
+func addressTemplate(qtype, addr, actorMatch string) []string {
+	return []string{
+		fmt.Sprintf("template IN %s %s {", qtype, resources.ActorDNSSuffix),
+		actorMatch,
+		fmt.Sprintf(`  answer "{{ .Name }} 60 IN %s %s"`, qtype, addr),
+		fallthroughDirective,
+		"}",
+	}
 }

cmd/atenet/internal/dns/corefile_test.go

@@ -17,49 +17,147 @@ package dns
 import (
 	"strings"
 	"testing"
+)
+
+// actorMatchDirective is the match line every template that scopes itself to
+// real actor names carries; soaAuthorityDirective is the record that makes the
+// negative answers cacheable. Both are spelled out rather than built from
+// resources.ResourceNameRegexPattern and ActorDNSSuffix: the rendered zone is a
+// wire contract, so a change to either constant should fail here instead of
+// being tracked silently.
+const (
+	actorMatchDirective   = `match "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\.[a-z0-9]([-a-z0-9]*[a-z0-9])?\.actors\.resources\.substrate\.ate\.dev\.$"`
+	soaAuthorityDirective = `authority "{{ .Zone }} 60 IN SOA ns.dns.{{ .Zone }} hostmaster.{{ .Zone }} (1 60 60 60 60)"`
+)
+
+// The zones below are compared whole rather than by substring because every
+// part of them is behavior: templates are evaluated in Corefile order, every
+// block carrying a "match" needs a "fallthrough" to reach the blocks after it,
+// the catch-all must be last and must not declare one, and the indentation has
+// to parse. See README.md for what the template plugin does with each.
+//
+// The head and tail are shared to keep the four goldens readable. That does not
+// weaken the ordering assertion: each golden is still the whole expected file,
+// with the address blocks spelled out between them.
+const (
+	wantZoneHead = `actors.resources.substrate.ate.dev:53 {
+  log
+  errors
+  health :8080
+  ready :8181
+  reload
+`
+	wantZoneTail = `  template ANY ANY actors.resources.substrate.ate.dev {
+    ` + actorMatchDirective + `
+    rcode NOERROR
+    ` + soaAuthorityDirective + `
+    fallthrough
+  }
+  template ANY ANY actors.resources.substrate.ate.dev {
+    rcode NXDOMAIN
+    ` + soaAuthorityDirective + `
+  }
+}
+`
+)
+
+const (
+	wantZoneIPv4 = wantZoneHead + `  template IN A actors.resources.substrate.ate.dev {
+    ` + actorMatchDirective + `
+    answer "{{ .Name }} 60 IN A 10.240.0.10"
+    fallthrough
+  }
+` + wantZoneTail
+
+	wantZoneIPv6 = wantZoneHead + `  template IN AAAA actors.resources.substrate.ate.dev {
+    ` + actorMatchDirective + `
+    answer "{{ .Name }} 60 IN AAAA fd00:10:96::8857"
+    fallthrough
+  }
+` + wantZoneTail
+
+	wantZoneDualStack = wantZoneHead + `  template IN A actors.resources.substrate.ate.dev {
+    ` + actorMatchDirective + `
+    answer "{{ .Name }} 60 IN A 10.96.233.69"
+    fallthrough
+  }
+  template IN AAAA actors.resources.substrate.ate.dev {
+    ` + actorMatchDirective + `
+    answer "{{ .Name }} 60 IN AAAA fd00:10:96::7373"
+    fallthrough
+  }
+` + wantZoneTail
 
-	"github.com/agent-substrate/substrate/internal/resources"
+	wantZoneNoAddresses = wantZoneHead + wantZoneTail
 )
 
+// zoneBody strips the "# Generated at <timestamp>" header.
+func zoneBody(t *testing.T, corefile string) string {
+	t.Helper()
+	header, body, ok := strings.Cut(corefile, "\n")
+	if !ok || !strings.HasPrefix(header, "# Generated at ") {
+		t.Fatalf("makeCoreFile() has no generated-at header, got first line %q", header)
+	}
+	return body
+}
+
 func TestMakeCoreFile(t *testing.T) {
 	tests := []struct {
 		name     string
-		routerIP string
-		expected []string
+		routerV4 string
+		routerV6 string
+		want     string
 	}{
 		{
-			name:     "standard local IP",
-			routerIP: "10.240.0.10",
-			expected: []string{
-				"actors.resources.substrate.ate.dev:53 {",
-				"log",
-				"errors",
-				"health :8080",
-				"ready :8181",
-				"reload",
-				"template IN A actors.resources.substrate.ate.dev {",
-				`match "^` + resources.ResourceNameRegexPattern + `\.` + resources.ResourceNameRegexPattern + `\.actors\.resources\.substrate\.ate\.dev\.$"`,
-				`answer "{{ .Name }} 60 IN A 10.240.0.10"`,
-			},
+			// AAAA is left to the NODATA template in the tail, which is the
+			// right answer for a name with no address of that type. Publishing
+			// the v4 ClusterIP as an AAAA instead would render a literal RR that
+			// loads clean and then fails dns.NewRR on every query.
+			name:     "IPv4 only",
+			routerV4: "10.240.0.10",
+			want:     wantZoneIPv4,
+		},
+		{
+			// The bug this change exists for: a v6-only cluster's sole ClusterIP
+			// used to be published as an A record, SERVFAILing the whole zone.
+			name:     "IPv6 only",
+			routerV6: "fd00:10:96::8857",
+			want:     wantZoneIPv6,
 		},
 		{
-			name:     "different IP",
-			routerIP: "192.168.1.1",
-			expected: []string{
-				"actors.resources.substrate.ate.dev:53 {",
-				`answer "{{ .Name }} 60 IN A 192.168.1.1"`,
-			},
+			name:     "dual stack",
+			routerV4: "10.96.233.69",
+			routerV6: "fd00:10:96::7373",
+			want:     wantZoneDualStack,
+		},
+		{
+			// The controller does not call makeCoreFile in this state, but the
+			// zone still has to be a loadable Corefile if it ever does: negative
+			// answers only, never a template with an empty address in it.
+			name: "no addresses",
+			want: wantZoneNoAddresses,
 		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			got := makeCoreFile(tc.routerIP)
-			for _, exp := range tc.expected {
-				if !strings.Contains(got, exp) {
-					t.Errorf("makeCoreFile(%q) missing expected substring %q\nGot:\n%s", tc.routerIP, exp, got)
-				}
+			got := zoneBody(t, makeCoreFile(tc.routerV4, tc.routerV6))
+			if got != tc.want {
+				t.Errorf("makeCoreFile(%q, %q) rendered an unexpected Corefile\nGot:\n%s\nWant:\n%s", tc.routerV4, tc.routerV6, got, tc.want)
 			}
 		})
 	}
 }
+
+// TestMakeCoreFileStable pins the property that keeps the reconcile loop quiet:
+// the render depends only on its arguments. reconcileCoreDNSConfig rewrites the
+// Corefile and signals CoreDNS whenever the render differs from what is on
+// disk, so anything time-varying in the output -- the "Generated at" stamp, in
+// particular -- would reload the DNS server on every tick.
+func TestMakeCoreFileStable(t *testing.T) {
+	first := makeCoreFile("10.240.0.10", "fd00:10:96::8857")
+	second := makeCoreFile("10.240.0.10", "fd00:10:96::8857")
+	if first != second {
+		t.Errorf("makeCoreFile() is not stable across calls; the reconcile loop would rewrite and reload every tick\nFirst:\n%s\nSecond:\n%s", first, second)
+	}
+}