diff --git a/cmd/mesh-controller/asker.go b/cmd/mesh-controller/asker.go index 23eaa01a..d9a43c4f 100644 --- a/cmd/mesh-controller/asker.go +++ b/cmd/mesh-controller/asker.go @@ -892,7 +892,7 @@ func (a *asker) Decided(ctx context.Context, body []byte) error { // A shared folder taken on the operator's warrant (novox/hq ADR 0325 §8): the act is composed again from the // record and held to what the option bound before the engine is asked. outcome, acted = a.decideTake(ctx, *r, act, w) - if acted == nil { + if acted == nil && !strings.HasPrefix(outcome, "nothing") { outcome = "done: " + outcome } case r.Take != nil: diff --git a/cmd/mesh-controller/gate.go b/cmd/mesh-controller/gate.go index 04df137f..c6f36cdb 100644 --- a/cmd/mesh-controller/gate.go +++ b/cmd/mesh-controller/gate.go @@ -814,11 +814,18 @@ func movesChangingAccesses(ctx context.Context, inv *inventory.Inventory, g *inv out[j.module] = true continue } - out[j.module] = !sameAccesses(before.Accesses, target.Accesses) + out[j.module] = !sameAccessUse(before, target) } return out } +// sameAccessUse says a move leaves a module's accesses as they were, users included (the review of e1c2b744): the +// same accesses at the same use, and the same containers declaring identity and mounting each — a container that +// starts or stops running as the module's identity changes who uses the folder. +func sameAccessUse(a, b catalogue.Manifest) bool { + return sameAccesses(a.Accesses, b.Accesses) && catalogue.AccessUsersShape(a) == catalogue.AccessUsersShape(b) +} + // sameAccesses says two manifests declare the same accesses, at the same use, in any order. func sameAccesses(a, b []catalogue.Access) bool { key := func(as []catalogue.Access) string { diff --git a/cmd/mesh-controller/kept_folders.go b/cmd/mesh-controller/kept_folders.go index 16cca5e9..369017d9 100644 --- a/cmd/mesh-controller/kept_folders.go +++ b/cmd/mesh-controller/kept_folders.go @@ -235,6 +235,24 @@ func (a *asker) decideTake(ctx context.Context, r asked, act conditions.Action, if a.take == nil { return "", errors.New("this controller cannot take a folder on a warrant") } + // **The folder must still wait on the filesystem the operator was asked about** (the review of e1c2b744): a take + // is about no condition, so nothing else rechecks it. A folder that moved to another filesystem, or no longer + // waits for the operator's word, is not taken: the ask is spent, and one for the filesystem it is on now is asked. + if a.folders != nil { + waiting, err := a.folders(ctx) + if err != nil { + return "", fmt.Errorf("whether the folder still waits on %s could not be read, so nothing is taken: %w", + t.Node, err) + } + still := false + for _, now := range waiting { + still = still || now.key() == t.key() + } + if !still { + return "nothing: the folder no longer waits to be kept on the filesystem the operator was asked about " + + "(it moved, or was taken or released meanwhile); asked again where it waits now", nil + } + } by := fmt.Sprintf("approved by %s via %s at %s (ask %s)", byWords(w), viaWords(w), w.At.Local().Format("2006-01-02 15:04"), r.ID) return a.take(ctx, t, r.ID, by) diff --git a/cmd/mesh-controller/kept_folders_test.go b/cmd/mesh-controller/kept_folders_test.go index 7ee4a5c0..e27b5998 100644 --- a/cmd/mesh-controller/kept_folders_test.go +++ b/cmd/mesh-controller/kept_folders_test.go @@ -404,8 +404,8 @@ func TestAFolderThatMovedIsAskedAboutAgain(t *testing.T) { } } -// The mesh's entries left while a disk was away (kind kept-entries), stated before the send, do not hold a delivery -// that did not bring them; stated since, they do — and they may name an access no longer declared. +// The mesh's entries left while a disk was away (kind kept-entries) do not hold a delivery whose move leaves the +// module's accesses as they were — they may name an access no longer declared; one that changes them is held. func TestEntriesLeftWhileADiskWasAwayDoNotHoldADeliveryThatDidNotBringThem(t *testing.T) { now := time.Now() since := now.Add(-time.Minute) @@ -413,15 +413,19 @@ func TestEntriesLeftWhileADiskWasAwayDoNotHoldADeliveryThatDidNotBringThem(t *te Target: moviesAt, State: link.StateUnhealthy, Reason: "the mesh's entries could not be taken out: its disk is away", Since: since.Add(-time.Hour)} f := gateFacts{now: now, waits: accessFacts(), groupsAdded: map[string]bool{"radarr": false}, - health: map[string]inventory.NodeHealth{"ace": {Node: "ace", HeardAt: now, - Resources: []inventory.ResourceHealth{left}}}} + accessesChanged: map[string]bool{"radarr": false}, health: map[string]inventory.NodeHealth{"ace": {Node: "ace", + HeardAt: now, Resources: []inventory.ResourceHealth{left}}}} if h, why := moduleHealthWord("radarr", "ace", since, f); h != healthPerson || !strings.Contains(why, "access-gone~entries") { t.Fatalf("entries left before the send read %v %q; want carried in a passing verdict", h, why) } left.Since = now f.health["ace"] = inventory.NodeHealth{Node: "ace", HeardAt: now, Resources: []inventory.ResourceHealth{left}} + if h, _ := moduleHealthWord("radarr", "ace", since, f); h != healthPerson { + t.Fatalf("entries stated since the send by a move that leaves accesses as they were read %v", h) + } + f.accessesChanged = nil // a move not known changes them if h, _ := moduleHealthWord("radarr", "ace", since, f); h != healthNotYet { - t.Fatalf("entries left by this send read %v; want judged unhealthy", h) + t.Fatalf("entries of a move not known read %v; want judged unhealthy", h) } } @@ -494,3 +498,66 @@ func TestTheEnginesAccessWaitsAndEntriesAreKept(t *testing.T) { t.Fatalf("the listing reads:\n%s", all) } } + +// Review of e1c2b744: a warrant for a folder on one filesystem never takes it on another. When the warrant comes and +// the folder no longer waits on the filesystem the ask named, nothing is sent, the ask is spent, and the operator is +// asked again for the filesystem it is on now. +func TestAWarrantForOneFilesystemIsNotCarriedOutOnAnother(t *testing.T) { + placed := "fs-000000000001" + r, taken := takeRig(t, func() map[string]map[string][]waitingFolder { + return waitingOn("ace", waitingFolder{module: "radarr", access: "movies", resource: "radarr.access-movies", + placed: placed}) + }) + ctx := context.Background() + if err := r.a.reconcile(ctx); err != nil { + t.Fatal(err) + } + a := takeAsks(r)[0] + placed = "fs-0000000000ff" // the folder moved before the operator answered + answerWith(t, r, warrantOn(a, "let-the-mesh-keep-it", r.now)) + if len(*taken) != 0 { + t.Fatalf("a warrant for %s was carried out on %s: %v", a.Take.Placed, placed, *taken) + } + if spent := r.store[a.ID]; spent.State == askOpen || !strings.HasPrefix(spent.Acted, "nothing") { + t.Fatalf("the ask after the folder moved: %s, %q", spent.State, spent.Acted) + } + if err := r.a.reconcile(ctx); err != nil { + t.Fatal(err) + } + var again []asked + for _, x := range takeAsks(r) { + if x.State == askOpen { + again = append(again, x) + } + } + if len(again) != 1 || again[0].Take.Placed != placed { + t.Fatalf("not asked again for the filesystem the folder is on now: %+v", again) + } +} + +// Review of e1c2b744: a move that changes who uses an access — a container of the module that starts declaring +// identity — changes the access, so an access the engine cannot judge is not passed uncounted. +func TestAMoveThatChangesAnAccesssUsersChangesTheAccess(t *testing.T) { + before := catalogue.Manifest{Module: "radarr", Accesses: radarrManifest.Accesses, Resources: []map[string]any{ + {"id": "app", "type": "container", "volumes": []any{"${access:movies}:/movies"}}}} + after := catalogue.Manifest{Module: "radarr", Accesses: radarrManifest.Accesses, Resources: []map[string]any{ + {"id": "app", "type": "container", "identity": map[string]any{"user": "run"}, + "volumes": []any{"${access:movies}:/movies"}}}} + if sameAccessUse(before, after) { + t.Fatal("a move that adds identity to the container mounting the access left it the same") + } + if !sameAccessUse(before, before) { + t.Fatal("an unchanged manifest reads changed") + } + // Entries left while a disk was away, stated before the send, of a move that changes the access: counted. + now := time.Now() + since := now.Add(-time.Minute) + left := inventory.ResourceHealth{Module: "radarr", Resource: "radarr.access-movies~entries", Kind: link.KindKeptEntries, + Target: moviesAt, State: link.StateUnhealthy, Since: since.Add(-time.Hour)} + f := gateFacts{now: now, waits: accessFacts(), groupsAdded: map[string]bool{"radarr": false}, + accessesChanged: map[string]bool{"radarr": true}, health: map[string]inventory.NodeHealth{"ace": {Node: "ace", + HeardAt: now, Resources: []inventory.ResourceHealth{left}}}} + if h, _ := moduleHealthWord("radarr", "ace", since, f); h != healthNotYet { + t.Fatalf("entries left before the send by a move that changes the access read %v; want counted", h) + } +} diff --git a/cmd/mesh-controller/module_health.go b/cmd/mesh-controller/module_health.go index b1db79a5..f0216f1d 100644 --- a/cmd/mesh-controller/module_health.go +++ b/cmd/mesh-controller/module_health.go @@ -678,12 +678,13 @@ func moduleHealthWord(module, machine string, since time.Time, f gateFacts) (hea continue } // **The mesh's entries left on a folder whose disk was away are a condition this build found** when its move - // leaves the module's accesses as they were, or they were stated before this send (novox/hq ADR 0325 §8): they - // may name an access no longer declared, they wait for the disk, and a delivery that did not bring them is not - // held for them — said in the verdict, as a directory used as found is. Otherwise they are judged unhealthy. + // leaves the module's accesses and their users as they were (novox/hq ADR 0325 §8): they may name an access no + // longer declared, they wait for the disk, and a delivery that did not bring them is not held for them — said in + // the verdict, as a directory used as found is. A move that changes the accesses is held to them, whenever they + // were stated (the review of e1c2b744). changed, known := f.accessesChanged[module] changes := !known || changed - if r.Kind == link.KindKeptEntries && r.State == link.StateUnhealthy && (!changes || r.Since.Before(since)) { + if r.Kind == link.KindKeptEntries && r.State == link.StateUnhealthy && !changes { leftover = append(leftover, r.Resource) continue } diff --git a/cmd/mesh-controller/plan.go b/cmd/mesh-controller/plan.go index 0311bb4f..7a84b392 100644 --- a/cmd/mesh-controller/plan.go +++ b/cmd/mesh-controller/plan.go @@ -977,7 +977,7 @@ func renderingFor(ctx context.Context, open *stores, node string, return catalogue.Rendering{}, inventory.Node{}, err } // Each module's identity and the folders taken here (novox/hq ADR 0325 §3, §8), to an engine that keeps folders. - keepsFolders, identities, keptFolders, err := folderFacts(ctx, inv, node, plan, choosing) + keepsFolders, identities, keptFolders, readyFolders, err := folderFacts(ctx, inv, node, plan, choosing) if err != nil { return catalogue.Rendering{}, inventory.Node{}, err } @@ -987,6 +987,7 @@ func renderingFor(ctx context.Context, open *stores, node string, KeepsFolders: keepsFolders, Identities: identities, KeptFolders: keptFolders, + ReadyFolders: readyFolders, BusMembership: memberships[node], Settings: settings, Generators: gens, Grants: grants, Needed: needed, Foreseen: foreseen, Ports: ports, Certificate: certificate, Authority: authority, Mesh: private, Names: names, @@ -1035,10 +1036,10 @@ func engineKeepsFolders(ctx context.Context, inv *inventory.Inventory, node stri // (Allocating), as a port is: a question about what would be sent reads what was allocated, and a module that has // none yet is composed without one. Nothing is read for an engine that does not keep folders. func folderFacts(ctx context.Context, inv *inventory.Inventory, node string, plan catalogue.Resolution, - choosing Choosing) (bool, map[string]catalogue.ModuleIdentity, map[string]bool, error) { + choosing Choosing) (bool, map[string]catalogue.ModuleIdentity, map[string]bool, map[string]bool, error) { keeps, err := engineKeepsFolders(ctx, inv, node) if err != nil || !keeps { - return false, nil, nil, err + return false, nil, nil, nil, err } identities := map[string]catalogue.ModuleIdentity{} for _, m := range plan.Modules { @@ -1053,7 +1054,7 @@ func folderFacts(ctx context.Context, inv *inventory.Inventory, node string, pla id, had, err = inv.IdentityOf(ctx, m.Module) } if err != nil { - return false, nil, nil, err + return false, nil, nil, nil, err } if had { identities[m.Module] = id @@ -1061,7 +1062,7 @@ func folderFacts(ctx context.Context, inv *inventory.Inventory, node string, pla } taken, err := inv.KeptFolders(ctx, node) if err != nil { - return false, nil, nil, err + return false, nil, nil, nil, err } kept := map[string]bool{} for path := range taken { @@ -1069,9 +1070,17 @@ func folderFacts(ctx context.Context, inv *inventory.Inventory, node string, pla } // **A folder that moved to another filesystem is not taken there until its new warrant** (ADR 0325 §8): the // machine's newest statement names, without a path, the filesystem each folder waiting for the operator is on now. + // And every folder the engine says it keeps (its access healthy): one it found empty, or took itself — what, with + // a taken one, lets a container switch to its identity (identityHeld). + ready := map[string]bool{} if stated, had, err := inv.HealthOf(ctx, node); err != nil { - return false, nil, nil, err + return false, nil, nil, nil, err } else if had { + for _, r := range stated.Resources { + if r.Kind == link.KindAccess && r.State == link.StateHealthy && r.Target != "" { + ready[filepath.Clean(r.Target)] = true + } + } for _, r := range stated.Resources { for _, w := range r.Waits { path := filepath.Clean(r.Target) @@ -1081,7 +1090,7 @@ func folderFacts(ctx context.Context, inv *inventory.Inventory, node string, pla } } } - return true, identities, kept, nil + return true, identities, kept, ready, nil } // zonesInTheMesh is every zone a module in the mesh declares, where the mesh placed it (novox/hq ADR diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index 5cd2345d..abe22e32 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -257,6 +257,9 @@ type Rendering struct { Identities map[string]ModuleIdentity // KeptFolders is every folder the operator let the mesh keep on this machine (ADR 0325 §8), by its clean path. KeptFolders map[string]bool + // ReadyFolders is every folder the engine states it keeps on this machine (its access healthy), by clean path: a + // folder it found empty and kept, or took itself. With KeptFolders, what lets a container switch to its identity. + ReadyFolders map[string]bool } // RootField is a user resource's field saying the account must never become root without a person @@ -583,6 +586,9 @@ func (r Resolution) compose(with Rendering, owner map[string]string, *identities = append(*identities, ident) } mountedByIdentity := dirsMountedByIdentity(m) + // Its containers run as it only once every read-write folder they mount is taken (the operator's question on + // ADR 0325): until then the identity is allocated, sent and named in the take's users, and not switched to. + runsAsIdent := hasIdent && !identityHeld(m, with) // What the mesh computes for this module goes FIRST, before the module's own resources. // @@ -1036,7 +1042,7 @@ func (r Resolution) compose(with Rendering, owner map[string]string, ownerInto(copied, placed) // And its identity (novox/hq ADR 0325 §2, §3): a container's resolved, or taken out where none is composed; // the owner of a directory its identity's containers mount. After the placement's owner: the identity wins. - if err := identityInto(copied, m, ident, hasIdent, mountedByIdentity); err != nil { + if err := identityInto(copied, m, ident, runsAsIdent, mountedByIdentity); err != nil { return nil, err } // **After settings, and that is the whole reason it is here.** A module's file diff --git a/internal/catalogue/module_identity.go b/internal/catalogue/module_identity.go index c87ce4ed..c715fa2c 100644 --- a/internal/catalogue/module_identity.go +++ b/internal/catalogue/module_identity.go @@ -187,6 +187,24 @@ func volumeSource(v string) string { return v } +// AccessUsersShape is who among a module's containers uses which access as the module's identity, in a form that +// compares: each container declaring identity with how it is told, and the accesses it mounts, sorted. Two manifests +// with the same shape give their accesses the same container users. +func AccessUsersShape(m Manifest) string { + var out []string + for _, r := range m.Resources { + v, declares := r[IdentityField] + if fmt.Sprint(r["type"]) != "container" || !declares { + continue + } + _, accesses := containerMounts(r) + sort.Strings(accesses) + out = append(out, fmt.Sprint(r["id"], v, accesses)) + } + sort.Strings(out) + return strings.Join(out, "\x01") +} + // accessMode is the mode an access a container mounts was declared at, by id or by "path:"; "" when the // manifest declares no such access. func accessMode(m Manifest, ref string) string { @@ -387,6 +405,49 @@ func identityInto(resource map[string]any, m Manifest, id ModuleIdentity, has bo return nil } +// identityHeld says module m's containers are not switched to its identity yet (the operator's question on ADR 0325): +// a container declaring `identity` mounts a read-write access whose folder is neither taken (KeptFolders) nor kept by +// the engine (ReadyFolders). Switched before the take adds the identity's entry, the app would run as the new uid and +// lose write on the operator's existing files; held, it runs as it did (its environment's PUID and PGID), the take's +// users already name the identity, and the switch comes in the apply after every such folder is taken. +func identityHeld(m Manifest, with Rendering) bool { + _, byID, err := accessesFor(m, with.Settings[m.Module]) + if err != nil { + return true // refused where the module itself is composed; never switched on a guess + } + for _, r := range m.Resources { + if _, declares := r[IdentityField]; !declares || fmt.Sprint(r["type"]) != "container" { + continue + } + _, refs := containerMounts(r) + for _, ref := range refs { + if accessMode(m, ref) != AccessReadWrite { + continue + } + path := byID[ref] + if strings.HasPrefix(ref, "path:") { + path = strings.TrimPrefix(ref, "path:") + } + path = filepath.Clean(path) + if path != "." && !with.KeptFolders[path] && !with.ReadyFolders[path] { + return true + } + } + } + return false +} + +// identityContainers is the composed ids of module m's containers that declare `identity`, whether or not it is sent. +func identityContainers(m Manifest) map[string]bool { + out := map[string]bool{} + for _, r := range m.Resources { + if _, declares := r[IdentityField]; declares && fmt.Sprint(r["type"]) == "container" { + out[m.Module+"."+fmt.Sprint(r["id"])] = true + } + } + return out +} + // dirsMountedByIdentity is the ids of module m's directories one of its containers declaring `identity` mounts. func dirsMountedByIdentity(m Manifest) map[string]bool { out := map[string]bool{} @@ -442,12 +503,13 @@ func (r Resolution) toolsAccount(m Manifest, with Rendering) string { // module's tools run as it, each at the access's use (§4: the engine keeps the widest). func usersInto(out []map[string]any, owner map[string]string, m Manifest, id ModuleIdentity, has bool, account string, with Rendering) { + // The containers that declare identity, whether or not it is sent yet: the take adds the entries of the identity + // to come, so they exist before the switch. + declaring := identityContainers(m) var containers []map[string]any for _, res := range out { - if res["type"] == "container" && owner[fmt.Sprint(res["id"])] == m.Module { - if _, declares := res[IdentityField]; declares && has { - containers = append(containers, res) - } + if res["type"] == "container" && owner[fmt.Sprint(res["id"])] == m.Module && declaring[fmt.Sprint(res["id"])] && has { + containers = append(containers, res) } } for _, res := range out { diff --git a/internal/catalogue/module_identity_test.go b/internal/catalogue/module_identity_test.go index 3303ef5a..0ded5053 100644 --- a/internal/catalogue/module_identity_test.go +++ b/internal/catalogue/module_identity_test.go @@ -78,7 +78,7 @@ func TestAModuleRunsAsItsIdentityTheSameOnEveryMachine(t *testing.T) { var seen []string for _, node := range []string{"ace", "novox"} { r := Resolution{Node: node, Account: "jochen", Modules: []Manifest{aLibrarian(t), aPlayer(t), theRuntime(t)}} - composed := composeFolders(t, r, keepingFolders(identities)) + composed := composeFolders(t, r, keepingFolders(identities, "/storage/media/movies")) // its read-write folder taken: switched got := byID(composed.Resources) app := got["librarian.app"] @@ -331,3 +331,40 @@ func TestAModuleWithoutIdentityIsWarnedNotRefused(t *testing.T) { t.Error("an identity module was warned rather than refused") } } + +// A container is not switched to its identity while a read-write folder it mounts holds data the mesh has not taken +// (the operator's question on ADR 0325): the app would run as the new uid before the take adds its entry, and lose +// write on the existing files. Its identity is held — no `identity` sent, its directories keep their owner — while +// the take's users already name the identity, so the entries exist before the switch; once every such folder is +// taken (or kept by the engine), the identity is sent in one apply. +func TestAContainerKeepsItsOldIdentityUntilItsDataIsTaken(t *testing.T) { + r := Resolution{Node: "ace", Account: "jochen", Modules: []Manifest{aLibrarian(t), theRuntime(t)}} + held := byID(composeFolders(t, r, keepingFolders(identities)).Resources) + if _, has := held["librarian.app"][IdentityField]; has { + t.Fatalf("the container was told its identity while its movies are not taken: %v", held["librarian.app"][IdentityField]) + } + if owner, has := held["librarian.config"]["owner"]; has { + t.Fatalf("its directory was given the owner %v while its identity is held", owner) + } + users, _ := json.Marshal(held["librarian."+AccessID("/storage/media/movies")]["users"]) + if !strings.Contains(string(users), `"identity":"mesh-librarian","uid":50001`) { + t.Fatalf("the access does not name the identity to come, so the take would not add its entry: %s", users) + } + + for name, with := range map[string]Rendering{ + "taken": keepingFolders(identities, "/storage/media/movies"), + "kept by the engine": func() Rendering { + w := keepingFolders(identities) + w.ReadyFolders = map[string]bool{"/storage/media/movies": true} + return w + }(), + } { + got := byID(composeFolders(t, r, with).Resources) + if _, has := got["librarian.app"][IdentityField]; !has { + t.Errorf("%s: the container is not told its identity", name) + } + if got["librarian.config"]["owner"] != "50001:50001" { + t.Errorf("%s: its directory is owned by %v", name, got["librarian.config"]["owner"]) + } + } +} diff --git a/internal/inventory/catalogue.go b/internal/inventory/catalogue.go index 1d586504..652dc5ee 100644 --- a/internal/inventory/catalogue.go +++ b/internal/inventory/catalogue.go @@ -970,6 +970,17 @@ func (i *Inventory) judgeSettings(ctx context.Context, nodeName, module string, if aerr != nil { return aerr } + if !has { + follows, ferr := i.followsABranch(ctx, module) + if ferr != nil { + return ferr + } + if !follows { + return fmt.Errorf("refused: %s on %s cannot compose with the layer %q — %w; and %s follows no branch "+ + "(it was registered by hand), so no build of it can be judged ahead of what is registered", module, + where, from, err, module) + } + } if !has || catalogue.JudgeSettings(ahead, layers, adopted) != nil || catalogue.ComposesWith(m, layers, adopted) != nil { return fmt.Errorf("refused: %s on %s cannot compose with the layer %q — %w; nor does the newest build of "+ "%s on the branch it follows declare it", module, where, from, err, module) @@ -990,15 +1001,26 @@ func (i *Inventory) judgeSettings(ctx context.Context, nodeName, module string, return nil } +// followsABranch says a module's record names the branch it follows. +func (i *Inventory) followsABranch(ctx context.Context, module string) (bool, error) { + var ref string + err := i.store.Pool().QueryRow(ctx, `select coalesce(ref, '') from module where name = $1`, module).Scan(&ref) + if errors.Is(err, pgx.ErrNoRows) { + return false, nil + } + return ref != "", err +} + // NewestManifestOnBranch is the manifest of a module's newest successful build on the branch its record follows // (novox/hq issue 575): what the delivery under way brings, before it registers — or the registered one's own, when -// nothing newer was built. False when no build of the module with a manifest is kept. +// nothing newer was built. Only a build of that very branch: never one of another branch or of none. False when no +// such build is kept, and for a module whose record follows no branch (registered by hand). func (i *Inventory) NewestManifestOnBranch(ctx context.Context, module string) (catalogue.Manifest, bool, error) { var raw []byte err := i.store.Pool().QueryRow(ctx, `select b.manifest from build b join module m on m.name = b.module where b.module = $1 and b.failed = '' and b.manifest is not null and b.manifest::text <> 'null' - and (coalesce(m.ref, '') = '' or b.ref = '' or b.ref = m.ref) + and coalesce(m.ref, '') <> '' and b.ref = m.ref order by b.at desc limit 1`, module).Scan(&raw) if errors.Is(err, pgx.ErrNoRows) { return catalogue.Manifest{}, false, nil diff --git a/internal/inventory/identities_test.go b/internal/inventory/identities_test.go index 1e6d303c..3d979e9f 100644 --- a/internal/inventory/identities_test.go +++ b/internal/inventory/identities_test.go @@ -2,6 +2,7 @@ package inventory import ( "encoding/json" + "fmt" "strings" "testing" @@ -168,3 +169,56 @@ func TestAPlacementIsJudgedAgainstTheBuildThatBringsTheAccess(t *testing.T) { t.Fatal("a placement no build declares was taken") } } + +// Review of e1c2b744: only a build of the branch the module's record follows judges a placement ahead of it — never one +// from another branch, nor one with no branch, and a module registered with no branch is refused as before, saying so. +func TestOnlyABuildOfTheFollowedBranchJudgesAPlacementAhead(t *testing.T) { + inv := fresh(t) + ctx := t.Context() + if _, err := inv.AddNode(ctx, "ace"); err != nil { + t.Fatal(err) + } + registered := catalogue.Manifest{Module: "importer", Version: "1", + Resources: []map[string]any{{"id": "server", "type": "container", "name": "importer", "image": "x"}}} + ahead := registered + ahead.Accesses = []catalogue.Access{{ID: "movies", Path: "/services/media/movies", Mode: catalogue.AccessReadWrite}} + raw, err := json.Marshal(ahead) + if err != nil { + t.Fatal(err) + } + placing := map[string]any{catalogue.AccessesSetting: map[string]any{"movies": "/storage/media/movies"}} + if err := inv.RegisterModule(ctx, registered, Source{Repository: "https://forge.invalid/media.git", Ref: "main", + BuiltFrom: "c0ffee"}); err != nil { + t.Fatal(err) + } + for i, ref := range []string{"feature", ""} { + b := aBuild(fmt.Sprintf("elsewhere%d", i), "importer", "") + b.Ref, b.Manifest = ref, raw + if err := inv.RecordBuild(ctx, b); err != nil { + t.Fatal(err) + } + } + if err := inv.JudgeSettings(ctx, "ace", "importer", placing); err == nil { + t.Fatal("a build of another branch, or of none, judged the placement") + } + // Registered by hand, with no branch: refused, saying why. + byHand := registered + byHand.Module = "byhand" + if err := inv.RegisterModule(ctx, byHand, Source{}); err != nil { + t.Fatal(err) + } + aheadByHand := ahead + aheadByHand.Module = "byhand" + if raw, err = json.Marshal(aheadByHand); err != nil { + t.Fatal(err) + } + b := aBuild("onmain", "byhand", "") + b.Ref, b.Manifest = "main", raw + if err := inv.RecordBuild(ctx, b); err != nil { + t.Fatal(err) + } + if err := inv.JudgeSettings(ctx, "ace", "byhand", placing); err == nil || + !strings.Contains(err.Error(), "follows no branch") { + t.Fatalf("a module with no branch recorded: %v", err) + } +} diff --git a/testdata/store-tests b/testdata/store-tests index 9b5358a5..5f7bf96b 100644 --- a/testdata/store-tests +++ b/testdata/store-tests @@ -34,3 +34,4 @@ ./internal/inventory TestAModuleIsGivenOneIdentityOnceAndNeverRoot ./internal/inventory TestAFolderIsTakenOnceUnderAWarrant ./internal/inventory TestAPlacementIsJudgedAgainstTheBuildThatBringsTheAccess +./internal/inventory TestOnlyABuildOfTheFollowedBranchJudgesAPlacementAhead