From 0fc5f168f4e99601d105c11b77583ed637dad93b Mon Sep 17 00:00:00 2001 From: a-palchikov Date: Mon, 19 Aug 2024 15:10:51 +0200 Subject: [PATCH] Address review comments: A secret is defined with a tuple (id, target) which can be aliased as (source, destination) using the following format: --mount=type=secret,id=FOO,target=/path/to/secret Existing secret rules: * if source (id) is empty, it is assumed to equal to path.Base(target) * if target (destination) is empty, it is assumed to equal to "/run/secrets/" + path.Base(id) Add an additional attribute to the mount command to specify that the secret should be mounted as environment variable: --mount=type=secret,id=FOO,env=FOO with the following semantics: * it should allow the old behavior of mounting the secret as path (using the target/destination attribute) * it should be optional (e.g. with no 'env' specified, the secret should be mounted as a file following the existing rules) * if specified, the file mount should be optional (uses empty target/destination as indicator) * if should be possible to mount a secret as both - an environment variable and as a file Signed-off-by: a-palchikov --- client/llb/exec.go | 53 +++++++++++++------ .../dockerfile2llb/convert_secrets.go | 22 ++++---- .../dockerfile/dockerfile_secrets_test.go | 8 +-- .../instructions/commands_runmount.go | 4 -- 4 files changed, 54 insertions(+), 33 deletions(-) diff --git a/client/llb/exec.go b/client/llb/exec.go index 37a1a3b6df41d..6850f21a7459a 100644 --- a/client/llb/exec.go +++ b/client/llb/exec.go @@ -6,6 +6,7 @@ import ( "fmt" "net" "sort" + "strings" "github.com/moby/buildkit/solver/pb" "github.com/moby/buildkit/util/system" @@ -395,19 +396,21 @@ func (e *ExecOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, [] Optional: s.Optional, }) } - pm := &pb.Mount{ - Input: pb.Empty, - Dest: s.Target, - MountType: pb.MountType_SECRET, - SecretOpt: &pb.SecretOpt{ - ID: s.ID, - Uid: uint32(s.UID), - Gid: uint32(s.GID), - Optional: s.Optional, - Mode: uint32(s.Mode), - }, + if s.Target != nil { + pm := &pb.Mount{ + Input: pb.Empty, + Dest: *s.Target, + MountType: pb.MountType_SECRET, + SecretOpt: &pb.SecretOpt{ + ID: s.ID, + Uid: uint32(s.UID), + Gid: uint32(s.GID), + Optional: s.Optional, + Mode: uint32(s.Mode), + }, + } + peo.Mounts = append(peo.Mounts, pm) } - peo.Mounts = append(peo.Mounts, pm) } for _, s := range e.ssh { @@ -679,7 +682,19 @@ type SSHInfo struct { // AddSecret is a RunOption that adds a secret to the exec. func AddSecret(dest string, opts ...SecretOption) RunOption { return runOptionFunc(func(ei *ExecInfo) { - s := &SecretInfo{ID: dest, Target: dest, Mode: 0400} + s := &SecretInfo{ID: dest, Target: &dest, Mode: 0400} + for _, opt := range opts { + opt.SetSecretOption(s) + } + ei.Secrets = append(ei.Secrets, *s) + }) +} + +// AddSecretWithDest is a RunOption that adds a secret to the exec +// with an optional destination. +func AddSecretWithDest(src string, dest *string, opts ...SecretOption) RunOption { + return runOptionFunc(func(ei *ExecInfo) { + s := &SecretInfo{ID: src, Target: dest, Mode: 0400} for _, opt := range opts { opt.SetSecretOption(s) } @@ -698,8 +713,9 @@ func (fn secretOptionFunc) SetSecretOption(si *SecretInfo) { } type SecretInfo struct { - ID string - Target string + ID string + // Target optionally specifies the target for the secret mount + Target *string // Env optionally names the environment variable for the secret Env *string Mode int @@ -725,7 +741,12 @@ func SecretAsEnv(v bool) SecretOption { si.Env = nil return } - si.Env = &si.Target + if si.Target == nil { + return + } + target := strings.Clone(*si.Target) + si.Env = &target + si.Target = nil }) } diff --git a/frontend/dockerfile/dockerfile2llb/convert_secrets.go b/frontend/dockerfile/dockerfile2llb/convert_secrets.go index c1cffa5aa1531..c68373b85f944 100644 --- a/frontend/dockerfile/dockerfile2llb/convert_secrets.go +++ b/frontend/dockerfile/dockerfile2llb/convert_secrets.go @@ -22,9 +22,17 @@ func dispatchSecret(d *dispatchState, m *instructions.Mount, loc []parser.Range) id = path.Base(m.Target) } - target := m.Target - if target == "" { - target = "/run/secrets/" + path.Base(id) + var target *string + if m.Target != "" { + target = &m.Target + } + + if m.Env == nil { + dest := m.Target + if dest == "" { + dest = "/run/secrets/" + path.Base(id) + } + target = &dest } if _, ok := d.outline.secrets[id]; !ok { @@ -40,11 +48,7 @@ func dispatchSecret(d *dispatchState, m *instructions.Mount, loc []parser.Range) opts = append(opts, llb.SecretOptional) } if m.Env != nil { - env := *m.Env - if env == "" { - env = path.Base(id) - } - opts = append(opts, llb.SecretAsEnvName(env)) + opts = append(opts, llb.SecretAsEnvName(*m.Env)) } if m.UID != nil || m.GID != nil || m.Mode != nil { @@ -63,5 +67,5 @@ func dispatchSecret(d *dispatchState, m *instructions.Mount, loc []parser.Range) opts = append(opts, llb.SecretFileOpt(uid, gid, mode)) } - return llb.AddSecret(target, opts...), nil + return llb.AddSecretWithDest(id, target, opts...), nil } diff --git a/frontend/dockerfile/dockerfile_secrets_test.go b/frontend/dockerfile/dockerfile_secrets_test.go index 5da0640efbfdd..3bc3e695ea02c 100644 --- a/frontend/dockerfile/dockerfile_secrets_test.go +++ b/frontend/dockerfile/dockerfile_secrets_test.go @@ -17,7 +17,7 @@ var secretsTests = integration.TestFuncs( testSecretFileParams, testSecretRequiredWithoutValue, testSecretAsEnviron, - testSecretAsEnvironWithOverride, + testSecretAsEnvironWithFileMount, ) func init() { @@ -89,7 +89,7 @@ func testSecretAsEnviron(t *testing.T, sb integration.Sandbox) { dockerfile := []byte(` FROM busybox -RUN --mount=type=secret,id=mysecret,env [ "$mysecret" == "pw" ] && [ -f /run/secrets/mysecret ] || false +RUN --mount=type=secret,id=mysecret,env=SECRET_ENV [ "$SECRET_ENV" == "pw" ] && [ ! -f /run/secrets/mysecret ] || false `) dir := integration.Tmpdir( @@ -113,13 +113,13 @@ RUN --mount=type=secret,id=mysecret,env [ "$mysecret" == "pw" ] && [ -f /run/sec require.NoError(t, err) } -func testSecretAsEnvironWithOverride(t *testing.T, sb integration.Sandbox) { +func testSecretAsEnvironWithFileMount(t *testing.T, sb integration.Sandbox) { integration.SkipOnPlatform(t, "windows") f := getFrontend(t, sb) dockerfile := []byte(` FROM busybox -RUN --mount=type=secret,id=mysecret,env=MY_SECRET [ "$MY_SECRET" == "pw" ] || false +RUN --mount=type=secret,id=mysecret,target=/run/secrets/secret,env=SECRET_ENV [ "$SECRET_ENV" == "pw" ] && [ -f /run/secrets/secret ] || false `) dir := integration.Tmpdir( diff --git a/frontend/dockerfile/instructions/commands_runmount.go b/frontend/dockerfile/instructions/commands_runmount.go index 7f615ffc5651f..335b524e98d88 100644 --- a/frontend/dockerfile/instructions/commands_runmount.go +++ b/frontend/dockerfile/instructions/commands_runmount.go @@ -139,7 +139,6 @@ func parseMount(val string, expander SingleWordExpander) (*Mount, error) { m := &Mount{Type: MountTypeBind} roAuto := true - envDefault := "" for _, field := range fields { key, value, ok := strings.Cut(field, "=") @@ -158,9 +157,6 @@ func parseMount(val string, expander SingleWordExpander) (*Mount, error) { m.ReadOnly = false roAuto = false continue - case "env": - m.Env = &envDefault - continue case "required": if m.Type == MountTypeSecret || m.Type == MountTypeSSH { m.Required = true