From 1fadf0a164c36e3ba5771b8823720e2180300404 Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Tue, 20 Sep 2022 22:44:37 +0200 Subject: [PATCH 1/6] [remote/downloader] Add incompatible flag for sending a list of headers as qualifier --- .../build/lib/remote/downloader/BUILD | 1 + .../downloader/GrpcRemoteDownloader.java | 56 +++++++++++------ .../lib/remote/options/RemoteOptions.java | 9 +++ .../build/lib/remote/downloader/BUILD | 1 + .../downloader/GrpcRemoteDownloaderTest.java | 62 ++++++++++++++++++- 5 files changed, 107 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD index d8a2e854024fbc..0fbc97537bfa57 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD @@ -14,6 +14,7 @@ java_library( name = "downloader", srcs = glob(["*.java"]), deps = [ + "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/remote:ReferenceCountedChannel", diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java index 83618be3cbcc2d..b3a08ad32a4d8f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java @@ -24,6 +24,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.bazel.repository.downloader.Downloader; import com.google.devtools.build.lib.bazel.repository.downloader.HashOutputStream; @@ -37,7 +38,9 @@ import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.vfs.Path; import com.google.gson.Gson; +import com.google.gson.JsonArray; import com.google.gson.JsonObject; +import com.google.gson.JsonPrimitive; import io.grpc.CallCredentials; import io.grpc.Channel; import io.grpc.StatusRuntimeException; @@ -67,7 +70,7 @@ public class GrpcRemoteDownloader implements AutoCloseable, Downloader { private final Optional credentials; private final RemoteRetrier retrier; private final RemoteCacheClient cacheClient; - private final RemoteOptions options; + private final RemoteOptions remoteOptions; private final AtomicBoolean closed = new AtomicBoolean(); @@ -79,20 +82,20 @@ public class GrpcRemoteDownloader implements AutoCloseable, Downloader { private static final String QUALIFIER_AUTH_HEADERS = "bazel.auth_headers"; public GrpcRemoteDownloader( - String buildRequestId, - String commandId, - ReferenceCountedChannel channel, - Optional credentials, - RemoteRetrier retrier, - RemoteCacheClient cacheClient, - RemoteOptions options) { + String buildRequestId, + String commandId, + ReferenceCountedChannel channel, + Optional credentials, + RemoteRetrier retrier, + RemoteCacheClient cacheClient, + RemoteOptions remoteOptions) { this.buildRequestId = buildRequestId; this.commandId = commandId; this.channel = channel; this.credentials = credentials; this.retrier = retrier; this.cacheClient = cacheClient; - this.options = options; + this.remoteOptions = remoteOptions; } @Override @@ -121,7 +124,13 @@ public void download( RemoteActionExecutionContext.create(metadata); final FetchBlobRequest request = - newFetchBlobRequest(options.remoteInstanceName, urls, authHeaders, checksum, canonicalId); + newFetchBlobRequest( + remoteOptions.remoteInstanceName, + urls, + authHeaders, + checksum, + canonicalId, + remoteOptions.remoteDownloaderSendAllHeaders); try { FetchBlobResponse response = retrier.execute( @@ -151,7 +160,8 @@ static FetchBlobRequest newFetchBlobRequest( List urls, Map>> authHeaders, com.google.common.base.Optional checksum, - String canonicalId) { + String canonicalId, + boolean includeAllHeaders) { FetchBlobRequest.Builder requestBuilder = FetchBlobRequest.newBuilder().setInstanceName(instanceName); for (URL url : urls) { @@ -172,7 +182,7 @@ static FetchBlobRequest newFetchBlobRequest( requestBuilder.addQualifiers( Qualifier.newBuilder() .setName(QUALIFIER_AUTH_HEADERS) - .setValue(authHeadersJson(authHeaders)) + .setValue(authHeadersJson(authHeaders, includeAllHeaders)) .build()); } @@ -184,9 +194,9 @@ private FetchBlockingStub fetchBlockingStub( return FetchGrpc.newBlockingStub(channel) .withInterceptors( TracingMetadataUtils.attachMetadataInterceptor(context.getRequestMetadata())) - .withInterceptors(TracingMetadataUtils.newDownloaderHeadersInterceptor(options)) + .withInterceptors(TracingMetadataUtils.newDownloaderHeadersInterceptor(remoteOptions)) .withCallCredentials(credentials.orElse(null)) - .withDeadlineAfter(options.remoteTimeout.getSeconds(), TimeUnit.SECONDS); + .withDeadlineAfter(remoteOptions.remoteTimeout.getSeconds(), TimeUnit.SECONDS); } private OutputStream newOutputStream( @@ -198,16 +208,24 @@ private OutputStream newOutputStream( return out; } - private static String authHeadersJson(Map>> authHeaders) { + private static String authHeadersJson(Map>> authHeaders, boolean includeAllHeaders) { Map subObjects = new TreeMap<>(); for (Map.Entry>> entry : authHeaders.entrySet()) { JsonObject subObject = new JsonObject(); Map> orderedHeaders = new TreeMap<>(entry.getValue()); for (Map.Entry> subEntry : orderedHeaders.entrySet()) { - // TODO(yannic): Introduce incompatible flag for including all headers, not just the first. - String value = Iterables.getFirst(subEntry.getValue(), null); - if (value != null) { - subObject.addProperty(subEntry.getKey(), value); + if (includeAllHeaders) { + JsonArray values = new JsonArray(subEntry.getValue().size()); + for (String value : subEntry.getValue()) { + values.add(value); + } + subObject.add(subEntry.getKey(), values); + } else { + // TODO(yannic): Introduce incompatible flag for including all headers, not just the first. + String value = Iterables.getFirst(subEntry.getValue(), null); + if (value != null) { + subObject.add(subEntry.getKey(), new JsonPrimitive(value)); + } } } subObjects.put(entry.getKey().toString(), subObject); 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 6acdaddbccd273..7960015054abe2 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 @@ -586,6 +586,15 @@ public RemoteOutputsStrategyConverter() { + "`all` to print always.") public ExecutionMessagePrintMode remotePrintExecutionMessages; + @Option( + name = "incompatible_remote_downloader_send_all_headers", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = "Whether to sens all headers to the remote downloader instead of just the first.") + public boolean remoteDownloaderSendAllHeaders; + // The below options are not configurable by users, only tests. // This is part of the effort to reduce the overall number of flags. diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD index 00b0bd0b6ff12c..f8243bdf981034 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD @@ -20,6 +20,7 @@ java_library( ], ), deps = [ + "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/bazel/repository/cache", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/events", diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java index baf64010da31eb..e4e41c13b917e1 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java @@ -26,11 +26,13 @@ import build.bazel.remote.asset.v1.Qualifier; import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.RequestMetadata; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.io.ByteStreams; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; +import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache.KeyType; import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException; @@ -309,13 +311,14 @@ public void testFetchBlobRequest() throws Exception { new URI("http://example.com"), ImmutableMap.of( "Some-Header", ImmutableList.of("some header content"), - "Another-Header", ImmutableList.of("another header content")), + "Another-Header", ImmutableList.of("another header content", "even more header content")), new URI("http://example.org"), - ImmutableMap.of("Org-Header", ImmutableList.of("org header content"))), + ImmutableMap.of("Org-Header", ImmutableList.of("org header content", "and a second one", "and a third one"))), com.google.common.base.Optional.of( Checksum.fromSubresourceIntegrity( "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")), - "canonical ID"); + "canonical ID", + /* includeAllHeaders= */ false); final String expectedAuthHeadersJson = "{" @@ -347,4 +350,57 @@ public void testFetchBlobRequest() throws Exception { .setValue(expectedAuthHeadersJson)) .build()); } + + @Test + public void testFetchBlobRequestWithAllHeaders() throws Exception { + FetchBlobRequest request = + GrpcRemoteDownloader.newFetchBlobRequest( + "instance name", + ImmutableList.of( + new URL("http://example.com/a"), + new URL("http://example.com/b"), + new URL("file:/not/limited/to/http")), + ImmutableMap.of( + new URI("http://example.com"), + ImmutableMap.of( + "Some-Header", ImmutableList.of("some header content"), + "Another-Header", ImmutableList.of("another header content", "even more header content")), + new URI("http://example.org"), + ImmutableMap.of("Org-Header", ImmutableList.of("org header content", "and a second one", "and a third one"))), + com.google.common.base.Optional.of( + Checksum.fromSubresourceIntegrity( + "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")), + "canonical ID", + /* includeAllHeaders= */ true); + + final String expectedAuthHeadersJson = + "{" + + "\"http://example.com\":{" + + "\"Another-Header\":[\"another header content\",\"even more header content\"]," + + "\"Some-Header\":[\"some header content\"]" + + "}," + + "\"http://example.org\":{" + + "\"Org-Header\":[\"org header content\",\"and a second one\",\"and a third one\"]" + + "}" + + "}"; + + assertThat(request) + .isEqualTo( + FetchBlobRequest.newBuilder() + .setInstanceName("instance name") + .addUris("http://example.com/a") + .addUris("http://example.com/b") + .addUris("file:/not/limited/to/http") + .addQualifiers( + Qualifier.newBuilder() + .setName("checksum.sri") + .setValue("sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")) + .addQualifiers( + Qualifier.newBuilder().setName("bazel.canonical_id").setValue("canonical ID")) + .addQualifiers( + Qualifier.newBuilder() + .setName("bazel.auth_headers") + .setValue(expectedAuthHeadersJson)) + .build()); + } } From 53ebd9dbf2c179b228d3cdc59a2b1ae423bc9041 Mon Sep 17 00:00:00 2001 From: Yannic Date: Tue, 20 Sep 2022 23:20:56 +0200 Subject: [PATCH 2/6] Update BUILD --- .../java/com/google/devtools/build/lib/remote/downloader/BUILD | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD index f8243bdf981034..00b0bd0b6ff12c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD @@ -20,7 +20,6 @@ java_library( ], ), deps = [ - "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/bazel/repository/cache", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/events", From 1b662891471f502cb79d1030d65b5facc2ebd93a Mon Sep 17 00:00:00 2001 From: Yannic Date: Tue, 20 Sep 2022 23:21:25 +0200 Subject: [PATCH 3/6] Update BUILD --- .../java/com/google/devtools/build/lib/remote/downloader/BUILD | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD index 0fbc97537bfa57..d8a2e854024fbc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD @@ -14,7 +14,6 @@ java_library( name = "downloader", srcs = glob(["*.java"]), deps = [ - "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/remote:ReferenceCountedChannel", From e00b5603e1d508b9d495693de8de808c9398ea1e Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Tue, 20 Sep 2022 23:34:50 +0200 Subject: [PATCH 4/6] rm imports --- .../build/lib/remote/downloader/GrpcRemoteDownloader.java | 1 - .../build/lib/remote/downloader/GrpcRemoteDownloaderTest.java | 2 -- 2 files changed, 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java index b3a08ad32a4d8f..a85efcaa75aa37 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java @@ -24,7 +24,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.common.collect.Iterables; -import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.bazel.repository.downloader.Downloader; import com.google.devtools.build.lib.bazel.repository.downloader.HashOutputStream; diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java index e4e41c13b917e1..ffd9536fd4949d 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java @@ -26,13 +26,11 @@ import build.bazel.remote.asset.v1.Qualifier; import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.RequestMetadata; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.io.ByteStreams; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; -import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache.KeyType; import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException; From 8f9ff78dd80087acf446dfff86c4da606e5c8e67 Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Tue, 20 Sep 2022 23:36:06 +0200 Subject: [PATCH 5/6] rm todo --- .../build/lib/remote/downloader/GrpcRemoteDownloader.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java index a85efcaa75aa37..bb735e9a27e42a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java @@ -220,10 +220,9 @@ private static String authHeadersJson(Map>> authHe } subObject.add(subEntry.getKey(), values); } else { - // TODO(yannic): Introduce incompatible flag for including all headers, not just the first. String value = Iterables.getFirst(subEntry.getValue(), null); if (value != null) { - subObject.add(subEntry.getKey(), new JsonPrimitive(value)); + subObject.addProperty(subEntry.getKey(), value); } } } From 3dc44e60d875b769db80f28cc79e25f6c1a8a918 Mon Sep 17 00:00:00 2001 From: Yannic Date: Wed, 28 Sep 2022 13:47:54 +0200 Subject: [PATCH 6/6] Update src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java --- .../google/devtools/build/lib/remote/options/RemoteOptions.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 0bb49858c6a09c..db1f85f10ba080 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 @@ -614,7 +614,7 @@ public RemoteOutputsStrategyConverter() { documentationCategory = OptionDocumentationCategory.REMOTE, effectTags = {OptionEffectTag.UNKNOWN}, metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, - help = "Whether to sens all headers to the remote downloader instead of just the first.") + help = "Whether to send all values of a multi-valued header to the remote downloader instead of just the first.") public boolean remoteDownloaderSendAllHeaders; // The below options are not configurable by users, only tests.