From 45cc85033406561311e3f2111ba60f94254b4ae8 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Wed, 20 Jan 2021 18:21:14 -0800 Subject: [PATCH 01/18] Revert "campaigns: temporary switch back to bind for non-root (#437)" This reverts commit a35c2e77cc199b273aa21ede521536941fcdaba7. --- internal/campaigns/workspace.go | 2 +- internal/campaigns/workspace_test.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/campaigns/workspace.go b/internal/campaigns/workspace.go index dabfc9c3af..b0b44b3a89 100644 --- a/internal/campaigns/workspace.go +++ b/internal/campaigns/workspace.go @@ -128,7 +128,7 @@ func detectBestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCrea } uids[uid] = struct{}{} - if len(uids) > 1 || uid != 0 { + if len(uids) > 1 { return workspaceCreatorBind } } diff --git a/internal/campaigns/workspace_test.go b/internal/campaigns/workspace_test.go index 450612f5d1..b2564992d3 100644 --- a/internal/campaigns/workspace_test.go +++ b/internal/campaigns/workspace_test.go @@ -38,8 +38,9 @@ func TestBestWorkspaceCreator(t *testing.T) { "same user": { behaviours: []imageBehaviour{ {image: "foo", behaviour: expect.Behaviour{Stdout: []byte("1000\n")}}, + {image: "bar", behaviour: expect.Behaviour{Stdout: []byte("1000\n")}}, }, - want: workspaceCreatorBind, + want: workspaceCreatorVolume, }, "different user": { behaviours: []imageBehaviour{ From 29e03a2a23a99a307dbc53b2c852e6d2a180ba58 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Tue, 19 Jan 2021 15:21:12 +0100 Subject: [PATCH 02/18] RIDICULOUS: Set chown in workspace volume and create temp file --- internal/campaigns/service.go | 9 +++++-- internal/campaigns/volume_workspace.go | 34 +++++++++++++++++++++++++- internal/campaigns/workspace.go | 20 +++++++++------ 3 files changed, 53 insertions(+), 10 deletions(-) diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index b049e9ef98..791a0bf5a6 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -213,16 +213,21 @@ func (svc *Service) NewRepoFetcher(dir string, cleanArchives bool) RepoFetcher { func (svc *Service) NewWorkspaceCreator(ctx context.Context, cacheDir, tempDir string, steps []Step) WorkspaceCreator { var workspace workspaceCreatorType + var workspaceCreatorVolumeUid int + if svc.workspace == "volume" { workspace = workspaceCreatorVolume } else if svc.workspace == "bind" { workspace = workspaceCreatorBind } else { - workspace = bestWorkspaceCreator(ctx, steps) + workspace, workspaceCreatorVolumeUid = bestWorkspaceCreator(ctx, steps) } if workspace == workspaceCreatorVolume { - return &dockerVolumeWorkspaceCreator{tempDir: tempDir} + return &dockerVolumeWorkspaceCreator{ + tempDir: tempDir, + uid: workspaceCreatorVolumeUid, + } } return &dockerBindWorkspaceCreator{dir: cacheDir} } diff --git a/internal/campaigns/volume_workspace.go b/internal/campaigns/volume_workspace.go index 0989a6aa84..5707c0e562 100644 --- a/internal/campaigns/volume_workspace.go +++ b/internal/campaigns/volume_workspace.go @@ -3,6 +3,7 @@ package campaigns import ( "bytes" "context" + "fmt" "io/ioutil" "os" @@ -15,6 +16,7 @@ import ( type dockerVolumeWorkspaceCreator struct { tempDir string + uid int } var _ WorkspaceCreator = &dockerVolumeWorkspaceCreator{} @@ -60,7 +62,7 @@ git commit --quiet --all --allow-empty -m src-action-exec return nil } -func (*dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w *dockerVolumeWorkspace, zip string) error { +func (c *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w *dockerVolumeWorkspace, zip string) error { // We want to mount that temporary file into a Docker container that has the // workspace volume attached, and unzip it into the volume. common, err := w.DockerRunOpts(ctx, "/work") @@ -68,6 +70,26 @@ func (*dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w return errors.Wrap(err, "generating run options") } + if c.uid != 0 { + owner := fmt.Sprintf("%d:%d", c.uid, c.uid) + + opts := append([]string{"run", "--rm", "--init"}, common...) + opts = append(opts, dockerVolumeWorkspaceImage, "chown", "-R", owner, "/work") + + if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { + return errors.Wrapf(err, "chown output:\n\n%s\n\n", string(out)) + } + + // LOL/FML: This seems to "fix" the problem of `chown` getting lost between + // the `chown` call above and the unzip below? + opts = append([]string{"run", "--rm", "--init"}, common...) + opts = append(opts, dockerVolumeWorkspaceImage, "touch", "/work/DELETE_ME") + + if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { + return errors.Wrapf(err, "chown output:\n\n%s\n\n", string(out)) + } + } + opts := append([]string{ "run", "--rm", @@ -81,6 +103,16 @@ func (*dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w return errors.Wrapf(err, "unzip output:\n\n%s\n\n", string(out)) } + // LOL/FML: delete stupid file + if c.uid != 0 { + opts = append([]string{"run", "--rm", "--init"}, common...) + opts = append(opts, dockerVolumeWorkspaceImage, "rm", "-rf", "/work/DELETE_ME") + + if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { + return errors.Wrapf(err, "chown output:\n\n%s\n\n", string(out)) + } + } + return nil } diff --git a/internal/campaigns/workspace.go b/internal/campaigns/workspace.go index b0b44b3a89..9d694352bd 100644 --- a/internal/campaigns/workspace.go +++ b/internal/campaigns/workspace.go @@ -55,7 +55,7 @@ const ( // bestWorkspaceCreator determines the correct workspace creator to use based on // the environment and campaign to be executed. -func bestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCreatorType { +func bestWorkspaceCreator(ctx context.Context, steps []Step) (workspaceCreatorType, int) { // The basic theory here is that we have two options: bind and volume. Bind // is battle tested and always safe, but can be slow on non-Linux platforms // because bind mounts are slow. Volume is faster on those platforms, but @@ -67,13 +67,13 @@ func bestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCreatorTyp // For the time being, we're only going to consider volume mode on Intel // macOS. if runtime.GOOS != "darwin" || runtime.GOARCH != "amd64" { - return workspaceCreatorBind + return workspaceCreatorBind, 0 } return detectBestWorkspaceCreator(ctx, steps) } -func detectBestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCreatorType { +func detectBestWorkspaceCreator(ctx context.Context, steps []Step) (workspaceCreatorType, int) { // OK, so we're interested in volume mode, but we need to take its // shortcomings around mixed user environments into account. // @@ -109,7 +109,7 @@ func detectBestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCrea // An error here likely indicates that `id` isn't available on the // path. That's OK: let's not make any assumptions at this point // about the image, and we'll default to the always safe option. - return workspaceCreatorBind + return workspaceCreatorBind, 0 } // POSIX specifies the output of `id -u` as the effective UID, @@ -124,14 +124,20 @@ func detectBestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCrea // // TODO: when logging is available at this level, we should log an // error at verbose level to make this easier to debug. - return workspaceCreatorBind + return workspaceCreatorBind, 0 } uids[uid] = struct{}{} if len(uids) > 1 { - return workspaceCreatorBind + return workspaceCreatorBind, 0 } } - return workspaceCreatorVolume + var uid int + for k := range uids { + uid = k + break // fml + } + + return workspaceCreatorVolume, uid } From 93cf49d3a9b71cae39de274018e911ab419fa619 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 19 Jan 2021 18:39:51 -0800 Subject: [PATCH 03/18] Detect UID and GID in the workspace creator. --- internal/campaigns/action.go | 49 -------- internal/campaigns/bind_workspace.go | 2 +- internal/campaigns/campaign_spec.go | 3 +- internal/campaigns/docker/cache.go | 32 ++++++ internal/campaigns/docker/image.go | 148 +++++++++++++++++++++++++ internal/campaigns/run_steps.go | 12 +- internal/campaigns/service.go | 62 +++-------- internal/campaigns/volume_workspace.go | 48 +++----- internal/campaigns/workspace.go | 61 +++------- 9 files changed, 241 insertions(+), 176 deletions(-) delete mode 100644 internal/campaigns/action.go create mode 100644 internal/campaigns/docker/cache.go create mode 100644 internal/campaigns/docker/image.go diff --git a/internal/campaigns/action.go b/internal/campaigns/action.go deleted file mode 100644 index 0891a551b0..0000000000 --- a/internal/campaigns/action.go +++ /dev/null @@ -1,49 +0,0 @@ -package campaigns - -import ( - "bytes" - "context" - "fmt" - "os/exec" - "strings" -) - -// getDockerImageContentDigest gets the content digest for the image. Note that this -// is different from the "distribution digest" (which is what you can use to specify -// an image to `docker run`, as in `my/image@sha256:xxx`). We need to use the -// content digest because the distribution digest is only computed for images that -// have been pulled from or pushed to a registry. See -// https://windsock.io/explaining-docker-image-ids/ under "A Final Twist" for a good -// explanation. -func getDockerImageContentDigest(ctx context.Context, image string) (string, error) { - // TODO!(sqs): is image id the right thing to use here? it is NOT the - // digest. but the digest is not calculated for all images (unless they are - // pulled/pushed from/to a registry), see - // https://github.com/moby/moby/issues/32016. - out, err := exec.CommandContext(ctx, "docker", "image", "inspect", "--format", "{{.Id}}", "--", image).CombinedOutput() - if err != nil { - if !strings.Contains(string(out), "No such image") { - return "", fmt.Errorf("error inspecting docker image %q: %s", image, bytes.TrimSpace(out)) - } - pullCmd := exec.CommandContext(ctx, "docker", "image", "pull", image) - - err = pullCmd.Start() - if err != nil { - return "", fmt.Errorf("error pulling docker image %q: %s", image, err) - } - err = pullCmd.Wait() - if err != nil { - return "", fmt.Errorf("error pulling docker image %q: %s", image, err) - } - } - out, err = exec.CommandContext(ctx, "docker", "image", "inspect", "--format", "{{.Id}}", "--", image).CombinedOutput() - // This time, the image MUST be present, so the issue must be something else. - if err != nil { - return "", fmt.Errorf("error inspecting docker image %q: %s", image, bytes.TrimSpace(out)) - } - id := string(bytes.TrimSpace(out)) - if id == "" { - return "", fmt.Errorf("unexpected empty docker image content ID for %q", image) - } - return id, nil -} diff --git a/internal/campaigns/bind_workspace.go b/internal/campaigns/bind_workspace.go index a1413dc55a..9d359b8dc1 100644 --- a/internal/campaigns/bind_workspace.go +++ b/internal/campaigns/bind_workspace.go @@ -21,7 +21,7 @@ type dockerBindWorkspaceCreator struct { var _ WorkspaceCreator = &dockerBindWorkspaceCreator{} -func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, zip string) (Workspace, error) { +func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, steps []Step, zip string) (Workspace, error) { w, err := wc.unzipToWorkspace(ctx, repo, zip) if err != nil { return nil, errors.Wrap(err, "unzipping the repository") diff --git a/internal/campaigns/campaign_spec.go b/internal/campaigns/campaign_spec.go index 0aefd0cb52..3dc5089d76 100644 --- a/internal/campaigns/campaign_spec.go +++ b/internal/campaigns/campaign_spec.go @@ -8,6 +8,7 @@ import ( "github.com/sourcegraph/campaignutils/env" "github.com/sourcegraph/campaignutils/overridable" "github.com/sourcegraph/campaignutils/yaml" + "github.com/sourcegraph/src-cli/internal/campaigns/docker" "github.com/sourcegraph/src-cli/schema" ) @@ -73,7 +74,7 @@ type Step struct { Files map[string]string `json:"files,omitempty" yaml:"files,omitempty"` Outputs Outputs `json:"outputs,omitempty" yaml:"outputs,omitempty"` - image string + image *docker.Image } type Outputs map[string]Output diff --git a/internal/campaigns/docker/cache.go b/internal/campaigns/docker/cache.go new file mode 100644 index 0000000000..2c29e171c0 --- /dev/null +++ b/internal/campaigns/docker/cache.go @@ -0,0 +1,32 @@ +package docker + +import "sync" + +// ImageCache is a cache of metadata about Docker images, indexed by name. +type ImageCache struct { + images map[string]*Image + imagesMu sync.Mutex +} + +// NewImageCache creates a new image cache. +func NewImageCache() *ImageCache { + return &ImageCache{ + images: make(map[string]*Image), + } +} + +// Get returns the image cache entry for the given Docker image. The name may be +// anything the Docker command line will accept as an image name: this will +// generally be IMAGE or IMAGE:TAG. +func (ic *ImageCache) Get(name string) *Image { + ic.imagesMu.Lock() + defer ic.imagesMu.Unlock() + + if image, ok := ic.images[name]; ok { + return image + } + + image := &Image{name: name} + ic.images[name] = image + return image +} diff --git a/internal/campaigns/docker/image.go b/internal/campaigns/docker/image.go new file mode 100644 index 0000000000..f1abb55954 --- /dev/null +++ b/internal/campaigns/docker/image.go @@ -0,0 +1,148 @@ +package docker + +import ( + "bytes" + "context" + "os/exec" + "strconv" + "strings" + "sync" + + "github.com/pkg/errors" +) + +// Image represents a Docker image, hopefully stored in the local cache. +type Image struct { + name string + + // There are lots of once fields below: basically, we're going to try fairly + // hard to prevent performing the same operations on the same image over and + // over, since some of them are expensive. + + digest string + digestOnce sync.Once + + ensureErr error + ensureOnce sync.Once + + uidGid UIDGID + uidGidOnce sync.Once +} + +// UIDGID represents a UID:GID pair. +type UIDGID struct { + UID int + GID int +} + +// Digest gets and returns the content digest for the image. Note that this is +// different from the "distribution digest" (which is what you can use to +// specify an image to `docker run`, as in `my/image@sha256:xxx`). We need to +// use the content digest because the distribution digest is only computed for +// images that have been pulled from or pushed to a registry. See +// https://windsock.io/explaining-docker-image-ids/ under "A Final Twist" for a +// good explanation. +func (image *Image) Digest(ctx context.Context) (string, error) { + var err error + image.digestOnce.Do(func() { + image.digest, err = func() (string, error) { + if err := image.Ensure(ctx); err != nil { + return "", err + } + + // TODO!(sqs): is image id the right thing to use here? it is NOT + // the digest. but the digest is not calculated for all images + // (unless they are pulled/pushed from/to a registry), see + // https://github.com/moby/moby/issues/32016. + out, err := exec.CommandContext(ctx, "docker", "image", "inspect", "--format", "{{.Id}}", "--", image.name).CombinedOutput() + if err != nil { + return "", errors.Wrapf(err, "inspecting docker image: %s", string(bytes.TrimSpace(out))) + } + id := string(bytes.TrimSpace(out)) + if id == "" { + return "", errors.Errorf("unexpected empty docker image content ID for %q", image) + } + return id, nil + }() + }) + + if err != nil { + return "", err + } + return image.digest, nil +} + +// Ensure ensures that the image has been pulled by Docker. Note that it does +// not attempt to pull a newer version of the image if it exists locally. +func (image *Image) Ensure(ctx context.Context) error { + image.ensureOnce.Do(func() { + image.ensureErr = func() error { + // docker image inspect will return a non-zero exit code if the image and + // tag don't exist locally, regardless of the format. + if err := exec.CommandContext(ctx, "docker", "image", "inspect", "--format", "1", image.name).Run(); err != nil { + // Let's try pulling the image. + if err := exec.CommandContext(ctx, "docker", "image", "pull", image.name).Run(); err != nil { + return errors.Wrap(err, "pulling image") + } + } + + return nil + }() + }) + + return image.ensureErr +} + +// UIDGID returns the user and group the container is configured to run as. +func (image *Image) UIDGID(ctx context.Context) (UIDGID, error) { + var err error + image.uidGidOnce.Do(func() { + image.uidGid, err = func() (UIDGID, error) { + stdout := new(bytes.Buffer) + + // Digest also implicitly means Ensure has been called. + digest, err := image.Digest(ctx) + if err != nil { + return UIDGID{}, errors.Wrap(err, "getting digest") + } + + args := []string{ + "run", + "--rm", + "--entrypoint", "/bin/sh", + digest, + "-c", "id -u; id -g", + } + cmd := exec.CommandContext(ctx, "docker", args...) + cmd.Stdout = stdout + + if err := cmd.Run(); err != nil { + return UIDGID{}, errors.Wrap(err, "running id") + } + + // POSIX specifies the output of `id -u` as the effective UID, + // terminated by a newline. `id -g` is the same, just for the GID. + raw := strings.TrimSpace(stdout.String()) + lines := strings.Split(raw, "\n") + if len(lines) < 2 { + // There's an id command on the path, but it's not returning + // POSIX compliant output. + return UIDGID{}, errors.Wrap(err, "invalid id output") + } + + uid, err := strconv.Atoi(lines[0]) + if err != nil { + return UIDGID{}, errors.Wrap(err, "malformed uid") + } + + gid, err := strconv.Atoi(lines[1]) + if err != nil { + return UIDGID{}, errors.Wrap(err, "malformed gid") + } + + return UIDGID{UID: uid, GID: gid}, nil + }() + }) + + return image.uidGid, err +} diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index 08fd7ecf3c..8e2552c3a7 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -39,7 +39,7 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr defer zip.Close() reportProgress("Initializing workspace") - workspace, err := wc.Create(ctx, repo, zip.Path()) + workspace, err := wc.Create(ctx, repo, steps, zip.Path()) if err != nil { return ExecutionResult{}, errors.Wrap(err, "creating workspace") } @@ -86,8 +86,14 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr } }() + // We need to grab the digest for the exact image we're using. + digest, err := step.image.Digest(ctx) + if err != nil { + return execResult, errors.Wrapf(err, "getting digest for %v", step.image) + } + // For now, we only support shell scripts provided via the Run field. - shell, containerTemp, err := probeImageForShell(ctx, step.image) + shell, containerTemp, err := probeImageForShell(ctx, digest) if err != nil { return execResult, errors.Wrapf(err, "probing image %q for shell", step.image) } @@ -189,7 +195,7 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr args = append(args, "--entrypoint", shell) cmd := exec.CommandContext(ctx, "docker", args...) - cmd.Args = append(cmd.Args, "--", step.image, containerTemp) + cmd.Args = append(cmd.Args, "--", digest, containerTemp) if dir := workspace.WorkDir(); dir != nil { cmd.Dir = *dir } diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index 791a0bf5a6..7987a8f401 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/sourcegraph/src-cli/internal/api" + "github.com/sourcegraph/src-cli/internal/campaigns/docker" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) @@ -22,6 +23,7 @@ type Service struct { allowUnsupported bool client api.Client features featureFlags + imageCache *docker.ImageCache workspace string } @@ -39,6 +41,7 @@ func NewService(opts *ServiceOpts) *Service { return &Service{ allowUnsupported: opts.AllowUnsupported, client: opts.Client, + imageCache: docker.NewImageCache(), workspace: opts.Workspace, } } @@ -213,14 +216,13 @@ func (svc *Service) NewRepoFetcher(dir string, cleanArchives bool) RepoFetcher { func (svc *Service) NewWorkspaceCreator(ctx context.Context, cacheDir, tempDir string, steps []Step) WorkspaceCreator { var workspace workspaceCreatorType - var workspaceCreatorVolumeUid int if svc.workspace == "volume" { workspace = workspaceCreatorVolume } else if svc.workspace == "bind" { workspace = workspaceCreatorBind } else { - workspace, workspaceCreatorVolumeUid = bestWorkspaceCreator(ctx, steps) + workspace = bestWorkspaceCreator(ctx, steps) } if workspace == workspaceCreatorVolume { @@ -232,29 +234,6 @@ func (svc *Service) NewWorkspaceCreator(ctx context.Context, cacheDir, tempDir s return &dockerBindWorkspaceCreator{dir: cacheDir} } -// dockerImageSet represents a set of Docker images that need to be pulled. The -// keys are the Docker image names; the values are a slice of pointers to -// strings that should be set to the content digest of the image. -type dockerImageSet map[string][]*string - -func (dis dockerImageSet) add(image string, digestPtr *string) { - if digestPtr == nil { - // Since we don't have a digest pointer here, we just need to ensure - // that the image exists at all in the map. - if _, ok := dis[image]; !ok { - dis[image] = []*string{} - } - } else { - // Either append the digest pointer to an existing map entry, or add a - // new entry if required. - if digests, ok := dis[image]; ok { - dis[image] = append(digests, digestPtr) - } else { - dis[image] = []*string{digestPtr} - } - } -} - // SetDockerImages updates the steps within the campaign spec to include the // exact content digest to be used when running each step, and ensures that all // Docker images are available, including any required by the service itself. @@ -262,30 +241,25 @@ func (dis dockerImageSet) add(image string, digestPtr *string) { // Progress information is reported back to the given progress function: perc // will be a value between 0.0 and 1.0, inclusive. func (svc *Service) SetDockerImages(ctx context.Context, spec *CampaignSpec, progress func(perc float64)) error { - images := dockerImageSet{} - for i, step := range spec.Steps { - images.add(step.Container, &spec.Steps[i].image) - } - - // We also need to ensure we have our own utility images available. - images.add(dockerVolumeWorkspaceImage, nil) - + total := len(spec.Steps) + 1 progress(0) - i := 0 - for image, digests := range images { - digest, err := getDockerImageContentDigest(ctx, image) - if err != nil { - return errors.Wrapf(err, "getting content digest for image %q", image) - } - for _, digestPtr := range digests { - *digestPtr = digest + + // TODO: this _really_ should be parallelised, since the image cache takes + // care to only pull the same image once. + for i := range spec.Steps { + spec.Steps[i].image = svc.imageCache.Get(spec.Steps[i].Container) + if err := spec.Steps[i].image.Ensure(ctx); err != nil { + return errors.Wrapf(err, "pulling image %q", spec.Steps[i].Container) } + progress(float64(i) / float64(total)) + } - progress(float64(i) / float64(len(images))) - i++ + // We also need to ensure we have our own utility images available. + if err := svc.imageCache.Get(dockerVolumeWorkspaceImage).Ensure(ctx); err != nil { + return errors.Wrapf(err, "pulling image %q", dockerVolumeWorkspaceImage) } - progress(1) + progress(1) return nil } diff --git a/internal/campaigns/volume_workspace.go b/internal/campaigns/volume_workspace.go index 5707c0e562..52db342d8e 100644 --- a/internal/campaigns/volume_workspace.go +++ b/internal/campaigns/volume_workspace.go @@ -21,14 +21,14 @@ type dockerVolumeWorkspaceCreator struct { var _ WorkspaceCreator = &dockerVolumeWorkspaceCreator{} -func (wc *dockerVolumeWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, zip string) (Workspace, error) { +func (wc *dockerVolumeWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, steps []Step, zip string) (Workspace, error) { volume, err := wc.createVolume(ctx) if err != nil { return nil, errors.Wrap(err, "creating Docker volume") } w := &dockerVolumeWorkspace{tempDir: wc.tempDir, volume: volume} - if err := wc.unzipRepoIntoVolume(ctx, w, zip); err != nil { + if err := wc.unzipRepoIntoVolume(ctx, w, steps, zip); err != nil { return nil, errors.Wrap(err, "unzipping repo into workspace") } @@ -62,7 +62,7 @@ git commit --quiet --all --allow-empty -m src-action-exec return nil } -func (c *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w *dockerVolumeWorkspace, zip string) error { +func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w *dockerVolumeWorkspace, steps []Step, zip string) error { // We want to mount that temporary file into a Docker container that has the // workspace volume attached, and unzip it into the volume. common, err := w.DockerRunOpts(ctx, "/work") @@ -70,30 +70,28 @@ func (c *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, return errors.Wrap(err, "generating run options") } - if c.uid != 0 { - owner := fmt.Sprintf("%d:%d", c.uid, c.uid) - - opts := append([]string{"run", "--rm", "--init"}, common...) - opts = append(opts, dockerVolumeWorkspaceImage, "chown", "-R", owner, "/work") - - if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { - return errors.Wrapf(err, "chown output:\n\n%s\n\n", string(out)) - } + ug, err := steps[0].image.UIDGID(ctx) + if err != nil { + return errors.Wrap(err, "getting container UID and GID") + } - // LOL/FML: This seems to "fix" the problem of `chown` getting lost between - // the `chown` call above and the unzip below? - opts = append([]string{"run", "--rm", "--init"}, common...) - opts = append(opts, dockerVolumeWorkspaceImage, "touch", "/work/DELETE_ME") + opts := append([]string{ + "run", + "--rm", + "--init", + "--workdir", "/work", + }, common...) + opts = append(opts, dockerVolumeWorkspaceImage, "sh", "-c", fmt.Sprintf("touch /work/foo; chown -R %d:%d /work", ug.UID, ug.GID)) - if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { - return errors.Wrapf(err, "chown output:\n\n%s\n\n", string(out)) - } + if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { + return errors.Wrapf(err, "chown output:\n\n%s\n\n", string(out)) } - opts := append([]string{ + opts = append([]string{ "run", "--rm", "--init", + "--user", fmt.Sprintf("%d:%d", ug.UID, ug.GID), "--workdir", "/work", "--mount", "type=bind,source=" + zip + ",target=/tmp/zip,ro", }, common...) @@ -103,16 +101,6 @@ func (c *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, return errors.Wrapf(err, "unzip output:\n\n%s\n\n", string(out)) } - // LOL/FML: delete stupid file - if c.uid != 0 { - opts = append([]string{"run", "--rm", "--init"}, common...) - opts = append(opts, dockerVolumeWorkspaceImage, "rm", "-rf", "/work/DELETE_ME") - - if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { - return errors.Wrapf(err, "chown output:\n\n%s\n\n", string(out)) - } - } - return nil } diff --git a/internal/campaigns/workspace.go b/internal/campaigns/workspace.go index 9d694352bd..4215ef3363 100644 --- a/internal/campaigns/workspace.go +++ b/internal/campaigns/workspace.go @@ -1,14 +1,10 @@ package campaigns import ( - "bytes" "context" "runtime" - "strconv" - "strings" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" - "github.com/sourcegraph/src-cli/internal/exec" ) // WorkspaceCreator implementations are used to create workspaces, which manage @@ -16,7 +12,7 @@ import ( // responsible for ultimately generating a diff. type WorkspaceCreator interface { // Create creates a new workspace for the given repository and ZIP file. - Create(ctx context.Context, repo *graphql.Repository, zip string) (Workspace, error) + Create(ctx context.Context, repo *graphql.Repository, steps []Step, zip string) (Workspace, error) } // Workspace implementations manage per-changeset storage when executing @@ -55,7 +51,7 @@ const ( // bestWorkspaceCreator determines the correct workspace creator to use based on // the environment and campaign to be executed. -func bestWorkspaceCreator(ctx context.Context, steps []Step) (workspaceCreatorType, int) { +func bestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCreatorType { // The basic theory here is that we have two options: bind and volume. Bind // is battle tested and always safe, but can be slow on non-Linux platforms // because bind mounts are slow. Volume is faster on those platforms, but @@ -67,13 +63,13 @@ func bestWorkspaceCreator(ctx context.Context, steps []Step) (workspaceCreatorTy // For the time being, we're only going to consider volume mode on Intel // macOS. if runtime.GOOS != "darwin" || runtime.GOARCH != "amd64" { - return workspaceCreatorBind, 0 + return workspaceCreatorBind } return detectBestWorkspaceCreator(ctx, steps) } -func detectBestWorkspaceCreator(ctx context.Context, steps []Step) (workspaceCreatorType, int) { +func detectBestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCreatorType { // OK, so we're interested in volume mode, but we need to take its // shortcomings around mixed user environments into account. // @@ -90,54 +86,23 @@ func detectBestWorkspaceCreator(ctx context.Context, steps []Step) (workspaceCre // In theory, we could make this more sensitive and complicated: a non-root // container that's followed by only root containers would actually be OK, // but let's keep it simple for now. - uids := make(map[int]struct{}) + var uid *int for _, step := range steps { - stdout := new(bytes.Buffer) - - args := []string{ - "run", - "--rm", - "--entrypoint", "/bin/sh", - step.image, - "-c", "id -u", - } - cmd := exec.CommandContext(ctx, "docker", args...) - cmd.Stdout = stdout - - if err := cmd.Run(); err != nil { + ug, err := step.image.UIDGID(ctx) + if err != nil { // An error here likely indicates that `id` isn't available on the // path. That's OK: let's not make any assumptions at this point // about the image, and we'll default to the always safe option. - return workspaceCreatorBind, 0 - } - - // POSIX specifies the output of `id -u` as the effective UID, - // terminated by a newline. - raw := strings.TrimSpace(stdout.String()) - uid, err := strconv.Atoi(raw) - if err != nil { - // This is a bit worse than the previous error case: there's an `id` - // command on the path, but it's not returning POSIX compliant - // output. That's weird, but we really don't need it to be terminal; - // let's fall back to bind mode. - // - // TODO: when logging is available at this level, we should log an - // error at verbose level to make this easier to debug. - return workspaceCreatorBind, 0 + return workspaceCreatorBind } - uids[uid] = struct{}{} - if len(uids) > 1 { - return workspaceCreatorBind, 0 + if uid == nil { + uid = &ug.UID + } else if *uid != ug.UID { + return workspaceCreatorBind } } - var uid int - for k := range uids { - uid = k - break // fml - } - - return workspaceCreatorVolume, uid + return workspaceCreatorVolume } From 86d00e29e50804eabf5160e3f4e7a078c5dc8c5b Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 19 Jan 2021 18:51:06 -0800 Subject: [PATCH 04/18] Randomise the dummy file. --- internal/campaigns/volume_workspace.go | 28 ++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/internal/campaigns/volume_workspace.go b/internal/campaigns/volume_workspace.go index 52db342d8e..17ca7e7eea 100644 --- a/internal/campaigns/volume_workspace.go +++ b/internal/campaigns/volume_workspace.go @@ -3,6 +3,8 @@ package campaigns import ( "bytes" "context" + "crypto/rand" + "encoding/hex" "fmt" "io/ioutil" "os" @@ -75,13 +77,30 @@ func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, return errors.Wrap(err, "getting container UID and GID") } + // We need to keep a temporary file in the volume before unzipping for the + // permissions to persist because... reasons. Rather than reading the + // potentially large ZIP file, we'll cheat a bit and just assume that if we + // create a file with an appropriately namespaced and random name, it's + // _probably_ OK. If you manage to reliably trigger an archive that has this + // file in it, we'll send you a hoodie or something. + randToken := make([]byte, 16) + if _, err := rand.Read(randToken); err != nil { + return errors.Wrap(err, "generating randomness") + } + dummy := fmt.Sprintf(".campaign-workspace-placeholder-%s", hex.EncodeToString(randToken)) + opts := append([]string{ "run", "--rm", "--init", "--workdir", "/work", }, common...) - opts = append(opts, dockerVolumeWorkspaceImage, "sh", "-c", fmt.Sprintf("touch /work/foo; chown -R %d:%d /work", ug.UID, ug.GID)) + opts = append( + opts, + dockerVolumeWorkspaceImage, + "sh", "-c", + fmt.Sprintf("touch /work/%s; chown -R %d:%d /work", dummy, ug.UID, ug.GID), + ) if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { return errors.Wrapf(err, "chown output:\n\n%s\n\n", string(out)) @@ -95,7 +114,12 @@ func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, "--workdir", "/work", "--mount", "type=bind,source=" + zip + ",target=/tmp/zip,ro", }, common...) - opts = append(opts, dockerVolumeWorkspaceImage, "unzip", "/tmp/zip") + opts = append( + opts, + dockerVolumeWorkspaceImage, + "sh", "-c", + fmt.Sprintf("unzip /tmp/zip; rm /work/%s", dummy), + ) if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { return errors.Wrapf(err, "unzip output:\n\n%s\n\n", string(out)) From d899444c0effb1a9b30edd94932ca0a64fc95f0e Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Wed, 20 Jan 2021 18:29:38 -0800 Subject: [PATCH 05/18] Fix rebase mistake. --- internal/campaigns/service.go | 5 +---- internal/campaigns/volume_workspace.go | 5 +---- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index 7987a8f401..e4dde68953 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -226,10 +226,7 @@ func (svc *Service) NewWorkspaceCreator(ctx context.Context, cacheDir, tempDir s } if workspace == workspaceCreatorVolume { - return &dockerVolumeWorkspaceCreator{ - tempDir: tempDir, - uid: workspaceCreatorVolumeUid, - } + return &dockerVolumeWorkspaceCreator{tempDir: tempDir} } return &dockerBindWorkspaceCreator{dir: cacheDir} } diff --git a/internal/campaigns/volume_workspace.go b/internal/campaigns/volume_workspace.go index 17ca7e7eea..1ba785dec4 100644 --- a/internal/campaigns/volume_workspace.go +++ b/internal/campaigns/volume_workspace.go @@ -16,10 +16,7 @@ import ( "github.com/sourcegraph/src-cli/internal/version" ) -type dockerVolumeWorkspaceCreator struct { - tempDir string - uid int -} +type dockerVolumeWorkspaceCreator struct{ tempDir string } var _ WorkspaceCreator = &dockerVolumeWorkspaceCreator{} From 4f18647b14be54ec03b200e0cb0a012db31f12f0 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Wed, 20 Jan 2021 18:30:41 -0800 Subject: [PATCH 06/18] Fix warning. --- internal/campaigns/docker/image.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/campaigns/docker/image.go b/internal/campaigns/docker/image.go index f1abb55954..e4c864fcec 100644 --- a/internal/campaigns/docker/image.go +++ b/internal/campaigns/docker/image.go @@ -60,7 +60,7 @@ func (image *Image) Digest(ctx context.Context) (string, error) { } id := string(bytes.TrimSpace(out)) if id == "" { - return "", errors.Errorf("unexpected empty docker image content ID for %q", image) + return "", errors.Errorf("unexpected empty docker image content ID for %q", image.name) } return id, nil }() From a85a05ca96a44ab5a061be41824ac47ac13e0540 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Wed, 20 Jan 2021 18:31:41 -0800 Subject: [PATCH 07/18] Fix error handling. --- internal/campaigns/docker/image.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/internal/campaigns/docker/image.go b/internal/campaigns/docker/image.go index e4c864fcec..1ec2087727 100644 --- a/internal/campaigns/docker/image.go +++ b/internal/campaigns/docker/image.go @@ -20,12 +20,14 @@ type Image struct { // over, since some of them are expensive. digest string + digestErr error digestOnce sync.Once ensureErr error ensureOnce sync.Once uidGid UIDGID + uidGidErr error uidGidOnce sync.Once } @@ -43,9 +45,8 @@ type UIDGID struct { // https://windsock.io/explaining-docker-image-ids/ under "A Final Twist" for a // good explanation. func (image *Image) Digest(ctx context.Context) (string, error) { - var err error image.digestOnce.Do(func() { - image.digest, err = func() (string, error) { + image.digest, image.digestErr = func() (string, error) { if err := image.Ensure(ctx); err != nil { return "", err } @@ -66,10 +67,7 @@ func (image *Image) Digest(ctx context.Context) (string, error) { }() }) - if err != nil { - return "", err - } - return image.digest, nil + return image.digest, image.digestErr } // Ensure ensures that the image has been pulled by Docker. Note that it does @@ -95,9 +93,8 @@ func (image *Image) Ensure(ctx context.Context) error { // UIDGID returns the user and group the container is configured to run as. func (image *Image) UIDGID(ctx context.Context) (UIDGID, error) { - var err error image.uidGidOnce.Do(func() { - image.uidGid, err = func() (UIDGID, error) { + image.uidGid, image.uidGidErr = func() (UIDGID, error) { stdout := new(bytes.Buffer) // Digest also implicitly means Ensure has been called. @@ -144,5 +141,5 @@ func (image *Image) UIDGID(ctx context.Context) (UIDGID, error) { }() }) - return image.uidGid, err + return image.uidGid, image.uidGidErr } From ecc64789f868cffeda957bb4d1965d30aaaa64de Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Wed, 20 Jan 2021 19:46:12 -0800 Subject: [PATCH 08/18] Various test updates. --- internal/campaigns/bind_workspace_test.go | 4 +- internal/campaigns/campaign_spec.go | 2 +- internal/campaigns/docker/cache.go | 8 +- internal/campaigns/docker/image.go | 34 +- internal/campaigns/volume_workspace.go | 50 +-- internal/campaigns/volume_workspace_test.go | 347 ++++++++++++++------ internal/campaigns/workspace_test.go | 77 ++--- 7 files changed, 335 insertions(+), 187 deletions(-) diff --git a/internal/campaigns/bind_workspace_test.go b/internal/campaigns/bind_workspace_test.go index 2ee90e7a38..95ec24b649 100644 --- a/internal/campaigns/bind_workspace_test.go +++ b/internal/campaigns/bind_workspace_test.go @@ -62,7 +62,7 @@ func TestDockerBindWorkspaceCreator_Create(t *testing.T) { testTempDir := workspaceTmpDir(t) creator := &dockerBindWorkspaceCreator{dir: testTempDir} - workspace, err := creator.Create(context.Background(), repo, archivePath) + workspace, err := creator.Create(context.Background(), repo, nil, archivePath) if err != nil { t.Fatalf("unexpected error: %s", err) } @@ -90,7 +90,7 @@ func TestDockerBindWorkspaceCreator_Create(t *testing.T) { badZip.Close() creator := &dockerBindWorkspaceCreator{dir: testTempDir} - if _, err := creator.Create(context.Background(), repo, badZipFile); err == nil { + if _, err := creator.Create(context.Background(), repo, nil, badZipFile); err == nil { t.Error("unexpected nil error") } }) diff --git a/internal/campaigns/campaign_spec.go b/internal/campaigns/campaign_spec.go index 3dc5089d76..5d94e0f9d6 100644 --- a/internal/campaigns/campaign_spec.go +++ b/internal/campaigns/campaign_spec.go @@ -74,7 +74,7 @@ type Step struct { Files map[string]string `json:"files,omitempty" yaml:"files,omitempty"` Outputs Outputs `json:"outputs,omitempty" yaml:"outputs,omitempty"` - image *docker.Image + image docker.Image } type Outputs map[string]Output diff --git a/internal/campaigns/docker/cache.go b/internal/campaigns/docker/cache.go index 2c29e171c0..798b2cae56 100644 --- a/internal/campaigns/docker/cache.go +++ b/internal/campaigns/docker/cache.go @@ -4,21 +4,21 @@ import "sync" // ImageCache is a cache of metadata about Docker images, indexed by name. type ImageCache struct { - images map[string]*Image + images map[string]Image imagesMu sync.Mutex } // NewImageCache creates a new image cache. func NewImageCache() *ImageCache { return &ImageCache{ - images: make(map[string]*Image), + images: make(map[string]Image), } } // Get returns the image cache entry for the given Docker image. The name may be // anything the Docker command line will accept as an image name: this will // generally be IMAGE or IMAGE:TAG. -func (ic *ImageCache) Get(name string) *Image { +func (ic *ImageCache) Get(name string) Image { ic.imagesMu.Lock() defer ic.imagesMu.Unlock() @@ -26,7 +26,7 @@ func (ic *ImageCache) Get(name string) *Image { return image } - image := &Image{name: name} + image := &image{name: name} ic.images[name] = image return image } diff --git a/internal/campaigns/docker/image.go b/internal/campaigns/docker/image.go index 1ec2087727..4db3001a75 100644 --- a/internal/campaigns/docker/image.go +++ b/internal/campaigns/docker/image.go @@ -3,6 +3,7 @@ package docker import ( "bytes" "context" + "fmt" "os/exec" "strconv" "strings" @@ -11,8 +12,27 @@ import ( "github.com/pkg/errors" ) +// UIDGID represents a UID:GID pair. +type UIDGID struct { + UID int + GID int +} + +func (ug UIDGID) String() string { + return fmt.Sprintf("%d:%d", ug.UID, ug.GID) +} + +// Root is a root:root user. +var Root = UIDGID{UID: 0, GID: 0} + // Image represents a Docker image, hopefully stored in the local cache. -type Image struct { +type Image interface { + Digest(context.Context) (string, error) + Ensure(context.Context) error + UIDGID(context.Context) (UIDGID, error) +} + +type image struct { name string // There are lots of once fields below: basically, we're going to try fairly @@ -31,12 +51,6 @@ type Image struct { uidGidOnce sync.Once } -// UIDGID represents a UID:GID pair. -type UIDGID struct { - UID int - GID int -} - // Digest gets and returns the content digest for the image. Note that this is // different from the "distribution digest" (which is what you can use to // specify an image to `docker run`, as in `my/image@sha256:xxx`). We need to @@ -44,7 +58,7 @@ type UIDGID struct { // images that have been pulled from or pushed to a registry. See // https://windsock.io/explaining-docker-image-ids/ under "A Final Twist" for a // good explanation. -func (image *Image) Digest(ctx context.Context) (string, error) { +func (image *image) Digest(ctx context.Context) (string, error) { image.digestOnce.Do(func() { image.digest, image.digestErr = func() (string, error) { if err := image.Ensure(ctx); err != nil { @@ -72,7 +86,7 @@ func (image *Image) Digest(ctx context.Context) (string, error) { // Ensure ensures that the image has been pulled by Docker. Note that it does // not attempt to pull a newer version of the image if it exists locally. -func (image *Image) Ensure(ctx context.Context) error { +func (image *image) Ensure(ctx context.Context) error { image.ensureOnce.Do(func() { image.ensureErr = func() error { // docker image inspect will return a non-zero exit code if the image and @@ -92,7 +106,7 @@ func (image *Image) Ensure(ctx context.Context) error { } // UIDGID returns the user and group the container is configured to run as. -func (image *Image) UIDGID(ctx context.Context) (UIDGID, error) { +func (image *image) UIDGID(ctx context.Context) (UIDGID, error) { image.uidGidOnce.Do(func() { image.uidGid, image.uidGidErr = func() (UIDGID, error) { stdout := new(bytes.Buffer) diff --git a/internal/campaigns/volume_workspace.go b/internal/campaigns/volume_workspace.go index 1ba785dec4..b34ed21927 100644 --- a/internal/campaigns/volume_workspace.go +++ b/internal/campaigns/volume_workspace.go @@ -11,6 +11,7 @@ import ( "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/campaigns/docker" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" "github.com/sourcegraph/src-cli/internal/exec" "github.com/sourcegraph/src-cli/internal/version" @@ -26,8 +27,21 @@ func (wc *dockerVolumeWorkspaceCreator) Create(ctx context.Context, repo *graphq return nil, errors.Wrap(err, "creating Docker volume") } - w := &dockerVolumeWorkspace{tempDir: wc.tempDir, volume: volume} - if err := wc.unzipRepoIntoVolume(ctx, w, steps, zip); err != nil { + // Figure out the user that containers will be run as. + ug := docker.UIDGID{} + if len(steps) > 0 { + var err error + if ug, err = steps[0].image.UIDGID(ctx); err != nil { + return nil, errors.Wrap(err, "getting container UID and GID") + } + } + + w := &dockerVolumeWorkspace{ + tempDir: wc.tempDir, + volume: volume, + uidGid: ug, + } + if err := wc.unzipRepoIntoVolume(ctx, w, zip); err != nil { return nil, errors.Wrap(err, "unzipping repo into workspace") } @@ -61,18 +75,9 @@ git commit --quiet --all --allow-empty -m src-action-exec return nil } -func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w *dockerVolumeWorkspace, steps []Step, zip string) error { +func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w *dockerVolumeWorkspace, zip string) error { // We want to mount that temporary file into a Docker container that has the // workspace volume attached, and unzip it into the volume. - common, err := w.DockerRunOpts(ctx, "/work") - if err != nil { - return errors.Wrap(err, "generating run options") - } - - ug, err := steps[0].image.UIDGID(ctx) - if err != nil { - return errors.Wrap(err, "getting container UID and GID") - } // We need to keep a temporary file in the volume before unzipping for the // permissions to persist because... reasons. Rather than reading the @@ -86,31 +91,32 @@ func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, } dummy := fmt.Sprintf(".campaign-workspace-placeholder-%s", hex.EncodeToString(randToken)) + // So, let's use that to set up the volume. opts := append([]string{ "run", "--rm", "--init", "--workdir", "/work", - }, common...) + }, w.dockerRunOptsWithUser(docker.Root, "/work")...) opts = append( opts, dockerVolumeWorkspaceImage, "sh", "-c", - fmt.Sprintf("touch /work/%s; chown -R %d:%d /work", dummy, ug.UID, ug.GID), + fmt.Sprintf("touch /work/%s; chown -R %s /work", dummy, w.uidGid.String()), ) if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { return errors.Wrapf(err, "chown output:\n\n%s\n\n", string(out)) } + // Now we can unzip the archive as the user and clean up the temporary file. opts = append([]string{ "run", "--rm", "--init", - "--user", fmt.Sprintf("%d:%d", ug.UID, ug.GID), "--workdir", "/work", "--mount", "type=bind,source=" + zip + ",target=/tmp/zip,ro", - }, common...) + }, w.dockerRunOptsWithUser(w.uidGid, "/work")...) opts = append( opts, dockerVolumeWorkspaceImage, @@ -132,6 +138,7 @@ func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, type dockerVolumeWorkspace struct { tempDir string volume string + uidGid docker.UIDGID } var _ Workspace = &dockerVolumeWorkspace{} @@ -142,9 +149,7 @@ func (w *dockerVolumeWorkspace) Close(ctx context.Context) error { } func (w *dockerVolumeWorkspace) DockerRunOpts(ctx context.Context, target string) ([]string, error) { - return []string{ - "--mount", "type=volume,source=" + w.volume + ",target=" + target, - }, nil + return w.dockerRunOptsWithUser(w.uidGid, target), nil } func (w *dockerVolumeWorkspace) WorkDir() *string { return nil } @@ -242,3 +247,10 @@ func (w *dockerVolumeWorkspace) runScript(ctx context.Context, target, script st return out, nil } + +func (w *dockerVolumeWorkspace) dockerRunOptsWithUser(ug docker.UIDGID, target string) []string { + return []string{ + "--user", ug.String(), + "--mount", "type=volume,source=" + w.volume + ",target=" + target, + } +} diff --git a/internal/campaigns/volume_workspace_test.go b/internal/campaigns/volume_workspace_test.go index fa805acc1a..27d1feafb7 100644 --- a/internal/campaigns/volume_workspace_test.go +++ b/internal/campaigns/volume_workspace_test.go @@ -11,6 +11,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/campaigns/docker" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" "github.com/sourcegraph/src-cli/internal/exec/expect" ) @@ -39,103 +40,222 @@ func TestVolumeWorkspaceCreator(t *testing.T) { DefaultBranch: &graphql.Branch{Name: "main"}, } - t.Run("success", func(t *testing.T) { - expect.Commands( - t, - expect.NewGlob( - expect.Behaviour{Stdout: []byte(volumeID)}, - "docker", "volume", "create", - ), - expect.NewGlob( - expect.Behaviour{}, - "docker", "run", "--rm", "--init", "--workdir", "/work", - "--mount", "type=bind,source=*,target=/tmp/zip,ro", - "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, - "unzip", "/tmp/zip", - ), - expect.NewGlob( - expect.Behaviour{}, - "docker", "run", "--rm", "--init", "--workdir", "/work", - "--mount", "type=bind,source=*,target=/run.sh,ro", - "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, - "sh", "/run.sh", - ), - ) - - if w, err := wc.Create(ctx, repo, zip); err != nil { - t.Errorf("unexpected error: %v", err) - } else if have := w.(*dockerVolumeWorkspace).volume; have != volumeID { - t.Errorf("unexpected volume: have=%q want=%q", have, volumeID) - } - }) - - t.Run("docker volume create failure", func(t *testing.T) { - expect.Commands( - t, - expect.NewGlob( - expect.Behaviour{ExitCode: 1}, - "docker", "volume", "create", - ), - ) - - if _, err := wc.Create(ctx, repo, zip); err == nil { - t.Error("unexpected nil error") - } - }) - - t.Run("unzip failure", func(t *testing.T) { - expect.Commands( - t, - expect.NewGlob( - expect.Behaviour{Stdout: []byte(volumeID)}, - "docker", "volume", "create", - ), - expect.NewGlob( - expect.Behaviour{ExitCode: 1}, - "docker", "run", "--rm", "--init", "--workdir", "/work", - "--mount", "type=bind,source=*,target=/tmp/zip,ro", - "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, - "unzip", "/tmp/zip", - ), - ) - - if _, err := wc.Create(ctx, repo, zip); err == nil { - t.Error("unexpected nil error") - } - }) - - t.Run("git init failure", func(t *testing.T) { - expect.Commands( - t, - expect.NewGlob( - expect.Behaviour{Stdout: []byte(volumeID)}, - "docker", "volume", "create", - ), - expect.NewGlob( - expect.Behaviour{}, - "docker", "run", "--rm", "--init", "--workdir", "/work", - "--mount", "type=bind,source=*,target=/tmp/zip,ro", - "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, - "unzip", "/tmp/zip", - ), - expect.NewGlob( - expect.Behaviour{ExitCode: 1}, - "docker", "run", "--rm", "--init", "--workdir", "/work", - "--mount", "type=bind,source=*,target=/run.sh,ro", - "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, - "sh", "/run.sh", - ), - ) - - if _, err := wc.Create(ctx, repo, zip); err == nil { - t.Error("unexpected nil error") - } - }) + for name, tc := range map[string]struct { + expectations []*expect.Expectation + steps []Step + wantErr bool + }{ + "no steps": { + expectations: []*expect.Expectation{ + expect.NewGlob( + expect.Behaviour{Stdout: []byte(volumeID)}, + "docker", "volume", "create", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "touch /work/*; chown -R 0:0 /work", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", + "--workdir", "/work", + "--mount", "type=bind,source=*,target=/tmp/zip,ro", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "unzip /tmp/zip; rm /work/*", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "/run.sh", + ), + }, + steps: []Step{}, + }, + "one root:root step": { + expectations: []*expect.Expectation{ + expect.NewGlob( + expect.Behaviour{Stdout: []byte(volumeID)}, + "docker", "volume", "create", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "touch /work/*; chown -R 0:0 /work", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", + "--workdir", "/work", + "--mount", "type=bind,source=*,target=/tmp/zip,ro", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "unzip /tmp/zip; rm /work/*", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "/run.sh", + ), + }, + steps: []Step{ + {image: &mockImage{uidGid: docker.Root}}, + }, + }, + "one user:user step": { + expectations: []*expect.Expectation{ + expect.NewGlob( + expect.Behaviour{Stdout: []byte(volumeID)}, + "docker", "volume", "create", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "touch /work/*; chown -R 1000:1000 /work", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", + "--workdir", "/work", + "--mount", "type=bind,source=*,target=/tmp/zip,ro", + "--user", "1000:1000", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "unzip /tmp/zip; rm /work/*", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--user", "1000:1000", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "/run.sh", + ), + }, + steps: []Step{ + {image: &mockImage{uidGid: docker.UIDGID{UID: 1000, GID: 1000}}}, + }, + }, + // TODO: add more success tests and a chown failure test + "docker volume create failure": { + expectations: []*expect.Expectation{ + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + "docker", "volume", "create", + ), + }, + steps: []Step{ + {image: &mockImage{uidGid: docker.Root}}, + }, + wantErr: true, + }, + "git init failure": { + expectations: []*expect.Expectation{ + expect.NewGlob( + expect.Behaviour{Stdout: []byte(volumeID)}, + "docker", "volume", "create", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "touch /work/*; chown -R 0:0 /work", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", + "--workdir", "/work", + "--mount", "type=bind,source=*,target=/tmp/zip,ro", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "unzip /tmp/zip; rm /work/*", + ), + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "/run.sh", + ), + }, + steps: []Step{ + {image: &mockImage{uidGid: docker.Root}}, + }, + wantErr: true, + }, + "unzip failure": { + expectations: []*expect.Expectation{ + expect.NewGlob( + expect.Behaviour{Stdout: []byte(volumeID)}, + "docker", "volume", "create", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "touch /work/*; chown -R 0:0 /work", + ), + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + "docker", "run", "--rm", "--init", + "--workdir", "/work", + "--mount", "type=bind,source=*,target=/tmp/zip,ro", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "unzip /tmp/zip; rm /work/*", + ), + }, + steps: []Step{ + {image: &mockImage{uidGid: docker.Root}}, + }, + wantErr: true, + }, + } { + t.Run(name, func(t *testing.T) { + expect.Commands(t, tc.expectations...) + w, err := wc.Create(ctx, repo, tc.steps, zip) + if tc.wantErr { + if err == nil { + t.Error("unexpected nil error") + } + } else { + if err != nil { + t.Errorf("unexpected error: %v", err) + } else if have := w.(*dockerVolumeWorkspace).volume; have != volumeID { + t.Errorf("unexpected volume: have=%q want=%q", have, volumeID) + } + } + }) + } } func TestVolumeWorkspace_Close(t *testing.T) { @@ -173,9 +293,13 @@ func TestVolumeWorkspace_Close(t *testing.T) { func TestVolumeWorkspace_DockerRunOpts(t *testing.T) { ctx := context.Background() - w := &dockerVolumeWorkspace{volume: "VOLUME"} + w := &dockerVolumeWorkspace{ + volume: "VOLUME", + uidGid: docker.UIDGID{UID: 1, GID: 2}, + } want := []string{ + "--user", "1:2", "--mount", "type=volume,source=VOLUME,target=TARGET", } have, err := w.DockerRunOpts(ctx, "TARGET") @@ -231,6 +355,7 @@ M internal/campaigns/volume_workspace_test.go expect.Behaviour{Stdout: bytes.TrimSpace([]byte(tc.stdout))}, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/run.sh,ro", + "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", dockerVolumeWorkspaceImage, "sh", "/run.sh", @@ -262,6 +387,7 @@ M internal/campaigns/volume_workspace_test.go behaviour, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/run.sh,ro", + "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", dockerVolumeWorkspaceImage, "sh", "/run.sh", @@ -307,6 +433,7 @@ index 06471f4..5f9d3fa 100644 expect.Behaviour{Stdout: []byte(want)}, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/run.sh,ro", + "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", dockerVolumeWorkspaceImage, "sh", "/run.sh", @@ -333,6 +460,7 @@ index 06471f4..5f9d3fa 100644 expect.Behaviour{ExitCode: 1}, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/run.sh,ro", + "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", dockerVolumeWorkspaceImage, "sh", "/run.sh", @@ -361,6 +489,7 @@ func TestVolumeWorkspace_runScript(t *testing.T) { glob := expect.NewGlobValidator( "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/run.sh,ro", + "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", dockerVolumeWorkspaceImage, "sh", "/run.sh", @@ -391,3 +520,25 @@ func TestVolumeWorkspace_runScript(t *testing.T) { t.Fatal(err) } } + +type mockImage struct { + digest string + digestErr error + ensureErr error + uidGid docker.UIDGID + uidGidErr error +} + +var _ docker.Image = &mockImage{} + +func (image *mockImage) Digest(ctx context.Context) (string, error) { + return image.digest, image.digestErr +} + +func (image *mockImage) Ensure(ctx context.Context) error { + return image.ensureErr +} + +func (image *mockImage) UIDGID(ctx context.Context) (docker.UIDGID, error) { + return image.uidGid, image.uidGidErr +} diff --git a/internal/campaigns/workspace_test.go b/internal/campaigns/workspace_test.go index b2564992d3..5d66243e41 100644 --- a/internal/campaigns/workspace_test.go +++ b/internal/campaigns/workspace_test.go @@ -5,6 +5,8 @@ import ( "runtime" "testing" + "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/campaigns/docker" "github.com/sourcegraph/src-cli/internal/exec/expect" ) @@ -17,84 +19,53 @@ func TestBestWorkspaceCreator(t *testing.T) { behaviour expect.Behaviour } for name, tc := range map[string]struct { - behaviours []imageBehaviour - want workspaceCreatorType + images []docker.Image + want workspaceCreatorType }{ "nil steps": { - behaviours: nil, - want: workspaceCreatorVolume, + images: nil, + want: workspaceCreatorVolume, }, "no steps": { - behaviours: []imageBehaviour{}, - want: workspaceCreatorVolume, + images: []docker.Image{}, + want: workspaceCreatorVolume, }, "root": { - behaviours: []imageBehaviour{ - {image: "foo", behaviour: expect.Behaviour{Stdout: []byte("0\n")}}, - {image: "bar", behaviour: expect.Behaviour{Stdout: []byte("0\n")}}, + images: []docker.Image{ + &mockImage{uidGid: docker.UIDGID{0, 0}}, }, want: workspaceCreatorVolume, }, "same user": { - behaviours: []imageBehaviour{ - {image: "foo", behaviour: expect.Behaviour{Stdout: []byte("1000\n")}}, - {image: "bar", behaviour: expect.Behaviour{Stdout: []byte("1000\n")}}, + images: []docker.Image{ + &mockImage{uidGid: docker.UIDGID{1000, 1000}}, + &mockImage{uidGid: docker.UIDGID{1000, 1000}}, }, want: workspaceCreatorVolume, }, "different user": { - behaviours: []imageBehaviour{ - {image: "foo", behaviour: expect.Behaviour{Stdout: []byte("0\n")}}, - {image: "bar", behaviour: expect.Behaviour{Stdout: []byte("1000\n")}}, + images: []docker.Image{ + &mockImage{uidGid: docker.UIDGID{1000, 1000}}, + &mockImage{uidGid: docker.UIDGID{0, 0}}, }, want: workspaceCreatorBind, }, - "invalid id output: string": { - behaviours: []imageBehaviour{ - {image: "foo", behaviour: expect.Behaviour{Stdout: []byte("xxx\n")}}, - }, - want: workspaceCreatorBind, - }, - "invalid id output: empty": { - behaviours: []imageBehaviour{ - {image: "foo", behaviour: expect.Behaviour{Stdout: []byte("")}}, - }, - want: workspaceCreatorBind, - }, - "error invoking id": { - behaviours: []imageBehaviour{ - {image: "foo", behaviour: expect.Behaviour{ExitCode: 1}}, + "id error": { + images: []docker.Image{ + &mockImage{uidGidErr: errors.New("foo")}, }, want: workspaceCreatorBind, }, } { t.Run(name, func(t *testing.T) { - var ( - commands []*expect.Expectation = nil - steps []Step = nil - ) - if tc.behaviours != nil { - commands = []*expect.Expectation{} - steps = []Step{} - for _, imageBehaviour := range tc.behaviours { - commands = append(commands, expect.NewGlob( - imageBehaviour.behaviour, - "docker", "run", "--rm", "--entrypoint", "/bin/sh", - imageBehaviour.image, "-c", "id -u", - )) - steps = append(steps, Step{image: imageBehaviour.image}) + var steps []Step + if tc.images != nil { + steps = make([]Step, len(tc.images)) + for i, image := range tc.images { + steps[i].image = image } } - if !isOverridden { - // If bestWorkspaceCreator() won't short circuit on this - // platform, we're going to run the Docker commands twice by - // definition. - expect.Commands(t, append(commands, commands...)...) - } else { - expect.Commands(t, commands...) - } - if isOverridden { // This is an overridden platform, so the workspace type will // always be bind from bestWorkspaceCreator(). From bc5eb0b880874ad414f677697727c4b137eb84a7 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Wed, 20 Jan 2021 19:46:35 -0800 Subject: [PATCH 09/18] Various test updates. --- internal/campaigns/volume_workspace_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/internal/campaigns/volume_workspace_test.go b/internal/campaigns/volume_workspace_test.go index 27d1feafb7..4c9669e68e 100644 --- a/internal/campaigns/volume_workspace_test.go +++ b/internal/campaigns/volume_workspace_test.go @@ -131,14 +131,14 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", dockerVolumeWorkspaceImage, - "sh", "-c", "touch /work/*; chown -R 1000:1000 /work", + "sh", "-c", "touch /work/*; chown -R 1:2 /work", ), expect.NewGlob( expect.Behaviour{}, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/tmp/zip,ro", - "--user", "1000:1000", + "--user", "1:2", "--mount", "type=volume,source="+volumeID+",target=/work", dockerVolumeWorkspaceImage, "sh", "-c", "unzip /tmp/zip; rm /work/*", @@ -147,17 +147,16 @@ func TestVolumeWorkspaceCreator(t *testing.T) { expect.Behaviour{}, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/run.sh,ro", - "--user", "1000:1000", + "--user", "1:2", "--mount", "type=volume,source="+volumeID+",target=/work", dockerVolumeWorkspaceImage, "sh", "/run.sh", ), }, steps: []Step{ - {image: &mockImage{uidGid: docker.UIDGID{UID: 1000, GID: 1000}}}, + {image: &mockImage{uidGid: docker.UIDGID{UID: 1, GID: 2}}}, }, }, - // TODO: add more success tests and a chown failure test "docker volume create failure": { expectations: []*expect.Expectation{ expect.NewGlob( From da6b79ded235d17fcdcaa1d62d09706b96beb7d9 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Wed, 20 Jan 2021 19:49:13 -0800 Subject: [PATCH 10/18] Fix step handling in integration test. --- internal/campaigns/executor_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index f0c714cd23..61a197b60e 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -314,6 +314,12 @@ repository_name=github.com/sourcegraph/src-cli`, template = tc.template } + for i := range tc.steps { + tc.steps[i].image = &mockImage{ + digest: tc.steps[i].Container, + } + } + for _, r := range tc.repos { executor.AddTask(r, tc.steps, tc.transform, template) } From 38ae7bc0a791a504e64cd4dbe4224fac27a0c43a Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 21 Jan 2021 16:38:32 -0800 Subject: [PATCH 11/18] Add missing test case. --- internal/campaigns/volume_workspace_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/internal/campaigns/volume_workspace_test.go b/internal/campaigns/volume_workspace_test.go index 4c9669e68e..7644efe908 100644 --- a/internal/campaigns/volume_workspace_test.go +++ b/internal/campaigns/volume_workspace_test.go @@ -169,6 +169,26 @@ func TestVolumeWorkspaceCreator(t *testing.T) { }, wantErr: true, }, + "chown failure": { + expectations: []*expect.Expectation{ + expect.NewGlob( + expect.Behaviour{Stdout: []byte(volumeID)}, + "docker", "volume", "create", + ), + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "touch /work/*; chown -R 0:0 /work", + ), + }, + steps: []Step{ + {image: &mockImage{uidGid: docker.Root}}, + }, + wantErr: true, + }, "git init failure": { expectations: []*expect.Expectation{ expect.NewGlob( From 3bde896615fb74bd3aa7c9379e91ed8d359ad590 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 21 Jan 2021 17:48:44 -0800 Subject: [PATCH 12/18] Fix invalid error. --- internal/campaigns/docker/image.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/campaigns/docker/image.go b/internal/campaigns/docker/image.go index 4db3001a75..c3a5ef17ab 100644 --- a/internal/campaigns/docker/image.go +++ b/internal/campaigns/docker/image.go @@ -138,7 +138,7 @@ func (image *image) UIDGID(ctx context.Context) (UIDGID, error) { if len(lines) < 2 { // There's an id command on the path, but it's not returning // POSIX compliant output. - return UIDGID{}, errors.Wrap(err, "invalid id output") + return UIDGID{}, errors.New("invalid id output") } uid, err := strconv.Atoi(lines[0]) From bf93c948e1dc1de6b50f3bf3a12fb4ea90a0ca4a Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 21 Jan 2021 17:48:59 -0800 Subject: [PATCH 13/18] Add unit tests. --- internal/campaigns/docker/cache_test.go | 23 ++ internal/campaigns/docker/image.go | 3 +- internal/campaigns/docker/image_test.go | 347 ++++++++++++++++++++++++ internal/campaigns/docker/main_test.go | 13 + 4 files changed, 385 insertions(+), 1 deletion(-) create mode 100644 internal/campaigns/docker/cache_test.go create mode 100644 internal/campaigns/docker/image_test.go create mode 100644 internal/campaigns/docker/main_test.go diff --git a/internal/campaigns/docker/cache_test.go b/internal/campaigns/docker/cache_test.go new file mode 100644 index 0000000000..a19278b73a --- /dev/null +++ b/internal/campaigns/docker/cache_test.go @@ -0,0 +1,23 @@ +package docker + +import "testing" + +func TestImageCache(t *testing.T) { + cache := NewImageCache() + if cache == nil { + t.Error("unexpected nil cache") + } + + have := cache.Get("foo") + if have == nil { + t.Error("unexpected nil error") + } + if name := have.(*image).name; name != "foo" { + t.Errorf("invalid name: have=%q want=%q", name, "foo") + } + + again := cache.Get("foo") + if have != again { + t.Errorf("invalid memoisation: first=%v second=%v", have, again) + } +} diff --git a/internal/campaigns/docker/image.go b/internal/campaigns/docker/image.go index c3a5ef17ab..3cd3a343cc 100644 --- a/internal/campaigns/docker/image.go +++ b/internal/campaigns/docker/image.go @@ -4,12 +4,13 @@ import ( "bytes" "context" "fmt" - "os/exec" "strconv" "strings" "sync" "github.com/pkg/errors" + + "github.com/sourcegraph/src-cli/internal/exec" ) // UIDGID represents a UID:GID pair. diff --git a/internal/campaigns/docker/image_test.go b/internal/campaigns/docker/image_test.go new file mode 100644 index 0000000000..6e525cf5eb --- /dev/null +++ b/internal/campaigns/docker/image_test.go @@ -0,0 +1,347 @@ +package docker + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/src-cli/internal/exec/expect" +) + +func TestImage_Digest(t *testing.T) { + ctx := context.Background() + + for name, tc := range map[string]struct { + expectations []*expect.Expectation + image *image + want string + wantErr bool + }{ + "success": { + expectations: digestSuccess("foo", "bar"), + image: &image{name: "foo"}, + want: "bar", + }, + "inspect invalid output": { + expectations: append( + ensureSuccess("foo"), + expect.NewGlob( + expect.Behaviour{}, + // Note the awkward escaping because these arguments are matched by + // glob. + "docker", "image", "inspect", "--format", `\{\{.Id}}`, "--", "foo", + ), + ), + image: &image{name: "foo"}, + wantErr: true, + }, + "inspect failure": { + expectations: digestFailure("foo"), + image: &image{name: "foo"}, + wantErr: true, + }, + "ensure failure": { + expectations: ensureFailure("foo"), + image: &image{name: "foo"}, + wantErr: true, + }, + } { + t.Run(name, func(t *testing.T) { + expect.Commands(t, tc.expectations...) + + // We'll call Digest twice to make sure the memoisation works. + test := func() (string, error) { + have, err := tc.image.Digest(ctx) + if tc.wantErr { + if err == nil { + t.Error("unexpected nil error") + } + } else if err != nil { + t.Errorf("unexpected error: %+v", err) + } else if have != tc.want { + t.Errorf("unexpected digest: have=%q want=%q", have, tc.want) + } + + return have, err + } + firstDigest, firstErr := test() + secondDigest, secondErr := test() + + if firstDigest != secondDigest { + t.Errorf("digests do not match: first=%q second=%q", firstDigest, secondDigest) + } + if firstErr != secondErr { + t.Errorf("errors do not match: first=%v second=%v", firstErr, secondErr) + } + }) + } +} + +func TestImage_Ensure(t *testing.T) { + ctx := context.Background() + + for name, tc := range map[string]struct { + expectations []*expect.Expectation + image *image + wantErr bool + }{ + "no pull required": { + expectations: ensureSuccess("foo"), + image: &image{name: "foo"}, + wantErr: false, + }, + "pull required": { + expectations: []*expect.Expectation{ + expect.NewGlob( + // docker image inspect returns 1 for non-existent images. + expect.Behaviour{ExitCode: 1}, + "docker", "image", "inspect", "--format", "1", "foo", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "image", "pull", "foo", + ), + }, + image: &image{name: "foo"}, + wantErr: false, + }, + "pull failed": { + expectations: ensureFailure("foo"), + image: &image{name: "foo"}, + wantErr: true, + }, + } { + t.Run(name, func(t *testing.T) { + expect.Commands(t, tc.expectations...) + + // We'll call Ensure twice to make sure the memoisation works. + test := func() error { + have := tc.image.Ensure(ctx) + if tc.wantErr { + if have == nil { + t.Error("unexpected nil error") + } + } else if have != nil { + t.Errorf("unexpected error: %+v", have) + } + + return have + } + first := test() + second := test() + + if first != second { + t.Errorf("errors do not match: first=%v second=%v", first, second) + } + }) + } +} + +func TestImage_UIDGID(t *testing.T) { + ctx := context.Background() + + for name, tc := range map[string]struct { + expectations []*expect.Expectation + image *image + want UIDGID + wantErr bool + }{ + "success": { + expectations: append( + digestSuccess("foo", "bar"), + uidGid("bar", expect.Behaviour{Stdout: []byte("1000\n2000\n")}), + ), + image: &image{name: "foo"}, + want: UIDGID{UID: 1000, GID: 2000}, + }, + // We should also make sure 0 works. Sometimes it's easy to miss. Just + // ask the Romans. + "success with zeroes": { + expectations: append( + digestSuccess("foo", "bar"), + uidGid("bar", expect.Behaviour{Stdout: []byte("0\n0\n")}), + ), + image: &image{name: "foo"}, + want: UIDGID{UID: 0, GID: 0}, + }, + // This is technically valid, because POSIX basically punts on the + // signedness of pid_t and gid_t. We don't really have a reason to + // disallow it. + "success with negative IDs": { + expectations: append( + digestSuccess("foo", "bar"), + uidGid("bar", expect.Behaviour{Stdout: []byte("-1000\n-2000\n")}), + ), + image: &image{name: "foo"}, + want: UIDGID{UID: -1000, GID: -2000}, + }, + // This is technically invalid, but should still succeed. Postel's Law + // and all that. + "success without trailing newline": { + expectations: append( + digestSuccess("foo", "bar"), + uidGid("bar", expect.Behaviour{Stdout: []byte("1000\n2000")}), + ), + image: &image{name: "foo"}, + want: UIDGID{UID: 1000, GID: 2000}, + }, + // As above, this is invalid, but we should still handle it. + "success with extra data": { + expectations: append( + digestSuccess("foo", "bar"), + uidGid("bar", expect.Behaviour{Stdout: []byte("1000\n2000\n3000\n")}), + ), + image: &image{name: "foo"}, + want: UIDGID{UID: 1000, GID: 2000}, + }, + // Now for some interesting failure cases. + "invalid output": { + expectations: append( + digestSuccess("foo", "bar"), + uidGid("bar", expect.Behaviour{Stdout: []byte("")}), + ), + image: &image{name: "foo"}, + wantErr: true, + }, + // This is ripped from the headlines^WDocker. + "missing id binary": { + expectations: append( + digestSuccess("foo", "bar"), + uidGid("bar", expect.Behaviour{ + ExitCode: 127, + Stderr: []byte("sh: id: not found")}), + ), + image: &image{name: "foo"}, + wantErr: true, + }, + // POSIX might allow negative IDs because, well, honestly, it was + // probably an oversight, but we shouldn't allow string IDs. That would + // be a bridge too far. + "string uid": { + expectations: append( + digestSuccess("foo", "bar"), + uidGid("bar", expect.Behaviour{Stdout: []byte("X\n2000\n")}), + ), + image: &image{name: "foo"}, + wantErr: true, + }, + "string gid": { + expectations: append( + digestSuccess("foo", "bar"), + uidGid("bar", expect.Behaviour{Stdout: []byte("1000\nX\n")}), + ), + image: &image{name: "foo"}, + wantErr: true, + }, + // Now for some more run of the mill failures. + "docker run failure": { + expectations: append( + digestSuccess("foo", "bar"), + uidGid("bar", expect.Behaviour{ExitCode: 1}), + ), + image: &image{name: "foo"}, + wantErr: true, + }, + "digest failure": { + expectations: digestFailure("foo"), + image: &image{name: "foo"}, + wantErr: true, + }, + } { + t.Run(name, func(t *testing.T) { + expect.Commands(t, tc.expectations...) + + // We'll call UIDGID twice to make sure the memoisation works. + test := func() (UIDGID, error) { + have, err := tc.image.UIDGID(ctx) + if tc.wantErr { + if err == nil { + t.Error("unexpected nil error") + } + } else if err != nil { + t.Errorf("unexpected error: %+v", err) + } else if diff := cmp.Diff(have, tc.want); diff != "" { + t.Errorf("unexpected uid/gid (-have +want):\n%s", diff) + } + + return have, err + } + firstUG, firstErr := test() + secondUG, secondErr := test() + + if diff := cmp.Diff(firstUG, secondUG); diff != "" { + t.Errorf("uid/gids do not match: (-first +second)\n%s", diff) + } + if firstErr != secondErr { + t.Errorf("errors do not match: first=%v second=%v", firstErr, secondErr) + } + }) + } +} + +func TestUIDGID(t *testing.T) { + have := UIDGID{UID: 1000, GID: 0}.String() + want := "1000:0" + if have != want { + t.Errorf("unexpected value: have=%q want=%q", have, want) + } +} + +// Set up some helper functions for expectations we'll be reusing. + +func digestFailure(name string) []*expect.Expectation { + return append( + ensureSuccess(name), + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + // Note the awkward escaping because these arguments are matched by + // glob. + "docker", "image", "inspect", "--format", `\{\{.Id}}`, "--", name, + ), + ) +} + +func digestSuccess(name, digest string) []*expect.Expectation { + return append( + ensureSuccess(name), + expect.NewGlob( + expect.Behaviour{Stdout: []byte(digest + "\n")}, + // Note the awkward escaping because these arguments are + // matched by glob. + "docker", "image", "inspect", "--format", `\{\{.Id}}`, "--", name, + ), + ) +} + +func ensureFailure(name string) []*expect.Expectation { + return []*expect.Expectation{ + expect.NewGlob( + // docker image inspect returns 1 for non-existent images. + expect.Behaviour{ExitCode: 1}, + "docker", "image", "inspect", "--format", "1", name, + ), + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + "docker", "image", "pull", name, + ), + } +} + +// ensureSuccess only provides the short circuit success path for Ensure() (that +// is, where no pull is required). +func ensureSuccess(name string) []*expect.Expectation { + return []*expect.Expectation{ + expect.NewGlob( + expect.Behaviour{}, + "docker", "image", "inspect", "--format", "1", name, + ), + } +} + +func uidGid(digest string, behaviour expect.Behaviour) *expect.Expectation { + return expect.NewGlob( + behaviour, + "docker", "run", "--rm", "--entrypoint", "/bin/sh", + digest, "-c", "id -u; id -g", + ) +} diff --git a/internal/campaigns/docker/main_test.go b/internal/campaigns/docker/main_test.go new file mode 100644 index 0000000000..6ce467da07 --- /dev/null +++ b/internal/campaigns/docker/main_test.go @@ -0,0 +1,13 @@ +package docker + +import ( + "os" + "testing" + + "github.com/sourcegraph/src-cli/internal/exec/expect" +) + +func TestMain(m *testing.M) { + code := expect.Handle(m) + os.Exit(code) +} From d81cded7e36cd339e35a33c4329fadc4214eda96 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 21 Jan 2021 17:51:13 -0800 Subject: [PATCH 14/18] Explain logic. --- internal/campaigns/volume_workspace.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/campaigns/volume_workspace.go b/internal/campaigns/volume_workspace.go index b34ed21927..a39db24116 100644 --- a/internal/campaigns/volume_workspace.go +++ b/internal/campaigns/volume_workspace.go @@ -92,6 +92,14 @@ func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, dummy := fmt.Sprintf(".campaign-workspace-placeholder-%s", hex.EncodeToString(randToken)) // So, let's use that to set up the volume. + // + // Theoretically, we could combine this `docker run` and the following one + // into one invocation. Doing so, however, is tricky: we'd have to su within + // the script being run, and Alpine requires a real user account and group; + // just having numeric IDs is insufficient. The logic to make this work is + // complicated enough that it feels brittle, and beyond what should be + // encoded in this function. Running `docker run` twice isn't ideal, but + // should be quick enough in general that it's not a huge concern. opts := append([]string{ "run", "--rm", From c1b7e32c5c34afce375172dd3da737f476a567c4 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 21 Jan 2021 17:53:05 -0800 Subject: [PATCH 15/18] Simplify per @mrnugget. --- internal/campaigns/docker/image.go | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/internal/campaigns/docker/image.go b/internal/campaigns/docker/image.go index 3cd3a343cc..09ddc86f9c 100644 --- a/internal/campaigns/docker/image.go +++ b/internal/campaigns/docker/image.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "fmt" - "strconv" "strings" "sync" @@ -135,24 +134,12 @@ func (image *image) UIDGID(ctx context.Context) (UIDGID, error) { // POSIX specifies the output of `id -u` as the effective UID, // terminated by a newline. `id -g` is the same, just for the GID. raw := strings.TrimSpace(stdout.String()) - lines := strings.Split(raw, "\n") - if len(lines) < 2 { - // There's an id command on the path, but it's not returning - // POSIX compliant output. - return UIDGID{}, errors.New("invalid id output") - } - - uid, err := strconv.Atoi(lines[0]) - if err != nil { - return UIDGID{}, errors.Wrap(err, "malformed uid") - } - - gid, err := strconv.Atoi(lines[1]) + var res UIDGID + _, err = fmt.Sscanf(raw, "%d\n%d", &res.UID, &res.GID) if err != nil { - return UIDGID{}, errors.Wrap(err, "malformed gid") + return res, errors.Wrapf(err, "malformed uid/gid: %q", raw) } - - return UIDGID{UID: uid, GID: gid}, nil + return res, nil }() }) From 35623804edb8e24b18dbe4cb29aed8d232bf0f34 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 21 Jan 2021 18:14:41 -0800 Subject: [PATCH 16/18] Fix expect error when the command is not on the PATH. --- internal/exec/expect/expect.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/internal/exec/expect/expect.go b/internal/exec/expect/expect.go index d2075afd8d..b4012af0bd 100644 --- a/internal/exec/expect/expect.go +++ b/internal/exec/expect/expect.go @@ -66,7 +66,19 @@ func Commands(t *testing.T, exp ...*Expectation) { // Create the command using the next level of middleware. (Which is // probably eventually os/exec.CommandContext().) - cmd := previous(ctx, name, arg...) + // + // The prepending of ./ to the command name looks completely insane, but + // there's a reason for it: if the name is a bare string like `docker`, + // then Go will attempt to resolve it using $PATH. If the command + // doesn't exist (because, say, we're running it in CI), then an error + // is embedded within the *Cmd that will be returned when it is run, + // even if we've subsequently rewritten the Path and Args fields to be + // valid. + // + // Prepending ./ means that Go doesn't need to look the command up in + // the $PATH and no error can be generated that way. Since we're going + // to overwrite the Path momentarily anyway, that's fine. + cmd := previous(ctx, "./"+name, arg...) if cmd == nil { t.Fatalf("unexpected nil *Cmd for %q with arguments %v", name, arg) } From 9f8b85345fe3167b9be1eb42722af32b6722bbe6 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 21 Jan 2021 18:19:26 -0800 Subject: [PATCH 17/18] Remove lint warnings. --- internal/campaigns/workspace_test.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/internal/campaigns/workspace_test.go b/internal/campaigns/workspace_test.go index 5d66243e41..0be5e87387 100644 --- a/internal/campaigns/workspace_test.go +++ b/internal/campaigns/workspace_test.go @@ -14,6 +14,10 @@ func TestBestWorkspaceCreator(t *testing.T) { ctx := context.Background() isOverridden := !(runtime.GOOS == "darwin" && runtime.GOARCH == "amd64") + uidGid := func(uid, gid int) docker.UIDGID { + return docker.UIDGID{UID: uid, GID: gid} + } + type imageBehaviour struct { image string behaviour expect.Behaviour @@ -32,21 +36,21 @@ func TestBestWorkspaceCreator(t *testing.T) { }, "root": { images: []docker.Image{ - &mockImage{uidGid: docker.UIDGID{0, 0}}, + &mockImage{uidGid: uidGid(0, 0)}, }, want: workspaceCreatorVolume, }, "same user": { images: []docker.Image{ - &mockImage{uidGid: docker.UIDGID{1000, 1000}}, - &mockImage{uidGid: docker.UIDGID{1000, 1000}}, + &mockImage{uidGid: uidGid(1000, 1000)}, + &mockImage{uidGid: uidGid(1000, 1000)}, }, want: workspaceCreatorVolume, }, "different user": { images: []docker.Image{ - &mockImage{uidGid: docker.UIDGID{1000, 1000}}, - &mockImage{uidGid: docker.UIDGID{0, 0}}, + &mockImage{uidGid: uidGid(1000, 1000)}, + &mockImage{uidGid: uidGid(0, 0)}, }, want: workspaceCreatorBind, }, From e512c4bc853fb03df2f08b264b7c0e9caeb13d38 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Fri, 22 Jan 2021 11:51:29 -0800 Subject: [PATCH 18/18] Define and use expect.Success. --- internal/campaigns/docker/image_test.go | 6 ++--- internal/campaigns/volume_workspace_test.go | 26 ++++++++++----------- internal/exec/expect/expect.go | 4 ++++ 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/internal/campaigns/docker/image_test.go b/internal/campaigns/docker/image_test.go index 6e525cf5eb..1ae31bc381 100644 --- a/internal/campaigns/docker/image_test.go +++ b/internal/campaigns/docker/image_test.go @@ -26,7 +26,7 @@ func TestImage_Digest(t *testing.T) { expectations: append( ensureSuccess("foo"), expect.NewGlob( - expect.Behaviour{}, + expect.Success, // Note the awkward escaping because these arguments are matched by // glob. "docker", "image", "inspect", "--format", `\{\{.Id}}`, "--", "foo", @@ -98,7 +98,7 @@ func TestImage_Ensure(t *testing.T) { "docker", "image", "inspect", "--format", "1", "foo", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "image", "pull", "foo", ), }, @@ -332,7 +332,7 @@ func ensureFailure(name string) []*expect.Expectation { func ensureSuccess(name string) []*expect.Expectation { return []*expect.Expectation{ expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "image", "inspect", "--format", "1", name, ), } diff --git a/internal/campaigns/volume_workspace_test.go b/internal/campaigns/volume_workspace_test.go index 7644efe908..ce448ab953 100644 --- a/internal/campaigns/volume_workspace_test.go +++ b/internal/campaigns/volume_workspace_test.go @@ -52,7 +52,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "volume", "create", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", @@ -60,7 +60,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "-c", "touch /work/*; chown -R 0:0 /work", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/tmp/zip,ro", @@ -70,7 +70,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "-c", "unzip /tmp/zip; rm /work/*", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "0:0", @@ -88,7 +88,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "volume", "create", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", @@ -96,7 +96,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "-c", "touch /work/*; chown -R 0:0 /work", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/tmp/zip,ro", @@ -106,7 +106,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "-c", "unzip /tmp/zip; rm /work/*", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "0:0", @@ -126,7 +126,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "volume", "create", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", @@ -134,7 +134,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "-c", "touch /work/*; chown -R 1:2 /work", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/tmp/zip,ro", @@ -144,7 +144,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "-c", "unzip /tmp/zip; rm /work/*", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "1:2", @@ -196,7 +196,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "volume", "create", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", @@ -204,7 +204,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "-c", "touch /work/*; chown -R 0:0 /work", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--mount", "type=bind,source=*,target=/tmp/zip,ro", @@ -235,7 +235,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "volume", "create", ), expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", @@ -285,7 +285,7 @@ func TestVolumeWorkspace_Close(t *testing.T) { expect.Commands( t, expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "volume", "rm", volumeID, ), ) diff --git a/internal/exec/expect/expect.go b/internal/exec/expect/expect.go index b4012af0bd..e8e56fabdb 100644 --- a/internal/exec/expect/expect.go +++ b/internal/exec/expect/expect.go @@ -47,6 +47,10 @@ type Behaviour struct { ExitCode int } +// Success defines a command behaviour that returns a 0 exit code and no other +// output. +var Success = Behaviour{} + // Commands defines a set of expected commands for the given test. Commands may // be called from any number of nested subtests, but must only be called once // from a single test function, as it uses (*testing.T).Cleanup to manage