diff --git a/docs/debugging.md b/docs/debugging.md index ded9f801..545fdfc4 100644 --- a/docs/debugging.md +++ b/docs/debugging.md @@ -10,14 +10,13 @@ Next, clone https://github.com/dependabot/dependabot-core. This project contains Try opening a terminal and run `script/dependabot update go_modules dependabot/cli --debug` in the `dependabot-core` project directory. This will drop you in an interactive session with the update ready to proceed. -To perform the update, you need to run two commands: +To perform the update, run: -- `bin/run fetch_files` - `bin/run update_files` -If the problem you are debugging is during the fetch step, the fetch_files command will be all you need to run. +This fetches the dependency files and then updates them. -If the problem is after the fetch step, you can repeatedly run update_files while you're debugging. +If the problem you are debugging is during the fetch step, run `bin/run fetch_files` instead. It needs somewhere to write the fetched file set, so set `DEPENDABOT_HANDOFF_PATH` first, for example `export DEPENDABOT_HANDOFF_PATH=/tmp/handoff.json`. `update_files` reads that file instead of fetching again when the variable is set, so you can repeatedly run `update_files` against a single fetch while you're debugging. In the example `script/dependabot` command above, try running an update in the container. @@ -32,7 +31,7 @@ Next, let's try adding a `debugger` statement. Open the `dependabot-core` projec > **Note** You don't have to restart your CLI session, the changes are automatically synced to the container! -In the interactive debugging session, run `bin/run fetch_files` and `bin/run update_files`. During the update_files command, the Ruby debugger will open. It should look something like this: +In the interactive debugging session, run `bin/run update_files`. During that command, the Ruby debugger will open. It should look something like this: ```ruby [11, 20] in ~/go_modules/lib/dependabot/go_modules/update_checker.rb @@ -60,7 +59,7 @@ At this prompt, you can run [debugger commands](https://github.com/ruby/debug) t If your Dependabot job is hanging and would like to figure out why, the CLI is the perfect tool for the job. -Start by running the update that recreates the hang with `dependabot update `. Once the hang is reproducible, run with the `--debug` flag and the run the `fetch_files` and `update_files` commands and wait until the job hangs. +Start by running the update that recreates the hang with `dependabot update `. Once the hang is reproducible, run with the `--debug` flag and then run the `update_files` command and wait until the job hangs. Once it does hang, hit CTL-C, and you'll get a stack trace leading you to the problematic code. diff --git a/internal/infra/proxy.go b/internal/infra/proxy.go index f3646334..31a4a3d8 100644 --- a/internal/infra/proxy.go +++ b/internal/infra/proxy.go @@ -10,6 +10,7 @@ import ( "path" "path/filepath" + "github.com/dependabot/cli/internal/model" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/mount" "github.com/docker/docker/api/types/network" @@ -32,6 +33,12 @@ type Proxy struct { } func NewProxy(ctx context.Context, cli *client.Client, params *RunParams, nets *Networks) (*Proxy, error) { + return newProxyWithCreds(ctx, cli, params, nets, params.Creds) +} + +// newProxyWithCreds builds a proxy serving a specific credential set, so the fetch +// and update sides can be given different ones. +func newProxyWithCreds(ctx context.Context, cli *client.Client, params *RunParams, nets *Networks, creds []model.Credential) (*Proxy, error) { // Generate secrets: ca, err := GenerateCertificateAuthority() if err != nil { @@ -40,7 +47,7 @@ func NewProxy(ctx context.Context, cli *client.Client, params *RunParams, nets * // Generate and write configuration to disk: proxyConfig := &Config{ - Credentials: params.Creds, + Credentials: creds, CA: ca, } diff --git a/internal/infra/run.go b/internal/infra/run.go index 8b51e07c..247a7606 100644 --- a/internal/infra/run.go +++ b/internal/infra/run.go @@ -18,6 +18,7 @@ import ( "time" "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/volume" "github.com/dependabot/cli/internal/model" "github.com/dependabot/cli/internal/server" @@ -31,12 +32,16 @@ import ( "gopkg.in/yaml.v3" ) -var runCmds = map[model.RunCommand]string{ - model.VersionCommand: "bin/run fetch_files && bin/run update_files", - model.UpdateFilesCommand: "bin/run fetch_files && bin/run update_files", - model.RecreateCommand: "bin/run fetch_files && bin/run update_files", - model.SecurityCommand: "bin/run fetch_files && bin/run update_files", - model.UpdateGraphCommand: "bin/run fetch_files && bin/run update_graph", +// fetchCmd fetches the dependency files. In the combined topology the update +// commands fetch for themselves, so it only runs in the split topology. +const fetchCmd = "bin/run fetch_files" + +var updateCmds = map[model.RunCommand]string{ + model.VersionCommand: "bin/run update_files", + model.UpdateFilesCommand: "bin/run update_files", + model.RecreateCommand: "bin/run update_files", + model.SecurityCommand: "bin/run update_files", + model.UpdateGraphCommand: "bin/run update_graph", } type RunParams struct { @@ -445,7 +450,16 @@ func runContainers(ctx context.Context, params RunParams) (err error) { defer collector.Close() } - updater, err := NewUpdater(ctx, cli, networks, ¶ms, prox, collector) + if params.Job.IsolatedFetchUpdate() { + return runIsolated(ctx, cli, networks, ¶ms, prox, collector) + } + + return runCombined(ctx, cli, networks, ¶ms, prox, collector) +} + +// runCombined runs fetch and update in a single container sharing a repo clone. +func runCombined(ctx context.Context, cli *client.Client, networks *Networks, params *RunParams, prox *Proxy, collector *Collector) (err error) { + updater, err := NewUpdater(ctx, cli, networks, params, prox, collector, "") if err != nil { return err } @@ -455,16 +469,8 @@ func runContainers(ctx context.Context, params RunParams) (err error) { } }() - // put the clone dir in the updater container to be used by during the update - if params.LocalDir != "" { - containerDir := guestRepoDir - if params.Job.UseCaseInsensitiveFileSystem() { - // since the updater is using the storage container, we need to populate the repo on that device because that's the directory that will be used for the update - containerDir = caseSensitiveRepoContentsPath - } - if err = putCloneDir(ctx, cli, updater, params.LocalDir, containerDir); err != nil { - return err - } + if err = placeCloneDir(ctx, cli, params, updater); err != nil { + return err } // update CA certificates as root prior to start debug shell or running dependabot commands @@ -473,25 +479,145 @@ func runContainers(ctx context.Context, params RunParams) (err error) { } if params.Debug { - if err := updater.RunShell(ctx, prox.url, params.ApiUrl, params.Job, params.UpdaterEnvironmentVariables); err != nil { - return err + return updater.RunShell(ctx, prox.url, params.ApiUrl, params.Job, params.UpdaterEnvironmentVariables) + } + + // Run dependabot commands as a dependabot user + env := userEnv(prox.url, params.ApiUrl, params.Job, params.UpdaterEnvironmentVariables) + if params.Flamegraph { + env = append(env, "FLAMEGRAPH=1") + } + if err := updater.RunCmd(ctx, updateCmds[params.Job.Command], dependabot, env...); err != nil { + return err + } + if params.Flamegraph { + getFromContainer(ctx, cli, updater.containerID, "/tmp/dependabot-flamegraph.html") + } + + return checkExitCode(params, updater) +} + +// runIsolated clones in one container and updates in another. The clone travels +// between them on a shared volume rather than being made twice. Each side gets its +// own proxy on its own network, so the update side cannot reach the fetch proxy. +func runIsolated(ctx context.Context, cli *client.Client, networks *Networks, params *RunParams, prox *Proxy, collector *Collector) (err error) { + if params.Debug { + return fmt.Errorf("--debug is not supported with the isolated_fetch_update experiment") + } + if params.Job.UseCaseInsensitiveFileSystem() { + return fmt.Errorf("isolated_fetch_update is not supported with use_case_insensitive_filesystem") + } + if params.CollectorConfigPath != "" { + return fmt.Errorf("the OpenTelemetry collector is not supported with the isolated_fetch_update experiment") + } + + repoVolume, err := cli.VolumeCreate(ctx, volume.CreateOptions{Labels: map[string]string{"dependabot-cli": "repo"}}) + if err != nil { + return fmt.Errorf("failed to create repo volume: %w", err) + } + defer func() { + if volumeErr := cli.VolumeRemove(context.Background(), repoVolume.Name, true); volumeErr != nil { + err = volumeErr } - } else { - // Run dependabot commands as a dependabot user - env := userEnv(prox.url, params.ApiUrl, params.Job, params.UpdaterEnvironmentVariables) - if params.Flamegraph { - env = append(env, "FLAMEGRAPH=1") + }() + + fetcher, err := NewUpdater(ctx, cli, networks, params, prox, collector, repoVolume.Name) + if err != nil { + return err + } + defer func() { + if fetcherErr := fetcher.Close(); fetcherErr != nil { + err = fetcherErr } - if err := updater.RunCmd(ctx, runCmds[params.Job.Command], dependabot, env...); err != nil { - return err + }() + + if err = placeCloneDir(ctx, cli, params, fetcher); err != nil { + return err + } + + if err = fetcher.RunCmd(ctx, "update-ca-certificates", root); err != nil { + return err + } + + fetchEnv := userEnv(prox.url, params.ApiUrl, params.Job, params.UpdaterEnvironmentVariables) + if err = fetcher.RunCmd(ctx, fetchCmd, dependabot, fetchEnv...); err != nil { + return err + } + if *fetcher.ExitCode != 0 { + return fmt.Errorf("fetch exited with code %d", *fetcher.ExitCode) + } + + // The update side gets its own network and proxy so its credentials can diverge + // from the fetch side's without the two containers being able to swap proxies. + updateNetworks, err := NewNetworks(ctx, cli) + if err != nil { + return fmt.Errorf("failed to create update networks: %w", err) + } + defer func() { + if netErr := updateNetworks.Close(); netErr != nil { + err = netErr } - if params.Flamegraph { - getFromContainer(ctx, cli, updater.containerID, "/tmp/dependabot-flamegraph.html") + }() + + // Same credentials as the fetch side for now. This is the seam where the repo-read + // credential gets dropped, once the update side no longer calls the target repo. + updateProxy, err := newProxyWithCreds(ctx, cli, params, updateNetworks, params.Creds) + if err != nil { + return fmt.Errorf("failed to create update proxy: %w", err) + } + defer func() { + if proxyErr := updateProxy.Close(); proxyErr != nil { + err = proxyErr } - // If the exit code is non-zero, error when using the `update` subcommand, but not the `test` subcommand. - if params.Expected == nil && *updater.ExitCode != 0 { - return fmt.Errorf("updater exited with code %d", *updater.ExitCode) + }() + go updateProxy.TailLogs(ctx, cli) + + updater, err := NewUpdater(ctx, cli, updateNetworks, params, updateProxy, collector, repoVolume.Name) + if err != nil { + return err + } + defer func() { + if updaterErr := updater.Close(); updaterErr != nil { + err = updaterErr } + }() + + if err = updater.RunCmd(ctx, "update-ca-certificates", root); err != nil { + return err + } + + env := userEnv(updateProxy.url, params.ApiUrl, params.Job, params.UpdaterEnvironmentVariables) + if params.Flamegraph { + env = append(env, "FLAMEGRAPH=1") + } + if err = updater.RunCmd(ctx, updateCmds[params.Job.Command], dependabot, env...); err != nil { + return err + } + if params.Flamegraph { + getFromContainer(ctx, cli, updater.containerID, "/tmp/dependabot-flamegraph.html") + } + + return checkExitCode(params, updater) +} + +func placeCloneDir(ctx context.Context, cli *client.Client, params *RunParams, updater *Updater) error { + if params.LocalDir == "" { + return nil + } + + containerDir := guestRepoDir + if params.Job.UseCaseInsensitiveFileSystem() { + // since the updater is using the storage container, we need to populate the repo on that device because that's the directory that will be used for the update + containerDir = caseSensitiveRepoContentsPath + } + + return putCloneDir(ctx, cli, updater, params.LocalDir, containerDir) +} + +// checkExitCode errors when using the `update` subcommand, but not the `test` subcommand. +func checkExitCode(params *RunParams, updater *Updater) error { + if params.Expected == 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 dd3a4955..ba0ee60d 100644 --- a/internal/infra/updater.go +++ b/internal/infra/updater.go @@ -2,6 +2,7 @@ package infra import ( "archive/tar" + "bufio" "bytes" "context" "encoding/json" @@ -22,7 +23,6 @@ import ( "github.com/docker/docker/api/types/network" "github.com/docker/docker/api/types/volume" "github.com/docker/docker/client" - "github.com/goware/prefixer" "github.com/moby/moby/pkg/stdcopy" ) @@ -32,6 +32,10 @@ const ( dependabot = "dependabot" ) +// maxLogLineSize bounds a single line of container output; job.json is echoed as +// one line by some updaters and exceeds bufio's default. +const maxLogLineSize = 16 * 1024 * 1024 + const ( guestInputDir = "/home/dependabot/dependabot-updater/job.json" guestOutput = "/home/dependabot/dependabot-updater/output.json" @@ -64,7 +68,17 @@ const ( ) // NewUpdater starts the update container interactively running /bin/sh, so it does not stop. -func NewUpdater(ctx context.Context, cli *client.Client, net *Networks, params *RunParams, prox *Proxy, collector *Collector) (*Updater, error) { +// repoVolume, when set, is a volume mounted at guestRepoDir so the clone made by the +// fetch container is the clone the update container uses. +func NewUpdater( + ctx context.Context, + cli *client.Client, + net *Networks, + params *RunParams, + prox *Proxy, + collector *Collector, + repoVolume string, +) (*Updater, error) { containerCfg := &container.Config{ User: dependabot, Image: params.UpdaterImage, @@ -99,6 +113,14 @@ func NewUpdater(ctx context.Context, cli *client.Client, net *Networks, params * }) } + if repoVolume != "" { + hostCfg.Mounts = append(hostCfg.Mounts, mount.Mount{ + Type: mount.TypeVolume, + Source: repoVolume, + Target: guestRepoDir, + }) + } + storageContainerID := "" storageVolumes := []string{} if params.Job.UseCaseInsensitiveFileSystem() { @@ -138,6 +160,14 @@ func NewUpdater(ctx context.Context, cli *client.Client, net *Networks, params * return nil, fmt.Errorf("failed to start updater container: %w", err) } + if repoVolume != "" { + // The volume mounts as root, so the dependabot user could not clone into it. + if err = updater.RunCmd(ctx, "chown "+dependabot+" "+guestRepoDir, root); err != nil { + updater.Close() + return nil, fmt.Errorf("failed to prepare the repo volume: %w", err) + } + } + return updater, nil } @@ -409,8 +439,10 @@ func (u *Updater) RunCmd(ctx context.Context, cmd, user string, env ...string) e } r, w := io.Pipe() + copyDone := make(chan struct{}) go func() { - _, _ = io.Copy(os.Stderr, prefixer.New(r, "updater | ")) + copyPrefixed(os.Stderr, r, "updater | ") + close(copyDone) }() ch := make(chan struct{}) @@ -426,6 +458,11 @@ func (u *Updater) RunCmd(ctx context.Context, cmd, user string, env ...string) e case <-ch: } + // Flush the pipe before returning, otherwise the tail of the command's output + // is lost when the caller moves on. + _ = w.Close() + <-copyDone + // check the exit code of the command execInspect, err := u.cli.ContainerExecInspect(ctx, execCreate.ID) if err != nil { @@ -437,6 +474,16 @@ func (u *Updater) RunCmd(ctx context.Context, cmd, user string, env ...string) e return nil } +// copyPrefixed writes each line read from r to w behind prefix, including a final +// line with no trailing newline. +func copyPrefixed(w io.Writer, r io.Reader, prefix string) { + scanner := bufio.NewScanner(r) + scanner.Buffer(make([]byte, 0, bufio.MaxScanTokenSize), maxLogLineSize) + for scanner.Scan() { + _, _ = fmt.Fprintf(w, "%s%s\n", prefix, scanner.Text()) + } +} + // Wait blocks until the condition is true. func (u *Updater) Wait(ctx context.Context, condition container.WaitCondition) error { wait, errCh := u.cli.ContainerWait(ctx, u.containerID, condition) diff --git a/internal/infra/updater_test.go b/internal/infra/updater_test.go index da037efe..64e19a5e 100644 --- a/internal/infra/updater_test.go +++ b/internal/infra/updater_test.go @@ -1,11 +1,12 @@ package infra import ( - "github.com/dependabot/cli/internal/model" "os" "path/filepath" "strings" "testing" + + "github.com/dependabot/cli/internal/model" ) func Test_mountOptions(t *testing.T) { diff --git a/internal/model/job.go b/internal/model/job.go index 5c6afdc1..f72f5743 100644 --- a/internal/model/job.go +++ b/internal/model/job.go @@ -66,6 +66,12 @@ func (j *Job) UseCaseInsensitiveFileSystem() bool { return j.PackageManager == "nuget" && j.experimentEnabled("use_case_insensitive_filesystem") } +// IsolatedFetchUpdate reports whether fetch and update run in separate containers +// that exchange a serialized file set instead of sharing a repo clone. +func (j *Job) IsolatedFetchUpdate() bool { + return j.experimentEnabled("isolated_fetch_update") +} + // experimentEnabled reports whether the named boolean experiment is enabled. // name is the canonical experiment name using underscores; the hyphenated // variant is also checked to accommodate both naming conventions. diff --git a/internal/model/job_test.go b/internal/model/job_test.go index f20a2ea9..a3026217 100644 --- a/internal/model/job_test.go +++ b/internal/model/job_test.go @@ -201,6 +201,30 @@ func TestUseCaseInsensitiveFileSystem(t *testing.T) { } } +func TestIsolatedFetchUpdate(t *testing.T) { + tests := []struct { + name string + experiments Experiment + want bool + }{ + {"nil experiments", nil, false}, + {"underscore true", Experiment{"isolated_fetch_update": true}, true}, + {"underscore false", Experiment{"isolated_fetch_update": false}, false}, + {"hyphen true", Experiment{"isolated-fetch-update": true}, true}, + {"non-bool value", Experiment{"isolated_fetch_update": "true"}, false}, + {"ecosystem flag only", Experiment{"isolated_fetch_update_go_modules": true}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + j := &Job{PackageManager: "go_modules", Experiments: tt.experiments} + if got := j.IsolatedFetchUpdate(); got != tt.want { + t.Errorf("IsolatedFetchUpdate() = %v, want %v", got, tt.want) + } + }) + } +} + func TestAllowedUpdateTypes(t *testing.T) { var input Input if err := yaml.Unmarshal([]byte(exampleJob), &input); err != nil { diff --git a/testdata/scripts/basic.txt b/testdata/scripts/basic.txt index 5bf82ce2..d0711086 100644 --- a/testdata/scripts/basic.txt +++ b/testdata/scripts/basic.txt @@ -5,8 +5,9 @@ exec docker build -qt dummy-updater . dependabot update go_modules dependabot/cli --updater-image dummy-updater # assert the dummy is working -stderr 'bin/run arguments: fetch_files' stderr 'bin/run arguments: update_files' +# the combined topology fetches inside update_files +! stderr 'bin/run arguments: fetch_files' exec docker rmi -f dummy-updater diff --git a/testdata/scripts/graph.txt b/testdata/scripts/graph.txt index d0553ed0..0374e2a7 100644 --- a/testdata/scripts/graph.txt +++ b/testdata/scripts/graph.txt @@ -5,15 +5,15 @@ exec docker build -qt graph-updater . dependabot graph go_modules dependabot/cli -o out.yml --updater-image graph-updater # assert the dummy is working -stderr 'bin/run arguments: fetch_files' stderr 'bin/run arguments: update_graph' +! stderr 'bin/run arguments: fetch_files' stderr '"enable_dependency_submission_poc":true' # Verify the smoke test can run the graph command dependabot test -f out.yml --updater-image graph-updater -stderr 'bin/run arguments: fetch_files' stderr 'bin/run arguments: update_graph' +! stderr 'bin/run arguments: fetch_files' exec docker rmi -f graph-updater diff --git a/testdata/scripts/isolated.txt b/testdata/scripts/isolated.txt new file mode 100644 index 00000000..07f0c918 --- /dev/null +++ b/testdata/scripts/isolated.txt @@ -0,0 +1,71 @@ +# The split topology clones in the fetch container and updates in another, with the +# clone travelling between them on a shared Docker volume rather than being made twice. + +exec docker build -qt isolated-updater . + +dependabot update -f job.yml --updater-image isolated-updater --local my-repo + +# both entrypoints run, in separate containers +stderr 'bin/run arguments: fetch_files' +stderr 'bin/run arguments: update_files' + +# the fetch container is the one that gets the clone +stderr 'fetch_files repo: \.git' +stderr 'fetch_files repo: hello.txt' + +# the update container sees the same clone on the volume, without cloning again +stderr 'update_files repo: \.git' +stderr 'update_files repo: hello.txt' +stderr 'update_files sees: written by fetch' + +# both sides point at the same contents path +stderr 'fetch_files env: DEPENDABOT_REPO_CONTENTS_PATH=/home/dependabot/dependabot-updater/repo' +stderr 'update_files env: DEPENDABOT_REPO_CONTENTS_PATH=/home/dependabot/dependabot-updater/repo' + +exec docker rmi -f isolated-updater + +-- job.yml -- +job: + package-manager: go_modules + source: + provider: github + repo: dependabot-fixtures/go-modules-lib + directory: / + experiments: + isolated_fetch_update: true + +-- my-repo/hello.txt -- +Hello, world! + +-- Dockerfile -- +FROM ubuntu:22.04 + +RUN apt-get update && apt-get install -y git + +RUN useradd dependabot +RUN mkdir -p /home/dependabot/dependabot-updater/repo && chown -R dependabot /home/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 "Updated those certificates for ya" + +-- run -- +#!/usr/bin/env bash + +echo "bin/run arguments: $1" +echo "$1 env: DEPENDABOT_REPO_CONTENTS_PATH=${DEPENDABOT_REPO_CONTENTS_PATH:-unset}" + +ls -a "$DEPENDABOT_REPO_CONTENTS_PATH" | sed "s/^/$1 repo: /" + +case "$1" in + fetch_files) + echo "written by fetch" > "$DEPENDABOT_REPO_CONTENTS_PATH/from-fetch.txt" + ;; + update_files) + echo "update_files sees: $(cat "$DEPENDABOT_REPO_CONTENTS_PATH/from-fetch.txt")" + ;; +esac diff --git a/testdata/scripts/smb-mount.txt b/testdata/scripts/smb-mount.txt index afdf1f7a..1f0f7f22 100644 --- a/testdata/scripts/smb-mount.txt +++ b/testdata/scripts/smb-mount.txt @@ -25,11 +25,8 @@ echo "Updated those certificates for ya" -- run -- #!/usr/bin/env bash -if [ "$1" = "fetch_files" ]; then - # git clone would have created this directory - mkdir -p "$DEPENDABOT_REPO_CONTENTS_PATH" - exit 0 -fi +# in the combined topology the clone happens inside update_files +mkdir -p "$DEPENDABOT_REPO_CONTENTS_PATH" echo "test file" > "$DEPENDABOT_REPO_CONTENTS_PATH/test.txt" if [ -e "$DEPENDABOT_CASE_INSENSITIVE_REPO_CONTENTS_PATH/TEST.TXT" ]; then