Exclude PassUnsafeEnv from remote digest

ChangeLog

@@ -6,6 +6,10 @@ Version 17.30.0
     * Make `git_*` built-in function failures more informative (#3525)
     * Fix go coverage when parsing blocks out of order (#3519)
     * Update to golang 1.26 (#3498)
+    * Add `Remote.ExcludePassUnsafeEnvVarsFromDigest` config option (disabled by
+      default) which, when enabled, excludes `PassUnsafeEnv` values from the remote
+      action digest so changing them no longer causes remote cache misses (matching
+      local cache behaviour).
 
 Version 17.29.1
 ---------------

docs/config.html

@@ -701,6 +701,17 @@ <h3 class="mt1 f6 lh-title" id="build.passunsafeenv">
           PassEnv, environment variable values are not used to calculate build
           target hashes.
         </p>
+        <p>
+          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
+          <a href="#remote.excludepassunsafeenvvarsfromdigest">Remote.ExcludePassUnsafeEnvVarsFromDigest</a>
+          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.
+        </p>
       </div>
     </li>
     <li>
@@ -1016,6 +1027,12 @@ <h3 class="mt1 f6 lh-title" id="remote.verifyoutputs">VerifyOutputs <span class=
         <p>{{ index .ConfigHelpText "remote.verifyoutputs" }}</p>
       </div>
     </li>
+    <li>
+      <div>
+        <h3 class="mt1 f6 lh-title" id="remote.excludepassunsafeenvvarsfromdigest">ExcludePassUnsafeEnvVarsFromDigest <span class="normal">(bool)</span></h3>
+        <p>{{ index .ConfigHelpText "remote.excludepassunsafeenvvarsfromdigest" }}</p>
+      </div>
+    </li>
     <li>
       <div>
         <h3 class="mt1 f6 lh-title" id="remote.uploaddirs">UploadDirs <span class="normal">(bool)</span></h3>

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 {

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
 }

src/remote/impl_test.go

@@ -8,6 +8,7 @@ import (
 	"net"
 	"os"
 	"regexp"
+	"sync/atomic"
 	"testing"
 
 	"cloud.google.com/go/longrunning/autogen/longrunningpb"
@@ -33,13 +34,23 @@ 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"
 	config.Remote.NumExecutors = 1
 	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)