From 8eaf2423bd999c9a6cc406b5c6b556c08ca0269d Mon Sep 17 00:00:00 2001 From: Yuan Gao Date: Tue, 18 Aug 2026 07:39:50 -0700 Subject: [PATCH 1/4] atenet/dns: answer non-A actor queries instead of SERVFAIL Before, the actor zone answered A queries and failed everything else -- AAAA for a valid actor, and any name in the zone that is not an actor. A failure reads as a temporary error rather than an answer, so clients retry it and then give up on the name; Alpine actors could not resolve each other at all, even on an IPv4-only cluster. After, those queries return a correct empty answer, and one that resolvers can cache. A unit test pins the whole rendered zone as a literal, so editing the name pattern or the suffix fails there rather than passing silently. --- cmd/atenet/internal/dns/README.md | 22 ++++++- cmd/atenet/internal/dns/corefile.go | 25 +++++++- cmd/atenet/internal/dns/corefile_test.go | 75 ++++++++++++++---------- 3 files changed, 87 insertions(+), 35 deletions(-) diff --git a/cmd/atenet/internal/dns/README.md b/cmd/atenet/internal/dns/README.md index a134e829ab..e89b06b62f 100644 --- a/cmd/atenet/internal/dns/README.md +++ b/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,33 @@ 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, rendered by `corefile.go`: ``` -# 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 " + fallthrough + } +# NODATA for a well-formed actor name on any other qtype (AAAA, HTTPS, SRV, ...). + 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)" } ``` +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. + ## Integration * CoreDNS: Update CoreDNS ConfigMap to add the stub resolver. diff --git a/cmd/atenet/internal/dns/corefile.go b/cmd/atenet/internal/dns/corefile.go index 0b301e7e29..2869e3a24d 100644 --- a/cmd/atenet/internal/dns/corefile.go +++ b/cmd/atenet/internal/dns/corefile.go @@ -30,6 +30,11 @@ func init() { } func buildTemplate() string { + const ( + fallthroughDirective = " fallthrough" + soaDirective = ` authority "{{ .Zone }} 60 IN SOA ns.dns.{{ .Zone }} hostmaster.{{ .Zone }} (1 60 60 60 60)"` + ) + // Build up the corefileTemplate programmatically to make it easier to understand. var directives []string // Plugins to enable. @@ -44,9 +49,27 @@ func buildTemplate() string { 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)) + actorMatch := fmt.Sprintf(` match "^%s\.%s\.%s\.$"`, resources.ResourceNameRegexPattern, resources.ResourceNameRegexPattern, escapedSuffix) + directives = append(directives, actorMatch) // Note the %s -- this will be filled with the router IP. directives = append(directives, ` answer "{{ .Name }} 60 IN A %s"`) + directives = append(directives, fallthroughDirective) + directives = append(directives, "}") + + // Valid actor names return NOERROR (NODATA) for non-A queries. + 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. diff --git a/cmd/atenet/internal/dns/corefile_test.go b/cmd/atenet/internal/dns/corefile_test.go index f13429e475..c8653ad7ef 100644 --- a/cmd/atenet/internal/dns/corefile_test.go +++ b/cmd/atenet/internal/dns/corefile_test.go @@ -15,50 +15,63 @@ package dns import ( + "fmt" "strings" "testing" - - "github.com/agent-substrate/substrate/internal/resources" ) +// 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 wantCorefileFmt = `actors.resources.substrate.ate.dev:53 { + log + errors + health :8080 + ready :8181 + reload + 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 %s" + fallthrough + } + 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 + } + 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)" + } +} +` + +// zoneBody strips the "# Generated at " 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 }{ - { - 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"`, - }, - }, - { - 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: "cluster IP", routerIP: "10.240.0.10"}, + {name: "different cluster IP", routerIP: "192.168.1.1"}, } 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.routerIP)) + want := fmt.Sprintf(wantCorefileFmt, tc.routerIP) + if got != want { + t.Errorf("makeCoreFile(%q) rendered an unexpected Corefile\nGot:\n%s\nWant:\n%s", tc.routerIP, got, want) } }) } From 86b6a8b539ca616206c939face713e75971df575 Mon Sep 17 00:00:00 2001 From: Yuan Gao Date: Fri, 7 Aug 2026 15:56:35 -0700 Subject: [PATCH 2/4] internal/ipfamily: add ClusterIPsByFamily Splits a Service's cluster IPs into its IPv4 and IPv6 entries, returning "" for a family the Service has no address in. No behavior change on its own -- nothing calls it until the AAAA change later in this series. It is shared rather than package-local because a Service with no ipFamilyPolicy is SingleStack, so one empty family is the steady state on every cluster, not an error, and each caller would otherwise have to decide that for itself. Unit tests cover single- and dual-stack Services and the unallocated and malformed cases. --- internal/ipfamily/ipfamily.go | 60 +++++++++++++++++++ internal/ipfamily/ipfamily_test.go | 95 ++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+) create mode 100644 internal/ipfamily/ipfamily.go create mode 100644 internal/ipfamily/ipfamily_test.go diff --git a/internal/ipfamily/ipfamily.go b/internal/ipfamily/ipfamily.go new file mode 100644 index 0000000000..a6a3641087 --- /dev/null +++ b/internal/ipfamily/ipfamily.go @@ -0,0 +1,60 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package ipfamily sorts Kubernetes addresses into their IP families. +package ipfamily + +import ( + "net/netip" + + corev1 "k8s.io/api/core/v1" +) + +// ClusterIPsByFamily splits a Service's cluster IPs into its IPv4 and IPv6 +// entries, returning "" for a family the Service does not have. +// +// A Service with no ipFamilyPolicy is SingleStack, so even on a dual-stack +// cluster it has exactly one ClusterIP and one of the two return values is +// empty. Callers must handle that: it is the steady state everywhere +// ipFamilyPolicy has not been set, and it is what distinguishes "this cluster +// has no address to offer in that family" from a misconfiguration. +// +// Spec.ClusterIPs is preferred over the singular Spec.ClusterIP, with a +// fallback to the latter because a Service built by hand (or by a fake client) +// may only set the scalar. Headless Services, and any entry that is not a +// parseable address, are skipped rather than returned. +func ClusterIPsByFamily(svc *corev1.Service) (v4, v6 string) { + ips := svc.Spec.ClusterIPs + if len(ips) == 0 && svc.Spec.ClusterIP != "" { + ips = []string{svc.Spec.ClusterIP} + } + for _, ip := range ips { + if ip == "" || ip == corev1.ClusterIPNone { + continue + } + // netip rather than net.IP: net.IP.To4 returns non-nil for a v4-mapped + // v6 address and would misfile it as IPv4. + addr, err := netip.ParseAddr(ip) + if err != nil { + continue + } + switch { + case addr.Is4() && v4 == "": + v4 = ip + case addr.Is6() && !addr.Is4In6() && v6 == "": + v6 = ip + } + } + return v4, v6 +} diff --git a/internal/ipfamily/ipfamily_test.go b/internal/ipfamily/ipfamily_test.go new file mode 100644 index 0000000000..9b60972534 --- /dev/null +++ b/internal/ipfamily/ipfamily_test.go @@ -0,0 +1,95 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package ipfamily + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" +) + +func TestClusterIPsByFamily(t *testing.T) { + tests := []struct { + name string + spec corev1.ServiceSpec + wantV4 string + wantV6 string + wantReason string + }{ + { + name: "single stack IPv4", + spec: corev1.ServiceSpec{ClusterIP: "10.96.0.10", ClusterIPs: []string{"10.96.0.10"}}, + wantV4: "10.96.0.10", + wantReason: "the default policy on an IPv4 cluster", + }, + { + name: "single stack IPv6", + spec: corev1.ServiceSpec{ClusterIP: "fd00:10:96::8857", ClusterIPs: []string{"fd00:10:96::8857"}}, + wantV6: "fd00:10:96::8857", + wantReason: "an IPv6-only cluster allocates a v6 ClusterIP with no ipFamilyPolicy set", + }, + { + name: "dual stack IPv4 primary", + spec: corev1.ServiceSpec{ClusterIP: "10.96.0.10", ClusterIPs: []string{"10.96.0.10", "fd00:10:96::8857"}}, + wantV4: "10.96.0.10", + wantV6: "fd00:10:96::8857", + wantReason: "both families are usable regardless of which one is primary", + }, + { + name: "dual stack IPv6 primary", + spec: corev1.ServiceSpec{ClusterIP: "fd00:10:96::8857", ClusterIPs: []string{"fd00:10:96::8857", "10.96.0.10"}}, + wantV4: "10.96.0.10", + wantV6: "fd00:10:96::8857", + wantReason: "the order of ClusterIPs is the family preference, not a family label", + }, + { + name: "scalar only", + spec: corev1.ServiceSpec{ClusterIP: "10.96.0.10"}, + wantV4: "10.96.0.10", + wantReason: "a hand-built Service may set only the singular field", + }, + { + name: "headless", + spec: corev1.ServiceSpec{ClusterIP: corev1.ClusterIPNone, ClusterIPs: []string{corev1.ClusterIPNone}}, + wantReason: "None is a sentinel, not an address", + }, + { + name: "not yet allocated", + spec: corev1.ServiceSpec{}, + wantReason: "a Service observed before the allocator has run has neither", + }, + { + name: "v4-mapped v6 belongs to neither family", + spec: corev1.ServiceSpec{ClusterIPs: []string{"::ffff:10.96.0.10"}}, + wantReason: "net.IP.To4 would misfile this as IPv4; kube never allocates one, so dropping it beats guessing", + }, + { + name: "unparseable entries are skipped", + spec: corev1.ServiceSpec{ClusterIPs: []string{"not-an-ip", "10.96.0.10"}}, + wantV4: "10.96.0.10", + wantReason: "a junk entry must not shadow a usable one", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + svc := &corev1.Service{Spec: tc.spec} + v4, v6 := ClusterIPsByFamily(svc) + if v4 != tc.wantV4 || v6 != tc.wantV6 { + t.Errorf("ClusterIPsByFamily(%+v) = (%q, %q), want (%q, %q): %s", tc.spec, v4, v6, tc.wantV4, tc.wantV6, tc.wantReason) + } + }) + } +} From 26b32945ce0208f792b6061aeb2f1fdfb9132501 Mon Sep 17 00:00:00 2001 From: Yuan Gao Date: Tue, 18 Aug 2026 08:44:24 -0700 Subject: [PATCH 3/4] atenet/dns: hoist the Corefile generation stamp to a package var No behavior change -- buildTemplate() already ran once, from init(). The next commit renders the Corefile on every call instead, where a stamp taken inline would differ each time: reconcile compares the render against the file on disk, so it would rewrite and reload CoreDNS every tick. --- cmd/atenet/internal/dns/corefile.go | 8 +++++++- cmd/atenet/internal/dns/corefile_test.go | 13 +++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/cmd/atenet/internal/dns/corefile.go b/cmd/atenet/internal/dns/corefile.go index 2869e3a24d..7b22cb5219 100644 --- a/cmd/atenet/internal/dns/corefile.go +++ b/cmd/atenet/internal/dns/corefile.go @@ -25,6 +25,12 @@ import ( // corefileTemplate is a Sprintf template for the CoreDNS configuration. var corefileTemplate string +// generatedAt stamps the rendered Corefile once per process, and must not be +// recomputed per render: 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 init() { corefileTemplate = buildTemplate() } @@ -74,7 +80,7 @@ func buildTemplate() string { // Generate the template. 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") diff --git a/cmd/atenet/internal/dns/corefile_test.go b/cmd/atenet/internal/dns/corefile_test.go index c8653ad7ef..58921b135b 100644 --- a/cmd/atenet/internal/dns/corefile_test.go +++ b/cmd/atenet/internal/dns/corefile_test.go @@ -76,3 +76,16 @@ func TestMakeCoreFile(t *testing.T) { }) } } + +// 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") + second := makeCoreFile("10.240.0.10") + 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) + } +} From 36cde91a2fd324c5c9f66831e297eef29e968d56 Mon Sep 17 00:00:00 2001 From: Yuan Gao Date: Tue, 18 Aug 2026 08:44:44 -0700 Subject: [PATCH 4/4] atenet/dns: publish the router's IPv6 ClusterIP as an AAAA Before, an actor name never resolved over IPv6: the zone published the router's primary cluster IP, always as an A record whatever family it was. On a dual-stack cluster the v6 address went unpublished; on an IPv6-only cluster the record was malformed, so every A query for an actor name failed. After, the zone publishes an address record per family the router has an address in, and answers empty for a family it has none in. Unit tests pin the rendered zone for each family combination, verified against the pinned coredns/coredns:1.11.1. --- cmd/atenet/internal/dns/README.md | 28 ++++- cmd/atenet/internal/dns/corefile.go | 65 +++++++----- cmd/atenet/internal/dns/corefile_test.go | 118 +++++++++++++++++---- cmd/atenet/internal/dns/dns.go | 18 ++-- cmd/atenet/internal/dns/dns_test.go | 127 +++++++++++++++++++++++ 5 files changed, 297 insertions(+), 59 deletions(-) diff --git a/cmd/atenet/internal/dns/README.md b/cmd/atenet/internal/dns/README.md index e89b06b62f..cb28e837f2 100644 --- a/cmd/atenet/internal/dns/README.md +++ b/cmd/atenet/internal/dns/README.md @@ -19,16 +19,26 @@ These are defined in manifests/ate-install/atenet-dns.yaml. * Deployment `ate-system:dns`. * Service `ate-system:dns` pointing to the Deployment. -Corefile, rendered by `corefile.go`: +`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. ``` # 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 " + answer "{{ .Name }} 60 IN A " fallthrough } -# NODATA for a well-formed actor name on any other qtype (AAAA, HTTPS, SRV, ...). +# 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 " + 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 @@ -42,9 +52,19 @@ Corefile, rendered by `corefile.go`: } ``` +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. +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 diff --git a/cmd/atenet/internal/dns/corefile.go b/cmd/atenet/internal/dns/corefile.go index 7b22cb5219..32d4043b74 100644 --- a/cmd/atenet/internal/dns/corefile.go +++ b/cmd/atenet/internal/dns/corefile.go @@ -22,26 +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)"` +) // generatedAt stamps the rendered Corefile once per process, and must not be -// recomputed per render: reconcileCoreDNSConfig decides whether to rewrite the +// 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 init() { - corefileTemplate = buildTemplate() -} - -func buildTemplate() string { - const ( - fallthroughDirective = " fallthrough" - soaDirective = ` authority "{{ .Zone }} 60 IN SOA ns.dns.{{ .Zone }} hostmaster.{{ .Zone }} (1 60 60 60 60)"` - ) - - // 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 ` 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") @@ -52,17 +57,19 @@ func buildTemplate() string { // Construct match pattern for ... 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, ".", `\.`) actorMatch := fmt.Sprintf(` match "^%s\.%s\.%s\.$"`, resources.ResourceNameRegexPattern, resources.ResourceNameRegexPattern, escapedSuffix) - directives = append(directives, actorMatch) - // Note the %s -- this will be filled with the router IP. - directives = append(directives, ` answer "{{ .Name }} 60 IN A %s"`) - directives = append(directives, fallthroughDirective) - directives = append(directives, "}") - // Valid actor names return NOERROR (NODATA) for non-A queries. + 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") @@ -78,7 +85,7 @@ func buildTemplate() string { directives = append(directives, soaDirective) directives = append(directives, "}") - // Generate the template. + // Generate the Corefile. b := strings.Builder{} fmt.Fprintf(&b, "# Generated at %s\n", generatedAt) fmt.Fprintf(&b, "%s:53 {\n ", resources.ActorDNSSuffix) @@ -88,6 +95,16 @@ func buildTemplate() string { 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, + "}", + } } diff --git a/cmd/atenet/internal/dns/corefile_test.go b/cmd/atenet/internal/dns/corefile_test.go index 58921b135b..c207926623 100644 --- a/cmd/atenet/internal/dns/corefile_test.go +++ b/cmd/atenet/internal/dns/corefile_test.go @@ -15,37 +15,81 @@ package dns import ( - "fmt" "strings" "testing" ) -// 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 wantCorefileFmt = `actors.resources.substrate.ate.dev:53 { +// 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 - 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 %s" - fallthrough - } - 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\.$" +` + wantZoneTail = ` template ANY ANY actors.resources.substrate.ate.dev { + ` + actorMatchDirective + ` rcode NOERROR - authority "{{ .Zone }} 60 IN SOA ns.dns.{{ .Zone }} hostmaster.{{ .Zone }} (1 60 60 60 60)" + ` + soaAuthorityDirective + ` fallthrough } 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)" + ` + 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 + + wantZoneNoAddresses = wantZoneHead + wantZoneTail +) // zoneBody strips the "# Generated at " header. func zoneBody(t *testing.T, corefile string) string { @@ -60,18 +104,46 @@ func zoneBody(t *testing.T, corefile string) string { func TestMakeCoreFile(t *testing.T) { tests := []struct { name string - routerIP string + routerV4 string + routerV6 string + want string }{ - {name: "cluster IP", routerIP: "10.240.0.10"}, - {name: "different cluster IP", routerIP: "192.168.1.1"}, + { + // 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: "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 := zoneBody(t, makeCoreFile(tc.routerIP)) - want := fmt.Sprintf(wantCorefileFmt, tc.routerIP) - if got != want { - t.Errorf("makeCoreFile(%q) rendered an unexpected Corefile\nGot:\n%s\nWant:\n%s", tc.routerIP, got, want) + 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) } }) } @@ -83,8 +155,8 @@ func TestMakeCoreFile(t *testing.T) { // 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") - second := makeCoreFile("10.240.0.10") + 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) } diff --git a/cmd/atenet/internal/dns/dns.go b/cmd/atenet/internal/dns/dns.go index cf2db99b69..96bc7df249 100644 --- a/cmd/atenet/internal/dns/dns.go +++ b/cmd/atenet/internal/dns/dns.go @@ -26,6 +26,7 @@ import ( "syscall" "time" + "github.com/agent-substrate/substrate/internal/ipfamily" "github.com/agent-substrate/substrate/internal/resources" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -50,7 +51,6 @@ type Controller struct { // Run the DNS orchestration loop until ctx is canceled. func (c *Controller) Run(ctx context.Context) error { slog.InfoContext(ctx, "DNS Controller started", slog.Duration("interval", c.Interval), slog.String("corefile", c.CorefilePath)) - slog.InfoContext(ctx, "Using template", "template", corefileTemplate) ticker := time.NewTicker(c.Interval) defer ticker.Stop() @@ -81,8 +81,10 @@ func (c *Controller) reconcile(ctx context.Context) error { return fmt.Errorf("failed to get atenet-router service: %w", err) } - routerIP := routerSvc.Spec.ClusterIP - if routerIP == "" || routerIP == "None" { + // Both families, not just Spec.ClusterIP: on an IPv6-only cluster the sole + // ClusterIP is a v6 address, and the zone has to publish it as an AAAA. + routerV4, routerV6 := ipfamily.ClusterIPsByFamily(routerSvc) + if routerV4 == "" && routerV6 == "" { slog.WarnContext(ctx, "atenet-router service has no ClusterIP yet, waiting...") return nil } @@ -104,7 +106,7 @@ func (c *Controller) reconcile(ctx context.Context) error { } // 3. Reconcile CoreDNS Corefile on shared volume - if err := c.reconcileCoreDNSConfig(ctx, routerIP); err != nil { + if err := c.reconcileCoreDNSConfig(ctx, routerV4, routerV6); err != nil { return fmt.Errorf("failed to reconcile CoreDNS config file: %w", err) } @@ -116,13 +118,13 @@ func (c *Controller) reconcile(ctx context.Context) error { return nil } -func (c *Controller) reconcileCoreDNSConfig(ctx context.Context, routerIP string) error { - expectedCorefile := makeCoreFile(routerIP) +func (c *Controller) reconcileCoreDNSConfig(ctx context.Context, routerV4, routerV6 string) error { + expectedCorefile := makeCoreFile(routerV4, routerV6) // Read Corefile from local shared volume path to see if it needs updating corefileBytes, err := os.ReadFile(c.CorefilePath) if err == nil && string(corefileBytes) == expectedCorefile { - slog.DebugContext(ctx, "CoreDNS Corefile is up-to-date", slog.String("routerIP", routerIP)) + slog.DebugContext(ctx, "CoreDNS Corefile is up-to-date", slog.String("routerIPv4", routerV4), slog.String("routerIPv6", routerV6)) return nil } @@ -130,7 +132,7 @@ func (c *Controller) reconcileCoreDNSConfig(ctx context.Context, routerIP string if err := os.WriteFile(c.CorefilePath, []byte(expectedCorefile), 0644); err != nil { return fmt.Errorf("failed to write updated Corefile to %s: %w", c.CorefilePath, err) } - slog.InfoContext(ctx, "CoreDNS Corefile updated", slog.String("routerIP", routerIP)) + slog.InfoContext(ctx, "CoreDNS Corefile updated", slog.String("routerIPv4", routerV4), slog.String("routerIPv6", routerV6)) // Signal CoreDNS process to reload if err := c.Reloader.Reload(ctx); err != nil { diff --git a/cmd/atenet/internal/dns/dns_test.go b/cmd/atenet/internal/dns/dns_test.go index 34116db284..34d23405d2 100644 --- a/cmd/atenet/internal/dns/dns_test.go +++ b/cmd/atenet/internal/dns/dns_test.go @@ -32,10 +32,12 @@ import ( type mockConfigReloader struct { reloaded bool + reloads int } func (m *mockConfigReloader) Reload(ctx context.Context) error { m.reloaded = true + m.reloads++ return nil } @@ -145,6 +147,131 @@ func TestReconcile(t *testing.T) { } } +// TestReconcileRouterIPFamilies covers what the controller publishes for each +// shape the atenet-router Service takes. The v6-only row is the one that used +// to be broken: the sole ClusterIP was read out of Spec.ClusterIP and written +// into an `IN A` answer, which loads as a valid Corefile and then SERVFAILs +// every query in the zone. +func TestReconcileRouterIPFamilies(t *testing.T) { + tests := []struct { + name string + // routerSpec is the atenet-router Service's spec; the dns Service is + // always a plain single-stack v4 one, since it feeds kube-dns rather + // than the zone under test. + routerSpec corev1.ServiceSpec + // wantAnswers are the answer lines the rendered Corefile must have. + wantAnswers []string + // wantNoAnswer, when true, means reconcile should leave the Corefile + // untouched rather than publish anything. + wantNoAnswer bool + }{ + { + name: "single stack IPv4", + routerSpec: corev1.ServiceSpec{ClusterIP: "10.0.0.1", ClusterIPs: []string{"10.0.0.1"}}, + wantAnswers: []string{`answer "{{ .Name }} 60 IN A 10.0.0.1"`}, + }, + { + name: "single stack IPv6", + routerSpec: corev1.ServiceSpec{ClusterIP: "fd00:10:96::8857", ClusterIPs: []string{"fd00:10:96::8857"}}, + wantAnswers: []string{`answer "{{ .Name }} 60 IN AAAA fd00:10:96::8857"`}, + }, + { + name: "dual stack", + routerSpec: corev1.ServiceSpec{ClusterIP: "10.0.0.1", ClusterIPs: []string{"10.0.0.1", "fd00:10:96::8857"}}, + wantAnswers: []string{ + `answer "{{ .Name }} 60 IN A 10.0.0.1"`, + `answer "{{ .Name }} 60 IN AAAA fd00:10:96::8857"`, + }, + }, + { + name: "not yet allocated", + routerSpec: corev1.ServiceSpec{}, + wantNoAnswer: true, + }, + { + name: "headless", + routerSpec: corev1.ServiceSpec{ClusterIP: corev1.ClusterIPNone, ClusterIPs: []string{corev1.ClusterIPNone}}, + wantNoAnswer: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + + routerSvc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "atenet-router", Namespace: "ate-system"}, + Spec: tc.routerSpec, + } + dnsSvc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "dns", Namespace: "ate-system"}, + Spec: corev1.ServiceSpec{ClusterIP: "10.0.0.2"}, + } + + const placeholder = "# not written yet\n" + corefilePath := filepath.Join(t.TempDir(), "Corefile") + if err := os.WriteFile(corefilePath, []byte(placeholder), 0644); err != nil { + t.Fatalf("failed to write initial Corefile: %v", err) + } + + reloader := &mockConfigReloader{} + controller := &Controller{ + Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(routerSvc, dnsSvc).Build(), + Interval: 1 * time.Second, + CorefilePath: corefilePath, + Reloader: reloader, + } + + ctx := context.Background() + if err := controller.reconcile(ctx); err != nil { + t.Fatalf("reconcile failed: %v", err) + } + + corefileBytes, err := os.ReadFile(corefilePath) + if err != nil { + t.Fatalf("failed to read Corefile: %v", err) + } + got := string(corefileBytes) + + if tc.wantNoAnswer { + if got != placeholder { + t.Errorf("reconcile() rewrote the Corefile for a Service with no usable ClusterIP; want it left alone\nGot:\n%s", got) + } + if reloader.reloads != 0 { + t.Errorf("reconcile() reloaded CoreDNS %d times for a Service with no usable ClusterIP, want 0", reloader.reloads) + } + return + } + + for _, want := range tc.wantAnswers { + if !strings.Contains(got, want) { + t.Errorf("reconcile() wrote a Corefile missing %q\nGot:\n%s", want, got) + } + } + // Exactly the expected answers and no others: an address template for + // a family the Service does not have would publish an unreachable + // address, and on the A side would not even parse as an RR. + if answers := strings.Count(got, `answer "`); answers != len(tc.wantAnswers) { + t.Errorf("reconcile() wrote %d answer directives, want %d\nGot:\n%s", answers, len(tc.wantAnswers), got) + } + if reloader.reloads != 1 { + t.Errorf("reconcile() reloaded CoreDNS %d times, want 1", reloader.reloads) + } + + // A second pass must be a no-op. The controller reconciles on a + // ticker, so anything unstable in the render -- a timestamp, most + // easily -- would rewrite the file and signal CoreDNS every interval. + if err := controller.reconcile(ctx); err != nil { + t.Fatalf("second reconcile failed: %v", err) + } + if reloader.reloads != 1 { + t.Errorf("second reconcile() reloaded CoreDNS again (%d total), want it to recognise the Corefile as up to date", reloader.reloads) + } + }) + } +} + func TestReconcileKubeDNSNotFound(t *testing.T) { scheme := runtime.NewScheme() _ = corev1.AddToScheme(scheme)