From 6c29268e1adf7056ca1d2a481fb80ce063f057d7 Mon Sep 17 00:00:00 2001 From: Ed Schouten Date: Mon, 22 Feb 2021 07:37:19 +0000 Subject: [PATCH] Allow overriding the hostname and instance name in bytestream:// URIs In cases where full network transparency doesn't exist, people may run Bazel with custom values of --remote_executor and --remote_instance_name to proxy gRPC traffic. Such proxies may do caching, tunneling and authentication. What's problematic about this is that the values of these command line flags aren't just used to establish a gRPC connection to a remote execution service. They also get logged by Bazel in build event streams in the form of bytestream:// URIs. This means that a build event stream generated on system A may contain bytestream:// URIs that are not valid on system B. Let's introduce a command line flag, --remote_bytestream_uri_prefix, that can be used to force generation of bytestream:// URIs that are canonical. --- .../ByteStreamBuildEventArtifactUploader.java | 7 +--- ...reamBuildEventArtifactUploaderFactory.java | 14 +++----- .../build/lib/remote/RemoteModule.java | 13 +++++-- .../lib/remote/options/RemoteOptions.java | 13 +++++++ ...eStreamBuildEventArtifactUploaderTest.java | 3 +- .../bazel/remote/remote_execution_test.sh | 35 +++++++++++++++++++ 6 files changed, 65 insertions(+), 20 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java index 92177fcf62b26b..70c83be2a916fd 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java @@ -63,16 +63,11 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted ByteStreamBuildEventArtifactUploader( ByteStreamUploader uploader, MissingDigestsFinder missingDigestsFinder, - String remoteServerName, + String remoteServerInstanceName, String buildRequestId, String commandId, - @Nullable String remoteInstanceName, int maxUploadThreads) { this.uploader = Preconditions.checkNotNull(uploader); - String remoteServerInstanceName = Preconditions.checkNotNull(remoteServerName); - if (!Strings.isNullOrEmpty(remoteInstanceName)) { - remoteServerInstanceName += "/" + remoteInstanceName; - } this.buildRequestId = buildRequestId; this.commandId = commandId; this.remoteServerInstanceName = remoteServerInstanceName; diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java index 1c27fbe703d0c7..5e1e2792e257a0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java @@ -27,25 +27,22 @@ class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactUploaderFactory { private final ByteStreamUploader uploader; - private final String remoteServerName; + private final String remoteServerInstanceName; private final String buildRequestId; private final String commandId; private final MissingDigestsFinder missingDigestsFinder; - @Nullable private final String remoteInstanceName; ByteStreamBuildEventArtifactUploaderFactory( ByteStreamUploader uploader, MissingDigestsFinder missingDigestsFinder, - String remoteServerName, + String remoteServerInstanceName, String buildRequestId, - String commandId, - @Nullable String remoteInstanceName) { + String commandId) { this.uploader = uploader; this.missingDigestsFinder = missingDigestsFinder; - this.remoteServerName = remoteServerName; + this.remoteServerInstanceName = remoteServerInstanceName; this.buildRequestId = buildRequestId; this.commandId = commandId; - this.remoteInstanceName = remoteInstanceName; } @Override @@ -53,10 +50,9 @@ public BuildEventArtifactUploader create(CommandEnvironment env) { return new ByteStreamBuildEventArtifactUploader( uploader.retain(), missingDigestsFinder, - remoteServerName, + remoteServerInstanceName, buildRequestId, commandId, - remoteInstanceName, env.getOptions().getOptions(RemoteOptions.class).buildEventUploadMaxThreads); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 443d4a8f95e26a..10a60aac717cdf 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -482,6 +482,14 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { } } + String remoteBytestreamUriPrefix = remoteOptions.remoteBytestreamUriPrefix; + if (Strings.isNullOrEmpty(remoteBytestreamUriPrefix)) { + remoteBytestreamUriPrefix = cacheChannel.authority(); + if (!Strings.isNullOrEmpty(remoteOptions.remoteInstanceName)) { + remoteBytestreamUriPrefix += "/" + remoteOptions.remoteInstanceName; + } + } + ByteStreamUploader uploader = new ByteStreamUploader( remoteOptions.remoteInstanceName, @@ -504,10 +512,9 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { new ByteStreamBuildEventArtifactUploaderFactory( uploader, cacheClient, - cacheChannel.authority(), + remoteBytestreamUriPrefix, buildRequestId, - invocationId, - remoteOptions.remoteInstanceName)); + invocationId)); if (enableRemoteExecution) { RemoteExecutionClient remoteExecutor; diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 9df464991f97cb..7f7725a565cbc7 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -181,6 +181,19 @@ public final class RemoteOptions extends OptionsBase { + " the unit is omitted, the value is interpreted as seconds.") public Duration remoteTimeout; + @Option( + name = "remote_bytestream_uri_prefix", + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "The hostname and instance name to be used in bytestream:// URIs that are written into " + + "build event streams. This option can be set when builds are performed using a " + + "proxy, which causes the values of --remote_executor and --remote_instance_name " + + "to no longer correspond to the canonical name of the remote execution service. " + + "When not set, it will default to \"${hostname}/${instance_name}\".") + public String remoteBytestreamUriPrefix; + /** Returns the specified duration. Assumes seconds if unitless. */ public static class RemoteTimeoutConverter implements Converter { private static final Pattern UNITLESS_REGEX = Pattern.compile("^[0-9]+$"); diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index 147f045f5793fb..993f53b2876a49 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -361,10 +361,9 @@ private ByteStreamBuildEventArtifactUploader newArtifactUploader( return new ByteStreamBuildEventArtifactUploader( uploader, missingDigestsFinder, - "localhost", + "localhost/instance", "none", "none", - "instance", /* maxUploadThreads= */ 100); } diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 1e4ab53f8e4b12..2724c6110861ca 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -1440,6 +1440,41 @@ EOF expect_log "uri:.*bytestream://localhost" } +function test_bytestream_uri_prefix() { + # Test that when --remote_bytestream_uri_prefix is set, bytestream:// + # URIs do not contain the hostname that's part of --remote_executor. + # They should use a fixed value instead. + mkdir -p a + cat > a/success.sh <<'EOF' +#!/bin/sh +exit 0 +EOF + chmod 755 a/success.sh + cat > a/BUILD <<'EOF' +sh_test( + name = "success_test", + srcs = ["success.sh"], +) + +genrule( + name = "foo", + srcs = [], + outs = ["foo.txt"], + cmd = "echo \"foo\" > \"$@\"", +) +EOF + + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + --remote_bytestream_uri_prefix=example.com/my-instance-name \ + --build_event_text_file=$TEST_log \ + //a:foo //a:success_test || fail "Failed to test //a:foo //a:success_test" + + expect_not_log 'uri:.*file://' + expect_log "uri:.*bytestream://example.com/my-instance-name/blobs" +} + # This test is derivative of test_bep_output_groups in # build_event_stream_test.sh, which verifies that successful output groups' # artifacts appear in BEP when a top-level target fails to build.