Skip to content

Commit 9296068

Browse files
Yanniccopybara-github
Yannic
authored and
copybara-github
committed
[remote/downloader] Don't include headers in FetchBlobRequest
Including the headers in the request is very inefficient as credentials should never change the content of the downloaded archive. In fact, given that Bazel verifies the checksum of the downloaded file, the credentials cannot possibly used in a way where they influence the outcome of the download (other than deciding whether or not the user is allowed to download the blob at all). Hence, the credentials should not be included in the request. Users that need to send credentials to the remote downloader should do so by passing the credentials as metadata to the gRPC call. Note that the remote downloader is behind an experimental flag, so this change does not need to go thorugh the incompatible change process. Closes bazelbuild#16595. PiperOrigin-RevId: 485576157 Change-Id: I8afc7c818e4eed63ac1f70c3e4c2115a1d365830
1 parent 9d250ed commit 9296068

File tree

3 files changed

+3
-141
lines changed

3 files changed

+3
-141
lines changed

‎src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ java_library(
2222
"//src/main/java/com/google/devtools/build/lib/remote/options",
2323
"//src/main/java/com/google/devtools/build/lib/remote/util",
2424
"//src/main/java/com/google/devtools/build/lib/vfs",
25-
"//third_party:gson",
2625
"//third_party:guava",
2726
"//third_party:jsr305",
2827
"//third_party/grpc-java:grpc-jar",

‎src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java‎

Lines changed: 2 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,6 @@
2323
import build.bazel.remote.execution.v2.RequestMetadata;
2424
import com.google.common.annotations.VisibleForTesting;
2525
import com.google.common.base.Strings;
26-
import com.google.common.collect.ImmutableSet;
27-
import com.google.common.collect.Iterables;
2826
import com.google.devtools.build.lib.bazel.repository.downloader.Checksum;
2927
import com.google.devtools.build.lib.bazel.repository.downloader.Downloader;
3028
import com.google.devtools.build.lib.bazel.repository.downloader.HashOutputStream;
@@ -38,9 +36,6 @@
3836
import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
3937
import com.google.devtools.build.lib.remote.util.Utils;
4038
import com.google.devtools.build.lib.vfs.Path;
41-
import com.google.gson.Gson;
42-
import com.google.gson.JsonArray;
43-
import com.google.gson.JsonObject;
4439
import io.grpc.CallCredentials;
4540
import io.grpc.Channel;
4641
import io.grpc.StatusRuntimeException;
@@ -51,7 +46,6 @@
5146
import java.util.List;
5247
import java.util.Map;
5348
import java.util.Optional;
54-
import java.util.TreeMap;
5549
import java.util.concurrent.TimeUnit;
5650
import java.util.concurrent.atomic.AtomicBoolean;
5751
import javax.annotation.Nullable;
@@ -82,7 +76,6 @@ public class GrpcRemoteDownloader implements AutoCloseable, Downloader {
8276
// supported by Bazel.
8377
private static final String QUALIFIER_CHECKSUM_SRI = "checksum.sri";
8478
private static final String QUALIFIER_CANONICAL_ID = "bazel.canonical_id";
85-
private static final String QUALIFIER_AUTH_HEADERS = "bazel.auth_headers";
8679

8780
public GrpcRemoteDownloader(
8881
String buildRequestId,
@@ -131,13 +124,7 @@ public void download(
131124
RemoteActionExecutionContext.create(metadata);
132125

133126
final FetchBlobRequest request =
134-
newFetchBlobRequest(
135-
options.remoteInstanceName,
136-
urls,
137-
authHeaders,
138-
checksum,
139-
canonicalId,
140-
options.remoteDownloaderSendAllHeaders);
127+
newFetchBlobRequest(options.remoteInstanceName, urls, checksum, canonicalId);
141128
try {
142129
FetchBlobResponse response =
143130
retrier.execute(
@@ -175,10 +162,8 @@ public void download(
175162
static FetchBlobRequest newFetchBlobRequest(
176163
String instanceName,
177164
List<URL> urls,
178-
Map<URI, Map<String, List<String>>> authHeaders,
179165
com.google.common.base.Optional<Checksum> checksum,
180-
String canonicalId,
181-
boolean includeAllHeaders) {
166+
String canonicalId) {
182167
FetchBlobRequest.Builder requestBuilder =
183168
FetchBlobRequest.newBuilder().setInstanceName(instanceName);
184169
for (URL url : urls) {
@@ -195,13 +180,6 @@ static FetchBlobRequest newFetchBlobRequest(
195180
requestBuilder.addQualifiers(
196181
Qualifier.newBuilder().setName(QUALIFIER_CANONICAL_ID).setValue(canonicalId).build());
197182
}
198-
if (!authHeaders.isEmpty()) {
199-
requestBuilder.addQualifiers(
200-
Qualifier.newBuilder()
201-
.setName(QUALIFIER_AUTH_HEADERS)
202-
.setValue(authHeadersJson(urls, authHeaders, includeAllHeaders))
203-
.build());
204-
}
205183

206184
return requestBuilder.build();
207185
}
@@ -224,43 +202,4 @@ private OutputStream newOutputStream(
224202
}
225203
return out;
226204
}
227-
228-
private static String authHeadersJson(
229-
List<URL> urls, Map<URI, Map<String, List<String>>> authHeaders, boolean includeAllHeaders) {
230-
ImmutableSet<String> hostSet =
231-
urls.stream().map(URL::getHost).collect(ImmutableSet.toImmutableSet());
232-
Map<String, JsonObject> subObjects = new TreeMap<>();
233-
for (Map.Entry<URI, Map<String, List<String>>> entry : authHeaders.entrySet()) {
234-
URI uri = entry.getKey();
235-
// Only add headers that are relevant to the hosts.
236-
if (!hostSet.contains(uri.getHost())) {
237-
continue;
238-
}
239-
240-
JsonObject subObject = new JsonObject();
241-
Map<String, List<String>> orderedHeaders = new TreeMap<>(entry.getValue());
242-
for (Map.Entry<String, List<String>> subEntry : orderedHeaders.entrySet()) {
243-
if (includeAllHeaders) {
244-
JsonArray values = new JsonArray(subEntry.getValue().size());
245-
for (String value : subEntry.getValue()) {
246-
values.add(value);
247-
}
248-
subObject.add(subEntry.getKey(), values);
249-
} else {
250-
String value = Iterables.getFirst(subEntry.getValue(), null);
251-
if (value != null) {
252-
subObject.addProperty(subEntry.getKey(), value);
253-
}
254-
}
255-
}
256-
subObjects.put(uri.toString(), subObject);
257-
}
258-
259-
JsonObject authHeadersJson = new JsonObject();
260-
for (Map.Entry<String, JsonObject> entry : subObjects.entrySet()) {
261-
authHeadersJson.add(entry.getKey(), entry.getValue());
262-
}
263-
264-
return new Gson().toJson(authHeadersJson);
265-
}
266205
}

‎src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java‎

Lines changed: 1 addition & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -350,82 +350,10 @@ public void testFetchBlobRequest() throws Exception {
350350
new URL("http://example.com/a"),
351351
new URL("http://example.com/b"),
352352
new URL("file:/not/limited/to/http")),
353-
ImmutableMap.of(
354-
new URI("http://example.com"),
355-
ImmutableMap.of(
356-
"Some-Header", ImmutableList.of("some header content"),
357-
"Another-Header",
358-
ImmutableList.of("another header content", "even more header content")),
359-
new URI("http://example.org"),
360-
ImmutableMap.of(
361-
"Org-Header",
362-
ImmutableList.of("org header content", "and a second one", "and a third one"))),
363353
com.google.common.base.Optional.<Checksum>of(
364354
Checksum.fromSubresourceIntegrity(
365355
"sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")),
366-
"canonical ID",
367-
/* includeAllHeaders= */ false);
368-
369-
final String expectedAuthHeadersJson =
370-
"{"
371-
+ "\"http://example.com\":{"
372-
+ "\"Another-Header\":\"another header content\","
373-
+ "\"Some-Header\":\"some header content\""
374-
+ "}"
375-
+ "}";
376-
377-
assertThat(request)
378-
.isEqualTo(
379-
FetchBlobRequest.newBuilder()
380-
.setInstanceName("instance name")
381-
.addUris("http://example.com/a")
382-
.addUris("http://example.com/b")
383-
.addUris("file:/not/limited/to/http")
384-
.addQualifiers(
385-
Qualifier.newBuilder()
386-
.setName("checksum.sri")
387-
.setValue("sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="))
388-
.addQualifiers(
389-
Qualifier.newBuilder().setName("bazel.canonical_id").setValue("canonical ID"))
390-
.addQualifiers(
391-
Qualifier.newBuilder()
392-
.setName("bazel.auth_headers")
393-
.setValue(expectedAuthHeadersJson))
394-
.build());
395-
}
396-
397-
@Test
398-
public void testFetchBlobRequestWithAllHeaders() throws Exception {
399-
FetchBlobRequest request =
400-
GrpcRemoteDownloader.newFetchBlobRequest(
401-
"instance name",
402-
ImmutableList.of(
403-
new URL("http://example.com/a"),
404-
new URL("http://example.com/b"),
405-
new URL("file:/not/limited/to/http")),
406-
ImmutableMap.of(
407-
new URI("http://example.com"),
408-
ImmutableMap.of(
409-
"Some-Header", ImmutableList.of("some header content"),
410-
"Another-Header",
411-
ImmutableList.of("another header content", "even more header content")),
412-
new URI("http://example.org"),
413-
ImmutableMap.of(
414-
"Org-Header",
415-
ImmutableList.of("org header content", "and a second one", "and a third one"))),
416-
com.google.common.base.Optional.<Checksum>of(
417-
Checksum.fromSubresourceIntegrity(
418-
"sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")),
419-
"canonical ID",
420-
/* includeAllHeaders= */ true);
421-
422-
final String expectedAuthHeadersJson =
423-
"{"
424-
+ "\"http://example.com\":{"
425-
+ "\"Another-Header\":[\"another header content\",\"even more header content\"],"
426-
+ "\"Some-Header\":[\"some header content\"]"
427-
+ "}"
428-
+ "}";
356+
"canonical ID");
429357

430358
assertThat(request)
431359
.isEqualTo(
@@ -440,10 +368,6 @@ public void testFetchBlobRequestWithAllHeaders() throws Exception {
440368
.setValue("sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="))
441369
.addQualifiers(
442370
Qualifier.newBuilder().setName("bazel.canonical_id").setValue("canonical ID"))
443-
.addQualifiers(
444-
Qualifier.newBuilder()
445-
.setName("bazel.auth_headers")
446-
.setValue(expectedAuthHeadersJson))
447371
.build());
448372
}
449373
}

0 commit comments

Comments
 (0)