From 75f5d65bd7f19ef6b24967fcd3891e2ac342e65b Mon Sep 17 00:00:00 2001 From: Ed Schouten Date: Fri, 12 Mar 2021 06:28:42 -0800 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. Closes #13085. PiperOrigin-RevId: 362508252 --- .../ByteStreamBuildEventArtifactUploader.java | 9 +---- ...reamBuildEventArtifactUploaderFactory.java | 15 +++----- .../build/lib/remote/RemoteModule.java | 15 ++++---- .../lib/remote/options/RemoteOptions.java | 13 +++++++ ...eStreamBuildEventArtifactUploaderTest.java | 3 +- .../bazel/remote/remote_execution_test.sh | 35 +++++++++++++++++++ 6 files changed, 64 insertions(+), 26 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..c52d19989a28af 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 @@ -16,7 +16,6 @@ import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.RequestMetadata; import com.google.common.base.Preconditions; -import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -45,7 +44,6 @@ import java.util.Set; import java.util.concurrent.Executors; import java.util.concurrent.atomic.AtomicBoolean; -import javax.annotation.Nullable; /** A {@link BuildEventArtifactUploader} backed by {@link ByteStreamUploader}. */ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted @@ -63,16 +61,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..e2a1e27769f3dc 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 @@ -18,7 +18,6 @@ import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.runtime.BuildEventArtifactUploaderFactory; import com.google.devtools.build.lib.runtime.CommandEnvironment; -import javax.annotation.Nullable; /** * A factory for {@link ByteStreamBuildEventArtifactUploader}. @@ -27,25 +26,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 +49,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 253830b94b3590..5791445c086c7e 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 @@ -467,6 +467,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, @@ -487,12 +495,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { uploader.release(); buildEventArtifactUploaderFactoryDelegate.init( new ByteStreamBuildEventArtifactUploaderFactory( - uploader, - cacheClient, - cacheChannel.authority(), - buildRequestId, - invocationId, - remoteOptions.remoteInstanceName)); + uploader, cacheClient, remoteBytestreamUriPrefix, buildRequestId, 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 777cf921ba0679..1c558adf040d73 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 @@ -373,10 +373,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 3a828423b7e458..0f177dfb485183 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.