fix: ensure nested frontend builds get secret translation (#7595)

* fix: ensure nested frontend builds get secret translation When doing a nested frontend build - i.e. when dockerfile.v0 calls gateway.v0 (triggered by a syntax directive), we need to ensure that the second frontend gets the same secret translation as each prior layer. To do this, we heavily refactor the filtering gateway and essentially have it pass itself into the frontend Solve - this propagates the filtering all the way down, so secrets can be correctly accessed. Signed-off-by: Justin Chadwell <[email protected]> * tests: add test for remote frontend Signed-off-by: Justin Chadwell <[email protected]> * chore: add changelog Signed-off-by: Justin Chadwell <[email protected]> --------- Signed-off-by: Justin Chadwell <[email protected]>
dagger · Jun 11, 2024 · 058730d · 058730d
1 parent 4a44a52
commit 058730d
Show file tree

Hide file tree

Showing 3 changed files with 120 additions and 91 deletions.
diff --git a/.changes/unreleased/Fixed-20240610-162518.yaml b/.changes/unreleased/Fixed-20240610-162518.yaml
@@ -0,0 +1,6 @@
+kind: Fixed
+body: 'core: handle secrets in dockerfile builds with syntax directives'
+time: 2024-06-10T16:25:18.328274656+01:00
+custom:
+  Author: jedevc
+  PR: "7595"
diff --git a/core/integration/container_test.go b/core/integration/container_test.go
@@ -240,23 +240,37 @@ CMD echo "stage2"
 	t.Run("with build secrets", func(t *testing.T) {
 		sec := c.SetSecret("my-secret", "barbar")
 
-		src := contextDir.
-			WithNewFile("Dockerfile",
-				`FROM golang:1.18.2-alpine
+		dockerfile := `FROM golang:1.18.2-alpine
 WORKDIR /src
 RUN --mount=type=secret,id=my-secret,required=true test "$(cat /run/secrets/my-secret)" = "barbar"
 RUN --mount=type=secret,id=my-secret,required=true cp /run/secrets/my-secret /secret
-CMD cat /secret
-`)
+CMD cat /secret && (cat /secret | tr "[a-z]" "[A-Z]")
+`
 
-		stdout, err := c.Container().Build(src, dagger.ContainerBuildOpts{
-			Secrets: []*dagger.Secret{sec},
-		}).Stdout(ctx)
-		require.NoError(t, err)
-		require.Contains(t, stdout, "***")
+		t.Run("builtin frontend", func(t *testing.T) {
+			src := contextDir.WithNewFile("Dockerfile", dockerfile)
+
+			stdout, err := c.Container().Build(src, dagger.ContainerBuildOpts{
+				Secrets: []*dagger.Secret{sec},
+			}).Stdout(ctx)
+			require.NoError(t, err)
+			require.Contains(t, stdout, "***")
+			require.Contains(t, stdout, "BARBAR")
+		})
+
+		t.Run("remote frontend", func(t *testing.T) {
+			src := contextDir.WithNewFile("Dockerfile", "#syntax=docker/dockerfile:1\n"+dockerfile)
+
+			stdout, err := c.Container().Build(src, dagger.ContainerBuildOpts{
+				Secrets: []*dagger.Secret{sec},
+			}).Stdout(ctx)
+			require.NoError(t, err)
+			require.Contains(t, stdout, "***")
+			require.Contains(t, stdout, "BARBAR")
+		})
 	})
 
-	t.Run("with input build secrets", func(t *testing.T) {
+	t.Run("prevent duplicate secret transform", func(t *testing.T) {
 		sec := c.SetSecret("my-secret", "barbar")
 
 		// src is a directory that has a secret dependency in it's build graph

diff --git a/engine/buildkit/client.go b/engine/buildkit/client.go
@@ -311,52 +311,14 @@ func (c *Client) Solve(ctx context.Context, req bkgw.SolveRequest) (_ *Result, r
 	// include upstream cache imports, if any
 	req.CacheImports = c.UpstreamCacheImports
 
-	// include exec metadata that isn't included in the cache key
-	var llbRes *bkfrontend.Result
-	switch {
-	case req.Definition != nil && req.Definition.Def != nil:
-		llbRes, err = c.llbBridge.Solve(ctx, req, c.ID())
-		if err != nil {
-			return nil, wrapError(ctx, err, c.ID())
-		}
-	case req.Frontend != "":
-		// HACK: don't force evaluation like this, we can write custom frontend
-		// wrappers (for dockerfile.v0 and gateway.v0) that read from ctx to
-		// replace the llbBridge it knows about.
-		// This current implementation may be limited when it comes to
-		// implement provenance/etc.
-
-		f, ok := c.Frontends[req.Frontend]
-		if !ok {
-			return nil, fmt.Errorf("invalid frontend: %s", req.Frontend)
-		}
-
-		gw := newFilterGateway(c.llbBridge, req)
-		gw.secretTranslator = ctx.Value("secret-translator").(func(string) (string, error))
-
-		llbRes, err = f.Solve(
-			ctx,
-			gw,
-			c.worker, // also implements Executor
-			req.FrontendOpt,
-			req.FrontendInputs,
-			c.ID(),
-			c.SessionManager,
-		)
-		if err != nil {
-			return nil, err
-		}
-		if req.Evaluate {
-			err = llbRes.EachRef(func(ref bksolver.ResultProxy) error {
-				_, err := ref.Result(ctx)
-				return err
-			})
-			if err != nil {
-				return nil, err
-			}
-		}
-	default:
-		llbRes = &bkfrontend.Result{}
+	// handle secret translation
+	gw := newFilterGateway(c, req)
+	if v := ctx.Value("secret-translator"); v != nil {
+		gw.secretTranslator = v.(func(string) (string, error))
+	}
+	llbRes, err := gw.Solve(ctx, req, c.ID())
+	if err != nil {
+		return nil, wrapError(ctx, err, c.ID())
 	}
 
 	res, err := solverresult.ConvertResult(llbRes, func(rp bksolver.ResultProxy) (*ref, error) {
@@ -791,12 +753,15 @@ type filteringGateway struct {
 	// in the secret store.
 	secretTranslator func(string) (string, error)
 
+	// client is the top-most client that is owning the filtering process
+	client *Client
+
 	// skipInputs specifies op digests that were part of the request inputs and
 	// so shouldn't be processed.
 	skipInputs map[digest.Digest]struct{}
 }
 
-func newFilterGateway(bridge bkfrontend.FrontendLLBBridge, req bkgw.SolveRequest) *filteringGateway {
+func newFilterGateway(client *Client, req bkgw.SolveRequest) *filteringGateway {
 	inputs := map[digest.Digest]struct{}{}
 	for _, inp := range req.FrontendInputs {
 		for _, def := range inp.Def {
@@ -805,54 +770,98 @@ func newFilterGateway(bridge bkfrontend.FrontendLLBBridge, req bkgw.SolveRequest
 	}
 
 	return &filteringGateway{
-		FrontendLLBBridge: bridge,
-		skipInputs:        inputs,
+		client:            client,
+		FrontendLLBBridge: client.llbBridge,
+
+		skipInputs: inputs,
 	}
 }
 
 func (gw *filteringGateway) Solve(ctx context.Context, req bkfrontend.SolveRequest, sid string) (*bkfrontend.Result, error) {
-	if req.Definition != nil && req.Definition.Def != nil {
-		dag, err := DefToDAG(req.Definition)
-		if err != nil {
-			return nil, err
-		}
-		if err := dag.Walk(func(dag *OpDAG) error {
-			if _, ok := gw.skipInputs[*dag.OpDigest]; ok {
-				return SkipInputs
+	switch {
+	case req.Definition != nil && req.Definition.Def != nil:
+		if gw.secretTranslator != nil {
+			dag, err := DefToDAG(req.Definition)
+			if err != nil {
+				return nil, err
 			}
 
-			execOp, ok := dag.AsExec()
-			if !ok {
-				return nil
-			}
+			if err := dag.Walk(func(dag *OpDAG) error {
+				if _, ok := gw.skipInputs[*dag.OpDigest]; ok {
+					return SkipInputs
+				}
 
-			for _, secret := range execOp.ExecOp.GetSecretenv() {
-				secret.ID, err = gw.secretTranslator(secret.ID)
-				if err != nil {
-					return err
+				execOp, ok := dag.AsExec()
+				if !ok {
+					return nil
 				}
-			}
-			for _, mount := range execOp.ExecOp.GetMounts() {
-				if mount.MountType != bksolverpb.MountType_SECRET {
-					continue
+
+				for _, secret := range execOp.ExecOp.GetSecretenv() {
+					secret.ID, err = gw.secretTranslator(secret.ID)
+					if err != nil {
+						return err
+					}
 				}
-				secret := mount.SecretOpt
-				secret.ID, err = gw.secretTranslator(secret.ID)
-				if err != nil {
-					return err
+				for _, mount := range execOp.ExecOp.GetMounts() {
+					if mount.MountType != bksolverpb.MountType_SECRET {
+						continue
+					}
+					secret := mount.SecretOpt
+					secret.ID, err = gw.secretTranslator(secret.ID)
+					if err != nil {
+						return err
+					}
 				}
+				return nil
+			}); err != nil {
+				return nil, err
 			}
-			return nil
-		}); err != nil {
-			return nil, err
+
+			newDef, err := dag.Marshal()
+			if err != nil {
+				return nil, err
+			}
+			req.Definition = newDef
+		}
+
+		return gw.FrontendLLBBridge.Solve(ctx, req, sid)
+
+	case req.Frontend != "":
+		// HACK: don't force evaluation like this, we can write custom frontend
+		// wrappers (for dockerfile.v0 and gateway.v0) that read from ctx to
+		// replace the llbBridge it knows about.
+		// This current implementation may be limited when it comes to
+		// implement provenance/etc.
+
+		f, ok := gw.client.Frontends[req.Frontend]
+		if !ok {
+			return nil, fmt.Errorf("invalid frontend: %s", req.Frontend)
 		}
 
-		newDef, err := dag.Marshal()
+		llbRes, err := f.Solve(
+			ctx,
+			gw,
+			gw.client.worker, // also implements Executor
+			req.FrontendOpt,
+			req.FrontendInputs,
+			sid,
+			gw.client.SessionManager,
+		)
 		if err != nil {
 			return nil, err
 		}
-		req.Definition = newDef
-	}
+		if req.Evaluate {
+			err = llbRes.EachRef(func(ref bksolver.ResultProxy) error {
+				_, err := ref.Result(ctx)
+				return err
+			})
+			if err != nil {
+				return nil, err
+			}
+		}
+		return llbRes, nil
 
-	return gw.FrontendLLBBridge.Solve(ctx, req, sid)
+	default:
+		return &bkfrontend.Result{}, nil
+	}
 }