From 207b71cf889f6358a0a2f325f523972d1e7cd718 Mon Sep 17 00:00:00 2001 From: Jake Coffman Date: Fri, 22 Dec 2023 10:32:23 -0600 Subject: [PATCH 1/5] return Updater error codes --- cmd/dependabot/internal/cmd/update.go | 2 +- internal/infra/proxy.go | 2 +- internal/infra/run.go | 6 ++++- internal/infra/updater.go | 34 +++++++++++++++++++++++---- testdata/scripts/crash.txt | 24 +++++++++++++++++++ testdata/scripts/local.txt | 2 +- 6 files changed, 61 insertions(+), 9 deletions(-) create mode 100644 testdata/scripts/crash.txt diff --git a/cmd/dependabot/internal/cmd/update.go b/cmd/dependabot/internal/cmd/update.go index ed0200a..dad52ec 100644 --- a/cmd/dependabot/internal/cmd/update.go +++ b/cmd/dependabot/internal/cmd/update.go @@ -95,7 +95,7 @@ func NewUpdateCommand() *cobra.Command { if errors.Is(err, context.DeadlineExceeded) { log.Fatalf("update timed out after %s", flags.timeout) } - log.Fatalf("failed to run updater: %v", err) + log.Fatalf("updater failure: %v", err) } return nil diff --git a/internal/infra/proxy.go b/internal/infra/proxy.go index f64a305..2b758a9 100644 --- a/internal/infra/proxy.go +++ b/internal/infra/proxy.go @@ -177,7 +177,7 @@ func (p *Proxy) Close() (err error) { }() // Check the error code if the container has already exited, so we can pass it along to the caller. If the proxy - //crashes we want the CLI to error out. Unlike the Updater it should never stop on its own. + //crashes we want the CLI to error out. containerInfo, inspectErr := p.cli.ContainerInspect(context.Background(), p.containerID) if inspectErr != nil { return fmt.Errorf("failed to inspect proxy container: %w", inspectErr) diff --git a/internal/infra/run.go b/internal/infra/run.go index 30772cb..806eab2 100644 --- a/internal/infra/run.go +++ b/internal/infra/run.go @@ -389,7 +389,11 @@ func runContainers(ctx context.Context, params RunParams) (err error) { if err != nil { return err } - defer updater.Close() + defer func() { + if updaterErr := updater.Close(); updaterErr != nil { + err = updaterErr + } + }() // put the clone dir in the updater container to be used by during the update if params.LocalDir != "" { diff --git a/internal/infra/updater.go b/internal/infra/updater.go index ca24130..5de3904 100644 --- a/internal/infra/updater.go +++ b/internal/infra/updater.go @@ -251,19 +251,29 @@ func (u *Updater) RunCmd(ctx context.Context, cmd, user string, env ...string) e _, _ = io.Copy(os.Stderr, prefixer.New(r, "updater | ")) }() - // blocks until update is complete or ctl-c ch := make(chan struct{}) go func() { _, _ = stdcopy.StdCopy(w, w, execResp.Reader) ch <- struct{}{} }() + // blocks until update is complete or ctl-c select { case <-ctx.Done(): return ctx.Err() case <-ch: } + // check the exit code of the command + execInspect, err := u.cli.ContainerExecInspect(ctx, execCreate.ID) + if err != nil { + return fmt.Errorf("failed to inspect exec: %w", err) + } + + if execInspect.ExitCode != 0 { + return fmt.Errorf("updater exited with code: %v", execInspect.ExitCode) + } + return nil } @@ -282,10 +292,24 @@ func (u *Updater) Wait(ctx context.Context, condition container.WaitCondition) e } // Close kills and deletes the container and deletes updater mount paths related to the run. -func (u *Updater) Close() error { - return u.cli.ContainerRemove(context.Background(), u.containerID, types.ContainerRemoveOptions{ - Force: true, - }) +func (u *Updater) Close() (err error) { + defer func() { + removeErr := u.cli.ContainerRemove(context.Background(), u.containerID, types.ContainerRemoveOptions{Force: true}) + if removeErr != nil { + err = fmt.Errorf("failed to remove proxy container: %w", removeErr) + } + }() + + // Handle non-zero exit codes. + containerInfo, inspectErr := u.cli.ContainerInspect(context.Background(), u.containerID) + if inspectErr != nil { + return fmt.Errorf("failed to inspect proxy container: %w", inspectErr) + } + if containerInfo.State.ExitCode != 0 { + return fmt.Errorf("updater container exited with non-zero exit code: %d", containerInfo.State.ExitCode) + } + + return } // JobFile is the payload passed to file updater containers. diff --git a/testdata/scripts/crash.txt b/testdata/scripts/crash.txt new file mode 100644 index 0000000..3e586b0 --- /dev/null +++ b/testdata/scripts/crash.txt @@ -0,0 +1,24 @@ +exec docker build -t crashy-updater . + +! dependabot update go_modules dependabot/cli --updater-image crashy-updater +stderr 'updater exited with code: 2' + +exec docker rmi -f crashy-updater + +-- Dockerfile -- +FROM ubuntu:22.04 + +RUN useradd dependabot + +COPY --chown=dependabot --chmod=755 update-ca-certificates /usr/bin/update-ca-certificates +COPY --chown=dependabot --chmod=755 run bin/run + +-- update-ca-certificates -- +#!/usr/bin/env bash + +echo "Updating CA certificates..." + +-- run -- +#!/usr/bin/env bash + +exit 2 diff --git a/testdata/scripts/local.txt b/testdata/scripts/local.txt index 1e3fc0f..37ce8f3 100644 --- a/testdata/scripts/local.txt +++ b/testdata/scripts/local.txt @@ -1,7 +1,7 @@ exec docker build -qt local-updater . # The ls command in run will fail since this isn't a real updater -dependabot update go_modules dependabot-fixtures/go-modules-lib --updater-image local-updater +! dependabot update go_modules dependabot-fixtures/go-modules-lib --updater-image local-updater stderr 'No such file or directory' # The local flag should create the repo directory and put my-repo in it From d566ba6de29462f05a10995e66a4b7bfca85d9a4 Mon Sep 17 00:00:00 2001 From: Jake Coffman Date: Fri, 22 Dec 2023 10:52:38 -0600 Subject: [PATCH 2/5] too noisy --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1edc68a..453b440 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,7 +23,7 @@ jobs: - name: Test # -count=2 ensures that test fixtures cleanup after themselves # because any leftover state will generally cause the second run to fail. - run: go test -shuffle=on -count=2 -race -cover -v -timeout=5m ./... + run: go test -shuffle=on -count=2 -race -cover -timeout=5m ./... lint: runs-on: ubuntu-latest From e4b32bdea7c5cfc652dee6b75438b571bff9f10a Mon Sep 17 00:00:00 2001 From: Jake Coffman Date: Fri, 22 Dec 2023 12:06:53 -0600 Subject: [PATCH 3/5] this test no longer needed --- internal/infra/run_test.go | 173 ------------------------------------- 1 file changed, 173 deletions(-) diff --git a/internal/infra/run_test.go b/internal/infra/run_test.go index 916794b..cd603b9 100644 --- a/internal/infra/run_test.go +++ b/internal/infra/run_test.go @@ -1,10 +1,7 @@ package infra import ( - "archive/tar" - "bytes" "context" - "io" "net/http" "os" "reflect" @@ -13,11 +10,7 @@ import ( "github.com/dependabot/cli/internal/server" - "gopkg.in/yaml.v3" - "github.com/dependabot/cli/internal/model" - "github.com/docker/docker/api/types" - "github.com/moby/moby/client" ) func Test_checkCredAccess(t *testing.T) { @@ -166,169 +159,3 @@ func Test_generateIgnoreConditions(t *testing.T) { } }) } - -func TestRun(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } - - cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) - if err != nil { - t.Fatal(err) - } - - ctx := context.Background() - - var buildContext bytes.Buffer - tw := tar.NewWriter(&buildContext) - _ = addFileToArchive(tw, "/Dockerfile", 0644, dockerFile) - _ = addFileToArchive(tw, "/test_main.go", 0644, testMain) - _ = tw.Close() - - UpdaterImageName := "test-updater" - resp, err := cli.ImageBuild(ctx, &buildContext, types.ImageBuildOptions{Tags: []string{UpdaterImageName}}) - if err != nil { - t.Fatal(err) - } - - _, _ = io.Copy(io.Discard, resp.Body) - _ = resp.Body.Close() - - defer func() { - _, _ = cli.ImageRemove(ctx, UpdaterImageName, types.ImageRemoveOptions{}) - }() - - cred := model.Credential{ - "type": "git_source", - "host": "github.com", - "username": "x-access-token", - "password": "$LOCAL_GITHUB_ACCESS_TOKEN", - } - - os.Setenv("LOCAL_GITHUB_ACCESS_TOKEN", "test-token") - err = Run(RunParams{ - PullImages: true, - Job: &model.Job{ - PackageManager: "ecosystem", - Source: model.Source{ - Repo: "org/name", - }, - }, - Creds: []model.Credential{cred}, - UpdaterImage: UpdaterImageName, - Output: "out.yaml", - }) - if err != nil { - t.Error(err) - } - - f, err := os.Open("out.yaml") - if err != nil { - t.Fatal(err) - } - defer f.Close() - - var output model.Scenario - if err = yaml.NewDecoder(f).Decode(&output); err != nil { - t.Fatal(err) - } - - if !reflect.DeepEqual(output.Input.Credentials, []model.Credential{cred}) { - t.Error("unexpected credentials", output.Input.Credentials) - } - if output.Input.Credentials[0]["password"] != "$LOCAL_GITHUB_ACCESS_TOKEN" { - t.Error("expected password to be masked") - } -} - -const dockerFile = ` -FROM golang:1.21 - -# needed to run update-ca-certificates -RUN apt-get update && apt-get install -y ca-certificates -# cli will try to start as dependabot -RUN useradd -ms /bin/bash dependabot - -# need to be the user for permissions to work -USER dependabot -WORKDIR /home/dependabot -COPY *.go . -# cli runs bin/run (twice) to do an update, so put exe there -RUN go mod init cli_test && go mod tidy && go build -o bin/run -` - -const testMain = `package main - -import ( - "bytes" - "context" - "encoding/xml" - "log" - "net" - "net/http" - "os" - "os/exec" - "strings" - "sync" - "time" -) - -func main() { - if os.Args[1] == "update_files" { - return - } - var wg sync.WaitGroup - - // print the line that fails - log.SetFlags(log.Lshortfile) - - go connectivityCheck(&wg) - checkIfRoot() - proxyCheck() - - wg.Wait() -} - -func checkIfRoot() { - buf := &bytes.Buffer{} - cmd := exec.Command("id", "-u") - cmd.Stdout = buf - if err := cmd.Run(); err != nil { - log.Fatalln(err) - } - userID := strings.TrimSpace(buf.String()) - if userID == "0" { - log.Fatalln("User is root") - } -} - -func connectivityCheck(wg *sync.WaitGroup) { - wg.Add(1) - - var d net.Dialer - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - - _, err := d.DialContext(ctx, "tcp", "1.1.1.1:22") - if err != nil && err.Error() != "dial tcp 1.1.1.1:22: i/o timeout" { - log.Fatalln(err) - } - if err == nil { - log.Fatalln("a connection shouldn't be possible") - } - - wg.Done() -} - -func proxyCheck() { - res, err := http.Get("https://example.com") - if err != nil { - log.Fatalln(err) - } - defer res.Body.Close() - var v any - if err = xml.NewDecoder(res.Body).Decode(&v); err != nil { - log.Fatalln(err) - } -} -` From 7bd0c7e5fb04c074601b73a3a97cde8702d67781 Mon Sep 17 00:00:00 2001 From: Jake Coffman Date: Wed, 3 Jan 2024 09:54:44 -0600 Subject: [PATCH 4/5] exit code errors the update subcommand only --- internal/infra/run.go | 4 ++++ internal/infra/updater.go | 7 ++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/infra/run.go b/internal/infra/run.go index 806eab2..a67824f 100644 --- a/internal/infra/run.go +++ b/internal/infra/run.go @@ -411,6 +411,10 @@ func runContainers(ctx context.Context, params RunParams) (err error) { if err := updater.RunCmd(ctx, cmd, dependabot, userEnv(prox.url, params.ApiUrl)...); err != nil { return err } + // If the exit code is non-zero, error when using the `update` subcommand, but not the `test` subcommand. + if len(params.Expected) > 0 && updater.ExitCode != nil && *updater.ExitCode != 0 { + return fmt.Errorf("updater exited with code %d", *updater.ExitCode) + } } return nil diff --git a/internal/infra/updater.go b/internal/infra/updater.go index 5de3904..78c543d 100644 --- a/internal/infra/updater.go +++ b/internal/infra/updater.go @@ -38,6 +38,9 @@ const ( type Updater struct { cli *client.Client containerID string + + // ExitCode is set once an Updater command has completed. + ExitCode *int } const ( @@ -270,9 +273,7 @@ func (u *Updater) RunCmd(ctx context.Context, cmd, user string, env ...string) e return fmt.Errorf("failed to inspect exec: %w", err) } - if execInspect.ExitCode != 0 { - return fmt.Errorf("updater exited with code: %v", execInspect.ExitCode) - } + u.ExitCode = &execInspect.ExitCode return nil } From e69f79cbd0a4fca1a0c850bf163f69c867d99ade Mon Sep 17 00:00:00 2001 From: Jake Coffman Date: Wed, 3 Jan 2024 11:19:20 -0600 Subject: [PATCH 5/5] test for failure --- internal/infra/run.go | 2 +- testdata/scripts/crash.txt | 24 ------------------------ testdata/scripts/fail.txt | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 25 deletions(-) delete mode 100644 testdata/scripts/crash.txt create mode 100644 testdata/scripts/fail.txt diff --git a/internal/infra/run.go b/internal/infra/run.go index a67824f..f19db8c 100644 --- a/internal/infra/run.go +++ b/internal/infra/run.go @@ -412,7 +412,7 @@ func runContainers(ctx context.Context, params RunParams) (err error) { return err } // If the exit code is non-zero, error when using the `update` subcommand, but not the `test` subcommand. - if len(params.Expected) > 0 && updater.ExitCode != nil && *updater.ExitCode != 0 { + if params.Expected == nil && *updater.ExitCode != 0 { return fmt.Errorf("updater exited with code %d", *updater.ExitCode) } } diff --git a/testdata/scripts/crash.txt b/testdata/scripts/crash.txt deleted file mode 100644 index 3e586b0..0000000 --- a/testdata/scripts/crash.txt +++ /dev/null @@ -1,24 +0,0 @@ -exec docker build -t crashy-updater . - -! dependabot update go_modules dependabot/cli --updater-image crashy-updater -stderr 'updater exited with code: 2' - -exec docker rmi -f crashy-updater - --- Dockerfile -- -FROM ubuntu:22.04 - -RUN useradd dependabot - -COPY --chown=dependabot --chmod=755 update-ca-certificates /usr/bin/update-ca-certificates -COPY --chown=dependabot --chmod=755 run bin/run - --- update-ca-certificates -- -#!/usr/bin/env bash - -echo "Updating CA certificates..." - --- run -- -#!/usr/bin/env bash - -exit 2 diff --git a/testdata/scripts/fail.txt b/testdata/scripts/fail.txt new file mode 100644 index 0000000..c0ae73b --- /dev/null +++ b/testdata/scripts/fail.txt @@ -0,0 +1,37 @@ +exec docker build -t fail-updater . + +! dependabot update go_modules dependabot/cli --updater-image fail-updater +stderr 'updater failure: updater exited with code 2' + +# Assert that the test command doesn't fail if the updater fails +dependabot test -f input.yml --updater-image fail-updater + +exec docker rmi -f fail-updater + +-- Dockerfile -- +FROM ubuntu:22.04 + +RUN useradd dependabot + +COPY --chown=dependabot --chmod=755 update-ca-certificates /usr/bin/update-ca-certificates +COPY --chown=dependabot --chmod=755 run bin/run + +-- update-ca-certificates -- +#!/usr/bin/env bash + +echo "Updating CA certificates..." + +-- run -- +#!/usr/bin/env bash + +exit 2 + +-- input.yml -- +input: + job: + package-manager: go_modules + source: + repo: dependabot/cli + directory: / +output: + -