diff --git a/cmd/mesh-controller/checks.go b/cmd/mesh-controller/checks.go index 866c2988..1c7eefda 100644 --- a/cmd/mesh-controller/checks.go +++ b/cmd/mesh-controller/checks.go @@ -370,7 +370,7 @@ func releaseBranchOf(ctx context.Context, p link.PullUpdated, scope checkScope, // trunkOf is the branch a module's source follows: main when it records none, or records a commit. func trunkOf(s inventory.Source) string { - if t := followedBranch(s.Ref); t != "" { + if t := strings.TrimSpace(followedBranch(s.Ref)); t != "" { return t } return "main" diff --git a/cmd/mesh-controller/release_branch_test.go b/cmd/mesh-controller/release_branch_test.go index 52cf43d8..298b1588 100644 --- a/cmd/mesh-controller/release_branch_test.go +++ b/cmd/mesh-controller/release_branch_test.go @@ -261,3 +261,13 @@ func TestATrunkIsTheBranchTheRepositoryFollows(t *testing.T) { } } } + +// trunkOf: the branch a source follows, and main when it records none or records a commit (issue 215's rule). +func TestATrunkIsMainWhenTheSourceRecordsNone(t *testing.T) { + for ref, want := range map[string]string{"": "main", "main": "main", "master": "master", "nox-mesh": "nox-mesh", + "0123456789abcdef0123456789abcdef01234567": "main", " ": "main"} { + if got := trunkOf(inventory.Source{Ref: ref}); got != want { + t.Errorf("a source recording %q follows %q, not %q", ref, got, want) + } + } +} diff --git a/internal/builder/check.go b/internal/builder/check.go index 240a0901..202a9abf 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -429,10 +429,17 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry if dir == name || !safeName.MatchString(dir) { continue } - if dir == "mesh-lab" && b.Else != "" { - // **mesh-lab is read at its trunk, always** (novox/hq ADR 0358): its replays run with the container - // runtime's socket, so a branch of a release's name — which anyone may create — is never what runs there. - b.Ref, b.Else = b.Else, "" + if dir == "mesh-lab" { + // **mesh-lab is never read at a release branch** (novox/hq ADR 0358): its replays run with the container + // runtime's socket, so a branch of a release's name — which anyone may create — is never what runs there, + // whether it is asked with a fallback or without. + if b.Else != "" || (spec.Release != "" && b.Ref == spec.Release) { + b.Ref = b.Else + if b.Ref == "" { + b.Ref = "main" + } + } + b.Else = "" } if err := cloneAt(b.Repository, b.Ref, b.Else, dir); err != nil { return CheckVerdict{}, fmt.Errorf("beside it, %w", err) @@ -565,21 +572,47 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry user = fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()) } in := func(image, dir string, env []string, command ...string) []string { + home := workspace + for _, e := range env { + if strings.HasPrefix(e, "HOME=") { + home = strings.TrimPrefix(e, "HOME=") + } + } // As the builder itself: what a check writes into the workspace is the builder's to remove. args := []string{"run", "--rm", "--network", "host", "--volume", workspace + ":" + workspace, "--workdir", dir, - "--user", user, "--env", "HOME=" + workspace, + "--user", user, "--env", "HOME=" + home, // The check's own checkouts, whoever cloned them: git in a container of another user than the // one that cloned refuses a repository it does not own ("dubious ownership"), and Go's build // stamps the version from git — a judge that would not build for want of it. "--env", "GIT_CONFIG_COUNT=1", "--env", "GIT_CONFIG_KEY_0=safe.directory", "--env", "GIT_CONFIG_VALUE_0=*"} for _, e := range env { - args = append(args, "--env", e) + if !strings.HasPrefix(e, "HOME=") { + args = append(args, "--env", e) + } } return append(append(args, image), command...) } inToolchain := func(dir string, env []string, command ...string) []string { return in(spec.Toolchain, dir, env, command...) } + // **Every judge is built where no pull request can reach** (novox/hq issue 482, ADR 0358 review): the workspace's + // Go cache, HOME and git configuration are written by pull requests' own scripts, and git runs during a Go build + // to stamp its version — a planted hook would rewrite the judge's sources before they compile. So a judge is built + // with a HOME, configuration and Go cache of its own, outside the workspace and made fresh for this check, no + // global or system git configuration, no Go environment file, and no version stamping from git. + judgeHome, err := os.MkdirTemp("", "mesh-check-judge-") + if err != nil { + return CheckVerdict{}, err + } + defer os.RemoveAll(judgeHome) + if withinDir(workspace, judgeHome) { + return CheckVerdict{}, fmt.Errorf("the temporary directory %s is inside the workspace %s, which a check's "+ + "containers mount: no judge is built where a pull request could reach it", judgeHome, workspace) + } + inJudgeBuild := func(dir string, _ []string, command ...string) []string { + args := in(spec.Toolchain, dir, JudgeBuildEnv(judgeHome), command...) + return append([]string{args[0], "--volume", judgeHome + ":" + judgeHome}, args[1:]...) + } var out tail // running runs one container of the check, its output into the report, in a process group of its own. running := func(args []string) error { @@ -591,7 +624,7 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry // The judge. Its failing to build is the change's fault when the change is the judge or its validator, // and the check's when it is the controller the mesh runs. - gate, judgeFault, err := judgeFor(ctx, labelled, run, spec, root, tree, judged.BesideAt, inToolchain, say) + gate, judgeFault, err := judgeFor(ctx, labelled, run, spec, root, tree, judged.BesideAt, inJudgeBuild, say) if err != nil { return CheckVerdict{}, err } @@ -1046,8 +1079,7 @@ func judgeFor(ctx context.Context, labelled, run Runner, spec CheckSpec, root, t switch spec.Judge { case "self": bin := filepath.Join(root, "bin", "judge-of-itself") - if _, err := labelled(ctx, root, "docker", inToolchain(tree, - []string{"CGO_ENABLED=0", "GOFLAGS=-mod=vendor", "GOPROXY=off", "GOCACHE=" + filepath.Join(filepath.Dir(root), "go-cache")}, + if _, err := labelled(ctx, root, "docker", inToolchain(tree, nil, "go", "build", "-o", bin, "./cmd/mesh-controller")...); err != nil { return "", "the change's controller, which judges itself, does not build: " + firstLine(err.Error()), nil } @@ -1080,8 +1112,7 @@ func judgeFor(ctx context.Context, labelled, run Runner, spec CheckSpec, root, t } } withValidator := filepath.Join(root, "bin", "judge-with-this-validator") - if _, err := labelled(ctx, root, "docker", inToolchain(filepath.Join(root, judgeTree), - []string{"CGO_ENABLED=0", "GOFLAGS=-mod=vendor", "GOPROXY=off", "GOCACHE=" + filepath.Join(filepath.Dir(root), "go-cache")}, + if _, err := labelled(ctx, root, "docker", inToolchain(filepath.Join(root, judgeTree), nil, "go", "build", "-o", withValidator, "./cmd/mesh-controller")...); err != nil { if spec.Release != "" { return "", spec.Release + "'s controller does not build with this change's validator: " + @@ -1135,12 +1166,8 @@ func releaseJudge(ctx context.Context, run, plain Runner, spec CheckSpec, root s } } } - // **Built with a cache of its own, never the workspace's** (novox/hq issue 482): the workspace's Go cache and HOME are - // written by every pull request's own script, so a poisoned cache entry or Go environment file would otherwise be - // built into the judge kept for this head. A fresh cache under this check's root, made before anything of the - // change runs, and no Go environment file. - if _, err := run(ctx, root, "docker", inToolchain(filepath.Join(root, dir), - []string{"CGO_ENABLED=0", "GOFLAGS=-mod=vendor", "GOPROXY=off", "GOENV=off", "GOCACHE=" + judgeGoCache(root)}, + // Built where no pull request reaches (JudgeBuildEnv): a judge kept for this head is the head's, nothing else. + if _, err := run(ctx, root, "docker", inToolchain(filepath.Join(root, dir), nil, "go", "build", "-o", bin, "./cmd/mesh-controller")...); err != nil { return "", "", fmt.Errorf("the controller of %s does not build%s: %w", spec.Release, with, err) } @@ -1150,9 +1177,15 @@ func releaseJudge(ctx context.Context, run, plain Runner, spec CheckSpec, root s return bin, dir, nil } -// judgeGoCache is the Go cache a release branch's judge is built with: fresh for each check, under its root, which is -// emptied before the check clones anything — never the workspace's shared cache. -func judgeGoCache(root string) string { return filepath.Join(root, "judge-go-cache") } +// JudgeBuildEnv is the environment every judge is built in (novox/hq issue 482): home, a directory outside the +// workspace made fresh for the check, as HOME, configuration and Go cache; no global or system git configuration; no +// Go environment file; and no version stamped from git, so no git runs during the build at all. +func JudgeBuildEnv(home string) []string { + return []string{"HOME=" + home, "XDG_CONFIG_HOME=" + filepath.Join(home, ".config"), + "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_NOSYSTEM=1", "GOENV=off", + "GOCACHE=" + filepath.Join(home, "go-cache"), "GOPATH=" + filepath.Join(home, "go"), + "CGO_ENABLED=0", "GOFLAGS=-mod=vendor -buildvcs=false", "GOPROXY=off"} +} // judgesKept is how many release-branch judges a judge cache keeps: a head moved past is not built from again. const judgesKept = 4 @@ -1514,8 +1547,7 @@ func judge(ctx context.Context, run, plain Runner, root string, inToolchain func say func(step, format string, args ...any)) (string, string, error) { bin := filepath.Join(root, "bin", "mesh-controller") build := func(dir string) error { - _, err := run(ctx, root, "docker", inToolchain(filepath.Join(root, dir), - []string{"CGO_ENABLED=0", "GOFLAGS=-mod=vendor", "GOPROXY=off", "GOCACHE=" + filepath.Join(filepath.Dir(root), "go-cache")}, + _, err := run(ctx, root, "docker", inToolchain(filepath.Join(root, dir), nil, "go", "build", "-o", bin, "./cmd/mesh-controller")...) return err } diff --git a/internal/builder/release_branch_test.go b/internal/builder/release_branch_test.go index 1ca1609b..d5509687 100644 --- a/internal/builder/release_branch_test.go +++ b/internal/builder/release_branch_test.go @@ -144,6 +144,7 @@ run) if [ -n "$detached" ]; then echo a-container; exit 0; fi shift if [ "$1" = go ]; then echo "GOCACHE=$GOCACHE GOENV=$GOENV $*" >> "` + built + `"; fi + if [ "$1" = go ] && [ "$2" = build ] && [ -n "$JUDGE_HOMES" ]; then echo "$HOME" >> "$JUDGE_HOMES"; fi cd "$workdir" && exec "$@" ;; esac exit 1 @@ -258,8 +259,8 @@ func TestACataloguePullRequestIntoAReleaseBranchIsJudgedByThatBranchsController( // **The judge is built with a cache no pull request's script can write** (issue 482): a fresh one under the // check's own root, never the workspace's shared go-cache, and no Go environment file from the workspace's HOME. said, _ := os.ReadFile(built) - if !strings.Contains(string(said), "/check/judge-go-cache GOENV=off go build") || - strings.Contains(string(said), "/go-cache GOENV") { + if !strings.Contains(string(said), "/mesh-check-judge-") || !strings.Contains(string(said), "GOENV=off go build") || + strings.Contains(string(said), "/check/../go-cache") { t.Fatalf("the release judge was built with the workspace's cache or Go environment:\n%s", said) } @@ -412,3 +413,135 @@ func TestAReleaseBranchCheckReadsMeshLabAtItsTrunkEvenWhenItHasThatBranch(t *tes labMain[:12]) } } + +// aPlantedWorkspace is a workspace a pull request's own script has written into, as it can: a git configuration in its +// HOME whose fsmonitor hook says each directory git runs in, a git configuration under XDG_CONFIG_HOME doing the same, +// and a Go environment file that would hand the judge's build a tool of its own. It answers the workspace and the +// file the hook writes to. +func aPlantedWorkspace(t *testing.T) (string, string) { + t.Helper() + workspace, logs := t.TempDir(), t.TempDir() + ran := filepath.Join(logs, "hook-ran") + hook := filepath.Join(workspace, "hook.sh") + files := map[string]string{ + hook: "#!/bin/sh\necho \"$PWD\" >> " + ran + "\n", + filepath.Join(workspace, ".gitconfig"): "[core]\n\tfsmonitor = " + hook + "\n", + filepath.Join(workspace, ".config", "git", "config"): "[core]\n\tfsmonitor = " + hook + "\n", + filepath.Join(workspace, ".config", "go", "env"): "GOFLAGS=-toolexec=" + hook + "\n", + } + for name, body := range files { + if err := os.MkdirAll(filepath.Dir(name), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(name, []byte(body), 0o755); err != nil { + t.Fatal(err) + } + } + return workspace, ran +} + +// judgeRanTheHook says whether the planted hook ran in a judge's tree: the controller beside the check, or the +// change's own when it judges itself. +func judgeRanTheHook(t *testing.T, ran string) string { + t.Helper() + body, _ := os.ReadFile(ran) + for _, line := range strings.Split(string(body), "\n") { + if strings.HasSuffix(line, "/check/mesh-controller") { + return string(body) + } + } + return "" +} + +// **Every judge is built where no pull request reaches** (issue 482, ADR 0358 review): a git configuration with a +// hook, and a Go environment file, planted in the workspace by an earlier pull request's script, do not run during +// the release judge's build — while the pull request's own script, in the workspace, still meets them. +func TestAHookPlantedInTheWorkspaceDoesNotRunDuringTheReleaseJudgesBuild(t *testing.T) { + aRuntimeThatBuildsTheJudge(t) + homes := filepath.Join(t.TempDir(), "homes") + t.Setenv("JUDGE_HOMES", homes) + registry := aFactsRegistry(t) + workspace, ran := aPlantedWorkspace(t) + beside, _, _, _ := theReleasedMesh(t) + catalogue, head := aPullRequestInto(t, map[string]string{"modules/gitea/module.json": `{"module":"gitea"}`, + CheckScript: "git status >/dev/null 2>&1; true\n"}, releaseBranch, map[string]string{"modules/gitea/index.ts": "x"}) + v, err := Check(t.Context(), Command, CheckSpec{ID: "check-704-planted", Repository: catalogue, Ref: head, + Owner: "novox", Repo: "mesh-catalog", Number: 286, Base: releaseBranch, Release: releaseBranch, + JudgeCache: t.TempDir(), Paths: []string{"modules/gitea/index.ts"}, Modules: []string{"gitea"}, Beside: beside, + Toolchain: "golang"}, workspace, registry, GitCredential{}, nil) + if err != nil { + t.Fatal(err) + } + if v.Gate == nil || v.Gate.Verdict != "pass" { + t.Fatalf("the check did not judge: %+v\n%s", v.Gate, v.Report) + } + if body, _ := os.ReadFile(ran); !strings.Contains(string(body), "/check/mesh-catalog") { + t.Fatalf("the planted hook never ran, so this test proves nothing: %q", body) + } + if said := judgeRanTheHook(t, ran); said != "" { + t.Fatalf("the hook planted in the workspace ran during the judge's build:\n%s", said) + } + if body, _ := os.ReadFile(homes); len(body) == 0 || strings.Contains(string(body), workspace) { + t.Fatalf("the judge was built with a HOME in the workspace: %q", body) + } +} + +// The same for the judge built with a node-engine change's own validator, into a release branch (ADR 0358 §2). +func TestAHookPlantedInTheWorkspaceDoesNotRunDuringTheValidatorJudgesBuild(t *testing.T) { + aRuntimeThatBuildsTheJudge(t) + homes := filepath.Join(t.TempDir(), "homes") + t.Setenv("JUDGE_HOMES", homes) + registry := aFactsRegistry(t) + workspace, ran := aPlantedWorkspace(t) + beside, _, _, _ := theReleasedMesh(t) + delete(beside, "mesh-host") + files := aHostSaying("the release branch's validator") + files["module.json"] = `{"module":"mesh-host"}` + files[CheckScript] = "git status >/dev/null 2>&1; true\n" + host, head := aPullRequestInto(t, files, releaseBranch, aHostSaying("this change's validator")) + v, err := Check(t.Context(), Command, CheckSpec{ID: "check-704-planted-validator", Repository: host, Ref: head, + Owner: "novox", Repo: "mesh-host", Number: 111, Base: releaseBranch, Release: releaseBranch, Judge: "validator", + JudgeCache: t.TempDir(), Paths: []string{"validate/v.go"}, Modules: []string{"mesh-host"}, Beside: beside, + Toolchain: "golang"}, workspace, registry, GitCredential{}, nil) + if err != nil { + t.Fatal(err) + } + if v.Gate == nil || !strings.HasSuffix(v.Gate.Summary, "the release branch's controller with this change's validator") { + t.Fatalf("a node-engine change into a release branch was not judged by that branch's controller with its own "+ + "validator: %+v\n%s", v.Gate, v.Report) + } + if body, _ := os.ReadFile(ran); !strings.Contains(string(body), "/check/mesh-host") { + t.Fatalf("the planted hook never ran, so this test proves nothing: %q", body) + } + if said := judgeRanTheHook(t, ran); said != "" { + t.Fatalf("the hook planted in the workspace ran during a judge's build:\n%s", said) + } + if body, _ := os.ReadFile(homes); strings.Count(string(body), "\n") != 2 || strings.Contains(string(body), workspace) { + t.Fatalf("the judges were not both built with a HOME outside the workspace: %q", body) + } +} + +// **mesh-lab is never read at a release branch**, asked with a fallback or without (ADR 0358). +func TestTheBuildSeatNeverReadsMeshLabAtAReleaseBranch(t *testing.T) { + aRuntimeThatBuildsTheJudge(t) + registry := aFactsRegistry(t) + lab, labMain, _ := aRepositoryWithARelease(t, map[string]string{"README": "the lab"}, + map[string]string{"README": "a branch anyone could push"}) + for _, asked := range []Beside{{Repository: lab, Ref: releaseBranch}, {Repository: lab, Ref: releaseBranch, Else: "main"}} { + beside, _, _, _ := theReleasedMesh(t) + beside["mesh-lab"] = asked + catalogue, head := aPullRequestInto(t, map[string]string{"modules/gitea/module.json": `{"module":"gitea"}`, + CheckScript: "true\n"}, releaseBranch, map[string]string{"modules/gitea/index.ts": "x"}) + v, err := Check(t.Context(), Command, CheckSpec{ID: "check-704-lab-asked", Repository: catalogue, Ref: head, + Owner: "novox", Repo: "mesh-catalog", Number: 286, Base: releaseBranch, Release: releaseBranch, + JudgeCache: t.TempDir(), Paths: []string{"modules/gitea/index.ts"}, Modules: []string{"gitea"}, Beside: beside, + Toolchain: "golang"}, t.TempDir(), registry, GitCredential{}, nil) + if err != nil { + t.Fatal(err) + } + if v.BesideAt["mesh-lab"] != "main" || v.Beside["mesh-lab"] != labMain[:12] { + t.Fatalf("mesh-lab asked as %+v was read at %q (%s), not main at %s", asked, v.BesideAt["mesh-lab"], + v.Beside["mesh-lab"], labMain[:12]) + } + } +}