From 8dbbde0037264c1db4b229a09f98a61ab4ca06b0 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 669c646e42bfb3..d5f9a1485a21d1 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 @@ -469,6 +469,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, @@ -489,12 +497,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 f41b16a16dd640..01b785281010aa 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 @@ -374,10 +374,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.