From 38c501912fc4efc14abc0741d19f5f8e8763afcb Mon Sep 17 00:00:00 2001 From: Yannic Date: Thu, 10 Nov 2022 16:25:41 +0100 Subject: [PATCH] [remote/downloader] Migrate `Downloader` to take `Credentials` (#16732) Progress on https://github.com/bazelbuild/bazel/issues/15856 Closes #16601. PiperOrigin-RevId: 485837451 Change-Id: I785882d0ff3e171dcaee6aa6f628bca9232c730a --- .../lib/authandtls/StaticCredentials.java | 62 +++++++++++++++++++ .../downloader/DelegatingDownloader.java | 6 +- .../downloader/DownloadManager.java | 5 +- .../repository/downloader/Downloader.java | 6 +- .../downloader/HttpConnectorMultiplexer.java | 37 +++++------ .../repository/downloader/HttpDownloader.java | 10 +-- .../build/lib/remote/downloader/BUILD | 1 + .../downloader/GrpcRemoteDownloader.java | 6 +- .../HttpConnectorMultiplexerTest.java | 7 ++- .../downloader/HttpDownloaderTest.java | 11 ++-- .../build/lib/remote/downloader/BUILD | 1 + .../downloader/GrpcRemoteDownloaderTest.java | 5 +- 12 files changed, 111 insertions(+), 46 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/authandtls/StaticCredentials.java diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/StaticCredentials.java b/src/main/java/com/google/devtools/build/lib/authandtls/StaticCredentials.java new file mode 100644 index 00000000000000..ada6dc040bd750 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/authandtls/StaticCredentials.java @@ -0,0 +1,62 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.authandtls; + +import com.google.auth.Credentials; +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; +import java.net.URI; +import java.util.List; +import java.util.Map; + +/** Implementation of {@link Credentials} which provides a static set of credentials. */ +public final class StaticCredentials extends Credentials { + public static final StaticCredentials EMPTY = new StaticCredentials(ImmutableMap.of()); + + private final ImmutableMap>> credentials; + + public StaticCredentials(Map>> credentials) { + Preconditions.checkNotNull(credentials); + + this.credentials = ImmutableMap.copyOf(credentials); + } + + @Override + public String getAuthenticationType() { + return "static"; + } + + @Override + public Map> getRequestMetadata(URI uri) { + Preconditions.checkNotNull(uri); + + return credentials.getOrDefault(uri, ImmutableMap.of()); + } + + @Override + public boolean hasRequestMetadata() { + return true; + } + + @Override + public boolean hasRequestMetadataOnly() { + return true; + } + + @Override + public void refresh() { + // Can't refresh static credentials. + } +} diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DelegatingDownloader.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DelegatingDownloader.java index 2527c48783a5b2..e9f83c4e0d8ced 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DelegatingDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DelegatingDownloader.java @@ -14,11 +14,11 @@ package com.google.devtools.build.lib.bazel.repository.downloader; +import com.google.auth.Credentials; import com.google.common.base.Optional; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.vfs.Path; import java.io.IOException; -import java.net.URI; import java.net.URL; import java.util.List; import java.util.Map; @@ -47,7 +47,7 @@ public void setDelegate(@Nullable Downloader delegate) { @Override public void download( List urls, - Map>> authHeaders, + Credentials credentials, Optional checksum, String canonicalId, Path destination, @@ -60,6 +60,6 @@ public void download( downloader = delegate; } downloader.download( - urls, authHeaders, checksum, canonicalId, destination, eventHandler, clientEnv, type); + urls, credentials, checksum, canonicalId, destination, eventHandler, clientEnv, type); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java index bf65090e936fb3..05250dc08608a1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java @@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.authandtls.StaticCredentials; import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache; import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache.KeyType; import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCacheHitEvent; @@ -256,7 +257,7 @@ public Path download( try { downloader.download( rewrittenUrls, - rewrittenAuthHeaders, + new StaticCredentials(rewrittenAuthHeaders), checksum, canonicalId, destination, @@ -337,7 +338,7 @@ public byte[] downloadAndReadOneUrl( for (int attempt = 0; attempt <= retries; ++attempt) { try { return httpDownloader.downloadAndReadOneUrl( - rewrittenUrls.get(0), authHeaders, eventHandler, clientEnv); + rewrittenUrls.get(0), new StaticCredentials(authHeaders), eventHandler, clientEnv); } catch (ContentLengthMismatchException e) { if (attempt == retries) { throw e; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Downloader.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Downloader.java index 0ee4fea434deca..7124f50e45e008 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Downloader.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Downloader.java @@ -14,11 +14,11 @@ package com.google.devtools.build.lib.bazel.repository.downloader; +import com.google.auth.Credentials; import com.google.common.base.Optional; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.vfs.Path; import java.io.IOException; -import java.net.URI; import java.net.URL; import java.util.List; import java.util.Map; @@ -33,7 +33,7 @@ public interface Downloader { * caller is responsible for cleaning up outputs of failed downloads. * * @param urls list of mirror URLs with identical content - * @param authHeaders map of authentication headers per URL + * @param credentials credentials to use when connecting to URLs * @param checksum valid checksum which is checked, or absent to disable * @param output path to the destination file to write * @param type extension, e.g. "tar.gz" to force on downloaded filename, or empty to not do this @@ -42,7 +42,7 @@ public interface Downloader { */ void download( List urls, - Map>> authHeaders, + Credentials credentials, Optional checksum, String canonicalId, Path output, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java index 8cff89b45c2cd2..d9ba40058b51e3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.bazel.repository.downloader; +import com.google.auth.Credentials; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.base.Optional; @@ -21,13 +22,13 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.BlazeVersionInfo; +import com.google.devtools.build.lib.authandtls.StaticCredentials; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import java.io.IOException; import java.io.InputStream; import java.io.InterruptedIOException; -import java.net.URI; import java.net.URISyntaxException; import java.net.URL; import java.net.URLConnection; @@ -74,7 +75,7 @@ final class HttpConnectorMultiplexer { } public HttpStream connect(URL url, Optional checksum) throws IOException { - return connect(url, checksum, ImmutableMap.of(), Optional.absent()); + return connect(url, checksum, StaticCredentials.EMPTY, Optional.absent()); } /** @@ -87,7 +88,7 @@ public HttpStream connect(URL url, Optional checksum) throws IOExcepti * * @param url the URL to conenct to. can be: file, http, or https * @param checksum checksum lazily checked on entire payload, or empty to disable - * @param authHeaders the authentication headers + * @param credentials the credentials * @param type extension, e.g. "tar.gz" to force on downloaded filename, or empty to not do this * @return an {@link InputStream} of response payload * @throws IOException if all mirrors are down and contains suppressed exception of each attempt @@ -95,17 +96,14 @@ public HttpStream connect(URL url, Optional checksum) throws IOExcepti * @throws IllegalArgumentException if {@code urls} is empty or has an unsupported protocol */ public HttpStream connect( - URL url, - Optional checksum, - Map>> authHeaders, - Optional type) + URL url, Optional checksum, Credentials credentials, Optional type) throws IOException { Preconditions.checkArgument(HttpUtils.isUrlSupportedByDownloader(url)); if (Thread.interrupted()) { throw new InterruptedIOException(); } Function>> headerFunction = - getHeaderFunction(REQUEST_HEADERS, authHeaders); + getHeaderFunction(REQUEST_HEADERS, credentials); URLConnection connection = connector.connect(url, headerFunction); return httpStreamFactory.create( connection, @@ -127,21 +125,20 @@ public HttpStream connect( @VisibleForTesting static Function>> getHeaderFunction( - Map> baseHeaders, - Map>> additionalHeaders) { + Map> baseHeaders, Credentials credentials) { + Preconditions.checkNotNull(baseHeaders); + Preconditions.checkNotNull(credentials); + return url -> { - ImmutableMap> headers = ImmutableMap.copyOf(baseHeaders); + Map> headers = new HashMap<>(baseHeaders); try { - if (additionalHeaders.containsKey(url.toURI())) { - Map> newHeaders = new HashMap<>(headers); - newHeaders.putAll(additionalHeaders.get(url.toURI())); - headers = ImmutableMap.copyOf(newHeaders); - } - } catch (URISyntaxException e) { - // If we can't convert the URL to a URI (because it is syntactically malformed), still try - // to do the connection, not adding authentication information as we cannot look it up. + headers.putAll(credentials.getRequestMetadata(url.toURI())); + } catch (URISyntaxException | IOException e) { + // If we can't convert the URL to a URI (because it is syntactically malformed), or fetching + // credentials fails for any other reason, still try to do the connection, not adding + // authentication information as we cannot look it up. } - return headers; + return ImmutableMap.copyOf(headers); }; } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java index 79ab9091c08639..78b321600b503e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.bazel.repository.downloader; +import com.google.auth.Credentials; import com.google.common.base.Optional; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; @@ -31,7 +32,6 @@ import java.io.InterruptedIOException; import java.io.OutputStream; import java.net.SocketTimeoutException; -import java.net.URI; import java.net.URL; import java.util.ArrayList; import java.util.List; @@ -63,7 +63,7 @@ public void setTimeoutScaling(float timeoutScaling) { @Override public void download( List urls, - Map>> authHeaders, + Credentials credentials, Optional checksum, String canonicalId, Path destination, @@ -82,7 +82,7 @@ public void download( for (URL url : urls) { SEMAPHORE.acquire(); - try (HttpStream payload = multiplexer.connect(url, checksum, authHeaders, type); + try (HttpStream payload = multiplexer.connect(url, checksum, credentials, type); OutputStream out = destination.getOutputStream()) { try { ByteStreams.copy(payload, out); @@ -132,7 +132,7 @@ public void download( /** Downloads the contents of one URL and reads it into a byte array. */ public byte[] downloadAndReadOneUrl( URL url, - Map>> authHeaders, + Credentials credentials, ExtendedEventHandler eventHandler, Map clientEnv) throws IOException, InterruptedException { @@ -141,7 +141,7 @@ public byte[] downloadAndReadOneUrl( ByteArrayOutputStream out = new ByteArrayOutputStream(); SEMAPHORE.acquire(); try (HttpStream payload = - multiplexer.connect(url, Optional.absent(), authHeaders, Optional.absent())) { + multiplexer.connect(url, Optional.absent(), credentials, Optional.absent())) { ByteStreams.copy(payload, out); } catch (SocketTimeoutException e) { // SocketTimeoutExceptions are InterruptedIOExceptions; however they do not signify 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 1804656458feb5..50e7658687fd34 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 @@ -22,6 +22,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/remote/util", "//src/main/java/com/google/devtools/build/lib/vfs", + "//third_party:auth", "//third_party:guava", "//third_party:jsr305", "//third_party/grpc-java:grpc-jar", 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 be7f5411ed8573..35350585105559 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 @@ -21,6 +21,7 @@ import build.bazel.remote.asset.v1.Qualifier; import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.RequestMetadata; +import com.google.auth.Credentials; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; @@ -41,7 +42,6 @@ import io.grpc.StatusRuntimeException; import java.io.IOException; import java.io.OutputStream; -import java.net.URI; import java.net.URL; import java.util.List; import java.util.Map; @@ -110,7 +110,7 @@ public void close() { @Override public void download( List urls, - Map>> authHeaders, + Credentials credentials, com.google.common.base.Optional checksum, String canonicalId, Path destination, @@ -154,7 +154,7 @@ public void download( eventHandler.handle( Event.warn("Remote Cache: " + Utils.grpcAwareErrorMessage(e, verboseFailures))); fallbackDownloader.download( - urls, authHeaders, checksum, canonicalId, destination, eventHandler, clientEnv, type); + urls, credentials, checksum, canonicalId, destination, eventHandler, clientEnv, type); } } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java index 4d61114c1e990d..5c52f32a40e656 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java @@ -32,6 +32,7 @@ import com.google.common.base.Optional; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.authandtls.StaticCredentials; import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache.KeyType; import com.google.devtools.build.lib.bazel.repository.downloader.RetryingInputStream.Reconnector; import com.google.devtools.build.lib.events.EventHandler; @@ -163,7 +164,8 @@ public void testHeaderComputationFunction() throws Exception { ImmutableMap.of("Authentication", ImmutableList.of("Zm9vOmZvb3NlY3JldA=="))); Function>> headerFunction = - HttpConnectorMultiplexer.getHeaderFunction(baseHeaders, additionalHeaders); + HttpConnectorMultiplexer.getHeaderFunction( + baseHeaders, new StaticCredentials(additionalHeaders)); // Unreleated URL assertThat(headerFunction.apply(new URL("http://example.org/some/path/file.txt"))) @@ -215,7 +217,8 @@ public void testHeaderComputationFunction() throws Exception { ImmutableMap> annonAuth = ImmutableMap.of("Authentication", ImmutableList.of("YW5vbnltb3VzOmZvb0BleGFtcGxlLm9yZw==")); Function>> combinedHeaders = - HttpConnectorMultiplexer.getHeaderFunction(annonAuth, additionalHeaders); + HttpConnectorMultiplexer.getHeaderFunction( + annonAuth, new StaticCredentials(additionalHeaders)); assertThat(combinedHeaders.apply(new URL("http://hosting.example.com/user/foo/file.txt"))) .containsExactly("Authentication", ImmutableList.of("Zm9vOmZvb3NlY3JldA==")); assertThat(combinedHeaders.apply(new URL("http://unreleated.example.org/user/foo/file.txt"))) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java index 82d89662e82389..ee5378ead6c760 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java @@ -28,6 +28,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.io.ByteStreams; +import com.google.devtools.build.lib.authandtls.StaticCredentials; import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -370,7 +371,7 @@ public void downloadOneUrl_ok() throws IOException, InterruptedException { httpDownloader.download( Collections.singletonList( new URL(String.format("http://localhost:%d/foo", server.getLocalPort()))), - Collections.emptyMap(), + StaticCredentials.EMPTY, Optional.absent(), "testCanonicalId", destination, @@ -409,7 +410,7 @@ public void downloadOneUrl_notFound() throws IOException, InterruptedException { httpDownloader.download( Collections.singletonList( new URL(String.format("http://localhost:%d/foo", server.getLocalPort()))), - Collections.emptyMap(), + StaticCredentials.EMPTY, Optional.absent(), "testCanonicalId", fs.getPath(workingDir.newFile().getAbsolutePath()), @@ -469,7 +470,7 @@ public void downloadTwoUrls_firstNotFoundAndSecondOk() throws IOException, Inter Path destination = fs.getPath(workingDir.newFile().getAbsolutePath()); httpDownloader.download( urls, - Collections.emptyMap(), + StaticCredentials.EMPTY, Optional.absent(), "testCanonicalId", destination, @@ -507,7 +508,7 @@ public void downloadAndReadOneUrl_ok() throws IOException, InterruptedException new String( httpDownloader.downloadAndReadOneUrl( new URL(String.format("http://localhost:%d/foo", server.getLocalPort())), - ImmutableMap.of(), + StaticCredentials.EMPTY, eventHandler, Collections.emptyMap()), UTF_8)) @@ -542,7 +543,7 @@ public void downloadAndReadOneUrl_notFound() throws IOException, InterruptedExce () -> httpDownloader.downloadAndReadOneUrl( new URL(String.format("http://localhost:%d/foo", server.getLocalPort())), - ImmutableMap.of(), + StaticCredentials.EMPTY, eventHandler, Collections.emptyMap())); } 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 296149935450e7..c424a99cf5f863 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 c08093d8b4f081..1ca3bd175f69e0 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 @@ -33,6 +33,7 @@ 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.StaticCredentials; 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.Downloader; @@ -66,7 +67,6 @@ import io.reactivex.rxjava3.core.Single; import java.io.IOException; import java.io.InputStream; -import java.net.URI; import java.net.URL; import java.util.List; import java.util.Map; @@ -172,7 +172,6 @@ private static byte[] downloadBlob( guavaChecksum = com.google.common.base.Optional.of(checksum.get()); } - final ImmutableMap>> authHeaders = ImmutableMap.of(); final String canonicalId = ""; final ExtendedEventHandler eventHandler = mock(ExtendedEventHandler.class); final Map clientEnv = ImmutableMap.of(); @@ -181,7 +180,7 @@ private static byte[] downloadBlob( final Path destination = scratch.resolve("output file path"); downloader.download( urls, - authHeaders, + StaticCredentials.EMPTY, guavaChecksum, canonicalId, destination,