+ When building remotely, the values are by default included in the
+ remote action cache key, so changing them does cause remote cache
+ misses. Enabling the
+ Remote.ExcludePassUnsafeEnvVarsFromDigest
+ option keeps these values out of the remote action cache key (while
+ still passing them to the executed action), so changing them no longer
+ causes remote cache misses. Note that with it enabled a cached result
+ may be reused (and shared between users) even if the PassUnsafeEnv
+ values differ from those it was originally built with.
+
UploadDirs (bool)
diff --git a/src/core/config.go b/src/core/config.go
index a54d97b6ca..ba8f987cf9 100644
--- a/src/core/config.go
+++ b/src/core/config.go
@@ -518,7 +518,7 @@ type Configuration struct {
Xattrs bool `help:"True (the default) to attempt to use xattrs to record file metadata. If false Please will fall back to using additional files where needed, which is more compatible but has slightly worse performance."`
Nonce string `help:"This is an arbitrary string that is added to the hash of every build target. It provides a way to force a rebuild of everything when it's changed.\nWe will bump the default of this whenever we think it's required - although it's been a pretty long time now and we hope that'll continue."`
PassEnv []string `help:"A list of environment variables to pass from the current environment to build rules. For example\n\nPassEnv = HTTP_PROXY\n\nwould copy your HTTP_PROXY environment variable to the build env for any rules."`
- PassUnsafeEnv []string `help:"Similar to PassEnv, a list of environment variables to pass from the current environment to build rules. Unlike PassEnv, the environment variable values are not used when calculating build target hashes."`
+ PassUnsafeEnv []string `help:"Similar to PassEnv, a list of environment variables to pass from the current environment to build rules. Unlike PassEnv, the environment variable values are not used when calculating build target hashes. For remote execution this is controlled by Remote.ExcludePassUnsafeEnvVarsFromDigest, which by default also keeps these values out of the remote action cache key."`
HTTPProxy cli.URL `help:"A URL to use as a proxy server for downloads. Only applies to internal ones - e.g. self-updates or remote_file rules."`
HashCheckers []string `help:"Set of hash algos supported by the 'hashes' argument on build rules. Defaults to: sha1,sha256,blake3." options:"sha1,sha256,blake3,xxhash,crc32,crc64"`
HashFunction string `help:"The hash function to use internally for build actions." options:"sha1,sha256,blake3,xxhash,crc32,crc64"`
@@ -562,23 +562,24 @@ type Configuration struct {
ExcludeableTargets []BuildLabel `help:"If set, only targets that match these wildcards will be allowed to opt out of the sandbox"`
} `help:"A config section describing settings relating to sandboxing of build actions."`
Remote struct {
- URL string `help:"URL for the remote server."`
- CASURL string `help:"URL for the CAS service, if it is different to the main one."`
- AssetURL string `help:"URL for the remote asset server, if it is different to the main one."`
- NumExecutors int `help:"Maximum number of remote executors to use simultaneously."`
- Instance string `help:"Remote instance name to request; depending on the server this may be required."`
- Name string `help:"A name for this worker instance. This is attached to artifacts uploaded to remote storage." example:"agent-001"`
- DisplayURL string `help:"A URL to browse the remote server with (e.g. using buildbarn-browser). Only used when printing hashes."`
- TokenFile string `help:"A file containing a token that is attached to outgoing RPCs to authenticate them. This is somewhat bespoke; we are still investigating further options for authentication."`
- Timeout cli.Duration `help:"Timeout for connections made to the remote server."`
- Secure bool `help:"Whether to use TLS for communication or not."`
- VerifyOutputs bool `help:"Whether to verify all outputs are present after a cached remote execution action. Depending on your server implementation, you may require this to ensure files are really present."`
- UploadDirs bool `help:"Uploads individual directory blobs after build actions. This might not be necessary with some servers, but if you aren't sure, you should leave it on."`
- OptionalOutputsRequired bool `help:"Requires that any optional outputs of build actions (optional test outputs, coverage when not opted out of) are produced. By default this is a non-fatal failure, but the actions may not cache remotely."`
- Shell string `help:"Path to the shell to use to execute actions in. Default is 'bash' which will be looked up by the server."`
- Platform []string `help:"Platform properties to request from remote workers, in the format key=value."`
- CacheDuration cli.Duration `help:"Length of time before we re-check locally cached build actions. Default is unlimited."`
- BuildID string `help:"ID of the build action that's being run, to attach to remote requests. If not set then one is automatically generated."`
+ URL string `help:"URL for the remote server."`
+ CASURL string `help:"URL for the CAS service, if it is different to the main one."`
+ AssetURL string `help:"URL for the remote asset server, if it is different to the main one."`
+ NumExecutors int `help:"Maximum number of remote executors to use simultaneously."`
+ Instance string `help:"Remote instance name to request; depending on the server this may be required."`
+ Name string `help:"A name for this worker instance. This is attached to artifacts uploaded to remote storage." example:"agent-001"`
+ DisplayURL string `help:"A URL to browse the remote server with (e.g. using buildbarn-browser). Only used when printing hashes."`
+ TokenFile string `help:"A file containing a token that is attached to outgoing RPCs to authenticate them. This is somewhat bespoke; we are still investigating further options for authentication."`
+ Timeout cli.Duration `help:"Timeout for connections made to the remote server."`
+ Secure bool `help:"Whether to use TLS for communication or not."`
+ VerifyOutputs bool `help:"Whether to verify all outputs are present after a cached remote execution action. Depending on your server implementation, you may require this to ensure files are really present."`
+ ExcludePassUnsafeEnvVarsFromDigest bool `help:"Whether to exclude the values of PassUnsafeEnv environment variables from the remote action digest. When enabled, PassUnsafeEnv values are still passed to the executed action but do not contribute to the cache key, so changing them does not cause remote cache misses (matching local cache behaviour). This is disabled by default; note that enabling it means a cached result may be reused (and shared between users) even if the PassUnsafeEnv values differ from those it was built with."`
+ UploadDirs bool `help:"Uploads individual directory blobs after build actions. This might not be necessary with some servers, but if you aren't sure, you should leave it on."`
+ OptionalOutputsRequired bool `help:"Requires that any optional outputs of build actions (optional test outputs, coverage when not opted out of) are produced. By default this is a non-fatal failure, but the actions may not cache remotely."`
+ Shell string `help:"Path to the shell to use to execute actions in. Default is 'bash' which will be looked up by the server."`
+ Platform []string `help:"Platform properties to request from remote workers, in the format key=value."`
+ CacheDuration cli.Duration `help:"Length of time before we re-check locally cached build actions. Default is unlimited."`
+ BuildID string `help:"ID of the build action that's being run, to attach to remote requests. If not set then one is automatically generated."`
} `help:"Settings related to remote execution & caching using the Google remote execution APIs. This section is still experimental and subject to change."`
Size map[string]*Size `help:"Named sizes of targets; these are the definitions of what can be passed to the 'size' argument."`
Cover struct {
diff --git a/src/remote/action.go b/src/remote/action.go
index 2098ec48c3..3370986351 100644
--- a/src/remote/action.go
+++ b/src/remote/action.go
@@ -40,7 +40,7 @@ func (c *Client) uploadAction(target *core.BuildTarget, isTest, isRun bool, run
}
inputRootEntry, inputRootDigest := c.protoEntry(inputRoot)
ch <- inputRootEntry
- command, err = c.buildCommand(target, inputRoot, isTest, isRun, target.Stamp, run)
+ command, err = c.buildCommand(target, inputRoot, isTest, isRun, target.Stamp, false, run)
if err != nil {
return err
}
@@ -60,13 +60,14 @@ func (c *Client) uploadAction(target *core.BuildTarget, isTest, isRun bool, run
}
// buildAction creates a build action for a target and returns the command and the action digest. No uploading is done.
-func (c *Client) buildAction(target *core.BuildTarget, isTest, stamp bool, run int) (*pb.Command, *pb.Digest, error) {
+// If canonical is true the values of PassUnsafeEnv variables are excluded from the digest (see buildCommand).
+func (c *Client) buildAction(target *core.BuildTarget, isTest, stamp, canonical bool, run int) (*pb.Command, *pb.Digest, error) {
inputRoot, err := c.uploadInputs(nil, target, isTest)
if err != nil {
return nil, nil, err
}
inputRootDigest := c.digestMessage(inputRoot)
- command, err := c.buildCommand(target, inputRoot, isTest, false, stamp, run)
+ command, err := c.buildCommand(target, inputRoot, isTest, false, stamp, canonical, run)
if err != nil {
return nil, nil, err
}
@@ -81,10 +82,13 @@ func (c *Client) buildAction(target *core.BuildTarget, isTest, stamp bool, run i
}
// buildCommand builds the command for a single target.
-func (c *Client) buildCommand(target *core.BuildTarget, inputRoot *pb.Directory, isTest, isRun, stamp bool, run int) (*pb.Command, error) {
+// If canonical is true, the values of PassUnsafeEnv variables are stripped from the environment so that
+// they do not contribute to the action digest; this is used to compute a stable cache-key action that is
+// never actually executed (see Client.build).
+func (c *Client) buildCommand(target *core.BuildTarget, inputRoot *pb.Directory, isTest, isRun, stamp, canonical bool, run int) (*pb.Command, error) {
state := c.state.ForTarget(target)
if isTest {
- return c.buildTestCommand(state, target, run)
+ return c.buildTestCommand(state, target, canonical, run)
} else if isRun {
return c.buildRunCommand(state, target)
}
@@ -126,14 +130,56 @@ func (c *Client) buildCommand(target *core.BuildTarget, inputRoot *pb.Directory,
cmd = "true"
}
cmd, err := core.ReplaceSequences(state, target, cmd)
+ env := c.stampedBuildEnvironment(state, target, inputRoot, stamp, isTest || isRun)
+ if canonical {
+ c.stripUnsafeEnv(state, target, env)
+ }
return &pb.Command{
Platform: c.targetPlatformProperties(target),
Arguments: process.BashCommand(c.shellPath, commandPrefixBuilder.String()+cmd, state.Config.Build.ExitOnError),
- EnvironmentVariables: c.buildEnv(target, c.stampedBuildEnvironment(state, target, inputRoot, stamp, isTest || isRun), target.Sandbox),
+ EnvironmentVariables: c.buildEnv(target, env, target.Sandbox),
OutputPaths: outs,
}, err
}
+// excludesUnsafeEnv reports whether PassUnsafeEnv values should be kept out of the action digest for this
+// target. This considers both the global [Build] PassUnsafeEnv config keyword and the target's own
+// pass_unsafe_env attribute, mirroring how the local cache excludes both from its hash.
+func (c *Client) excludesUnsafeEnv(state *core.BuildState, target *core.BuildTarget) bool {
+ return c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest &&
+ (len(state.Config.Build.PassUnsafeEnv) > 0 || (target.PassUnsafeEnv != nil && len(*target.PassUnsafeEnv) > 0))
+}
+
+// stripUnsafeEnv removes the values of PassUnsafeEnv variables from the given environment so that they do
+// not contribute to the action digest. Both the global [Build] PassUnsafeEnv config keyword and the
+// target's pass_unsafe_env attribute are considered. Variables that are also listed in PassEnv (config or
+// target level) are left intact, since those values are intentionally part of the cache key.
+func (c *Client) stripUnsafeEnv(state *core.BuildState, target *core.BuildTarget, env core.BuildEnv) {
+ if !c.excludesUnsafeEnv(state, target) {
+ return
+ }
+ safe := map[string]bool{}
+ for _, e := range state.Config.Build.PassEnv {
+ safe[e] = true
+ }
+ if target.PassEnv != nil {
+ for _, e := range *target.PassEnv {
+ safe[e] = true
+ }
+ }
+ strip := func(vars []string) {
+ for _, e := range vars {
+ if !safe[e] {
+ delete(env, e)
+ }
+ }
+ }
+ strip(state.Config.Build.PassUnsafeEnv)
+ if target.PassUnsafeEnv != nil {
+ strip(*target.PassUnsafeEnv)
+ }
+}
+
// stampedBuildEnvironment returns a build environment, optionally with a stamp if stamp is true.
func (c *Client) stampedBuildEnvironment(state *core.BuildState, target *core.BuildTarget, inputRoot *pb.Directory, stamp, isRuntime bool) core.BuildEnv {
if target.IsFilegroup {
@@ -146,7 +192,7 @@ func (c *Client) stampedBuildEnvironment(state *core.BuildState, target *core.Bu
}
// buildTestCommand builds a command for a target when testing.
-func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarget, run int) (*pb.Command, error) {
+func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarget, canonical bool, run int) (*pb.Command, error) {
paths := target.Test.Outputs
if target.NeedCoverage(state) {
paths = append(paths, core.CoverageFile)
@@ -159,6 +205,10 @@ func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarg
commandPrefix += `export TEST="$TEST_DIR/` + outs[0] + `" && `
}
cmd, err := core.ReplaceTestSequences(state, target, target.GetTestCommand(state))
+ env := core.TestEnvironment(state, target, ".", run)
+ if canonical {
+ c.stripUnsafeEnv(state, target, env)
+ }
return &pb.Command{
Platform: &pb.Platform{
Properties: []*pb.Platform_Property{
@@ -169,7 +219,7 @@ func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarg
},
},
Arguments: process.BashCommand(c.shellPath, commandPrefix+cmd, state.Config.Build.ExitOnError),
- EnvironmentVariables: c.buildEnv(nil, core.TestEnvironment(state, target, ".", run), target.Test.Sandbox),
+ EnvironmentVariables: c.buildEnv(nil, env, target.Test.Sandbox),
OutputPaths: paths,
}, err
}
diff --git a/src/remote/impl_test.go b/src/remote/impl_test.go
index 338871a678..2b26b758e2 100644
--- a/src/remote/impl_test.go
+++ b/src/remote/impl_test.go
@@ -8,6 +8,7 @@ import (
"net"
"os"
"regexp"
+ "sync/atomic"
"testing"
"cloud.google.com/go/longrunning/autogen/longrunningpb"
@@ -33,6 +34,13 @@ func newClient() *Client {
}
func newClientInstance(name string) *Client {
+ return newClientInstanceWith(name, nil)
+}
+
+// newClientInstanceWith builds a client, optionally applying configure to the configuration before the
+// build state (and the client's async initialisation) is created. This matters for config values that are
+// captured once, e.g. the build environment derived from PassUnsafeEnv.
+func newClientInstanceWith(name string, configure func(*core.Configuration)) *Client {
config := core.DefaultConfiguration()
config.Build.Path = []string{"/usr/local/bin", "/usr/bin", "/bin"}
config.Build.HashFunction = "sha256"
@@ -40,6 +48,9 @@ func newClientInstance(name string) *Client {
config.Remote.Instance = name
config.Remote.Secure = false
config.Remote.Platform = []string{"OSFamily=linux"}
+ if configure != nil {
+ configure(config)
+ }
state := core.NewBuildState(config)
state.Config.Remote.URL = "127.0.0.1:9987"
state.Config.Remote.AssetURL = state.Config.Remote.URL
@@ -55,6 +66,7 @@ type testServer struct {
blobs map[string][]byte
bytestreams map[string][]byte
mockActionResult *pb.ActionResult
+ executions atomic.Int64
}
func (s *testServer) GetCapabilities(ctx context.Context, req *pb.GetCapabilitiesRequest) (*pb.ServerCapabilities, error) {
@@ -86,6 +98,7 @@ func (s *testServer) Reset() {
s.blobs = map[string][]byte{}
s.bytestreams = map[string][]byte{}
s.mockActionResult = nil
+ s.executions.Store(0)
}
func (s *testServer) GetActionResult(ctx context.Context, req *pb.GetActionResultRequest) (*pb.ActionResult, error) {
@@ -249,6 +262,7 @@ func (s *testServer) QueryWriteStatus(ctx context.Context, req *bs.QueryWriteSta
}
func (s *testServer) Execute(req *pb.ExecuteRequest, srv pb.Execution_ExecuteServer) error {
+ s.executions.Add(1)
mm := func(msg protoreflect.ProtoMessage) *anypb.Any {
a := &anypb.Any{}
a.MarshalFrom(msg)
diff --git a/src/remote/remote.go b/src/remote/remote.go
index ee7ec220df..aa31d02f62 100644
--- a/src/remote/remote.go
+++ b/src/remote/remote.go
@@ -76,7 +76,10 @@ type Client struct {
subrepoTrees map[core.BuildLabel]*pb.Tree
outputMutex sync.RWMutex
- // The unstamped build action digests. Stamped and test digests are not stored.
+ // The cache-key build action digests. Stamped and test digests are not stored.
+ // For most targets this is the plain (unstamped) action digest, but for stamped targets and those
+ // whose PassUnsafeEnv values are excluded from the digest it is the canonical digest that omits those
+ // volatile inputs (see Client.build); that is the digest results are stored & looked up under.
// This isn't just a cache - it is needed because building a target can modify the target and things like plz hash
// --detailed and --shell will fail to get the right action digest.
unstampedBuildActionDigests actionDigestMap
@@ -412,36 +415,40 @@ func (c *Client) Run(target *core.BuildTarget) error {
// build implements the actual build of a target.
func (c *Client) build(target *core.BuildTarget) (*core.BuildMetadata, *pb.ActionResult, *pb.Digest, error) {
needStdout := target.PostBuildFunction != nil
- // If we're gonna stamp the target, first check the unstamped equivalent that we store results under.
- // This implements the rules of stamp whereby we don't force rebuilds every time e.g. the SCM revision changes.
- var unstampedDigest *pb.Digest
- if target.Stamp {
- command, digest, err := c.buildAction(target, false, false, 0)
+ // Some targets carry volatile inputs that we deliberately keep out of the cache key: the stamp
+ // (e.g. the SCM revision) and the values of PassUnsafeEnv variables (when configured). For these we
+ // store & look up results under a "cache-key" action digest that omits those inputs, while still
+ // executing the real action that includes them. This implements the rule whereby e.g. the SCM
+ // revision or an unsafe env var changing doesn't force a rebuild.
+ useCacheKeyDigest := target.Stamp || c.excludesUnsafeEnv(c.state.ForTarget(target), target)
+ var cacheKeyDigest *pb.Digest
+ if useCacheKeyDigest {
+ command, digest, err := c.buildAction(target, false, false, true, 0)
if err != nil {
return nil, nil, nil, err
} else if metadata, ar := c.maybeRetrieveResults(target, command, digest, false, needStdout, 0); metadata != nil {
c.unstampedBuildActionDigests.Put(target.Label, digest)
return metadata, ar, digest, nil
}
- unstampedDigest = digest
+ cacheKeyDigest = digest
}
- command, stampedDigest, err := c.buildAction(target, false, true, 0)
+ command, stampedDigest, err := c.buildAction(target, false, true, false, 0)
if err != nil {
return nil, nil, nil, err
}
metadata, ar, err := c.execute(target, command, stampedDigest, false, needStdout, 0)
- if target.Stamp && err == nil {
- err = c.verifyActionResult(target, command, unstampedDigest, ar, c.state.Config.Remote.VerifyOutputs, false)
+ if useCacheKeyDigest && err == nil {
+ err = c.verifyActionResult(target, command, cacheKeyDigest, ar, c.state.Config.Remote.VerifyOutputs, false)
if err == nil {
- // Store results under unstamped digest too.
- c.locallyCacheResults(target, unstampedDigest, metadata)
+ // Store results under the cache-key digest too.
+ c.locallyCacheResults(target, cacheKeyDigest, metadata)
}
c.client.UpdateActionResult(context.Background(), &pb.UpdateActionResultRequest{
InstanceName: c.instance,
- ActionDigest: unstampedDigest,
+ ActionDigest: cacheKeyDigest,
ActionResult: ar,
})
- c.unstampedBuildActionDigests.Put(target.Label, unstampedDigest)
+ c.unstampedBuildActionDigests.Put(target.Label, cacheKeyDigest)
} else {
c.unstampedBuildActionDigests.Put(target.Label, stampedDigest)
}
@@ -608,11 +615,34 @@ func (c *Client) Test(target *core.BuildTarget, run int) (metadata *core.BuildMe
if err := c.CheckInitialised(); err != nil {
return nil, err
}
- command, digest, err := c.buildAction(target, true, false, run)
- if err != nil {
- return nil, err
+ // As in build(), if PassUnsafeEnv values are excluded from the digest we look results up under a
+ // cache-key digest that omits them, while executing the real action that includes them.
+ var ar *pb.ActionResult
+ var cacheKeyDigest *pb.Digest
+ if c.excludesUnsafeEnv(c.state.ForTarget(target), target) {
+ command, digest, cErr := c.buildAction(target, true, false, true, run)
+ if cErr != nil {
+ return nil, cErr
+ }
+ if metadata, ar = c.maybeRetrieveResults(target, command, digest, true, false, run); ar == nil {
+ cacheKeyDigest = digest // Cache miss; remember to backfill once we've executed the real action.
+ }
+ }
+ if ar == nil {
+ command, digest, bErr := c.buildAction(target, true, false, false, run)
+ if bErr != nil {
+ return nil, bErr
+ }
+ metadata, ar, err = c.execute(target, command, digest, true, false, run)
+ if err == nil && ar != nil && cacheKeyDigest != nil {
+ // Backfill the cache-key digest so later runs with differing PassUnsafeEnv values hit.
+ c.client.UpdateActionResult(context.Background(), &pb.UpdateActionResultRequest{
+ InstanceName: c.instance,
+ ActionDigest: cacheKeyDigest,
+ ActionResult: ar,
+ })
+ }
}
- metadata, ar, err := c.execute(target, command, digest, true, false, run)
if ar != nil {
_, dlErr := c.client.DownloadActionOutputs(context.Background(), ar, target.TestDir(run), c.fileMetadataCache)
diff --git a/src/remote/remote_test.go b/src/remote/remote_test.go
index 05af2cd9ba..29630144cc 100644
--- a/src/remote/remote_test.go
+++ b/src/remote/remote_test.go
@@ -1,6 +1,7 @@
package remote
import (
+ "fmt"
"os"
"path/filepath"
"strings"
@@ -166,7 +167,7 @@ func TestNoAbsolutePaths(t *testing.T) {
target.AddOutput("remote_test")
target.AddSource(core.FileLabel{Package: "package", File: "file"})
target.AddTool(tool.Label)
- cmd, _ := c.buildCommand(target, &pb.Directory{}, false, false, false, 0)
+ cmd, _ := c.buildCommand(target, &pb.Directory{}, false, false, false, false, 0)
testDir := os.Getenv("TEST_DIR")
for _, env := range cmd.EnvironmentVariables {
if !strings.HasPrefix(env.Value, "//") {
@@ -185,7 +186,7 @@ func TestNoAbsolutePaths2(t *testing.T) {
target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target5"})
target.AddOutput("remote_test")
target.AddTool(core.SystemPathLabel{Path: []string{os.Getenv("TMP_DIR")}, Name: "remote_test"})
- cmd, _ := c.buildCommand(target, &pb.Directory{}, false, false, false, 0)
+ cmd, _ := c.buildCommand(target, &pb.Directory{}, false, false, false, false, 0)
for _, env := range cmd.EnvironmentVariables {
if !strings.HasPrefix(env.Value, "//") {
assert.False(t, filepath.IsAbs(env.Value), "Env var %s has an absolute path: %s", env.Name, env.Value)
@@ -199,17 +200,181 @@ func TestRemoteFilesHashConsistently(t *testing.T) {
target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "download"})
target.IsRemoteFile = true
target.AddSource(core.URLLabel("https://localhost/file"))
- cmd, digest, err := c.buildAction(target, false, false, 0)
+ cmd, digest, err := c.buildAction(target, false, false, false, 0)
assert.NoError(t, err)
// After we change this path, the rule should still give back the same protos since it is
// not relevant to how we fetch a remote asset.
c.state.Config.Build.Path = []string{"/usr/bin/nope"}
- cmd2, digest2, err := c.buildAction(target, false, false, 0)
+ cmd2, digest2, err := c.buildAction(target, false, false, false, 0)
assert.NoError(t, err)
assert.Equal(t, cmd, cmd2)
assert.Equal(t, digest, digest2)
}
+// envContainsValue reports whether any environment variable in the command has the given value.
+func envContainsValue(cmd *pb.Command, value string) bool {
+ for _, e := range cmd.EnvironmentVariables {
+ if e.Value == value {
+ return true
+ }
+ }
+ return false
+}
+
+func TestPassUnsafeEnvExcludedFromDigest(t *testing.T) {
+ c := newClientInstance("unsafe_env")
+ assert.False(t, c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest, "should default to false")
+
+ target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "unsafe"})
+ target.AddOutput("out.txt")
+ target.Command = "echo hello > $OUT"
+ unsafe := []string{"MY_UNSAFE_VAR"}
+ target.PassUnsafeEnv = &unsafe
+ target.BuildTimeout = time.Minute
+
+ // With the feature disabled (the default) the value contributes to the cache-key digest.
+ t.Setenv("MY_UNSAFE_VAR", "first")
+ _, disabledDigest1, err := c.buildAction(target, false, false, true, 0)
+ require.NoError(t, err)
+ t.Setenv("MY_UNSAFE_VAR", "second")
+ _, disabledDigest2, err := c.buildAction(target, false, false, true, 0)
+ require.NoError(t, err)
+ assert.NotEqual(t, disabledDigest1.Hash, disabledDigest2.Hash)
+
+ // Enable the feature; now the value must be excluded from the cache-key digest.
+ c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true
+
+ t.Setenv("MY_UNSAFE_VAR", "first")
+ canon1, canonDigest1, err := c.buildAction(target, false, false, true, 0)
+ require.NoError(t, err)
+ real1, realDigest1, err := c.buildAction(target, false, true, false, 0)
+ require.NoError(t, err)
+
+ t.Setenv("MY_UNSAFE_VAR", "second")
+ canon2, canonDigest2, err := c.buildAction(target, false, false, true, 0)
+ require.NoError(t, err)
+ _, realDigest2, err := c.buildAction(target, false, true, false, 0)
+ require.NoError(t, err)
+
+ // The cache-key (canonical) digest is stable across changes to the PassUnsafeEnv value, and the value
+ // never appears in the canonical command's environment.
+ assert.Equal(t, canonDigest1.Hash, canonDigest2.Hash)
+ assert.False(t, envContainsValue(canon1, "first"))
+ assert.False(t, envContainsValue(canon2, "second"))
+
+ // The executed action still includes the real value, so its digest changes when the value changes.
+ assert.NotEqual(t, realDigest1.Hash, realDigest2.Hash)
+ assert.True(t, envContainsValue(real1, "first"))
+}
+
+// TestStripUnsafeEnvConfigLevel checks the stripping logic removes values from the global [Build]
+// PassUnsafeEnv config keyword while retaining PassEnv values (which are intentionally part of the cache key).
+func TestStripUnsafeEnvConfigLevel(t *testing.T) {
+ c := newClientInstance("strip")
+ c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true
+ c.state.Config.Build.PassUnsafeEnv = []string{"CFG_UNSAFE"}
+ c.state.Config.Build.PassEnv = []string{"CFG_SAFE"}
+
+ target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "x"})
+ unsafe := []string{"TGT_UNSAFE"}
+ target.PassUnsafeEnv = &unsafe
+
+ env := core.BuildEnv{
+ "CFG_UNSAFE": "secret",
+ "CFG_SAFE": "keep",
+ "TGT_UNSAFE": "alsosecret",
+ "OTHER": "v",
+ }
+ c.stripUnsafeEnv(c.state.ForTarget(target), target, env)
+
+ _, cfgUnsafePresent := env["CFG_UNSAFE"]
+ _, tgtUnsafePresent := env["TGT_UNSAFE"]
+ assert.False(t, cfgUnsafePresent, "config-level PassUnsafeEnv should be stripped")
+ assert.False(t, tgtUnsafePresent, "target-level PassUnsafeEnv should be stripped")
+ assert.Equal(t, "keep", env["CFG_SAFE"], "PassEnv must be retained")
+ assert.Equal(t, "v", env["OTHER"], "unrelated env must be retained")
+}
+
+// TestConfigPassUnsafeEnvExcludedFromDigest checks that values declared via the global [Build]
+// PassUnsafeEnv config keyword (not just the per-target attribute) are excluded from the cache-key digest.
+// Because config-level values are captured once per config object, this uses a separate client per value
+// and sets the config before the client (and its async init) is created.
+func TestConfigPassUnsafeEnvExcludedFromDigest(t *testing.T) {
+ canonicalAndReal := func(value string) (string, string) {
+ t.Setenv("MY_CFG_UNSAFE", value)
+ c := newClientInstanceWith(fmt.Sprintf("cfg-unsafe-%d", time.Now().UnixNano()), func(config *core.Configuration) {
+ config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true
+ config.Build.PassUnsafeEnv = []string{"MY_CFG_UNSAFE"}
+ })
+ target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "cfgunsafe"})
+ target.AddOutput("out.txt")
+ target.Command = "echo hello > $OUT"
+ target.BuildTimeout = time.Minute
+ canon, canonDigest, err := c.buildAction(target, false, false, true, 0)
+ require.NoError(t, err)
+ require.False(t, envContainsValue(canon, value), "config-level PassUnsafeEnv value must not be in the canonical command")
+ real, realDigest, err := c.buildAction(target, false, true, false, 0)
+ require.NoError(t, err)
+ require.True(t, envContainsValue(real, value), "executed action should include the config-level value")
+ return canonDigest.Hash, realDigest.Hash
+ }
+
+ canonFirst, realFirst := canonicalAndReal("first")
+ canonSecond, realSecond := canonicalAndReal("second")
+
+ assert.Equal(t, canonFirst, canonSecond, "config-level PassUnsafeEnv value must not affect the cache-key digest")
+ assert.NotEqual(t, realFirst, realSecond, "the executed action should still include the config-level value")
+}
+
+// TestPassUnsafeEnvRemoteCacheHitAcrossValues is an end-to-end test against the in-process testServer
+// that proves changing a PassUnsafeEnv value does not re-execute the action: the first build executes
+// and backfills the cache-key digest, and a second build with a *different* value (and a fresh client,
+// so a cold local cache) is served from the remote action cache instead of being re-executed.
+func TestPassUnsafeEnvRemoteCacheHitAcrossValues(t *testing.T) {
+ server.Reset()
+ defer server.Reset()
+
+ // Use unique instance names per run so each client's (disk-backed) local metadata store is empty.
+ // This guarantees the first build is a cold miss and forces the second build through the shared
+ // remote action cache (which is keyed by digest only, so it is shared across instances). Both names
+ // are something other than "test"/"mock" so the test server takes its default execution branch.
+ base := fmt.Sprintf("unsafe-%d", time.Now().UnixNano())
+
+ newTarget := func() *core.BuildTarget {
+ target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target2"})
+ target.AddSource(core.FileLabel{File: "src1.txt", Package: "package"})
+ target.AddSource(core.FileLabel{File: "src2.txt", Package: "package"})
+ target.AddOutput("out2.txt")
+ target.BuildTimeout = time.Minute
+ target.Command = "echo hello && echo test > $OUT"
+ unsafe := []string{"MY_UNSAFE_VAR"}
+ target.PassUnsafeEnv = &unsafe
+ return target
+ }
+
+ // First build: cold cache, so the action is executed once and the result is backfilled under the
+ // value-independent cache-key digest.
+ c1 := newClientInstance(base + "-a")
+ c1.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true
+ require.NoError(t, c1.CheckInitialised())
+ t.Setenv("MY_UNSAFE_VAR", "first")
+ md1, _, _, err := c1.build(newTarget())
+ require.NoError(t, err)
+ assert.False(t, md1.Cached, "first build should not be cached")
+ assert.EqualValues(t, 1, server.executions.Load(), "first build should execute the action once")
+
+ // Second build: different PassUnsafeEnv value and a separate client with an empty local cache. It
+ // must be a remote cache hit on the cache-key digest, not a re-execution.
+ c2 := newClientInstance(base + "-b")
+ c2.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true
+ require.NoError(t, c2.CheckInitialised())
+ t.Setenv("MY_UNSAFE_VAR", "second")
+ md2, _, _, err := c2.build(newTarget())
+ require.NoError(t, err)
+ assert.True(t, md2.Cached, "build with a different PassUnsafeEnv value should be a cache hit")
+ assert.EqualValues(t, 1, server.executions.Load(), "changing only PassUnsafeEnv must not re-execute the action")
+}
+
func TestOutDirsSetOutsOnTarget(t *testing.T) {
c := newClientInstance("mock")
@@ -318,7 +483,7 @@ func TestTargetPlatform(t *testing.T) {
c := newClientInstance("platform_test")
c.platform = convertPlatform(c.state.Config.Remote.Platform) // Bit of a hack but we can't go through the normal path.
target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target"})
- cmd, err := c.buildCommand(target, &pb.Directory{}, false, false, false, 0)
+ cmd, err := c.buildCommand(target, &pb.Directory{}, false, false, false, false, 0)
assert.NoError(t, err)
assert.Equal(t, &pb.Platform{
Properties: []*pb.Platform_Property{
@@ -330,7 +495,7 @@ func TestTargetPlatform(t *testing.T) {
}, cmd.Platform) //nolint:staticcheck
target.Labels = []string{"remote-platform-property:size=chomky"}
- cmd, err = c.buildCommand(target, &pb.Directory{}, false, false, false, 0)
+ cmd, err = c.buildCommand(target, &pb.Directory{}, false, false, false, false, 0)
assert.NoError(t, err)
assert.Equal(t, &pb.Platform{
Properties: []*pb.Platform_Property{