Allow gateway exec-ing into a failed solve with an exec op

Signed-off-by: Edgar Lee <edgarl@netflix.com>

client/build_test.go

@@ -41,6 +41,8 @@ func TestClientGatewayIntegration(t *testing.T) {
 		testClientGatewayContainerMounts,
 		testClientGatewayContainerPID1Tty,
 		testClientGatewayContainerExecTty,
+		testClientGatewayExecOpError,
+		testClientGatewayFileOpError,
 	}, integration.WithMirroredImages(integration.OfficialImages("busybox:latest")))
 }
 
@@ -950,6 +952,157 @@ func testClientGatewayContainerExecTty(t *testing.T, sb integration.Sandbox) {
 	checkAllReleasable(t, c, sb, true)
 }
 
+// testClientGatewayExecOpError is testing gateway exec back into the modified
+// mounts of a failed execop during a solve.
+func testClientGatewayExecOpError(t *testing.T, sb integration.Sandbox) {
+	requiresLinux(t)
+
+	ctx := context.TODO()
+
+	c, err := New(ctx, sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	id := identity.NewID()
+	st := llb.Image("alpine").Run(
+		llb.Dir("/src"),
+		llb.AddMount("/src", llb.Scratch()),
+		llb.Shlexf("sh -c \"echo %s > output && fail\"", id),
+	).Root()
+
+	def, err := st.Marshal(ctx)
+	require.NoError(t, err)
+
+	b := func(ctx context.Context, c client.Client) (*client.Result, error) {
+		_, solveErr := c.Solve(ctx, client.SolveRequest{
+			Evaluate:   true,
+			Definition: def.ToPB(),
+		})
+		require.Error(t, solveErr)
+
+		var se *errdefs.SolveError
+		require.True(t, errors.As(solveErr, &se))
+
+		solveExec, ok := se.Solve.Op.(*errdefs.Solve_Exec)
+		require.True(t, ok)
+
+		exec := solveExec.Exec
+
+		var mounts []client.Mount
+		for _, mnt := range exec.Mounts {
+			mounts = append(mounts, client.Mount{
+				Selector:  mnt.Selector,
+				Dest:      mnt.Dest,
+				ResultID:  se.Solve.OutputIDs[mnt.Output],
+				Readonly:  mnt.Readonly,
+				MountType: mnt.MountType,
+				CacheOpt:  mnt.CacheOpt,
+				SecretOpt: mnt.SecretOpt,
+				SSHOpt:    mnt.SSHOpt,
+			})
+		}
+
+		ctr, err := c.NewContainer(ctx, client.NewContainerRequest{
+			Mounts:  mounts,
+			NetMode: exec.Network,
+		})
+		require.NoError(t, err)
+		defer ctr.Release(ctx)
+
+		meta := exec.Meta
+		output := bytes.NewBuffer(nil)
+
+		proc, err := ctr.Start(ctx, client.StartRequest{
+			Args:         []string{"cat", "output"},
+			Env:          meta.Env,
+			User:         meta.User,
+			Cwd:          meta.Cwd,
+			Stdout:       &nopCloser{output},
+			SecurityMode: exec.Security,
+		})
+		require.NoError(t, err)
+
+		err = proc.Wait()
+		require.NoError(t, err)
+		require.Equal(t, id, strings.TrimSpace(output.String()))
+
+		return nil, solveErr
+	}
+
+	_, err = c.Build(ctx, SolveOpt{}, "buildkit_test", b, nil)
+	require.Error(t, err)
+
+	checkAllReleasable(t, c, sb, true)
+}
+
+// testClientGatewayFileOpError is testing gateway exec back into the modified
+// mounts of a failed fileop during a solve.
+func testClientGatewayFileOpError(t *testing.T, sb integration.Sandbox) {
+	requiresLinux(t)
+
+	ctx := context.TODO()
+
+	c, err := New(ctx, sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	st := llb.Image("alpine").File(
+		llb.Mkfile("/notfound/foo", 0600, []byte("data")),
+	)
+
+	def, err := st.Marshal(ctx)
+	require.NoError(t, err)
+
+	b := func(ctx context.Context, c client.Client) (*client.Result, error) {
+		_, solveErr := c.Solve(ctx, client.SolveRequest{
+			Evaluate:   true,
+			Definition: def.ToPB(),
+		})
+		require.Error(t, solveErr)
+
+		var se *errdefs.SolveError
+		require.True(t, errors.As(solveErr, &se))
+
+		solveFile, ok := se.Solve.Op.(*errdefs.Solve_File)
+		require.True(t, ok)
+
+		file := solveFile.File
+		require.Len(t, file.Actions, 1)
+		cp := file.Actions[0]
+
+		ctr, err := c.NewContainer(ctx, client.NewContainerRequest{
+			Mounts: []client.Mount{{
+				Dest:      "/",
+				MountType: pb.MountType_BIND,
+				ResultID:  se.Solve.InputIDs[cp.Input],
+			}},
+		})
+		require.NoError(t, err)
+		defer ctr.Release(ctx)
+
+		// Exec into the filesystem and sanity check that we get an error when
+		// trying to find the directory.
+
+		output := bytes.NewBuffer(nil)
+		proc, err := ctr.Start(ctx, client.StartRequest{
+			Args:   []string{"ls", "/notfound"},
+			Cwd:    "/",
+			Stdout: &nopCloser{output},
+		})
+		require.NoError(t, err)
+
+		err = proc.Wait()
+		require.Error(t, err)
+
+		return nil, solveErr
+	}
+
+	_, err = c.Build(ctx, SolveOpt{}, "buildkit_test", b, nil)
+	require.Error(t, err)
+
+	checkAllReleasable(t, c, sb, true)
+}
+
 type nopCloser struct {
 	io.Writer
 }

frontend/gateway/client/client.go

@@ -32,6 +32,7 @@ type NewContainerRequest struct {
 type Mount struct {
 	Selector  string
 	Dest      string
+	ResultID  string
 	Ref       Reference
 	Readonly  bool
 	MountType pb.MountType
@@ -102,6 +103,7 @@ type StatRequest struct {
 
 // SolveRequest is same as frontend.SolveRequest but avoiding dependency
 type SolveRequest struct {
+	Evaluate       bool
 	Definition     *pb.Definition
 	Frontend       string
 	FrontendOpt    map[string]string

frontend/gateway/container.go

@@ -11,7 +11,6 @@ import (
 	"github.com/moby/buildkit/executor"
 	"github.com/moby/buildkit/frontend/gateway/client"
 	"github.com/moby/buildkit/session"
-	"github.com/moby/buildkit/solver"
 	"github.com/moby/buildkit/solver/llbsolver/mounts"
 	opspb "github.com/moby/buildkit/solver/pb"
 	"github.com/moby/buildkit/util/stack"
@@ -36,7 +35,7 @@ type Mount struct {
 	Selector  string
 	Readonly  bool
 	MountType opspb.MountType
-	RefProxy  solver.ResultProxy
+	WorkerRef *worker.WorkerRef
 	CacheOpt  *opspb.CacheOpt
 	SecretOpt *opspb.SecretOpt
 	SSHOpt    *opspb.SSHOpt
@@ -81,22 +80,14 @@ func NewContainer(ctx context.Context, e executor.Executor, sm *session.Manager,
 	mnts := req.Mounts
 
 	for i, m := range mnts {
-		if m.Dest == opspb.RootMount && m.RefProxy != nil {
-			res, err := m.RefProxy.Result(ctx)
-			if err != nil {
-				return nil, stack.Enable(err)
-			}
-			workerRef, ok := res.Sys().(*worker.WorkerRef)
-			if !ok {
-				return nil, errors.Errorf("invalid reference for exec %T", res.Sys())
-			}
-
+		if m.Dest == opspb.RootMount && m.WorkerRef != nil {
 			name := fmt.Sprintf("container %s", req.ContainerID)
-			mm = mounts.NewMountManager(name, workerRef.Worker.CacheManager(), sm, workerRef.Worker.MetadataStore())
+			mm = mounts.NewMountManager(name, m.WorkerRef.Worker.CacheManager(), sm, m.WorkerRef.Worker.MetadataStore())
 
-			ctr.rootFS = workerRef.ImmutableRef
+			ctr.rootFS = m.WorkerRef.ImmutableRef
 			if !m.Readonly {
-				ctr.rootFS, err = makeMutable(workerRef.Worker, workerRef.ImmutableRef)
+				var err error
+				ctr.rootFS, err = makeMutable(m.WorkerRef.Worker, m.WorkerRef.ImmutableRef)
 				if err != nil {
 					return nil, stack.Enable(err)
 				}
@@ -115,20 +106,13 @@ func NewContainer(ctx context.Context, e executor.Executor, sm *session.Manager,
 	for _, m := range mnts {
 		var ref cache.ImmutableRef
 		var mountable cache.Mountable
-		if m.RefProxy != nil {
-			res, err := m.RefProxy.Result(ctx)
-			if err != nil {
-				return nil, stack.Enable(err)
-			}
-			workerRef, ok := res.Sys().(*worker.WorkerRef)
-			if !ok {
-				return nil, errors.Errorf("invalid reference for exec %T", res.Sys())
-			}
-			ref = workerRef.ImmutableRef
+		if m.WorkerRef != nil {
+			ref = m.WorkerRef.ImmutableRef
 			mountable = ref
 
 			if !m.Readonly {
-				mountable, err = makeMutable(workerRef.Worker, ref)
+				var err error
+				mountable, err = makeMutable(m.WorkerRef.Worker, ref)
 				if err != nil {
 					return nil, stack.Enable(err)
 				}

frontend/gateway/forwarder/forward.go

@@ -48,6 +48,7 @@ type bridgeClient struct {
 
 func (c *bridgeClient) Solve(ctx context.Context, req client.SolveRequest) (*client.Result, error) {
 	res, err := c.FrontendLLBBridge.Solve(ctx, frontend.SolveRequest{
+		Evaluate:       req.Evaluate,
 		Definition:     req.Definition,
 		Frontend:       req.Frontend,
 		FrontendOpt:    req.FrontendOpt,
@@ -161,20 +162,31 @@ func (c *bridgeClient) NewContainer(ctx context.Context, req client.NewContainer
 	}
 
 	for _, m := range req.Mounts {
-		var refProxy solver.ResultProxy
+		var workerRef *worker.WorkerRef
+		// TODO: What about ResultID?
+		// The *bridgeClient doesn't have a workerRefByID map.
 		if m.Ref != nil {
-			var ok bool
-			refProxy, ok = m.Ref.(*ref)
+			refProxy, ok := m.Ref.(*ref)
 			if !ok {
 				return nil, errors.Errorf("unexpected Ref type: %T", m.Ref)
 			}
+
+			res, err := refProxy.Result(ctx)
+			if err != nil {
+				return nil, err
+			}
+
+			workerRef, ok = res.Sys().(*worker.WorkerRef)
+			if !ok {
+				return nil, errors.Errorf("invalid ref: %T", res.Sys())
+			}
 		}
 		ctrReq.Mounts = append(ctrReq.Mounts, gateway.Mount{
 			Dest:      m.Dest,
 			Selector:  m.Selector,
 			Readonly:  m.Readonly,
 			MountType: m.MountType,
-			RefProxy:  refProxy,
+			WorkerRef: workerRef,
 			CacheOpt:  m.CacheOpt,
 			SecretOpt: m.SecretOpt,
 			SSHOpt:    m.SSHOpt,