diff --git a/.github/workflows/companion-version.yml b/.github/workflows/companion-version.yml new file mode 100644 index 00000000..ac391ce2 --- /dev/null +++ b/.github/workflows/companion-version.yml @@ -0,0 +1,34 @@ +name: companion-version + +# A change to a companion's image inputs must bump its version: publishing +# skips a tag the registry already has, and a forced republish swaps the image +# under every machine that cached the tag. companions/proto 0.0.17 shipped the +# streaming Python facade that way (core#837). +# +# It runs on every pull request, with no path filter, so it can be a required +# check of main's ruleset: a path-filtered workflow that does not run leaves a +# required check pending forever. internal/companionbump reads which files are +# a companion's inputs from the Dockerfiles themselves; a change that touches +# none passes at once. +on: + pull_request: + +permissions: + contents: read + +jobs: + companion-version: + name: Companion version + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + - name: every changed companion carries a new version + env: + BASE_REF: origin/${{ github.base_ref }} + run: go run ./internal/companionbump "$BASE_REF" diff --git a/companions/buildspec.go b/companions/buildspec.go index a7e4f8c7..c389380c 100644 --- a/companions/buildspec.go +++ b/companions/buildspec.go @@ -2,7 +2,11 @@ package companions import ( "fmt" + "os" + "path" + "path/filepath" "slices" + "strings" ) // BaseImageArg is the Docker build argument a companion Dockerfile declares @@ -106,3 +110,142 @@ func BuildSpecs() ([]BuildSpec, error) { } return out, nil } + +// Instruction is one logical Dockerfile instruction: its physical lines +// joined where a line ends in the escape character, comment lines inside it +// dropped, and Line the physical line it starts on. +type Instruction struct { + Line int + Text string +} + +// Instructions splits a Dockerfile into its logical instructions. A COPY +// written over several lines is one instruction, so its sources on the later +// lines are read with it rather than missed. +func Instructions(dockerfile string) []Instruction { + var out []Instruction + var current []string + start := 0 + for number, raw := range strings.Split(dockerfile, "\n") { + line := strings.TrimRight(raw, " \t\r") + trimmed := strings.TrimSpace(line) + if len(current) == 0 && (trimmed == "" || strings.HasPrefix(trimmed, "#")) { + continue + } + if len(current) > 0 && strings.HasPrefix(trimmed, "#") { + continue // a comment inside a continued instruction is not part of it + } + if len(current) == 0 { + start = number + 1 + } + if strings.HasSuffix(line, "\\") { + current = append(current, strings.TrimSuffix(line, "\\")) + continue + } + current = append(current, line) + out = append(out, Instruction{Line: start, Text: strings.Join(current, " ")}) + current = nil + } + if len(current) > 0 { + out = append(out, Instruction{Line: start, Text: strings.Join(current, " ")}) + } + return out +} + +// CopyKind classifies an instruction for CopySources. +type CopyKind int + +const ( + // NotCopy is any instruction that is neither COPY nor ADD. + NotCopy CopyKind = iota + // FromStage is COPY --from=, which reads from another image + // rather than from the build context. + FromStage + // FromContext is a COPY or ADD that reads paths out of the build context. + // An ADD source that is a URL or a git remote is not one, and is left out. + FromContext + // Unparsed is a COPY or ADD whose shape this parser does not understand — + // the JSON form, or one with no source and destination. It is reported + // rather than skipped: a copy nobody reads is an image input nothing + // checks. + Unparsed +) + +// CopySources returns the build-context sources of a COPY or ADD instruction, +// in any case (Dockerfile instructions are case-insensitive). Pass it a +// logical instruction (Instructions), not a physical line. +// +// Leading flags are stripped rather than abandoning the line. Skipping any +// flagged COPY would leave `COPY --chown=…` and `COPY --link` — both ordinary +// context reads — unread, so their sources would escape every check. +func CopySources(instruction string) ([]string, CopyKind) { + fields := strings.Fields(instruction) + if len(fields) == 0 { + return nil, NotCopy + } + command := strings.ToUpper(fields[0]) + if command != "COPY" && command != "ADD" { + return nil, NotCopy + } + operands := fields[1:] + for len(operands) > 0 && strings.HasPrefix(operands[0], "--") { + if strings.HasPrefix(operands[0], "--from=") { + return nil, FromStage + } + operands = operands[1:] + } + if len(operands) < 2 || strings.HasPrefix(operands[0], "[") { + return nil, Unparsed + } + var sources []string + for _, source := range operands[:len(operands)-1] { + if strings.HasPrefix(source, "<<") { + continue // a heredoc: its content is in the Dockerfile itself + } + if command == "ADD" && remoteSource(source) { + continue + } + sources = append(sources, source) + } + return sources, FromContext +} + +// remoteSource reports whether an ADD source is fetched rather than read from +// the build context. +func remoteSource(source string) bool { + for _, prefix := range []string{"http://", "https://", "git@", "git://", "ssh://"} { + if strings.HasPrefix(source, prefix) { + return true + } + } + return false +} + +// Inputs returns the repository-root-relative POSIX paths, read from the +// checkout at root, that this companion's image is built from in this +// repository: its Dockerfile and every build-context COPY or ADD source, a file or a +// directory. The staged CLI binary is not one: it is built from codefly-dev/cli +// at publish time. A change under any input changes the image, so it needs a +// new Version: publishing skips a tag the registry already has. +func (s BuildSpec) Inputs(root string) ([]string, error) { + content, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(s.Dockerfile))) //nolint:gosec // the spec's own Dockerfile under the checkout root + if err != nil { + return nil, fmt.Errorf("cannot read the <%s> Dockerfile: %w", s.Name, err) + } + inputs := []string{s.Dockerfile} + for _, instruction := range Instructions(string(content)) { + sources, kind := CopySources(instruction.Text) + switch kind { + case Unparsed: + return nil, fmt.Errorf("%s:%d is a COPY or ADD whose sources cannot be read: %s", s.Dockerfile, instruction.Line, strings.TrimSpace(instruction.Text)) + case FromContext: + for _, source := range sources { + if s.CLIBinary != "" && source == s.CLIBinary { + continue + } + inputs = append(inputs, path.Clean(path.Join(s.Context, source))) + } + } + } + return inputs, nil +} diff --git a/companions/buildspec_test.go b/companions/buildspec_test.go index 1d576b17..82e28257 100644 --- a/companions/buildspec_test.go +++ b/companions/buildspec_test.go @@ -216,64 +216,40 @@ func TestBuildSpecsMatchWhatTheCurrentPublisherApplies(t *testing.T) { } } -// copyKind classifies a Dockerfile line for copySources. -type copyKind int - -const ( - // notCopy is any line that is not a COPY instruction. - notCopy copyKind = iota - // fromStage is COPY --from=, which reads from another image - // rather than from the build context. - fromStage - // fromContext is a COPY that reads paths out of the build context. - fromContext - // unparsed is a COPY whose shape this parser does not understand — the - // JSON form, or one with no source and destination. It is reported rather - // than skipped: a COPY nobody checks is exactly the drift these tests - // exist to catch. - unparsed -) - -// copySources returns the build-context sources of a COPY instruction. -// -// Leading flags are stripped rather than abandoning the line. Skipping any -// flagged COPY would leave `COPY --chown=…` and `COPY --link` — both ordinary -// context reads — unguarded, so a bad path in one would reach the builder. -func copySources(line string) ([]string, copyKind) { - fields := strings.Fields(line) - if len(fields) == 0 || fields[0] != "COPY" { - return nil, notCopy - } - operands := fields[1:] - for len(operands) > 0 && strings.HasPrefix(operands[0], "--") { - if strings.HasPrefix(operands[0], "--from=") { - return nil, fromStage - } - operands = operands[1:] - } - if len(operands) < 2 || strings.HasPrefix(operands[0], "[") { - return nil, unparsed - } - return operands[:len(operands)-1], fromContext +// A COPY spread over continued lines is one instruction: its later sources +// are inputs too, and a comment inside it is not part of it. +func TestInstructionsJoinContinuedLines(t *testing.T) { + dockerfile := "FROM alpine\n# a comment\nCOPY a \\\n # inside\n b /dst/\nRUN true\n" + instructions := companions.Instructions(dockerfile) + require.Len(t, instructions, 3) + require.Equal(t, 3, instructions[1].Line) + sources, kind := companions.CopySources(instructions[1].Text) + require.Equal(t, companions.FromContext, kind) + require.Equal(t, []string{"a", "b"}, sources) } func TestCopySourcesReadsEveryContextCopyShape(t *testing.T) { for line, expected := range map[string]struct { sources []string - kind copyKind + kind companions.CopyKind }{ - "COPY facades/go /src": {[]string{"facades/go"}, fromContext}, - "COPY a b /dst/": {[]string{"a", "b"}, fromContext}, - "COPY --chown=1000:1000 facades/ts /opt": {[]string{"facades/ts"}, fromContext}, - "COPY --link --chown=root:root bin/x /bin/x": {[]string{"bin/x"}, fromContext}, - " COPY bin/linux/${TARGETARCH}/codefly /bin/c": {[]string{"bin/linux/${TARGETARCH}/codefly"}, fromContext}, - "COPY --from=builder /go/bin/buf /usr/local/": {nil, fromStage}, - "RUN apk add --no-cache bash": {nil, notCopy}, - "": {nil, notCopy}, - `COPY ["a", "b"]`: {nil, unparsed}, - "COPY onlyone": {nil, unparsed}, + "COPY facades/go /src": {[]string{"facades/go"}, companions.FromContext}, + "COPY a b /dst/": {[]string{"a", "b"}, companions.FromContext}, + "COPY --chown=1000:1000 facades/ts /opt": {[]string{"facades/ts"}, companions.FromContext}, + "COPY --link --chown=root:root bin/x /bin/x": {[]string{"bin/x"}, companions.FromContext}, + " COPY bin/linux/${TARGETARCH}/codefly /bin/c": {[]string{"bin/linux/${TARGETARCH}/codefly"}, companions.FromContext}, + "COPY --from=builder /go/bin/buf /usr/local/": {nil, companions.FromStage}, + "RUN apk add --no-cache bash": {nil, companions.NotCopy}, + "": {nil, companions.NotCopy}, + "copy facades/py /opt": {[]string{"facades/py"}, companions.FromContext}, + "ADD entrypoint.sh /usr/local/bin/": {[]string{"entrypoint.sh"}, companions.FromContext}, + "add --chown=1 a https://example.com/x.tgz /d/": {[]string{"a"}, companions.FromContext}, + "ADD git@github.com:o/r.git /src": {nil, companions.FromContext}, + "COPY < +// +// A companion's tag is its info.codefly.yaml version, and publishing skips a +// tag the registry already has. A change that keeps the version therefore +// either never ships or, forced, swaps the image under a tag machines have +// already cached, and those machines keep the old one. Run from the +// repository root of a checkout with 's history. A companion +// whose inputs (BuildSpec.Inputs) changed since its merge base with +// must carry a version above the one at . Taking the +// base's tip rather than the merge base also refuses a version that another +// change on the base already took. +package main + +import ( + "errors" + "fmt" + "os" + "os/exec" + "strings" + + "github.com/Masterminds/semver" + "github.com/codefly-dev/core/companions" + "gopkg.in/yaml.v3" +) + +// companion is what the check needs to know about one companion. +type companion struct { + name string + version string // at HEAD + inputs []string // repository-root-relative files and directories +} + +// unbumped returns one refusal for every companion with a changed input whose +// version is not above its version at the base. base returns the version the +// base declares, and false when the companion does not exist there yet. +func unbumped(all []companion, changed []string, base func(name string) (string, bool, error)) ([]string, error) { + var refusals []string + for _, c := range all { + touched := touchedInputs(c.inputs, changed) + if len(touched) == 0 { + continue + } + was, existed, err := base(c.name) + if err != nil { + return nil, err + } + if !existed { + continue + } + now, err := semver.NewVersion(c.version) + if err != nil { + return nil, fmt.Errorf("companion <%s>: %w", c.name, err) + } + before, err := semver.NewVersion(was) + if err != nil { + return nil, fmt.Errorf("companion <%s> at the base: %w", c.name, err) + } + if now.GreaterThan(before) { + continue + } + refusals = append(refusals, fmt.Sprintf( + "companion <%s> changes its image (%s) but companions/%s/info.codefly.yaml says %s, and the base says %s: bump it above %s", + c.name, strings.Join(touched, ", "), c.name, c.version, was, was)) + } + return refusals, nil +} + +// touchedInputs returns the changed files that are an input or lie under one. +func touchedInputs(inputs, changed []string) []string { + var touched []string + for _, file := range changed { + for _, input := range inputs { + if file == input || strings.HasPrefix(file, input+"/") { + touched = append(touched, file) + break + } + } + } + return touched +} + +func git(args ...string) (string, error) { + out, err := exec.Command("git", args...).Output() //nolint:gosec // fixed git subcommands; the one variable is the base ref the workflow passes + if err != nil { + var exit *exec.ExitError + if errors.As(err, &exit) { + return "", fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(string(exit.Stderr))) + } + return "", fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + } + return string(out), nil +} + +// changedFiles lists every path that differs between baseRef's merge base and +// HEAD. --no-renames lists a moved file at both of its paths: moving a file out +// of an input changes the image as surely as editing it. +func changedFiles(baseRef string) ([]string, error) { + diff, err := git("diff", "--name-only", "--no-renames", baseRef+"...HEAD") + if err != nil { + return nil, err + } + return strings.Fields(diff), nil +} + +func run(baseRef string) ([]string, error) { + specs, err := companions.BuildSpecs() + if err != nil { + return nil, err + } + all := make([]companion, 0, len(specs)) + for _, spec := range specs { + inputs, inputsErr := spec.Inputs(".") + if inputsErr != nil { + return nil, inputsErr + } + all = append(all, companion{name: spec.Name, version: spec.Version, inputs: inputs}) + } + changed, err := changedFiles(baseRef) + if err != nil { + return nil, err + } + base := func(name string) (string, bool, error) { + manifest := "companions/" + name + "/info.codefly.yaml" + if _, err := git("cat-file", "-e", baseRef+":"+manifest); err != nil { + return "", false, nil + } + content, err := git("show", baseRef+":"+manifest) + if err != nil { + return "", false, err + } + var info struct { + Version string `yaml:"version"` + } + if err := yaml.Unmarshal([]byte(content), &info); err != nil { + return "", false, fmt.Errorf("%s at %s: %w", manifest, baseRef, err) + } + return info.Version, true, nil + } + return unbumped(all, changed, base) +} + +func main() { + if len(os.Args) != 2 { + fmt.Fprintln(os.Stderr, "usage: companionbump ") + os.Exit(2) + } + refusals, err := run(os.Args[1]) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + for _, refusal := range refusals { + fmt.Fprintln(os.Stderr, refusal) + } + if len(refusals) > 0 { + os.Exit(1) + } + fmt.Println("every companion whose image changed carries a new version") +} diff --git a/internal/companionbump/main_test.go b/internal/companionbump/main_test.go new file mode 100644 index 00000000..7c21f4cf --- /dev/null +++ b/internal/companionbump/main_test.go @@ -0,0 +1,123 @@ +package main + +import ( + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/codefly-dev/core/companions" + "github.com/stretchr/testify/require" +) + +var proto = companion{ + name: "proto", + version: "0.0.17", + inputs: []string{"companions/proto/Dockerfile", "companions/proto/facades/python"}, +} + +func baseAt(versions map[string]string) func(string) (string, bool, error) { + return func(name string) (string, bool, error) { + v, ok := versions[name] + return v, ok, nil + } +} + +// The case that shipped: the Python facade plugin changed and proto stayed +// 0.0.17, so the republished 0.0.17 never reached a machine that had it. +func TestAnInputChangeKeepingTheVersionIsRefused(t *testing.T) { + refusals, err := unbumped([]companion{proto}, + []string{"companions/proto/facades/python/protoc_gen_codefly_facade_python.py"}, + baseAt(map[string]string{"proto": "0.0.17"})) + require.NoError(t, err) + require.Len(t, refusals, 1) + require.Contains(t, refusals[0], "companion ") + require.Contains(t, refusals[0], "companions/proto/facades/python/protoc_gen_codefly_facade_python.py") + require.Contains(t, refusals[0], "bump it above 0.0.17") +} + +func TestABumpedVersionPasses(t *testing.T) { + bumped := proto + bumped.version = "0.0.18" + refusals, err := unbumped([]companion{bumped}, + []string{"companions/proto/Dockerfile"}, baseAt(map[string]string{"proto": "0.0.17"})) + require.NoError(t, err) + require.Empty(t, refusals) +} + +// Two branches taking the same new version would publish two images under one +// tag; the base's tip already holding it refuses the second. +func TestAVersionTheBaseAlreadyTookIsRefused(t *testing.T) { + bumped := proto + bumped.version = "0.0.18" + refusals, err := unbumped([]companion{bumped}, + []string{"companions/proto/Dockerfile"}, baseAt(map[string]string{"proto": "0.0.18"})) + require.NoError(t, err) + require.Len(t, refusals, 1) +} + +func TestAChangeOutsideTheInputsNeedsNoBump(t *testing.T) { + refusals, err := unbumped([]companion{proto}, []string{ + "companions/proto/README.md", + "companions/proto/testdata/facade/test_facade_gen.py", + "companions/proto/facades/pythonic/x.py", // a sibling sharing the prefix, not under the input + }, baseAt(map[string]string{"proto": "0.0.17"})) + require.NoError(t, err) + require.Empty(t, refusals) +} + +func TestANewCompanionNeedsNoBump(t *testing.T) { + refusals, err := unbumped([]companion{proto}, + []string{"companions/proto/Dockerfile"}, baseAt(map[string]string{})) + require.NoError(t, err) + require.Empty(t, refusals) +} + +// The real specs: every companion's Dockerfile is an input, and proto's facade +// plugins (the files 08716e9 changed) are inputs of proto. +func TestTheCheckoutsInputsCoverTheFacadePlugins(t *testing.T) { + specs, err := companions.BuildSpecs() + require.NoError(t, err) + byName := map[string][]string{} + for _, spec := range specs { + inputs, err := spec.Inputs("../..") + require.NoError(t, err) + require.Contains(t, inputs, spec.Dockerfile) + byName[spec.Name] = inputs + } + require.NotEmpty(t, touchedInputs(byName["proto"], + []string{"companions/proto/facades/python/protoc_gen_codefly_facade_python.py"})) + require.Empty(t, touchedInputs(byName["proto"], []string{"companions/proto/README.md"})) + require.NotContains(t, byName["codefly"], "bin/linux/${TARGETARCH}/codefly") +} + +// Moving a file out of an input changes the image: git's rename detection +// would list only the destination, outside every input, and let it through. +func TestAFileMovedOutOfAnInputIsAChangeToIt(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t", "GIT_CONFIG_GLOBAL=/dev/null") + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, out) + } + run("init", "-q", "-b", "main") + plugin := filepath.Join("companions", "proto", "facades", "python", "plugin.py") + require.NoError(t, os.MkdirAll(filepath.Dir(plugin), 0o755)) + require.NoError(t, os.WriteFile(plugin, []byte("print('the facade plugin, long enough to be detected as a rename')\n"), 0o644)) + run("add", ".") + run("commit", "-q", "-m", "base") + run("checkout", "-q", "-b", "change") + require.NoError(t, os.MkdirAll("elsewhere", 0o755)) + run("mv", plugin, filepath.Join("elsewhere", "plugin.py")) + run("commit", "-q", "-m", "move") + + changed, err := changedFiles("main") + require.NoError(t, err) + require.Contains(t, changed, "companions/proto/facades/python/plugin.py") + refusals, err := unbumped([]companion{proto}, changed, baseAt(map[string]string{"proto": "0.0.17"})) + require.NoError(t, err) + require.Len(t, refusals, 1) +}