From 296064c7999d776d97ea351913fbcd7e89c36f48 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 5 Oct 2026 23:39:15 +0200 Subject: [PATCH] Test the resolver file as the uplink's, and refuse a second writer of a fact's path (hq ADR 0223) The catalogue moves /etc/resolv.conf from resolv-conf to the three uplink modules. A rendered fact was not compared with other modules' paths, so two modules could each write the resolver file on one machine, the last winning every apply; a fact's path now counts as its module's. --- internal/catalogue/resolve.go | 17 +++ internal/catalogue/resolver_manifests_test.go | 84 ++++++----- internal/catalogue/roster.go | 16 ++- internal/catalogue/two_resolvers_test.go | 135 +++++++++++++----- 4 files changed, 180 insertions(+), 72 deletions(-) diff --git a/internal/catalogue/resolve.go b/internal/catalogue/resolve.go index 55a31d7..2970872 100644 --- a/internal/catalogue/resolve.go +++ b/internal/catalogue/resolve.go @@ -939,6 +939,23 @@ func checkResources(modules []Manifest) []string { } } } + // **A file the mesh renders for a module is that module's path too** (novox/hq ADR 0223). The + // machine's resolver file is a fact the uplink's holder asks for, and a second module asking + // for it, or declaring it, would have the host write one path twice in every apply, the last + // one winning. A fact under the operator's home is placed per account and compared nowhere. + for _, name := range sortedFacts(m.Facts) { + fact := m.Facts[name] + if fact.Home || !strings.HasPrefix(fact.Path, "/") { + continue + } + key := "path " + fact.Path + if other, taken := owner[key]; taken && other != m.Module { + problems = append(problems, fmt.Sprintf( + "%s and %s both declare the path %q", other, m.Module, fact.Path)) + } + owner[key] = m.Module + ownedPath[fact.Path] = m.Module + } } // An accessed path is the operator's, so no module may declare it as one of its own diff --git a/internal/catalogue/resolver_manifests_test.go b/internal/catalogue/resolver_manifests_test.go index 33f7fdf..32415c6 100644 --- a/internal/catalogue/resolver_manifests_test.go +++ b/internal/catalogue/resolver_manifests_test.go @@ -15,8 +15,8 @@ import ( // same arrangement, and to the two things a resolver here must never do — read resolv.conf for // its upstreams, or take an address systemd-resolved holds. -// resolverShelf is the two resolver modules and the container runtime beside something that -// answers `mesh-addressing`. +// resolverShelf is the resolver, an uplink module that writes what the machine asks (novox/hq ADR +// 0223), and the container runtime beside something that answers `mesh-addressing`. // The networking module that really does is composed in the controller and cannot be imported // here, so a stand-in offers the same word; what is under test is the manifests, not the network. func resolverShelf(t *testing.T) map[string]Manifest { @@ -24,7 +24,7 @@ func resolverShelf(t *testing.T) map[string]Manifest { shelf := map[string]Manifest{ "net": {Module: "net", Version: "1", Provides: []Offer{{Name: "mesh-addressing"}}}, } - for _, name := range []string{"dnsmasq", "resolv-conf", "docker"} { + for _, name := range []string{"dnsmasq", "systemd-networkd", "docker"} { shelf[name] = catalogueManifest(t, name) } return shelf @@ -87,21 +87,23 @@ func TestTheResolverForwardsToFixedUpstreamsAndNeverReadsResolvConf(t *testing.T // And the file that decides what the machine asks names every one of the mesh's resolvers, by // address, from the seat's holders, and no public one (ADR 0223): a resolver library that asks // every listed server at once takes the first reply, and a public "no such name" for a mesh name - // won it. - fact, ok := catalogueManifest(t, "resolv-conf").Facts["resolvers"] - if !ok || fact.Path != "/etc/resolv.conf" { - t.Fatalf("the machine's resolver file is not rendered from the roster: %+v", fact) - } - if !strings.Contains(fact.Template, `{{range index .Holders "mesh-dns-resolver"}}nameserver {{.Address}}`) { - t.Errorf("resolv.conf does not list every holder of the mesh's resolver:\n%s", fact.Template) - } - for _, line := range strings.Split(fact.Template, "\n") { - if strings.HasPrefix(line, "nameserver ") && !strings.Contains(line, "{{") { - t.Errorf("resolv.conf names a resolver of its own beside the mesh's: %s", line) + // won it. Written by every uplink module, since the uplink's holder owns the file. + for _, uplink := range uplinks { + fact, ok := catalogueManifest(t, uplink).Facts["resolvers"] + if !ok || fact.Path != "/etc/resolv.conf" { + t.Fatalf("%s's resolver file is not rendered from the roster: %+v", uplink, fact) + } + if !strings.Contains(fact.Template, `{{range index .Holders "mesh-dns-resolver"}}nameserver {{.Address}}`) { + t.Errorf("%s's resolv.conf does not list every holder of the mesh's resolver:\n%s", uplink, fact.Template) + } + for _, line := range strings.Split(fact.Template, "\n") { + if strings.HasPrefix(line, "nameserver ") && !strings.Contains(line, "{{") { + t.Errorf("%s's resolv.conf names a resolver of its own beside the mesh's: %s", uplink, line) + } + } + if !strings.Contains(fact.Template, "options timeout:1 attempts:2 edns0\n") { + t.Errorf("%s: a silent resolver is not passed over after one short wait:\n%s", uplink, fact.Template) } - } - if !strings.Contains(fact.Template, "options timeout:1 attempts:2 edns0\n") { - t.Errorf("a silent resolver is not passed over after one short wait:\n%s", fact.Template) } } @@ -110,9 +112,10 @@ func TestTheResolverForwardsToFixedUpstreamsAndNeverReadsResolvConf(t *testing.T // that file, the machine pointed at the resolver by address, and the runtime given no resolver of // its own but kept running across a restart (ADR 0196). func TestTheResolverAndWhatAsksItComposeOnOneMachine(t *testing.T) { - got, err := Resolve(resolverShelf(t), []string{"dnsmasq", "resolv-conf", "docker"}, + got, err := Resolve(resolverShelf(t), []string{"dnsmasq", "systemd-networkd", "docker"}, Node{Name: "anchor", At: "anchor.internal", Capabilities: map[string]bool{ - "package-manager": true, "service-manager": true, "privileged": true}}, World{}) + "package-manager": true, "service-manager": true, "privileged": true, + "uplink-systemd-networkd": true}}, World{}) if err != nil { t.Fatal(err) } @@ -170,7 +173,7 @@ func TestTheResolverAndWhatAsksItComposeOnOneMachine(t *testing.T) { t.Errorf("the resolver still writes the runtime's dns: %v", ids["dnsmasq.runtime-dns"]) } for id := range ids { - if strings.HasPrefix(id, "resolv-conf.runtime") { + if strings.HasPrefix(id, "systemd-networkd.runtime") { t.Errorf("what the machine asks still writes the runtime's file: %s", id) } } @@ -205,29 +208,40 @@ func TestTheResolverAndWhatAsksItComposeOnOneMachine(t *testing.T) { t.Errorf("the runtime is not reloaded when its file changes, so live-restore never takes effect") } - resolv := ids["resolv-conf.fact-resolvers"] + resolv := ids["systemd-networkd.fact-resolvers"] if resolv == nil || !strings.Contains(resolv["content"].(string), "\nnameserver 10.42.0.1\noptions ") { t.Fatalf("the machine is not pointed at the resolver by address, and only at it: %v", resolv) } } -// Two modules deciding what a machine asks are refused on one machine, as before — the claim -// exists so they never take turns overwriting each other. -// -// **The second is made up.** The catalogue's only other claimant, a systemd-resolved split-DNS -// module, was retired with the choice against a stub on every node (novox/hq ADR 0196, ADR 0220); -// what is under test is the claim, so any second module claiming it will do. +// Two modules deciding what a machine asks are refused on one machine, as before — now that the file +// is the uplink's (novox/hq ADR 0223), by two things: a machine runs one network manager, and no other +// module may write the resolver file beside the uplink's, as a file or as a rendered fact. func TestTwoThingsDecidingWhatAMachineAsksAreRefused(t *testing.T) { shelf := resolverShelf(t) - shelf["other-resolver-config"] = Manifest{Module: "other-resolver-config", Version: "1", - Claims: []Claim{{Name: "node-resolver-config", Scope: ScopeNode}}} - _, err := Resolve(shelf, []string{"dnsmasq", "resolv-conf", "other-resolver-config"}, - Node{Name: "anchor", At: "anchor.internal"}, World{}) - if err == nil { - t.Fatal("resolv-conf and a second module deciding what the machine asks were both assigned to one machine") + shelf["networkmanager"] = catalogueManifest(t, "networkmanager") + node := Node{Name: "anchor", At: "anchor.internal", Capabilities: map[string]bool{ + "package-manager": true, "service-manager": true, + "uplink-systemd-networkd": true, "uplink-networkmanager": true}} + _, err := Resolve(shelf, []string{"dnsmasq", "systemd-networkd", "networkmanager"}, node, World{}) + if err == nil || !strings.Contains(err.Error(), "node-uplink") { + t.Fatalf("two network managers were both assigned to one machine: %v", err) } - if !strings.Contains(err.Error(), "node-resolver-config") { - t.Fatalf("the refusal does not say what was claimed: %v", err) + + // The second is made up: what is under test is that the resolver file has one owner, so any + // module asking the mesh to render it — or declaring it — will do. + for name, m := range map[string]Manifest{ + "a-rendered-one": {Module: "a-rendered-one", Version: "1", + Facts: map[string]RosterFile{"mine": {Path: "/etc/resolv.conf", Template: "nameserver 10.42.0.1\n"}}}, + "a-declared-one": {Module: "a-declared-one", Version: "1", Resources: []map[string]any{ + {"id": "mine", "type": "file", "path": "/etc/resolv.conf", "content": "nameserver 10.42.0.1\n"}}}, + } { + shelf := resolverShelf(t) + shelf[name] = m + _, err := Resolve(shelf, []string{"dnsmasq", "systemd-networkd", name}, node, World{}) + if err == nil || !strings.Contains(err.Error(), "/etc/resolv.conf") { + t.Errorf("%s wrote the resolver file beside the uplink's: %v", name, err) + } } } diff --git a/internal/catalogue/roster.go b/internal/catalogue/roster.go index ec03027..fbf39d8 100644 --- a/internal/catalogue/roster.go +++ b/internal/catalogue/roster.go @@ -112,11 +112,7 @@ func FactsFrom(m Manifest, r Resolution, with Rendering) ([]map[string]any, erro if len(m.Facts) == 0 { return nil, nil } - names := make([]string, 0, len(m.Facts)) - for name := range m.Facts { - names = append(names, name) - } - sort.Strings(names) + names := sortedFacts(m.Facts) view := rosterView{ Node: r.Node, @@ -279,3 +275,13 @@ func holdersFrom(holders map[string]map[string]string, accounts map[string]strin } return out } + +// sortedFacts is a module's fact names in order, so what is said about them is said the same way twice. +func sortedFacts(facts map[string]RosterFile) []string { + out := make([]string, 0, len(facts)) + for name := range facts { + out = append(out, name) + } + sort.Strings(out) + return out +} diff --git a/internal/catalogue/two_resolvers_test.go b/internal/catalogue/two_resolvers_test.go index a94a848..13fb532 100644 --- a/internal/catalogue/two_resolvers_test.go +++ b/internal/catalogue/two_resolvers_test.go @@ -10,6 +10,7 @@ import ( // every holder — its own first when it is one — and no public resolver. ADR 0196 listed the mesh's // resolver then a public one, and musl asks both at once and takes the first reply: from the home // server the public "no such name" for the anchor's mesh name won, every time, in every Alpine build. +// The file is written by the module holding the machine's uplink (ADR 0223 part 2). // resolverMachines is the anchor and the home server holding the resolver, and a laptop holding nothing. var resolverMachines = map[string]string{ @@ -21,14 +22,32 @@ var bothResolvers = []Held{ {Claim: "mesh-dns-resolver", Scope: ScopeMesh, Node: "home", Module: "dnsmasq"}, } -// twoResolverShelf is the resolver, what asks it, and a stand-in answering `mesh-addressing`. +// uplinks is every module in the catalogue holding node-uplink, and so writing the machine's resolver +// file (novox/hq ADR 0223 part 2): the program that would otherwise rewrite it is the one that writes it. +var uplinks = []string{"dhcpcd", "networkmanager", "systemd-networkd"} + +// managing is a machine as each uplink module needs it: able to install, run a service and run the +// manager that module is for. +func managing(node string) Node { + caps := map[string]bool{"package-manager": true, "service-manager": true} + for _, u := range uplinks { + caps["uplink-"+u] = true + } + return Node{Name: node, At: node + ".internal", Capabilities: caps} +} + +// twoResolverShelf is the resolver, the uplink modules that write what a machine asks, and a stand-in +// answering `mesh-addressing`. func twoResolverShelf(t *testing.T) map[string]Manifest { t.Helper() - return map[string]Manifest{ - "net": {Module: "net", Version: "1", Provides: []Offer{{Name: "mesh-addressing"}}}, - "dnsmasq": catalogueManifest(t, "dnsmasq"), - "resolv-conf": catalogueManifest(t, "resolv-conf"), + out := map[string]Manifest{ + "net": {Module: "net", Version: "1", Provides: []Offer{{Name: "mesh-addressing"}}}, + "dnsmasq": catalogueManifest(t, "dnsmasq"), } + for _, u := range uplinks { + out[u] = catalogueManifest(t, u) + } + return out } // worldWithout is the rest of the mesh as a plan for one machine sees it: every other holder's claim @@ -47,12 +66,13 @@ func worldWithout(node string) World { } // resolvConfOn resolves and composes one machine and answers with the nameservers its resolver file -// lists, in order, and the file. -func resolvConfOn(t *testing.T, node string, assigned []string) ([]string, string) { +// lists, in order, and the file. The file is the uplink's — `uplink` is one of the assigned modules — +// and nothing else on the machine declares that path. +func resolvConfOn(t *testing.T, node, uplink string, assigned []string) ([]string, string) { t.Helper() - got, err := Resolve(twoResolverShelf(t), assigned, Node{Name: node, At: node + ".internal"}, worldWithout(node)) + got, err := Resolve(twoResolverShelf(t), assigned, managing(node), worldWithout(node)) if err != nil { - t.Fatalf("%s does not resolve with two resolvers on record: %v", node, err) + t.Fatalf("%s with %s does not resolve with two resolvers on record: %v", node, uplink, err) } out, err := got.Declaration(Rendering{ Names: resolverMachines, Machines: resolverMachines, Suffix: "internal", @@ -61,11 +81,20 @@ func resolvConfOn(t *testing.T, node string, assigned []string) ([]string, strin Needed: map[string]map[string]string{"dnsmasq": {"broker": "sealed"}}, }) if err != nil { - t.Fatalf("%s does not compose: %v", node, err) + t.Fatalf("%s with %s does not compose: %v", node, uplink, err) } - file := byID(out)["resolv-conf.fact-resolvers"] - if file == nil || file["path"] != "/etc/resolv.conf" { - t.Fatalf("%s was given no resolver file: %v", node, file) + var file map[string]any + for _, r := range out { + if r["path"] != "/etc/resolv.conf" { + continue + } + if file != nil { + t.Fatalf("%s declares /etc/resolv.conf twice: %v and %v", node, file["id"], r["id"]) + } + file = r + } + if file == nil || file["id"] != uplink+".fact-resolvers" { + t.Fatalf("%s's resolver file is not %s's: %v", node, uplink, file) } content, _ := file["content"].(string) var servers []string @@ -77,40 +106,82 @@ func resolvConfOn(t *testing.T, node string, assigned []string) ([]string, strin return servers, content } -func TestTwoResolversComposeOnBothHoldersAndEachListsItselfFirst(t *testing.T) { - for node, want := range map[string][]string{ - "anchor": {"10.42.0.1", "10.42.0.3"}, - "home": {"10.42.0.3", "10.42.0.1"}, - } { - servers, content := resolvConfOn(t, node, []string{"dnsmasq", "resolv-conf"}) - if strings.Join(servers, " ") != strings.Join(want, " ") { - t.Errorf("%s lists %v; itself first, then the other holder: %v\n%s", node, servers, want, content) +// Per uplink module: each writes a resolver file listing both holders, the machine's own first on a +// holder, and only the holders on a machine that is none. +func TestEveryUplinkListsBothResolversItsOwnFirst(t *testing.T) { + for _, uplink := range uplinks { + for node, want := range map[string][]string{ + "anchor": {"10.42.0.1", "10.42.0.3"}, + "home": {"10.42.0.3", "10.42.0.1"}, + "laptop": {"10.42.0.1", "10.42.0.3"}, + } { + assigned := []string{uplink} + if node != "laptop" { + assigned = append(assigned, "dnsmasq") + } + servers, content := resolvConfOn(t, node, uplink, assigned) + if strings.Join(servers, " ") != strings.Join(want, " ") { + t.Errorf("%s on %s lists %v; want %v\n%s", uplink, node, servers, want, content) + } + for _, public := range []string{"1.1.1.1", "8.8.8.8", "9.9.9.9"} { + if strings.Contains(content, public) { + t.Errorf("%s on %s lists a public resolver beside the mesh's (ADR 0223):\n%s", uplink, node, content) + } + } + if !strings.HasSuffix(content, "\noptions timeout:1 attempts:2 edns0\n") { + t.Errorf("%s on %s does not end with two short attempts:\n%s", uplink, node, content) + } } } } -func TestAMachineHoldingNoResolverListsBothAndNoPublicOne(t *testing.T) { - servers, content := resolvConfOn(t, "laptop", []string{"resolv-conf"}) - if strings.Join(servers, " ") != "10.42.0.1 10.42.0.3" { - t.Errorf("the laptop lists %v; every holder, by name, and nothing else:\n%s", servers, content) - } - for _, public := range []string{"1.1.1.1", "8.8.8.8", "9.9.9.9"} { - if strings.Contains(content, public) { - t.Errorf("a public resolver is listed beside the mesh's (ADR 0223):\n%s", content) +// One file, whichever manager the machine runs: the three modules carry the same template, so a +// machine changing its manager changes nothing in what it asks, and a fix made to one is made to all. +func TestEveryUplinkWritesTheSameResolverFile(t *testing.T) { + var first, firstOf string + for _, uplink := range uplinks { + fact, ok := catalogueManifest(t, uplink).Facts["resolvers"] + if !ok || fact.Path != "/etc/resolv.conf" || fact.Shared || fact.Home { + t.Fatalf("%s does not write the machine's resolver file whole: %+v", uplink, fact) } + if first == "" { + first, firstOf = fact.Template, uplink + continue + } + if fact.Template != first { + t.Errorf("%s's resolver file differs from %s's; the three are kept identical", uplink, firstOf) + } + } +} + +// The requirement stays with what writes the file (ADR 0223 part 1): a machine is refused when nothing +// in the mesh resolves, rather than given a file listing nothing. +func TestEveryUplinkRequiresTheMeshsResolver(t *testing.T) { + for _, uplink := range uplinks { + found := false + for _, r := range catalogueManifest(t, uplink).Requires { + found = found || r == "wildcard-resolution" + } + if !found { + t.Errorf("%s writes the resolver file and does not require wildcard-resolution", uplink) + } + } + _, err := Resolve(twoResolverShelf(t), []string{"networkmanager"}, managing("laptop"), World{}) + if err == nil || !strings.Contains(err.Error(), "wildcard-resolution") { + t.Errorf("an uplink was composed on a mesh with no resolver: %v", err) } } // A holder answers its own requirement, even though the other holder sorts first (issue 258 kept). func TestAHolderAnswersItsOwnRequirement(t *testing.T) { - got, err := Resolve(twoResolverShelf(t), []string{"dnsmasq", "resolv-conf"}, - Node{Name: "home", At: "home.internal"}, worldWithout("home")) + got, err := Resolve(twoResolverShelf(t), []string{"dnsmasq", "networkmanager"}, + managing("home"), worldWithout("home")) if err != nil { t.Fatal(err) } for _, n := range got.Needs { if n.Name == "wildcard-resolution" && n.From != "home" { - t.Errorf("the home server's resolver configuration is bound to %s; it holds the seat itself", n.From) + t.Errorf("the home server's uplink is bound to %s; it holds the seat itself", n.From) } } }