diff --git a/cmd/mesh-controller/collect.go b/cmd/mesh-controller/collect.go index 5464800..6d01e2a 100644 --- a/cmd/mesh-controller/collect.go +++ b/cmd/mesh-controller/collect.go @@ -60,7 +60,7 @@ func collect(ctx context.Context, inv *inventory.Inventory) { store := artifacts.Store{Address: address} var done []string - var left int + var left, skipped int for i, reference := range references { if i >= mostPerSweep || within.Err() != nil { left = len(references) - i @@ -73,10 +73,22 @@ func collect(ctx context.Context, inv *inventory.Inventory) { done = append(done, reference) continue } - // **Stopped at the first refusal, not pushed through.** A store that refuses one refuses - // all of them — deletion disabled, the store down, the network gone — so going on would - // be a hundred identical failures and a hundred identical log lines in front of whoever - // was building something. + if errors.Is(err, artifacts.ErrNotOurs) { + // **A fact about this record, so this record is skipped** (novox/hq issue 226). Not + // marked collected — the mesh did not remove it and should not claim to — and not a + // reason to stop, because the store was never asked. One of these at the front of + // the oldest-first order ended every sweep until this. + skipped++ + if skipped == 1 { + fmt.Fprintf(os.Stderr, + "the sweep will not address %s and went on: %v\n", reference, err) + } + continue + } + // **Stopped at the first refusal by the STORE, not pushed through.** A store that refuses + // one refuses all of them — deletion disabled, the store down, the network gone — so + // going on would be a hundred identical failures and a hundred identical log lines in + // front of whoever was building something. fmt.Fprintf(os.Stderr, "the artifact store kept %s, so nothing more was asked of it: %v\n", reference, err) left = len(references) - i @@ -97,6 +109,9 @@ func collect(ctx context.Context, inv *inventory.Inventory) { if left > 0 { fmt.Fprintf(os.Stderr, "%d more to collect; the next build asks again\n", left) } + if skipped > 0 { + fmt.Fprintf(os.Stderr, "%d artifact(s) the sweep will not address were skipped\n", skipped) + } } // mostPerSweep is how many artifacts one sweep will ask about. Enough that a mesh building diff --git a/internal/artifacts/store.go b/internal/artifacts/store.go index 02b79d8..ca80e65 100644 --- a/internal/artifacts/store.go +++ b/internal/artifacts/store.go @@ -8,6 +8,7 @@ package artifacts import ( "context" + "errors" "fmt" "net/http" "strings" @@ -26,7 +27,15 @@ type Store struct { } // Gone is the answer when the store does not hold it: the outcome wanted, already true. -var Gone = fmt.Errorf("the store does not hold it") +var Gone = errors.New("the store does not hold it") + +// ErrNotOurs is a reference this sweep will not address: not the mesh's own, or naming nothing +// the store holds by digest. +// +// **A fact about the record, not about the store** (novox/hq issue 226). The two deserve opposite +// responses — skip one and go on, abandon the sweep for the other — and collapsing them into "an +// error" is how a cautious loop became one that did nothing while reporting the right number. +var ErrNotOurs = errors.New("not a reference into the mesh's artifact store") // LetGo asks the store to drop one artifact the mesh recorded making. // @@ -38,12 +47,17 @@ var Gone = fmt.Errorf("the store does not hold it") // wants the artifact absent, and it is. It is distinguished from success only so a caller can say // which of the two happened. func (s Store) LetGo(ctx context.Context, reference string) error { + // **Strict, and deliberately** (novox/hq issue 226). Only a reference the mesh keeps in its + // own vocabulary is addressed here. `Recorded` would read `docker.io/library/registry@sha256:…` + // as the mesh's too — it cannot tell one registry host from another — so normalising belongs + // where the provenance is known, which is the sweep reading its own build records, not here + // where the only job is to refuse anything that is not plainly ours. path, kept := catalogue.InArtifactStore(reference) if !kept { // Nothing the mesh put in its own store. Refused rather than attempted: composing a // delete for a reference of unknown shape is how a sweep reaches something that is not - // the mesh's. - return fmt.Errorf("%s is not a reference into the mesh's artifact store", reference) + // the mesh's. Distinguished from a store that refuses, so a sweep skips this and goes on. + return fmt.Errorf("%w: %s", ErrNotOurs, reference) } if s.Address == "" { return fmt.Errorf("this mesh has no artifact store on its network to ask about %s", reference) @@ -95,5 +109,5 @@ func split(path string) (repository, kind, digest string, err error) { if before, after, ok := strings.Cut(path, "/blobs/sha256:"); ok { return before, "blobs", "sha256:" + after, nil } - return "", "", "", fmt.Errorf("%q names nothing the store holds by digest", path) + return "", "", "", fmt.Errorf("%w: %q names nothing the store holds by digest", ErrNotOurs, path) } diff --git a/internal/artifacts/store_test.go b/internal/artifacts/store_test.go index 3b433c8..ee09086 100644 --- a/internal/artifacts/store_test.go +++ b/internal/artifacts/store_test.go @@ -92,3 +92,26 @@ func TestAReferenceThatIsNotTheMeshsOwnIsNeverAsked(t *testing.T) { t.Fatalf("the store was asked about %v", *asked) } } + +// A reference this sweep will not address says so as ErrNotOurs, which is a fact about the +// record and not about the store (novox/hq issue 226). +// +// The sweep skips one and abandons itself for the other, so they cannot be the same error. The +// first live run met a reference recorded with the store's old address, read the refusal as "the +// store refuses everything", and collected none of the 1681 it had found. +func TestAReferenceThisSweepWillNotAddressIsToldApartFromAStoreRefusing(t *testing.T) { + store, asked := fakeStore(t, http.StatusAccepted) + for _, reference := range []string{ + "docker.io/library/registry@sha256:abc123", + "127.0.0.1:5100/mesh-tools/build@sha256:abc123", + "1.4.2", + } { + err := store.LetGo(context.Background(), reference) + if !errors.Is(err, ErrNotOurs) { + t.Errorf("%s answered %v; a sweep must be able to skip it and go on", reference, err) + } + } + if len(*asked) != 0 { + t.Fatalf("the store was asked about %v", *asked) + } +} diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index 473f8fe..6303bcb 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -610,14 +610,14 @@ func (r Resolution) compose(with Rendering, owner map[string]string, // and nothing would say so. continue } - first = append(first, map[string]any{ + first = append(first, ownedBy(r.provisionsAs(m), map[string]any{ // One file per holder — the consumer's module with its local name after it // where it keeps several (ADR 0094); the lab found two files with one id. "id": GrantID(to, g.Consumer+"."+holderAs(g.From, g.Local)), "type": "file", "path": grantPath(m.Grants[to], g.Consumer, holderAs(g.From, g.Local)), "sealed": g.Sealed, - }) + })) } } for _, to := range sortedKeys(m.Binds) { @@ -1240,6 +1240,28 @@ type Contribution struct { Derived map[string]any `json:"derived,omitempty"` } +// provisionsAs is the account that reads what the mesh writes for this provider: the one secret +// per consumer it must open to set that consumer's password (novox/hq issue 225). +// +// **A root-owned 0600 file is one that process cannot read**, which is the same sentence already +// written above for a module's own secrets — and the grant secret is the other kind of secret +// the mesh writes for a module, so it is the same rule. +// +// Which account depends on where the module's code runs. A module whose code is a bundle is run +// by the node's tool runtime, as the node's account ([ADR 0198](0198)); one still in a container +// is whatever it declares as its secrets owner. Nothing names these paths, so the rule that +// claims a bundle's other files by the words that name them (givenTo) cannot reach them: the +// harness composes a grant secret's path from the contributions file, not from a word. +// +// Empty is root, which is what it was and what a module with no bundle and no declared owner +// still wants. +func (r Resolution) provisionsAs(m Manifest) string { + if len(m.Bundles) > 0 && r.Account != "" { + return r.Account + } + return m.SecretsOwner +} + // grantPath is where one consumer's sealed credential lands on the providing machine. // // Suffixed, so the directory can also hold whatever the module writing it keeps there and so a diff --git a/internal/catalogue/grant_secret_owner_test.go b/internal/catalogue/grant_secret_owner_test.go new file mode 100644 index 0000000..5fdf7fe --- /dev/null +++ b/internal/catalogue/grant_secret_owner_test.go @@ -0,0 +1,103 @@ +package catalogue + +import ( + "strings" + "testing" +) + +// A grant secret is read by whatever provisions, and that stopped being root (novox/hq issue 225). +// +// The mesh seals one credential per consumer beside the provider's contributions file. The +// provider's harness reads both: the file to learn who asked, the secret to set their password. +// While a module's own code ran in a container as root, a root-owned 0600 file was readable by +// the thing that needed it. ADR 0198 moved that code under the node's runtime, which runs as the +// operator's account — and the secret stayed root's. +// +// **The cost was silence.** The harness says `secret not readable yet`, which is true and +// ordinary on the first pass, so four thousand refusals in three hours read as patience. No user +// was ever created, and two consumers crash-looped against a database that had never heard of +// them. +// +// The same reasoning is already written for a module's *own* secrets, three hundred lines above: +// "a root-owned 0600 file is one that process cannot read". This is that rule reaching the other +// kind of secret the mesh writes for a module. + +// aProviderWithABundle is a provider whose code is a bundle the node's runtime runs — the shape +// every TypeScript provisioner has since ADR 0198. +func aProviderWithABundle() Manifest { + return Manifest{ + Module: "mongodb", Version: "1", + Provides: FromAnywhere("mongodb-database"), + Receives: map[string]string{"mongodb-database": "/var/lib/mongodb/grants/mesh.json"}, + Grants: map[string]string{"mongodb-database": "/var/lib/mongodb/grants"}, + Bundles: []Bundle{{Name: "code", Language: "typescript"}}, + Resources: []map[string]any{{ + "id": "server", "type": "container", "name": "mongodb-server", + "image": "mongo@sha256:" + strings.Repeat("a", 64), + }}, + } +} + +func TestAGrantSecretIsOwnedByTheAccountThatProvisions(t *testing.T) { + r, err := Resolve(shelf(aProviderWithABundle()), []string{"mongodb"}, reachable(), World{}) + if err != nil { + t.Fatal(err) + } + r.Account = "operator" + out, err := r.Declaration(Rendering{Grants: []Grant{{ + Provision: "mongodb-database", Consumer: "workstation", From: "photos", Slug: "photos", + Values: map[string]any{}, Sealed: "c2VhbGVk", + }}}) + if err != nil { + t.Fatal(err) + } + + var secret map[string]any + for _, res := range out { + if res["type"] == "file" && strings.HasSuffix(fmtPath(res), ".secret") { + secret = res + } + } + if secret == nil { + t.Fatalf("no grant secret was composed at all: %v", out) + } + if got := secret["owner"]; got != "operator" { + t.Fatalf("the grant secret at %v belongs to %v; the provisioner runs as %q and a "+ + "root-owned 0600 file is one it cannot read — which is silent, because the harness "+ + "calls it \"not readable yet\"", fmtPath(secret), got, "operator") + } +} + +// And a provider whose code still runs in a container keeps the owner it declares, so this +// changes nothing for the modules the runtime has not taken. +func TestAContainerProvidersGrantSecretKeepsItsDeclaredOwner(t *testing.T) { + m := aProviderWithABundle() + m.Bundles = nil + m.SecretsOwner = "65534:65534" + r, err := Resolve(shelf(m), []string{"mongodb"}, reachable(), World{}) + if err != nil { + t.Fatal(err) + } + r.Account = "operator" + out, err := r.Declaration(Rendering{Grants: []Grant{{ + Provision: "mongodb-database", Consumer: "workstation", From: "photos", Slug: "photos", + Values: map[string]any{}, Sealed: "c2VhbGVk", + }}}) + if err != nil { + t.Fatal(err) + } + for _, res := range out { + if res["type"] == "file" && strings.HasSuffix(fmtPath(res), ".secret") { + if got := res["owner"]; got != "65534:65534" { + t.Fatalf("a container provider's grant secret belongs to %v, not what it declares", got) + } + return + } + } + t.Fatal("no grant secret was composed") +} + +func fmtPath(r map[string]any) string { + p, _ := r["path"].(string) + return p +} diff --git a/internal/catalogue/published_ports_test.go b/internal/catalogue/published_ports_test.go new file mode 100644 index 0000000..48036d2 --- /dev/null +++ b/internal/catalogue/published_ports_test.go @@ -0,0 +1,75 @@ +package catalogue + +import ( + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// A container publishes only a port its module declares (novox/hq issue 227). +// +// **The short form is a question the mesh answers.** `"80"` means *publish what the software +// calls 80*, and the mesh fills in the machine's half from the port it assigned +// ([ADR 0038](0038)). It can only assign one for a port the module declared in `listens` — so a +// container publishing a number that appears nowhere in `listens` gets no assignment, and +// `publishedOn` falls back to the number as written. It escapes to the machine. +// +// That is how the photo module asked for port 80 on the control node, where the reverse proxy +// holds it: it declared its web endpoint at 4001, published a bare 80, and the container never +// started. Four other modules publish 80 quite safely — because they declare 80, so the mesh +// gives them a machine port for it. The difference is the declaration, not the number. +// +// A mapping written the long way is a module pinning both halves on purpose and is left alone. +func TestEveryPublishedPortIsOneItsModuleDeclares(t *testing.T) { + root := catalogueRoot(t) + entries, err := os.ReadDir(filepath.Join(root, "modules")) + if err != nil { + t.Fatal(err) + } + var escaped []string + for _, entry := range entries { + if !entry.IsDir() { + continue + } + raw, err := os.ReadFile(filepath.Join(root, "modules", entry.Name(), "module.json")) + if err != nil { + continue + } + m, err := ParseManifest(raw) + if err != nil { + // Whether every manifest parses is TestEveryCatalogueManifestParses's question. + continue + } + declared := map[int]bool{} + for _, l := range m.Listens { + declared[l.Port] = true + } + for _, r := range m.Resources { + if fmt.Sprint(r["type"]) != "container" { + continue + } + listed, _ := r["ports"].([]any) + for _, p := range listed { + written := strings.Split(fmt.Sprint(p), "/")[0] + if strings.Contains(written, ":") { + continue // pinned by hand, both halves, on purpose + } + port, err := strconv.Atoi(strings.TrimSpace(written)) + if err != nil || declared[port] { + continue + } + escaped = append(escaped, fmt.Sprintf( + "%s's %v publishes %d, and %s declares no such port — the mesh has nothing "+ + "to assign, so %d reaches the machine as written", + m.Module, r["id"], port, m.Module, port)) + } + } + } + if len(escaped) > 0 { + t.Fatalf("a container may publish only a port its module declares:\n - %s", + strings.Join(escaped, "\n - ")) + } +} diff --git a/internal/inventory/collection.go b/internal/inventory/collection.go index 4b5bf67..3f6cde4 100644 --- a/internal/inventory/collection.go +++ b/internal/inventory/collection.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "strings" + + "github.com/novox/mesh-controller/internal/catalogue" ) // What the artifact store keeps, and what it may let go (novox/hq ADR 0189, issue 108). @@ -71,11 +73,12 @@ func (i *Inventory) ToCollect(ctx context.Context) ([]string, error) { continue } for _, a := range made { - if a.Reference == "" || keep[a.Reference] || collected[a.Reference] || seen[a.Reference] { + reference := asRecorded(a.Reference) + if reference == "" || keep[reference] || collected[reference] || seen[reference] { continue } - seen[a.Reference] = true - out = append(out, a.Reference) + seen[reference] = true + out = append(out, reference) } } return out, rows.Err() @@ -127,8 +130,8 @@ func (i *Inventory) keptReferences(ctx context.Context) (map[string]bool, error) continue } for _, a := range made { - if a.Reference != "" { - keep[a.Reference] = true + if reference := asRecorded(a.Reference); reference != "" { + keep[reference] = true } } } @@ -186,16 +189,35 @@ func (i *Inventory) everyReferenceMade(ctx context.Context) ([]string, error) { continue } for _, a := range made { - if a.Reference == "" || seen[a.Reference] { + reference := asRecorded(a.Reference) + if reference == "" || seen[reference] { continue } - seen[a.Reference] = true - out = append(out, a.Reference) + seen[reference] = true + out = append(out, reference) } } return out, rows.Err() } +// asRecorded is an artifact reference in the one vocabulary the sweep speaks (novox/hq issue 226). +// +// **Every reference here came from a build record, so every one of them is the mesh's own.** That +// is what makes it safe to normalise: references kept before the store's address stopped being +// written are `:/@sha256:…` (04-ISSUES/102), and `Recorded` reads those as the +// `artifact-store://` references the rest of the mesh uses. Done here rather than when the store +// is asked, because `Recorded` cannot tell one registry host from another — only the provenance +// can, and the provenance is here. +// +// The oldest artifacts are exactly the ones recorded the old way, and exactly the ones a +// sweep reaches first. Untranslated, the first of them ended every sweep. +func asRecorded(reference string) string { + if reference == "" { + return "" + } + return catalogue.Recorded(reference) +} + // digestIn is the `sha256:` a reference names, empty when it names none. func digestIn(reference string) string { for _, marker := range []string{"@sha256:", "/sha256:"} { diff --git a/internal/inventory/collection_test.go b/internal/inventory/collection_test.go index b28c3ec..c7aaf77 100644 --- a/internal/inventory/collection_test.go +++ b/internal/inventory/collection_test.go @@ -145,3 +145,48 @@ func TestAFailedBuildNamesNothingToCollectAndEachModuleIsCountedOnItsOwn(t *test t.Fatalf("offered %v; want only web's oldest — db's three are all within its five", go_) } } + +// An artifact recorded with the store's old address is offered for collection, in the vocabulary +// the rest of the mesh speaks (novox/hq issue 226). +// +// Before references were kept without an address the mesh recorded +// `:/@sha256:…` (04-ISSUES/102). Those are the oldest artifacts, which makes +// them exactly the ones an oldest-first sweep reaches first — and the first live run met one, +// read "I will not address this" as "the store refuses everything", and collected none of 1681. +func TestAnArtifactRecordedWithAnAddressIsOfferedAsTheMeshRecordsOne(t *testing.T) { + inv := fresh(t) + ctx := context.Background() + + // The oldest build published the old way; five newer ones fill the module's five. + old := aBuild("a00", "tools", "") + old.Made = []Artifact{{Name: "build", Kind: "image", + Reference: "127.0.0.1:5100/tools/build@sha256:" + fmt.Sprintf("%064x", 1)}} + if err := inv.RecordBuild(ctx, old); err != nil { + t.Fatal(err) + } + for i := 2; i <= 6; i++ { + built(t, inv, fmt.Sprintf("a%02d", i), "tools", i) + } + + go_, err := inv.ToCollect(ctx) + if err != nil { + t.Fatal(err) + } + want := ref("tools", "build", 1) + if len(go_) != 1 || go_[0] != want { + t.Fatalf("offered %v; want %q — the address is a route to the artifact, not part of its "+ + "name, and the sweep speaks the name", go_, want) + } + // And marking it collected uses that same name, so the next sweep does not offer it again + // under a spelling it has not seen. + if err := inv.MarkCollected(ctx, go_); err != nil { + t.Fatal(err) + } + again, err := inv.ToCollect(ctx) + if err != nil { + t.Fatal(err) + } + if len(again) != 0 { + t.Fatalf("offered %v again after collecting it", again) + } +}