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.