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/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 0aefd0cb52..5d94e0f9d6 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..798b2cae56 --- /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/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 new file mode 100644 index 0000000000..09ddc86f9c --- /dev/null +++ b/internal/campaigns/docker/image.go @@ -0,0 +1,147 @@ +package docker + +import ( + "bytes" + "context" + "fmt" + "strings" + "sync" + + "github.com/pkg/errors" + + "github.com/sourcegraph/src-cli/internal/exec" +) + +// 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 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 + // hard to prevent performing the same operations on the same image over and + // 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 +} + +// 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) { + image.digestOnce.Do(func() { + image.digest, image.digestErr = 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.name) + } + return id, nil + }() + }) + + return image.digest, image.digestErr +} + +// 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) { + image.uidGidOnce.Do(func() { + image.uidGid, image.uidGidErr = 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()) + var res UIDGID + _, err = fmt.Sscanf(raw, "%d\n%d", &res.UID, &res.GID) + if err != nil { + return res, errors.Wrapf(err, "malformed uid/gid: %q", raw) + } + return res, nil + }() + }) + + return image.uidGid, image.uidGidErr +} diff --git a/internal/campaigns/docker/image_test.go b/internal/campaigns/docker/image_test.go new file mode 100644 index 0000000000..1ae31bc381 --- /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.Success, + // 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.Success, + "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.Success, + "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) +} 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) } 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 b049e9ef98..e4dde68953 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,6 +216,7 @@ 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 + if svc.workspace == "volume" { workspace = workspaceCreatorVolume } else if svc.workspace == "bind" { @@ -227,29 +231,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. @@ -257,30 +238,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 0989a6aa84..a39db24116 100644 --- a/internal/campaigns/volume_workspace.go +++ b/internal/campaigns/volume_workspace.go @@ -3,29 +3,44 @@ package campaigns import ( "bytes" "context" + "crypto/rand" + "encoding/hex" + "fmt" "io/ioutil" "os" "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" ) -type dockerVolumeWorkspaceCreator struct { - tempDir string -} +type dockerVolumeWorkspaceCreator struct{ tempDir string } 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} + // 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") } @@ -60,22 +75,62 @@ git commit --quiet --all --allow-empty -m src-action-exec return nil } -func (*dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w *dockerVolumeWorkspace, 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") + + // 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)) + // 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", "--init", "--workdir", "/work", + }, w.dockerRunOptsWithUser(docker.Root, "/work")...) + opts = append( + opts, + dockerVolumeWorkspaceImage, + "sh", "-c", + 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", + "--workdir", "/work", "--mount", "type=bind,source=" + zip + ",target=/tmp/zip,ro", - }, common...) - opts = append(opts, dockerVolumeWorkspaceImage, "unzip", "/tmp/zip") + }, w.dockerRunOptsWithUser(w.uidGid, "/work")...) + 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)) @@ -91,6 +146,7 @@ func (*dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w type dockerVolumeWorkspace struct { tempDir string volume string + uidGid docker.UIDGID } var _ Workspace = &dockerVolumeWorkspace{} @@ -101,9 +157,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 } @@ -201,3 +255,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..ce448ab953 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,241 @@ 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.Success, + "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.Success, + "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.Success, + "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.Success, + "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.Success, + "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.Success, + "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.Success, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--user", "0:0", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "touch /work/*; chown -R 1:2 /work", + ), + expect.NewGlob( + expect.Success, + "docker", "run", "--rm", "--init", + "--workdir", "/work", + "--mount", "type=bind,source=*,target=/tmp/zip,ro", + "--user", "1:2", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "-c", "unzip /tmp/zip; rm /work/*", + ), + expect.NewGlob( + expect.Success, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--user", "1:2", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerVolumeWorkspaceImage, + "sh", "/run.sh", + ), + }, + steps: []Step{ + {image: &mockImage{uidGid: docker.UIDGID{UID: 1, GID: 2}}}, + }, + }, + "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, + }, + "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( + expect.Behaviour{Stdout: []byte(volumeID)}, + "docker", "volume", "create", + ), + expect.NewGlob( + expect.Success, + "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.Success, + "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.Success, + "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) { @@ -146,7 +285,7 @@ func TestVolumeWorkspace_Close(t *testing.T) { expect.Commands( t, expect.NewGlob( - expect.Behaviour{}, + expect.Success, "docker", "volume", "rm", volumeID, ), ) @@ -173,9 +312,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 +374,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 +406,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 +452,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 +479,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 +508,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 +539,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.go b/internal/campaigns/workspace.go index dabfc9c3af..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 @@ -90,45 +86,20 @@ func detectBestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCrea // 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 } - // 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 - } - - uids[uid] = struct{}{} - if len(uids) > 1 || uid != 0 { + if uid == nil { + uid = &ug.UID + } else if *uid != ug.UID { return workspaceCreatorBind } } diff --git a/internal/campaigns/workspace_test.go b/internal/campaigns/workspace_test.go index 450612f5d1..0be5e87387 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" ) @@ -12,88 +14,62 @@ 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 } 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: uidGid(0, 0)}, }, want: workspaceCreatorVolume, }, "same user": { - behaviours: []imageBehaviour{ - {image: "foo", behaviour: expect.Behaviour{Stdout: []byte("1000\n")}}, + images: []docker.Image{ + &mockImage{uidGid: uidGid(1000, 1000)}, + &mockImage{uidGid: uidGid(1000, 1000)}, }, - want: workspaceCreatorBind, + want: workspaceCreatorVolume, }, "different user": { - behaviours: []imageBehaviour{ - {image: "foo", behaviour: expect.Behaviour{Stdout: []byte("0\n")}}, - {image: "bar", behaviour: expect.Behaviour{Stdout: []byte("1000\n")}}, - }, - want: workspaceCreatorBind, - }, - "invalid id output: string": { - behaviours: []imageBehaviour{ - {image: "foo", behaviour: expect.Behaviour{Stdout: []byte("xxx\n")}}, + images: []docker.Image{ + &mockImage{uidGid: uidGid(1000, 1000)}, + &mockImage{uidGid: uidGid(0, 0)}, }, 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(). diff --git a/internal/exec/expect/expect.go b/internal/exec/expect/expect.go index d2075afd8d..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 @@ -66,7 +70,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) }