From f151de103fda8295ee695910d6b8cbd8f1fd662f Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 12 Sep 2026 16:45:50 +0200 Subject: [PATCH] Build a module from a repository and a path within it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The builder cloned a repository and read the manifest at its root, which means one repository per module. Nothing we have is shaped that way, so the builder could be asked to build nothing that exists (novox/hq ADR 0069). The path travels the whole way — named when asking, carried in the request, used to read the manifest and as the context everything is produced from, echoed back in the result, and recorded as part of where a module came from. Without that last part the mesh could notice a module was behind its source and then be unable to rebuild it, which is the worst of both. A path climbing out of the clone is refused: a machine whose job is building other people's repositories must not read whatever else is on its disk. Claude-Session: https://claude.ai/code/session_01D6qtiYU3P9jk3pnAXyAFyx --- cmd/mesh-builder/main.go | 8 ++- cmd/mesh-control/build.go | 24 +++++--- internal/builder/builder.go | 49 +++++++++++++-- internal/builder/builder_test.go | 60 ++++++++++++++++--- internal/inventory/catalogue.go | 27 ++++++--- ...20-a-module-is-a-repository-and-a-path.sql | 16 +++++ internal/link/build.go | 9 ++- 7 files changed, 160 insertions(+), 33 deletions(-) create mode 100644 internal/inventory/migrations/0020-a-module-is-a-repository-and-a-path.sql diff --git a/cmd/mesh-builder/main.go b/cmd/mesh-builder/main.go index 5e9b7f8..2724459 100644 --- a/cmd/mesh-builder/main.go +++ b/cmd/mesh-builder/main.go @@ -152,16 +152,20 @@ func answer(ctx context.Context, channel *amqp.Channel, publisher builder.Publis } result := link.BuildResult{ - ID: request.ID, Repository: request.Repository, Ref: request.Ref, On: on, + ID: request.ID, Repository: request.Repository, Path: request.Path, + Ref: request.Ref, On: on, } fmt.Printf("building %s", request.Repository) + if request.Path != "" { + fmt.Printf(" at %s", request.Path) + } if request.Ref != "" { fmt.Printf(" at %s", request.Ref) } fmt.Println() built, err := builder.Build(ctx, builder.Command, publisher, - request.Repository, request.Ref, workspace) + request.Repository, request.Path, request.Ref, workspace) if err != nil { // A failure is a result. A build that fails and says nothing is indistinguishable from a // builder that is not running, and those want completely different responses. diff --git a/cmd/mesh-control/build.go b/cmd/mesh-control/build.go index 4204f39..76b7103 100644 --- a/cmd/mesh-control/build.go +++ b/cmd/mesh-control/build.go @@ -33,6 +33,9 @@ import ( func buildCommand(ctx context.Context, args []string) error { set := flag.NewFlagSet("build", flag.ContinueOnError) ref := set.String("ref", "", "the branch, tag or commit to build") + // A module is a repository and a path within it (novox/hq ADR 0069). Empty is the repository's + // root, which is the ordinary case and why this is a flag rather than a second argument. + path := set.String("path", "", "the module's directory inside the repository") wait := set.Duration("wait", 10*time.Minute, "how long to wait for a builder to answer") dryRun := set.Bool("dry-run", false, "build and print the manifest, recording nothing") // Every module whose source has moved, rather than one named repository. @@ -58,9 +61,9 @@ func buildCommand(ctx context.Context, args []string) error { } if *dryRun { - return buildAndShow(ctx, positionals[0], *ref, *wait) + return buildAndShow(ctx, positionals[0], *path, *ref, *wait) } - return buildOne(ctx, positionals[0], *ref, *wait) + return buildOne(ctx, positionals[0], *path, *ref, *wait) } // buildFrom turns what a builder said into what the mesh keeps. @@ -300,7 +303,7 @@ func buildBehind(ctx context.Context, wait time.Duration) error { // Its own recorded ref, not its head commit: a module tracking a branch should be built // from that branch, and pinning to the commit the mesh happened to notice would quietly // turn a tracked branch into a pin. - if err := buildOne(ctx, e.Source.Repository, e.Source.Ref, wait); err != nil { + if err := buildOne(ctx, e.Source.Repository, e.Source.Path, e.Source.Ref, wait); err != nil { fmt.Printf(" %v\n", err) failed = append(failed, e.Manifest.Module) } @@ -318,7 +321,7 @@ func buildBehind(ctx context.Context, wait time.Duration) error { // buildOne asks a build machine for one repository and records everything that came back. // // Separated from the command so `--behind` can walk a list without a second path to the same act. -func buildOne(ctx context.Context, repository, ref string, wait time.Duration) error { +func buildOne(ctx context.Context, repository, path, ref string, wait time.Duration) error { ident, err := openIdentity(ctx) if err != nil { return err @@ -336,11 +339,15 @@ func buildOne(ctx context.Context, repository, ref string, wait time.Duration) e request := link.BuildRequest{ ID: fmt.Sprintf("%s-%d", "build", time.Now().UnixNano()), Repository: repository, + Path: path, Ref: ref, } fmt.Printf("asked for %s", request.Repository) + if path != "" { + fmt.Printf(" at %s", path) + } if ref != "" { - fmt.Printf(" at %s", ref) + fmt.Printf(" on %s", ref) } fmt.Println() @@ -383,7 +390,7 @@ func buildOne(ctx context.Context, repository, ref string, wait time.Duration) e // Recorded with where it came from, so "is this current?" is answerable without building it // again (novox/hq ADR 0009). if err := inv.RegisterModule(ctx, manifest, inventory.Source{ - Repository: result.Repository, Ref: result.Ref, + Repository: result.Repository, Path: result.Path, Ref: result.Ref, BuiltFrom: result.Commit, Head: result.Commit, }); err != nil { return err @@ -395,7 +402,7 @@ func buildOne(ctx context.Context, repository, ref string, wait time.Duration) e } // buildAndShow builds and prints the manifest without recording anything. -func buildAndShow(ctx context.Context, repository, ref string, wait time.Duration) error { +func buildAndShow(ctx context.Context, repository, path, ref string, wait time.Duration) error { ident, err := openIdentity(ctx) if err != nil { return err @@ -408,7 +415,8 @@ func buildAndShow(ctx context.Context, repository, ref string, wait time.Duratio defer server.Close() result, err := link.RequestBuild(ctx, server.Channel(), link.BuildRequest{ - ID: fmt.Sprintf("%s-%d", "build", time.Now().UnixNano()), Repository: repository, Ref: ref, + ID: fmt.Sprintf("%s-%d", "build", time.Now().UnixNano()), + Repository: repository, Path: path, Ref: ref, }, wait) if err != nil { return err diff --git a/internal/builder/builder.go b/internal/builder/builder.go index 7574280..bc1ba2b 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -57,7 +57,7 @@ type Result struct { // archive failed would otherwise leave half of itself in the store under a digest the mesh never // records — reachable, unreferenced, and indistinguishable from something in use. func Build(ctx context.Context, run Runner, publish Publisher, - repository, ref, workspace string) (Result, error) { + repository, path, ref, workspace string) (Result, error) { // Made rather than required. A builder that fails because the directory it was told to work // in does not exist is a builder that needs a setup step nobody documented. @@ -85,11 +85,20 @@ func Build(ctx context.Context, run Runner, publish Publisher, } commit = strings.TrimSpace(commit) - raw, err := os.ReadFile(filepath.Join(tree, ManifestName)) + // A module is a repository and a path within it (novox/hq ADR 0069). The ordinary case is an + // empty path, meaning the repository's root; a repository holding several modules names each + // by its own directory, which is what the catalogue is and what the system this replaces has + // always done. + within, err := inside(tree, path) + if err != nil { + return Result{}, err + } + + raw, err := os.ReadFile(filepath.Join(within, ManifestName)) if err != nil { return Result{}, fmt.Errorf( - "%s has no %s at its root, so there is nothing saying what it is: %w", - repository, ManifestName, err) + "%s has no %s at %s, so there is nothing saying what it is: %w", + repository, ManifestName, describe(path), err) } manifest, err := catalogue.ParseManifest(raw) if err != nil { @@ -103,7 +112,7 @@ func Build(ctx context.Context, run Runner, publish Publisher, // logs can be compared. sort.Slice(artifacts, func(i, j int) bool { return artifacts[i].Name < artifacts[j].Name }) for _, a := range artifacts { - made, err := one(ctx, run, publish, manifest.Module, tree, commit, a) + made, err := one(ctx, run, publish, manifest.Module, within, commit, a) if err != nil { return Result{}, err } @@ -118,6 +127,36 @@ func Build(ctx context.Context, run Runner, publish Publisher, return Result{Manifest: resolved, Commit: commit, Built: built}, nil } +// inside resolves a module's path within a clone, and refuses one that leaves it. +// +// **A build reads only its own tree.** A path of `../../etc` would otherwise make a build read — +// and an archive artifact publish — whatever the build machine happens to hold, which is the one +// thing a machine that builds other people's repositories must not do. +func inside(tree, path string) (string, error) { + if path == "" { + return tree, nil + } + if filepath.IsAbs(path) { + return "", fmt.Errorf( + "a module's path is inside its repository, and %q is an absolute path", path) + } + within := filepath.Join(tree, path) + rel, err := filepath.Rel(tree, within) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf( + "%q leaves the repository, and a build reads only its own tree", path) + } + return within, nil +} + +// describe says where a manifest was looked for, in words a person can act on. +func describe(path string) string { + if path == "" { + return "its root" + } + return path +} + // ManifestName is the one file a module repository must have. // // At the root, and named the same in every repository. A convention somebody can look for beats a diff --git a/internal/builder/builder_test.go b/internal/builder/builder_test.go index 0719b1f..6ef8ea2 100644 --- a/internal/builder/builder_test.go +++ b/internal/builder/builder_test.go @@ -108,7 +108,7 @@ func TestABuildProducesAManifestThePinsAreIn(t *testing.T) { r, workspace := aRepository(t, withBoth, map[string]string{ "Dockerfile": "FROM scratch", "files/theme.conf": "dark", }) - got, err := Build(context.Background(), r.run, r, "https://forge.invalid/meshboard.git", "", workspace) + got, err := Build(context.Background(), r.run, r, "https://forge.invalid/meshboard.git", "", "", workspace) if err != nil { t.Fatal(err) } @@ -134,7 +134,7 @@ func TestTwoBuildsOfOneCommitProduceOneDigest(t *testing.T) { }) // A year apart, so a packer carrying timestamps cannot accidentally agree. r.stamped = time.Date(2020+i, time.March, 3, 4, 5, 6, 0, time.UTC) - got, err := Build(context.Background(), r.run, r, "https://forge.invalid/x.git", "", workspace) + got, err := Build(context.Background(), r.run, r, "https://forge.invalid/x.git", "", "", workspace) if err != nil { t.Fatal(err) } @@ -154,7 +154,7 @@ func TestNothingIsPublishedUntilEverythingIsBuilt(t *testing.T) { // unreferenced, and indistinguishable from something in use. r, workspace := aRepository(t, withBoth, map[string]string{"Dockerfile": "FROM scratch"}) // `files` is missing, so packing the archive fails — after the image would have been pushed. - _, err := Build(context.Background(), r.run, r, "https://forge.invalid/x.git", "", workspace) + _, err := Build(context.Background(), r.run, r, "https://forge.invalid/x.git", "", "", workspace) if err == nil { t.Fatal("a build with a missing input succeeded") } @@ -166,7 +166,7 @@ func TestNothingIsPublishedUntilEverythingIsBuilt(t *testing.T) { func TestARepositoryWithNoManifestSaysSo(t *testing.T) { workspace := t.TempDir() r := &recorded{contents: map[string]string{"README.md": "nothing to see"}} - _, err := Build(context.Background(), r.run, r, "https://forge.invalid/x.git", "", workspace) + _, err := Build(context.Background(), r.run, r, "https://forge.invalid/x.git", "", "", workspace) if err == nil { t.Fatal("a repository with nothing saying what it is was built") } @@ -179,7 +179,7 @@ func TestAModuleThatBuildsNothingStillProducesAManifest(t *testing.T) { // Most of what a person installs is configuration. r, workspace := aRepository(t, `{"module":"shell","version":"1","resources":[ {"id":"rc","type":"file","path":"/etc/zsh/zshrc","content":"setopt"}]}`, nil) - got, err := Build(context.Background(), r.run, r, "https://forge.invalid/shell.git", "", workspace) + got, err := Build(context.Background(), r.run, r, "https://forge.invalid/shell.git", "", "", workspace) if err != nil { t.Fatal(err) } @@ -209,7 +209,7 @@ func TestTheTreeIsFreshEveryTime(t *testing.T) { if err := os.WriteFile(leftover, []byte("stale"), 0o644); err != nil { t.Fatal(err) } - if _, err := Build(context.Background(), r.run, r, "https://forge.invalid/x.git", "", workspace); err != nil { + if _, err := Build(context.Background(), r.run, r, "https://forge.invalid/x.git", "", "", workspace); err != nil { t.Fatal(err) } if _, err := os.Stat(leftover); err == nil { @@ -222,7 +222,7 @@ func TestABuildThatCannotPushFails(t *testing.T) { "Dockerfile": "FROM scratch", "files/a": "b", }) r.failPush = true - if _, err := Build(context.Background(), r.run, r, "https://forge.invalid/x.git", "", workspace); err == nil { + if _, err := Build(context.Background(), r.run, r, "https://forge.invalid/x.git", "", "", workspace); err == nil { t.Fatal("a build that could publish nothing reported success") } } @@ -236,7 +236,7 @@ func TestAnUpstreamImageIsMirroredRatherThanBuilt(t *testing.T) { "resources":[{"id":"db","type":"container","name":"mesh-postgres","artifact":"store"}]}` r, workspace := aRepository(t, mirrors, nil) - got, err := Build(context.Background(), r.run, r, "https://forge.invalid/postgres.git", "", workspace) + got, err := Build(context.Background(), r.run, r, "https://forge.invalid/postgres.git", "", "", workspace) if err != nil { t.Fatal(err) } @@ -287,3 +287,47 @@ func TestAnUpstreamReferenceIsNotAPathInTheRepository(t *testing.T) { t.Fatalf("a perfectly ordinary upstream reference was refused: %v", err) } } + +// **A module is a repository and a path within it** (novox/hq ADR 0069). The catalogue holds its +// modules one to a directory and the system this replaces has always built one that way, so a +// builder that could only read a repository's root could build none of what exists. +func TestAModuleIsBuiltFromItsPathWithinTheRepository(t *testing.T) { + r := &recorded{contents: map[string]string{ + "README.md": "this repository holds several modules", + "modules/shell/" + ManifestName: withBoth, + "modules/shell/Dockerfile": "FROM scratch", + "modules/shell/files/theme.conf": "dark", + // A second module beside it, so what is built is chosen by the path rather than by + // happening to be the only manifest in the clone. + "modules/other/" + ManifestName: `{"module":"other","version":"1"}`, + }} + got, err := Build(context.Background(), r.run, r, + "https://forge.invalid/catalogue.git", "modules/shell", "", t.TempDir()) + if err != nil { + t.Fatal(err) + } + if got.Manifest.Module != "meshboard" { + t.Fatalf("built %q, which is not the module at the path asked for", got.Manifest.Module) + } + if got.Manifest.Resources[0]["image"] == nil { + t.Fatalf("the image was not pinned: %v", got.Manifest.Resources[0]) + } +} + +// A build reads only its own tree. A path climbing out of the clone would otherwise let a build +// read — and an archive artifact publish — whatever the build machine happens to hold, which is +// the one thing a machine that builds other people's repositories must not do. +func TestAPathThatLeavesTheRepositoryIsRefused(t *testing.T) { + for _, escaping := range []string{"../../etc", "/etc"} { + r := &recorded{contents: map[string]string{ManifestName: withBoth}} + _, err := Build(context.Background(), r.run, r, + "https://forge.invalid/x.git", escaping, "", t.TempDir()) + if err == nil { + t.Fatalf("%q was accepted as a module's path", escaping) + } + if !strings.Contains(err.Error(), "leaves the repository") && + !strings.Contains(err.Error(), "absolute path") { + t.Fatalf("the refusal of %q does not say why: %v", escaping, err) + } + } +} diff --git a/internal/inventory/catalogue.go b/internal/inventory/catalogue.go index 04e6e2f..90e4285 100644 --- a/internal/inventory/catalogue.go +++ b/internal/inventory/catalogue.go @@ -23,7 +23,10 @@ var ErrStillAssigned = errors.New("that module is still assigned to nodes") // Source is where a module comes from and what has been built from it. type Source struct { Repository string - Ref string + // Path is the module's directory inside that repository (novox/hq ADR 0069). Empty is the + // repository's root, which is a real answer rather than a missing one. + Path string + Ref string // BuiltFrom is the commit the manifest the mesh holds was read at. BuiltFrom string // Head is the newest commit the source is known to have. @@ -57,17 +60,19 @@ func (i *Inventory) RegisterModule(ctx context.Context, m catalogue.Manifest, fr // record of where the module normally comes from — which is the only thing that would say, // afterwards, that the machine is running something nobody can rebuild. _, err = i.store.Pool().Exec(ctx, - `insert into module (name, manifest, version, source, ref, built_from, source_head) - values ($1, $2, nullif($3,''), nullif($4,''), nullif($5,''), nullif($6,''), nullif($6,'')) + `insert into module (name, manifest, version, source, source_path, ref, built_from, source_head) + values ($1, $2, nullif($3,''), nullif($4,''), $7, nullif($5,''), nullif($6,''), nullif($6,'')) on conflict (name) do update set manifest = excluded.manifest, version = excluded.version, registered = now(), source = coalesce(excluded.source, module.source), + source_path = case when excluded.source is null then module.source_path + else excluded.source_path end, ref = coalesce(excluded.ref, module.ref), built_from = coalesce(excluded.built_from, module.built_from), source_head = coalesce(excluded.built_from, module.source_head)`, - m.Module, raw, m.Version, from.Repository, from.Ref, from.BuiltFrom) + m.Module, raw, m.Version, from.Repository, from.Ref, from.BuiltFrom, from.Path) return err } @@ -92,9 +97,10 @@ func (i *Inventory) SourceMoved(ctx context.Context, module, head string) error func (i *Inventory) SourceOf(ctx context.Context, module string) (Source, error) { var s Source var repo, ref, built, head *string + var path string err := i.store.Pool().QueryRow(ctx, - `select source, ref, built_from, source_head from module where name = $1`, - module).Scan(&repo, &ref, &built, &head) + `select source, source_path, ref, built_from, source_head from module where name = $1`, + module).Scan(&repo, &path, &ref, &built, &head) if errors.Is(err, pgx.ErrNoRows) { return Source{}, fmt.Errorf("%w: %s", ErrNoSuchModule, module) } @@ -109,6 +115,9 @@ func (i *Inventory) SourceOf(ctx context.Context, module string) (Source, error) *pair.to = *pair.from } } + // Not in the loop above: the path is never null, because "the repository's root" is an answer + // rather than an absence. + s.Path = path return s, nil } @@ -746,13 +755,13 @@ type Entry struct { func (i *Inventory) Catalogued(ctx context.Context) ([]Entry, error) { rows, err := i.store.Pool().Query(ctx, `select m.name, m.manifest, - coalesce(m.source, ''), coalesce(m.ref, ''), + coalesce(m.source, ''), m.source_path, coalesce(m.ref, ''), coalesce(m.built_from, ''), coalesce(m.source_head, ''), coalesce(array_agg(n.name order by n.name) filter (where n.name is not null), '{}') from module m left join assignment a on a.module = m.name left join node n on n.id = a.node - group by m.name, m.manifest, m.source, m.ref, m.built_from, m.source_head + group by m.name, m.manifest, m.source, m.source_path, m.ref, m.built_from, m.source_head order by m.name`) if err != nil { return nil, err @@ -765,7 +774,7 @@ func (i *Inventory) Catalogued(ctx context.Context) ([]Entry, error) { var name string var source Source var on []string - if err := rows.Scan(&name, &raw, &source.Repository, &source.Ref, + if err := rows.Scan(&name, &raw, &source.Repository, &source.Path, &source.Ref, &source.BuiltFrom, &source.Head, &on); err != nil { return nil, err } diff --git a/internal/inventory/migrations/0020-a-module-is-a-repository-and-a-path.sql b/internal/inventory/migrations/0020-a-module-is-a-repository-and-a-path.sql new file mode 100644 index 0000000..743908b --- /dev/null +++ b/internal/inventory/migrations/0020-a-module-is-a-repository-and-a-path.sql @@ -0,0 +1,16 @@ +-- Where inside its repository a module lives. +-- +-- novox/hq ADR 0069. A module is a repository and a path within it. The mesh recorded the +-- repository and the ref and not the path, which was survivable only while every module was +-- assumed to sit at a repository's root — an assumption that matched nothing that exists. The +-- catalogue holds its modules one to a directory, and the system this replaces has always built a +-- module from a repository and a path. +-- +-- Without this column the mesh can record that a module's source has moved ahead of what it holds +-- and then be unable to rebuild it, because it cannot say which part of the repository the module +-- is. That is the failure this prevents: noticing, and then not being able to act. +-- +-- Empty rather than null, and defaulted, because "the repository's root" is a real answer and the +-- ordinary one — not an absence. Every module recorded before this keeps exactly the meaning it +-- had. +alter table module add column source_path text not null default ''; diff --git a/internal/link/build.go b/internal/link/build.go index e67f487..2b44228 100644 --- a/internal/link/build.go +++ b/internal/link/build.go @@ -47,6 +47,10 @@ type BuildRequest struct { // Ref is the branch, tag or commit. Empty means whatever the repository's default is, which // is the only case where the mesh does not know what it built until it has built it. Ref string `json:"ref,omitempty"` + // Path is the module's directory inside that repository (novox/hq ADR 0069). Empty means the + // repository root, which is the ordinary case; a repository holding several modules names + // each by its own directory. + Path string `json:"path,omitempty"` } // BuildResult is what a builder says back. @@ -57,7 +61,10 @@ type BuildRequest struct { type BuildResult struct { ID string `json:"id"` Repository string `json:"repository"` - Ref string `json:"ref,omitempty"` + // Path is echoed back, so what the mesh records as this module's source is what was actually + // built rather than what the asker meant (novox/hq ADR 0069). + Path string `json:"path,omitempty"` + Ref string `json:"ref,omitempty"` // On is the machine that did it, so a failure that is about one machine can be told from one // about the source.