diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java b/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java index f648038182e026..e3b4036858047b 100644 --- a/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java +++ b/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java @@ -174,7 +174,11 @@ public class AuthAndTLSOptions extends OptionsBase { converter = DurationConverter.class, documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, effectTags = {OptionEffectTag.UNKNOWN}, - help = "Configures the duration for which credentials from Credential Helpers are cached.") + help = + "Configures the duration for which credentials from Credential Helpers are cached.\n\n" + + "Invoking with a different value will adjust the lifetime of preexisting entries;" + + " pass zero to clear the cache. A clean command always clears the cache, regardless" + + " of this flag.") public Duration credentialHelperCacheTimeout; /** One of the values of the `--credential_helper` flag. */ diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/BUILD b/src/main/java/com/google/devtools/build/lib/authandtls/BUILD index 28d266de25043f..32f6e7f95e214e 100644 --- a/src/main/java/com/google/devtools/build/lib/authandtls/BUILD +++ b/src/main/java/com/google/devtools/build/lib/authandtls/BUILD @@ -22,6 +22,7 @@ java_library( "//src/main/java/com/google/devtools/common/options", "//third_party:auth", "//third_party:auto_value", + "//third_party:caffeine", "//third_party:guava", "//third_party:jsr305", "//third_party:netty", diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/GoogleAuthUtils.java b/src/main/java/com/google/devtools/build/lib/authandtls/GoogleAuthUtils.java index 93c80f5396576c..47ad9d92982988 100644 --- a/src/main/java/com/google/devtools/build/lib/authandtls/GoogleAuthUtils.java +++ b/src/main/java/com/google/devtools/build/lib/authandtls/GoogleAuthUtils.java @@ -14,11 +14,14 @@ package com.google.devtools.build.lib.authandtls; +import com.github.benmanes.caffeine.cache.Cache; import com.google.auth.Credentials; import com.google.auth.oauth2.GoogleCredentials; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperCredentials; import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperEnvironment; import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperProvider; @@ -47,6 +50,7 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; +import java.net.URI; import java.util.List; import java.util.Map; import java.util.Optional; @@ -235,6 +239,7 @@ public static CallCredentialsProvider newCallCredentialsProvider(@Nullable Crede */ public static Credentials newCredentials( CredentialHelperEnvironment credentialHelperEnvironment, + Cache>> credentialCache, CommandLinePathFactory commandLinePathFactory, FileSystem fileSystem, AuthAndTLSOptions authAndTlsOptions) @@ -244,12 +249,12 @@ public static Credentials newCredentials( Preconditions.checkNotNull(fileSystem); Preconditions.checkNotNull(authAndTlsOptions); - Optional credentials = newGoogleCredentials(authAndTlsOptions); + Optional fallbackCredentials = newGoogleCredentials(authAndTlsOptions); - if (credentials.isEmpty()) { + if (fallbackCredentials.isEmpty()) { // Fallback to .netrc if it exists. try { - credentials = + fallbackCredentials = newCredentialsFromNetrc(credentialHelperEnvironment.getClientEnvironment(), fileSystem); } catch (IOException e) { // TODO(yannic): Make this fail the build. @@ -263,8 +268,8 @@ public static Credentials newCredentials( commandLinePathFactory, authAndTlsOptions.credentialHelpers), credentialHelperEnvironment, - credentials, - authAndTlsOptions.credentialHelperCacheTimeout); + credentialCache, + fallbackCredentials); } /** diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/BUILD b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/BUILD index 62a58d7a82eea4..46b0a6df7b1e17 100644 --- a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/BUILD +++ b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/BUILD @@ -10,9 +10,23 @@ filegroup( visibility = ["//src:__subpackages__"], ) +java_library( + name = "credential_module", + srcs = ["CredentialModule.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib:runtime", + "//src/main/java/com/google/devtools/build/lib/authandtls", + "//third_party:caffeine", + "//third_party:guava", + ], +) + java_library( name = "credentialhelper", - srcs = glob(["*.java"]), + srcs = glob( + ["*.java"], + exclude = ["CredentialModule.java"], + ), deps = [ "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/profiler", diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java index 2219a595741fa4..c1f0a09f12bb7f 100644 --- a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java +++ b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java @@ -67,7 +67,7 @@ public Path getPath() { * @return The response from the subprocess. */ public GetCredentialsResponse getCredentials(CredentialHelperEnvironment environment, URI uri) - throws InterruptedException, IOException { + throws IOException { Preconditions.checkNotNull(environment); Preconditions.checkNotNull(uri); @@ -81,7 +81,16 @@ public GetCredentialsResponse getCredentials(CredentialHelperEnvironment environ GSON.toJson(GetCredentialsRequest.newBuilder().setUri(uri).build(), stdin); } - process.waitFor(); + try { + process.waitFor(); + } catch (InterruptedException e) { + throw new CredentialHelperException( + String.format( + Locale.US, + "Failed to get credentials for '%s' from helper '%s': process was interrupted", + uri, + path)); + } if (process.timedout()) { throw new CredentialHelperException( diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperCredentials.java b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperCredentials.java index ecc40e15707d8d..5de760857dd1bf 100644 --- a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperCredentials.java +++ b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperCredentials.java @@ -14,15 +14,13 @@ package com.google.devtools.build.lib.authandtls.credentialhelper; -import com.github.benmanes.caffeine.cache.CacheLoader; -import com.github.benmanes.caffeine.cache.Caffeine; -import com.github.benmanes.caffeine.cache.LoadingCache; +import com.github.benmanes.caffeine.cache.Cache; import com.google.auth.Credentials; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.io.IOException; import java.net.URI; -import java.time.Duration; import java.util.List; import java.util.Map; import java.util.Optional; @@ -33,29 +31,34 @@ * helper} as subprocess, falling back to another {@link Credentials} if no suitable helper exists. */ public class CredentialHelperCredentials extends Credentials { + private final CredentialHelperProvider credentialHelperProvider; + private final CredentialHelperEnvironment credentialHelperEnvironment; + private final Cache>> credentialCache; private final Optional fallbackCredentials; - private final LoadingCache credentialCache; + /** Wraps around an {@link IOException} so we can smuggle it through {@link Cache#get}. */ + public static final class WrappedIOException extends RuntimeException { + private final IOException wrapped; + + WrappedIOException(IOException e) { + super(e); + this.wrapped = e; + } + + IOException getWrapped() { + return wrapped; + } + } public CredentialHelperCredentials( CredentialHelperProvider credentialHelperProvider, CredentialHelperEnvironment credentialHelperEnvironment, - Optional fallbackCredentials, - Duration cacheTimeout) { - Preconditions.checkNotNull(credentialHelperProvider); - Preconditions.checkNotNull(credentialHelperEnvironment); + Cache>> credentialCache, + Optional fallbackCredentials) { + this.credentialHelperProvider = Preconditions.checkNotNull(credentialHelperProvider); + this.credentialHelperEnvironment = Preconditions.checkNotNull(credentialHelperEnvironment); + this.credentialCache = Preconditions.checkNotNull(credentialCache); this.fallbackCredentials = Preconditions.checkNotNull(fallbackCredentials); - Preconditions.checkNotNull(cacheTimeout); - Preconditions.checkArgument( - !cacheTimeout.isNegative() && !cacheTimeout.isZero(), - "Cache timeout must be greater than 0"); - - credentialCache = - Caffeine.newBuilder() - .expireAfterWrite(cacheTimeout) - .build( - new CredentialHelperCacheLoader( - credentialHelperProvider, credentialHelperEnvironment)); } @Override @@ -68,12 +71,18 @@ public String getAuthenticationType() { } @Override + @SuppressWarnings("unchecked") // Map> to Map> public Map> getRequestMetadata(URI uri) throws IOException { Preconditions.checkNotNull(uri); - Optional>> credentials = getRequestMetadataFromCredentialHelper(uri); - if (credentials.isPresent()) { - return credentials.get(); + ImmutableMap> credentials; + try { + credentials = credentialCache.get(uri, this::getCredentialsFromHelper); + } catch (WrappedIOException e) { + throw e.getWrapped(); + } + if (credentials != null) { + return (Map) credentials; } if (fallbackCredentials.isPresent()) { @@ -83,13 +92,28 @@ public Map> getRequestMetadata(URI uri) throws IOException return ImmutableMap.of(); } - @SuppressWarnings("unchecked") // Map> to Map> - private Optional>> getRequestMetadataFromCredentialHelper(URI uri) { + @Nullable + private ImmutableMap> getCredentialsFromHelper(URI uri) { Preconditions.checkNotNull(uri); - GetCredentialsResponse response = credentialCache.get(uri); + Optional maybeCredentialHelper = + credentialHelperProvider.findCredentialHelper(uri); + if (maybeCredentialHelper.isEmpty()) { + return null; + } + CredentialHelper credentialHelper = maybeCredentialHelper.get(); + + GetCredentialsResponse response; + try { + response = credentialHelper.getCredentials(credentialHelperEnvironment, uri); + } catch (IOException e) { + throw new WrappedIOException(e); + } + if (response == null) { + return null; + } - return Optional.ofNullable(response).map(value -> (Map) value.getHeaders()); + return response.getHeaders(); } @Override @@ -110,32 +134,4 @@ public void refresh() throws IOException { credentialCache.invalidateAll(); } - - private static final class CredentialHelperCacheLoader - implements CacheLoader { - private final CredentialHelperProvider credentialHelperProvider; - private final CredentialHelperEnvironment credentialHelperEnvironment; - - public CredentialHelperCacheLoader( - CredentialHelperProvider credentialHelperProvider, - CredentialHelperEnvironment credentialHelperEnvironment) { - this.credentialHelperProvider = Preconditions.checkNotNull(credentialHelperProvider); - this.credentialHelperEnvironment = Preconditions.checkNotNull(credentialHelperEnvironment); - } - - @Nullable - @Override - public GetCredentialsResponse load(URI uri) throws IOException, InterruptedException { - Preconditions.checkNotNull(uri); - - Optional maybeCredentialHelper = - credentialHelperProvider.findCredentialHelper(uri); - if (maybeCredentialHelper.isEmpty()) { - return null; - } - CredentialHelper credentialHelper = maybeCredentialHelper.get(); - - return credentialHelper.getCredentials(credentialHelperEnvironment, uri); - } - } } diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialModule.java b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialModule.java new file mode 100644 index 00000000000000..95af2af5fcced0 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialModule.java @@ -0,0 +1,52 @@ +// 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.credentialhelper; + +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; +import com.google.devtools.build.lib.runtime.BlazeModule; +import com.google.devtools.build.lib.runtime.CommandEnvironment; +import java.net.URI; +import java.time.Duration; + +/** A module whose sole purpose is to hold the credential cache which is shared by other modules. */ +public class CredentialModule extends BlazeModule { + private final Cache>> credentialCache = + Caffeine.newBuilder().expireAfterWrite(Duration.ZERO).build(); + + /** Returns the credential cache. */ + public Cache>> getCredentialCache() { + return credentialCache; + } + + @Override + public void beforeCommand(CommandEnvironment env) { + // Update the cache expiration policy according to the command options. + AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class); + credentialCache + .policy() + .expireAfterWrite() + .get() + .setExpiresAfter(authAndTlsOptions.credentialHelperCacheTimeout); + + // Clear the cache on clean. + if (env.getCommand().name().equals("clean")) { + credentialCache.invalidateAll(); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/BUILD index 968c540f26c1dd..51496d1d95d273 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/BUILD @@ -131,6 +131,7 @@ java_library( ":spawn_log_module", "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info", + "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module", "//src/main/java/com/google/devtools/build/lib/bazel/coverage", "//src/main/java/com/google/devtools/build/lib/bazel/debug:workspace-rule-module", "//src/main/java/com/google/devtools/build/lib/bazel/repository", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/Bazel.java b/src/main/java/com/google/devtools/build/lib/bazel/Bazel.java index c6c4df23b2aae3..86af64f80c09a1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/Bazel.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/Bazel.java @@ -17,6 +17,7 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.BlazeVersionInfo; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryDebugModule; +import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.BlazeRuntime; import java.io.IOException; @@ -42,6 +43,8 @@ public final class Bazel { // This module needs to be registered before any module providing a SpawnCache // implementation. com.google.devtools.build.lib.runtime.NoSpawnCacheModule.class, + // This module needs to be registered before any module that uses the credential cache. + CredentialModule.class, com.google.devtools.build.lib.runtime.CommandLogModule.class, com.google.devtools.build.lib.platform.SleepPreventionModule.class, com.google.devtools.build.lib.platform.SystemSuspensionModule.class, diff --git a/src/main/java/com/google/devtools/build/lib/buildeventservice/BUILD b/src/main/java/com/google/devtools/build/lib/buildeventservice/BUILD index 26f91c3091e532..4f7e9f17d91687 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventservice/BUILD +++ b/src/main/java/com/google/devtools/build/lib/buildeventservice/BUILD @@ -37,9 +37,11 @@ java_library( deps = [ ":buildeventservice-options", "//src/main/java/com/google/devtools/build/lib:runtime", + "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper", + "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module", "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/buildeventservice/client", "//src/main/java/com/google/devtools/build/lib/buildeventstream", diff --git a/src/main/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModule.java b/src/main/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModule.java index c0f6b5e7b30125..cd1e0ac1f94ec3 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModule.java +++ b/src/main/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModule.java @@ -22,12 +22,16 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.authandtls.GoogleAuthUtils; import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperEnvironment; +import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule; import com.google.devtools.build.lib.buildeventservice.client.BuildEventServiceClient; import com.google.devtools.build.lib.buildeventservice.client.BuildEventServiceGrpcClient; +import com.google.devtools.build.lib.runtime.BlazeRuntime; import com.google.devtools.build.lib.runtime.CommandEnvironment; +import com.google.devtools.build.lib.runtime.WorkspaceBuilder; import io.grpc.ClientInterceptor; import io.grpc.ManagedChannel; import io.grpc.Metadata; @@ -68,6 +72,15 @@ static BackendConfig create( private BuildEventServiceClient client; private BackendConfig config; + private CredentialModule credentialModule; + + @Override + public void workspaceInit( + BlazeRuntime runtime, BlazeDirectories directories, WorkspaceBuilder builder) { + Preconditions.checkState(credentialModule == null, "credentialModule must be null"); + credentialModule = Preconditions.checkNotNull(runtime.getBlazeModule(CredentialModule.class)); + } + @Override protected Class optionsClass() { return BuildEventServiceOptions.class; @@ -93,6 +106,7 @@ protected BuildEventServiceClient getBesClient( .setClientEnvironment(env.getClientEnv()) .setHelperExecutionTimeout(authAndTLSOptions.credentialHelperTimeout) .build(), + credentialModule.getCredentialCache(), env.getCommandLinePathFactory(), env.getRuntime().getFileSystem(), newConfig.authAndTLSOptions()); diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index 9a08c88a75084e..c101a40aee46f9 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -53,12 +53,14 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/actions:fileset_output_symlink", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", + "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context", "//src/main/java/com/google/devtools/build/lib/analysis/platform:platform_utils", "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper", + "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/buildeventstream", "//src/main/java/com/google/devtools/build/lib/clock", 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 2a60fbc3d77322..1ad9a82c362964 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 @@ -18,6 +18,8 @@ import build.bazel.remote.execution.v2.DigestFunction; import build.bazel.remote.execution.v2.ServerCapabilities; +import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy; +import com.github.benmanes.caffeine.cache.Cache; import com.google.auth.Credentials; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Ascii; @@ -27,6 +29,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.flogger.GoogleLogger; +import com.google.common.collect.ImmutableMap; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.ThreadFactoryBuilder; @@ -36,6 +39,7 @@ import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.AnalysisResult; +import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.RunfilesSupport; @@ -48,6 +52,7 @@ import com.google.devtools.build.lib.authandtls.CallCredentialsProvider; import com.google.devtools.build.lib.authandtls.GoogleAuthUtils; import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperEnvironment; +import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule; import com.google.devtools.build.lib.bazel.repository.downloader.Downloader; import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; import com.google.devtools.build.lib.buildeventstream.LocalFilesArtifactUploader; @@ -72,6 +77,7 @@ import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.runtime.BlazeModule; +import com.google.devtools.build.lib.runtime.BlazeRuntime; import com.google.devtools.build.lib.runtime.BuildEventArtifactUploaderFactory; import com.google.devtools.build.lib.runtime.Command; import com.google.devtools.build.lib.runtime.CommandEnvironment; @@ -79,6 +85,7 @@ import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutorFactory; import com.google.devtools.build.lib.runtime.ServerBuilder; +import com.google.devtools.build.lib.runtime.WorkspaceBuilder; import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution; @@ -156,6 +163,8 @@ public ManagedChannel newChannel( private final MutableSupplier remoteDownloaderSupplier = new MutableSupplier<>(); + private CredentialModule credentialModule; + @Override public void serverInit(OptionsParsingResult startupOptions, ServerBuilder builder) { builder.addBuildEventArtifactUploaderFactory( @@ -164,12 +173,16 @@ public void serverInit(OptionsParsingResult startupOptions, ServerBuilder builde builder.setDownloaderSupplier(remoteDownloaderSupplier); } - /** Returns whether remote execution should be available. */ + /** + * Returns whether remote execution should be available. + */ public static boolean shouldEnableRemoteExecution(RemoteOptions options) { return !Strings.isNullOrEmpty(options.remoteExecutor); } - /** Returns whether remote downloading should be available. */ + /** + * Returns whether remote downloading should be available. + */ private static boolean shouldEnableRemoteDownloader(RemoteOptions options) { return !Strings.isNullOrEmpty(options.remoteDownloader); } @@ -207,33 +220,16 @@ private static void verifyServerCapabilities( private void initHttpAndDiskCache( CommandEnvironment env, + Credentials credentials, AuthAndTLSOptions authAndTlsOptions, RemoteOptions remoteOptions, DigestUtil digestUtil) { - Credentials creds; - try { - creds = - newCredentials( - CredentialHelperEnvironment.newBuilder() - .setEventReporter(env.getReporter()) - .setWorkspacePath(env.getWorkspace()) - .setClientEnvironment(env.getClientEnv()) - .setHelperExecutionTimeout(authAndTlsOptions.credentialHelperTimeout) - .build(), - env.getCommandLinePathFactory(), - env.getRuntime().getFileSystem(), - authAndTlsOptions, - remoteOptions); - } catch (IOException e) { - handleInitFailure(env, e, Code.CREDENTIALS_INIT_FAILURE); - return; - } RemoteCacheClient cacheClient; try { cacheClient = RemoteCacheClientFactory.create( remoteOptions, - creds, + credentials, Preconditions.checkNotNull(env.getWorkingDirectory(), "workingDirectory"), digestUtil); } catch (IOException e) { @@ -246,6 +242,13 @@ private void initHttpAndDiskCache( executorService, env, remoteCache, /* retryScheduler= */ null, digestUtil); } + @Override + public void workspaceInit( + BlazeRuntime runtime, BlazeDirectories directories, WorkspaceBuilder builder) { + Preconditions.checkState(credentialModule == null, "credentialModule must be null"); + credentialModule = Preconditions.checkNotNull(runtime.getBlazeModule(CredentialModule.class)); + } + @Override public void beforeCommand(CommandEnvironment env) throws AbruptExitException { Preconditions.checkState(actionContextProvider == null, "actionContextProvider must be null"); @@ -330,8 +333,28 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { executorService = Executors.newCachedThreadPool(threadFactory); } + Credentials credentials; + try { + credentials = + createCredentials( + CredentialHelperEnvironment.newBuilder() + .setEventReporter(env.getReporter()) + .setWorkspacePath(env.getWorkspace()) + .setClientEnvironment(env.getClientEnv()) + .setHelperExecutionTimeout(authAndTlsOptions.credentialHelperTimeout) + .build(), + credentialModule.getCredentialCache(), + env.getCommandLinePathFactory(), + env.getRuntime().getFileSystem(), + authAndTlsOptions, + remoteOptions); + } catch (IOException e) { + handleInitFailure(env, e, Code.CREDENTIALS_INIT_FAILURE); + return; + } + if ((enableHttpCache || enableDiskCache) && !enableGrpcCache) { - initHttpAndDiskCache(env, authAndTlsOptions, remoteOptions, digestUtil); + initHttpAndDiskCache(env, credentials, authAndTlsOptions, remoteOptions, digestUtil); return; } @@ -428,27 +451,9 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { } } - CallCredentialsProvider callCredentialsProvider; - try { - callCredentialsProvider = - GoogleAuthUtils.newCallCredentialsProvider( - newCredentials( - CredentialHelperEnvironment.newBuilder() - .setEventReporter(env.getReporter()) - .setWorkspacePath(env.getWorkspace()) - .setClientEnvironment(env.getClientEnv()) - .setHelperExecutionTimeout(authAndTlsOptions.credentialHelperTimeout) - .build(), - env.getCommandLinePathFactory(), - env.getRuntime().getFileSystem(), - authAndTlsOptions, - remoteOptions)); - } catch (IOException e) { - handleInitFailure(env, e, Code.CREDENTIALS_INIT_FAILURE); - return; - } - - CallCredentials credentials = callCredentialsProvider.getCallCredentials(); + CallCredentialsProvider callCredentialsProvider = + GoogleAuthUtils.newCallCredentialsProvider(credentials); + CallCredentials callCredentials = callCredentialsProvider.getCallCredentials(); RemoteRetrier retrier = new RemoteRetrier( @@ -470,7 +475,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { verifyServerCapabilities( remoteOptions, execChannel, - credentials, + callCredentials, retrier, env, digestUtil, @@ -478,7 +483,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { verifyServerCapabilities( remoteOptions, cacheChannel, - credentials, + callCredentials, retrier, env, digestUtil, @@ -487,7 +492,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { verifyServerCapabilities( remoteOptions, execChannel, - credentials, + callCredentials, retrier, env, digestUtil, @@ -497,7 +502,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { verifyServerCapabilities( remoteOptions, cacheChannel, - credentials, + callCredentials, retrier, env, digestUtil, @@ -654,7 +659,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { buildRequestId, invocationId, downloaderChannel.retain(), - Optional.ofNullable(credentials), + Optional.ofNullable(callCredentials), retrier, cacheClient, remoteOptions)); @@ -1053,8 +1058,10 @@ RemoteActionContextProvider getActionContextProvider() { return actionContextProvider; } - static Credentials newCredentials( + @VisibleForTesting + static Credentials createCredentials( CredentialHelperEnvironment credentialHelperEnvironment, + Cache>> credentialCache, CommandLinePathFactory commandLinePathFactory, FileSystem fileSystem, AuthAndTLSOptions authAndTlsOptions, @@ -1062,7 +1069,11 @@ static Credentials newCredentials( throws IOException { Credentials credentials = GoogleAuthUtils.newCredentials( - credentialHelperEnvironment, commandLinePathFactory, fileSystem, authAndTlsOptions); + credentialHelperEnvironment, + credentialCache, + commandLinePathFactory, + fileSystem, + authAndTlsOptions); try { if (credentials != null diff --git a/src/test/java/com/google/devtools/build/lib/buildeventservice/BUILD b/src/test/java/com/google/devtools/build/lib/buildeventservice/BUILD index 9fa5d10224d7ec..38ea3dc92e7229 100644 --- a/src/test/java/com/google/devtools/build/lib/buildeventservice/BUILD +++ b/src/test/java/com/google/devtools/build/lib/buildeventservice/BUILD @@ -48,6 +48,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", "//src/main/java/com/google/devtools/build/lib/authandtls", + "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module", "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/buildeventservice", "//src/main/java/com/google/devtools/build/lib/buildeventservice:buildeventservice-options", diff --git a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java index 4ae061cabf211f..31bb5260f8599d 100644 --- a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; +import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule; import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.buildeventservice.BazelBuildEventServiceModule.BackendConfig; import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; @@ -155,6 +156,7 @@ public void beforeCommand(CommandEnvironment env) { } }) .addBlazeModule(new NoSpawnCacheModule()) + .addBlazeModule(new CredentialModule()) .addBlazeModule( new BazelBuildEventServiceModule() { @Override diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/BUILD b/src/test/java/com/google/devtools/build/lib/buildtool/BUILD index 367e442eaa9bb5..1563eae313a260 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/BUILD +++ b/src/test/java/com/google/devtools/build/lib/buildtool/BUILD @@ -560,6 +560,9 @@ java_test( "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", + "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module", + "//src/main/java/com/google/devtools/build/lib/buildeventservice", + "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/test/java/com/google/devtools/build/lib/analysis/util", diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 74ac0126b4b797..3ca14e511ecb29 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -55,6 +55,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/analysis:server_directories", "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper", + "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module", "//src/main/java/com/google/devtools/build/lib/buildeventstream", "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", @@ -95,6 +96,7 @@ java_test( "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", "//third_party:api_client", "//third_party:auth", + "//third_party:caffeine", "//third_party:guava", "//third_party:junit4", "//third_party:mockito", diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java index 5194ac0f383c7d..ba26c0945551cf 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java @@ -25,6 +25,8 @@ import build.bazel.remote.execution.v2.ExecutionCapabilities; import build.bazel.remote.execution.v2.GetCapabilitiesRequest; import build.bazel.remote.execution.v2.ServerCapabilities; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; import com.google.auth.Credentials; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -34,6 +36,7 @@ import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperEnvironment; +import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.BinTools; import com.google.devtools.build.lib.exec.ExecutionOptions; @@ -71,11 +74,14 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** Tests for {@link RemoteModule}. */ +/** + * Tests for {@link RemoteModule}. + */ @RunWith(JUnit4.class) public final class RemoteModuleTest { - private static CommandEnvironment createTestCommandEnvironment(RemoteOptions remoteOptions) + private static CommandEnvironment createTestCommandEnvironment( + RemoteModule remoteModule, RemoteOptions remoteOptions) throws IOException, AbruptExitException { CoreOptions coreOptions = Options.getDefaults(CoreOptions.class); CommonCommandOptions commonCommandOptions = Options.getDefaults(CommonCommandOptions.class); @@ -106,6 +112,8 @@ private static CommandEnvironment createTestCommandEnvironment(RemoteOptions rem .setServerDirectories(serverDirectories) .setStartupOptionsProvider( OptionsParser.builder().optionsClasses(BlazeServerStartupOptions.class).build()) + .addBlazeModule(new CredentialModule()) + .addBlazeModule(remoteModule) .build(); BlazeDirectories directories = @@ -181,7 +189,7 @@ public void testVerifyCapabilities_executionAndCacheForSingleEndpoint() throws E RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); remoteOptions.remoteExecutor = executionServerName; - CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); remoteModule.beforeCommand(env); @@ -220,7 +228,7 @@ public void testVerifyCapabilities_cacheOnlyEndpoint() throws Exception { RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); remoteOptions.remoteCache = cacheServerName; - CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); remoteModule.beforeCommand(env); @@ -266,7 +274,7 @@ public void testVerifyCapabilities_executionAndCacheForDifferentEndpoints() thro remoteOptions.remoteExecutor = executionServerName; remoteOptions.remoteCache = cacheServerName; - CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); remoteModule.beforeCommand(env); @@ -321,7 +329,7 @@ public void testVerifyCapabilities_executionOnlyAndCacheOnlyEndpoints() throws E remoteOptions.remoteExecutor = executionServerName; remoteOptions.remoteCache = cacheServerName; - CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); remoteModule.beforeCommand(env); @@ -359,7 +367,7 @@ public void testLocalFallback_shouldErrorForRemoteCacheWithoutRequiredCapabiliti (target, proxy, options, interceptors) -> InProcessChannelBuilder.forName(target).directExecutor().build()); - CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); assertThrows(AbruptExitException.class, () -> remoteModule.beforeCommand(env)); } finally { @@ -392,7 +400,7 @@ public void getCapabilities( (target, proxy, options, interceptors) -> InProcessChannelBuilder.forName(target).directExecutor().build()); - CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); assertThrows(AbruptExitException.class, () -> remoteModule.beforeCommand(env)); } finally { @@ -424,7 +432,7 @@ public void getCapabilities( (target, proxy, options, interceptors) -> InProcessChannelBuilder.forName(target).directExecutor().build()); - CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); remoteModule.beforeCommand(env); @@ -462,7 +470,7 @@ public void getCapabilities( (target, proxy, options, interceptors) -> InProcessChannelBuilder.forName(target).directExecutor().build()); - CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); remoteModule.beforeCommand(env); @@ -486,14 +494,18 @@ public void testNetrc_netrcWithoutRemoteCache() throws Exception { AuthAndTLSOptions authAndTLSOptions = Options.getDefaults(AuthAndTLSOptions.class); RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); + Cache>> credentialCache = + Caffeine.newBuilder().build(); + Credentials credentials = - RemoteModule.newCredentials( + RemoteModule.createCredentials( CredentialHelperEnvironment.newBuilder() .setEventReporter(new Reporter(new EventBus())) .setWorkspacePath(fileSystem.getPath("/workspace")) .setClientEnvironment(ImmutableMap.of("NETRC", netrc)) .setHelperExecutionTimeout(Duration.ZERO) .build(), + credentialCache, new CommandLinePathFactory(fileSystem, ImmutableMap.of()), fileSystem, authAndTLSOptions, diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index bd10bebec0dc14..3eff3cdd76300e 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -71,56 +71,76 @@ else declare -r EXE_EXT="" fi -function setup_credential_helper() { +function setup_credential_helper_test() { + # Each helper call atomically writes one byte to this file. + # We can later read the file to determine how many calls were made. + cat > "${TEST_TMPDIR}/credhelper_log" + cat > "${TEST_TMPDIR}/credhelper" <<'EOF' #!/usr/bin/env python3 +import os + +path = os.path.join(os.environ["TEST_TMPDIR"], "credhelper_log") +fd = os.open(path, os.O_WRONLY|os.O_CREAT|os.O_APPEND) +os.write(fd, b"1") +os.close(fd) + print("""{"headers":{"Authorization":["Bearer secret_token"]}}""") EOF chmod +x "${TEST_TMPDIR}/credhelper" -} - -function test_credential_helper_remote_cache() { - setup_credential_helper mkdir -p a cat > a/BUILD <<'EOF' -genrule( - name = "a", - outs = ["a.txt"], +[genrule( + name = x, + outs = [x + ".txt"], cmd = "touch $(OUTS)", -) +) for x in ["a", "b"]] EOF stop_worker start_worker --expected_authorization_token=secret_token +} + +function expect_credential_helper_calls() { + local -r expected=$1 + local -r actual=$(wc -c "${TEST_TMPDIR}/credhelper_log" | awk '{print $1}') + if [[ "$expected" != "$actual" ]]; then + fail "expected $expected instead of $actual credential helper calls" + fi +} + +function test_credential_helper_remote_cache() { + setup_credential_helper_test bazel build \ --remote_cache=grpc://localhost:${worker_port} \ //a:a >& $TEST_log && fail "Build without credentials should have failed" expect_log "Failed to query remote execution capabilities" + # Helper shouldn't have been called yet. + expect_credential_helper_calls 0 + bazel build \ --remote_cache=grpc://localhost:${worker_port} \ --experimental_credential_helper="${TEST_TMPDIR}/credhelper" \ //a:a >& $TEST_log || fail "Build with credentials should have succeeded" -} -function test_credential_helper_remote_execution() { - setup_credential_helper + # First build should have called helper for 4 distinct URIs. + expect_credential_helper_calls 4 - mkdir -p a + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --experimental_credential_helper="${TEST_TMPDIR}/credhelper" \ + //a:b >& $TEST_log || fail "Build with credentials should have succeeded" - cat > a/BUILD <<'EOF' -genrule( - name = "a", - outs = ["a.txt"], - cmd = "touch $(OUTS)", -) -EOF + # Second build should have hit the credentials cache. + expect_credential_helper_calls 4 +} - stop_worker - start_worker --expected_authorization_token=secret_token +function test_credential_helper_remote_execution() { + setup_credential_helper_test bazel build \ --spawn_strategy=remote \ @@ -128,11 +148,49 @@ EOF //a:a >& $TEST_log && fail "Build without credentials should have failed" expect_log "Failed to query remote execution capabilities" + # Helper shouldn't have been called yet. + expect_credential_helper_calls 0 + + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_credential_helper="${TEST_TMPDIR}/credhelper" \ + //a:a >& $TEST_log || fail "Build with credentials should have succeeded" + + # First build should have called helper for 5 distinct URIs. + expect_credential_helper_calls 5 + + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_credential_helper="${TEST_TMPDIR}/credhelper" \ + //a:b >& $TEST_log || fail "Build with credentials should have succeeded" + + # Second build should have hit the credentials cache. + expect_credential_helper_calls 5 +} + +function test_credential_helper_clear_cache() { + setup_credential_helper_test + bazel build \ --spawn_strategy=remote \ --remote_executor=grpc://localhost:${worker_port} \ --experimental_credential_helper="${TEST_TMPDIR}/credhelper" \ //a:a >& $TEST_log || fail "Build with credentials should have succeeded" + + expect_credential_helper_calls 5 + + bazel clean + + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_credential_helper="${TEST_TMPDIR}/credhelper" \ + //a:b >& $TEST_log || fail "Build with credentials should have succeeded" + + # Build after clean should have called helper again. + expect_credential_helper_calls 10 } function test_remote_grpc_cache_with_protocol() {