diff --git a/cmd/mesh-controller/network.go b/cmd/mesh-controller/network.go index 59325e1..7f62c24 100644 --- a/cmd/mesh-controller/network.go +++ b/cmd/mesh-controller/network.go @@ -348,11 +348,28 @@ func whereEveryoneIs(ctx context.Context, inv *inventory.Inventory, return nil, errors.New( "asked where everyone is without the catalogue, which cannot be answered") } - places, err := inv.Overlays(ctx) + places, err := onTheNetwork(ctx, inv, shelf) if err != nil { return nil, err } out := map[string]string{} + for _, p := range places { + out[p.Name] = overlay.InternalName(p.Name) + } + return out, nil +} + +// onTheNetwork is every placed machine that resolves the private network — has an address AND +// runs what puts it there. "Has an address" alone was true of every placed machine and told you +// nothing about whether anything could reach it; a name written for such a machine resolves to +// an address that does not answer, and a connection to it hangs (novox/hq issue 079). +func onTheNetwork(ctx context.Context, inv *inventory.Inventory, + shelf map[string]catalogue.Manifest) ([]inventory.Overlay, error) { + places, err := inv.Overlays(ctx) + if err != nil { + return nil, err + } + var out []inventory.Overlay for _, p := range places { if p.Address == "" { continue @@ -371,7 +388,7 @@ func whereEveryoneIs(ctx context.Context, inv *inventory.Inventory, for _, m := range got.Modules { for _, offered := range m.Offers() { if offered == overlay.Requirement { - out[p.Name] = overlay.InternalName(p.Name) + out = append(out, p) } } } @@ -379,14 +396,17 @@ func whereEveryoneIs(ctx context.Context, inv *inventory.Inventory, return out, nil } -// onThePrivateNetwork is every node's address on the overlay, sorted. +// onThePrivateNetwork is every node's address on the private network, sorted — the same set +// the names and the resolver mean by it (onTheNetwork), so a rule saying "from the mesh" admits +// exactly the machines the mesh names. // // A node with no address is left out rather than rendered as an empty source: an empty entry in a // source set is a syntax error in the rule file, and a rule file that does not load leaves the // node filtering whatever it was filtering before -- the one outcome worse than a wrong rule, // because nothing reports it. -func onThePrivateNetwork(ctx context.Context, inv *inventory.Inventory) ([]string, error) { - places, err := inv.Overlays(ctx) +func onThePrivateNetwork(ctx context.Context, inv *inventory.Inventory, + shelf map[string]catalogue.Manifest) ([]string, error) { + places, err := onTheNetwork(ctx, inv, shelf) if err != nil { return nil, err } @@ -400,22 +420,21 @@ func onThePrivateNetwork(ctx context.Context, inv *inventory.Inventory) ([]strin return out, nil } -// namesInTheMesh is every machine's internal name and the address behind it. +// namesInTheMesh is every machine's internal name and the address behind it — every machine +// that is on the private network, the same set the resolver means by that. // -// A machine with no address has no name: writing one that resolves to nothing is worse than not +// A machine that is not has no name: writing one that resolves to nothing is worse than not // writing it, because a connection to an address that does not answer hangs where a name that -// does not resolve fails at once and says so. That is the rule the hosts file already follows, -// and this is the same set read the same way. -func namesInTheMesh(ctx context.Context, inv *inventory.Inventory) (map[string]string, error) { - places, err := inv.Overlays(ctx) +// does not resolve fails at once and says so. A machine placed on the overlay but not running +// the module that puts it there is exactly that (novox/hq issue 079). +func namesInTheMesh(ctx context.Context, inv *inventory.Inventory, + shelf map[string]catalogue.Manifest) (map[string]string, error) { + places, err := onTheNetwork(ctx, inv, shelf) if err != nil { return nil, err } out := map[string]string{} for _, p := range places { - if strings.TrimSpace(p.Address) == "" { - continue - } out[overlay.InternalName(p.Name)] = p.Address } return out, nil diff --git a/cmd/mesh-controller/network_test.go b/cmd/mesh-controller/network_test.go index 789fe8a..ec42473 100644 --- a/cmd/mesh-controller/network_test.go +++ b/cmd/mesh-controller/network_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/novox/mesh-controller/internal/inventory" + "github.com/novox/mesh-controller/internal/overlay" ) // placementOf is what the mesh holds about where one node is. @@ -76,3 +77,34 @@ func TestAPlacementAndNothingTogetherIsRefused(t *testing.T) { t.Fatal("a placement and --nothing together was accepted") } } + +// A machine is named for the others only while it is on the private network — placed AND running +// what puts it there — the same set the resolver means by "on the private network". A machine +// that has an address and no networking is not named: a name resolving to an address that does +// not answer hangs where an unknown name fails at once (novox/hq issue 079). +func TestOnlyAMachineOnThePrivateNetworkIsNamed(t *testing.T) { + open := aMesh(t) // two placed machines, both assigned what puts them on the private network + ctx := t.Context() + shelf, err := open.inventory.Catalogue(ctx) + if err != nil { + t.Fatal(err) + } + names, err := namesInTheMesh(ctx, open.inventory, shelf) + if err != nil { + t.Fatal(err) + } + if names["anchor.internal"] != "10.77.0.1" || names["laptop.internal"] != "10.77.0.2" { + t.Fatalf("two machines on the network are not both named: %v", names) + } + // The laptop keeps its place and its address, and stops running the network. + if err := open.inventory.Unassign(ctx, "laptop", overlay.Name); err != nil { + t.Fatal(err) + } + names, err = namesInTheMesh(ctx, open.inventory, shelf) + if err != nil { + t.Fatal(err) + } + if _, still := names["laptop.internal"]; still || names["anchor.internal"] != "10.77.0.1" { + t.Fatalf("a machine that left the network is still named, or the one that stayed is not: %v", names) + } +} diff --git a/cmd/mesh-controller/plan.go b/cmd/mesh-controller/plan.go index db1ff0e..55d09fc 100644 --- a/cmd/mesh-controller/plan.go +++ b/cmd/mesh-controller/plan.go @@ -13,6 +13,7 @@ import ( "github.com/novox/mesh-controller/internal/catalogue" "github.com/novox/mesh-controller/internal/inventory" "github.com/novox/mesh-controller/internal/licences" + "github.com/novox/mesh-controller/internal/overlay" "net" "strconv" ) @@ -425,7 +426,12 @@ func declarationWith(ctx context.Context, open *stores, node string, // resolves to. Every node's address, including this one's: a machine reaching itself by its // own overlay address rather than by loopback is ordinary, and leaving it out would filter // the node's own traffic to itself with no rule naming why. - private, err := onThePrivateNetwork(ctx, inv) + // One reading of the catalogue for the three questions below that resolve the whole mesh. + shelf, err := inv.Catalogue(ctx) + if err != nil { + return nil, err + } + private, err := onThePrivateNetwork(ctx, inv, shelf) if err != nil { return nil, err } @@ -433,7 +439,7 @@ func declarationWith(ctx context.Context, open *stores, node string, // And every machine's name, so a container can reach one. The same set that writes the // machine's own hosts file — one reading, so a container and its machine cannot disagree // about where another machine is. - names, err := namesInTheMesh(ctx, inv) + names, err := namesInTheMesh(ctx, inv, shelf) if err != nil { return nil, err } @@ -481,7 +487,7 @@ func declarationWith(ctx context.Context, open *stores, node string, return plan.Declaration(catalogue.Rendering{ Settings: settings, Generators: gens, Grants: grants, Needed: needed, Ports: ports, Certificate: certificate, Authority: authority, Mesh: private, Names: names, - Foundation: foundation, Kept: kept}) + Suffix: overlay.Suffix(), Foundation: foundation, Kept: kept}) } // routeNamesInTheMesh is every routed name and the address of the node that serves it (novox/hq diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index 909322f..11d983c 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -92,6 +92,10 @@ type Rendering struct { // mesh" resolves to. Passed in for the same reason grants are: who else is on the network is // a fact about the mesh, and resolution answers questions about one machine. Mesh []string + // Suffix is what a machine's internal name ends in, as the control plane composed Names — + // `internal` unless the operator chose another — so a fact writing those names does not + // compose it a second time. + Suffix string // Kept is every operator-sealed secret in the mesh, for a module that `keeps` them. Nil when // nothing on this node keeps them, or the mesh has no operator key. @@ -524,7 +528,7 @@ func (r Resolution) Declaration(with Rendering) ([]map[string]any, error) { // plane's; making a name resolve is the module's software. Emitted as ordinary files under // this module's name, so they are applied, reported and removed exactly as anything else // it declares. - given, err := FactsInto(m, r, with.Names) + given, err := FactsInto(m, r, with.Names, with.Suffix) if err != nil { return nil, err } diff --git a/internal/catalogue/facts.go b/internal/catalogue/facts.go index d16c59c..baf919f 100644 --- a/internal/catalogue/facts.go +++ b/internal/catalogue/facts.go @@ -36,7 +36,7 @@ const ( // **A closed list.** A module asking for a fact the mesh does not have is asking for a file nobody // will write, and finding that out on a machine — as a daemon that starts, reads nothing, and // answers no queries — is worse than being told where the manifest is. -var facts = map[string]func(Resolution, map[string]string) string{ +var facts = map[string]func(Resolution, map[string]string, string) string{ FactNodeNames: nodeNames, FactNodeZones: nodeZones, } @@ -45,7 +45,7 @@ var facts = map[string]func(Resolution, map[string]string) string{ // // The module owns everything after the file exists: loading it, restarting on it, what a resolver // does with it. This only puts it there. -func FactsInto(m Manifest, r Resolution, addresses map[string]string) ([]map[string]any, error) { +func FactsInto(m Manifest, r Resolution, addresses map[string]string, suffix string) ([]map[string]any, error) { if len(m.Facts) == 0 { return nil, nil } @@ -70,7 +70,7 @@ func FactsInto(m Manifest, r Resolution, addresses map[string]string) ([]map[str } out = append(out, map[string]any{ "id": "fact-" + name, "type": "file", "path": path, "mode": "0644", - "content": write(r, addresses), + "content": write(r, addresses, suffix), }) } return out, nil @@ -92,7 +92,7 @@ func spokenFacts() string { // and does not yet know where it is, which is the ordinary state between adding a machine and it // joining. Writing the name anyway would give a name that resolves to nothing, and a connection to // that hangs; leaving it out fails at once and says the name is unknown. -func nodeNames(r Resolution, addresses map[string]string) string { +func nodeNames(r Resolution, addresses map[string]string, suffix string) string { var b strings.Builder b.WriteString("# Generated by the mesh. Do not edit — this file is replaced whenever a machine\n") b.WriteString("# joins or leaves, and an edit would survive until then and vanish.\n\n") @@ -106,10 +106,11 @@ func nodeNames(r Resolution, addresses map[string]string) string { b.WriteString("\n") for _, name := range sortedNames(addresses) { at := addresses[name] + internal, bare := meshName(name, suffix) // Its mesh name resolves to its address on the private network rather than to loopback, // so a service binding the name it was given stays reachable from everywhere else. - fmt.Fprintf(&b, "%s\t%s.internal\t%s", at, name, name) - if name == r.Node { + fmt.Fprintf(&b, "%s\t%s\t%s", at, internal, bare) + if bare == r.Node { b.WriteString("\t# this machine") } b.WriteString("\n") @@ -121,16 +122,33 @@ func nodeNames(r Resolution, addresses map[string]string) string { // // `*.homer.internal` is homer, which is the whole rule: if homer is at an address, so is anything // homer serves. A module wanting this runs the resolver; the mesh only says what is true. -func nodeZones(_ Resolution, addresses map[string]string) string { +func nodeZones(_ Resolution, addresses map[string]string, suffix string) string { var b strings.Builder b.WriteString("# Generated by the mesh. Do not edit — this file is replaced whenever a machine\n") b.WriteString("# joins or leaves, and an edit would survive until then and vanish.\n\n") for _, name := range sortedNames(addresses) { - fmt.Fprintf(&b, "address=/%s.internal/%s\n", name, addresses[name]) + internal, _ := meshName(name, suffix) + fmt.Fprintf(&b, "address=/%s/%s\n", internal, addresses[name]) } return b.String() } +// meshName is a machine's internal name and its bare one, from either. The control plane keys +// the names it hands a resolution by the internal name (`homer.internal`), the same map a +// container gets as its hosts; a caller that keys by the bare name gets the same answer. The +// suffix is the one the control plane composed those names with, handed down rather than written +// here a second time — the alternative was `homer.internal.internal` on every machine. +func meshName(name, suffix string) (internal, bare string) { + if suffix == "" { + suffix = "internal" + } + dotted := "." + strings.TrimPrefix(suffix, ".") + if strings.HasSuffix(name, dotted) { + return name, strings.TrimSuffix(name, dotted) + } + return name + dotted, name +} + func sortedNames(addresses map[string]string) []string { out := make([]string, 0, len(addresses)) for name, at := range addresses { diff --git a/internal/catalogue/facts_test.go b/internal/catalogue/facts_test.go index b543768..2540ab4 100644 --- a/internal/catalogue/facts_test.go +++ b/internal/catalogue/facts_test.go @@ -5,11 +5,12 @@ import ( "testing" ) -var threeMachines = map[string]string{"homer": "10.42.0.1", "marge": "10.42.0.2", "bart": ""} +// Keyed by the internal name, as the control plane hands them (issue 079). +var threeMachines = map[string]string{"homer.internal": "10.42.0.1", "marge.internal": "10.42.0.2", "bart.internal": ""} // **`*.homer.internal` is homer. That is the whole rule.** func TestEveryMachineIsAWildcardUnderItsOwnName(t *testing.T) { - out := nodeZones(Resolution{Node: "homer"}, threeMachines) + out := nodeZones(Resolution{Node: "homer"}, threeMachines, "") for _, want := range []string{ "address=/homer.internal/10.42.0.1", "address=/marge.internal/10.42.0.2", @@ -27,8 +28,8 @@ func TestEveryMachineIsAWildcardUnderItsOwnName(t *testing.T) { // unknown, which is a thing somebody can act on. func TestAMachineWithNoAddressIsNotNamed(t *testing.T) { for _, out := range []string{ - nodeNames(Resolution{Node: "homer"}, threeMachines), - nodeZones(Resolution{Node: "homer"}, threeMachines), + nodeNames(Resolution{Node: "homer"}, threeMachines, ""), + nodeZones(Resolution{Node: "homer"}, threeMachines, ""), } { if strings.Contains(out, "bart") { t.Fatalf("a machine with no address was named, so its name resolves to nothing:\n%s", out) @@ -39,7 +40,7 @@ func TestAMachineWithNoAddressIsNotNamed(t *testing.T) { // A machine's own mesh name points at its address on the private network, not at loopback — or a // service binding the name it was given is unreachable from everywhere else. func TestAMachinesOwnNameIsItsMeshAddress(t *testing.T) { - out := nodeNames(Resolution{Node: "homer"}, threeMachines) + out := nodeNames(Resolution{Node: "homer"}, threeMachines, "") var line string for _, l := range strings.Split(out, "\n") { if strings.Contains(l, "homer.internal") { @@ -58,7 +59,7 @@ func TestAMachinesOwnNameIsItsMeshAddress(t *testing.T) { // A module says where it wants a fact, and is given a file. func TestAModuleIsGivenTheFactsItAskedFor(t *testing.T) { m := Manifest{Module: "dnsmasq", Facts: map[string]string{FactNodeZones: "/etc/mesh/zones.conf"}} - given, err := FactsInto(m, Resolution{Node: "homer"}, threeMachines) + given, err := FactsInto(m, Resolution{Node: "homer"}, threeMachines, "") if err != nil { t.Fatal(err) } @@ -77,7 +78,7 @@ func TestAModuleIsGivenTheFactsItAskedFor(t *testing.T) { // starts, reads a file nobody wrote, and answers no queries is a much worse way to find out. func TestAskingForAFactTheMeshDoesNotHaveIsRefused(t *testing.T) { m := Manifest{Module: "dnsmasq", Facts: map[string]string{"the-weather": "/etc/weather"}} - _, err := FactsInto(m, Resolution{}, nil) + _, err := FactsInto(m, Resolution{}, nil, "") if err == nil { t.Fatal("a module asked for something nobody computes and was given nothing, silently") } @@ -91,7 +92,44 @@ func TestAskingForAFactTheMeshDoesNotHaveIsRefused(t *testing.T) { // And a relative path is refused, or a module decides where the mesh writes on a machine. func TestAFactMustBeAskedForAtAnAbsolutePath(t *testing.T) { m := Manifest{Module: "dnsmasq", Facts: map[string]string{FactNodeNames: "etc/hosts"}} - if _, err := FactsInto(m, Resolution{}, nil); err == nil { + if _, err := FactsInto(m, Resolution{}, nil, ""); err == nil { t.Fatal("a relative path was accepted") } } + +// **The names the control plane hands a resolution are already internal names** — `homer.internal`, +// the same map every container gets as its hosts. Appending the suffix again wrote +// `homer.internal.internal` into every hosts file and every resolver's zones, and the large mesh +// bed's name test was the first to read it back. Either key gives the same files. +func TestNamesKeyedByInternalNameAreNotSuffixedTwice(t *testing.T) { + internal := map[string]string{"homer.internal": "10.42.0.1", "marge.internal": "10.42.0.2"} + bare := map[string]string{"homer": "10.42.0.1", "marge": "10.42.0.2"} + if a, b := nodeZones(Resolution{Node: "homer"}, internal, ""), nodeZones(Resolution{Node: "homer"}, bare, ""); a != b { + t.Fatalf("the zones differ by how the names were keyed:\n%s\n---\n%s", a, b) + } + if a, b := nodeNames(Resolution{Node: "homer"}, internal, ""), nodeNames(Resolution{Node: "homer"}, bare, ""); a != b { + t.Fatalf("the hosts differ by how the names were keyed:\n%s\n---\n%s", a, b) + } + zones := nodeZones(Resolution{Node: "homer"}, internal, "") + if strings.Contains(zones, "internal.internal") || !strings.Contains(zones, "address=/homer.internal/10.42.0.1") { + t.Fatalf("the zones carry a doubled suffix or miss the name:\n%s", zones) + } + hosts := nodeNames(Resolution{Node: "homer"}, internal, "") + if !strings.Contains(hosts, "10.42.0.1\thomer.internal\thomer\t# this machine") { + t.Fatalf("the hosts line for the machine itself is not name, bare name and the mark:\n%s", hosts) + } +} + +// The suffix the control plane composed the names with is the one the facts write — an operator +// who chose another does not get `.internal` appended to it. +func TestTheFactsWriteTheSuffixTheNamesWereComposedWith(t *testing.T) { + names := map[string]string{"homer.lan": "10.42.0.1"} + zones := nodeZones(Resolution{Node: "homer"}, names, "lan") + if !strings.Contains(zones, "address=/homer.lan/10.42.0.1") || strings.Contains(zones, "internal") { + t.Fatalf("the zones do not carry the operator's suffix as given:\n%s", zones) + } + hosts := nodeNames(Resolution{Node: "homer"}, names, "lan") + if !strings.Contains(hosts, "10.42.0.1\thomer.lan\thomer\t# this machine") { + t.Fatalf("the hosts line does not carry the operator's suffix as given:\n%s", hosts) + } +}