From 4e469f73cf0cd838ee116c8704d563972ef3dcaa Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Tue, 22 Nov 2022 21:01:48 +0100 Subject: [PATCH 1/2] Keep credentials cached across build commands. When using a credential helper, the lifetime of the credential cache is currently tied to an individual command, which causes the helper to be called for every command resulting in poor incremental build latency for builds using a non-trivial helper. Since the cache must be shared by RemoteModule and BazelBuildServiceModule, I've introduced a new CredentialModule whose sole purpose is to provide access to it. --- .../devtools/build/lib/authandtls/BUILD | 1 + .../build/lib/authandtls/GoogleAuthUtils.java | 16 +++- .../lib/authandtls/credentialhelper/BUILD | 16 +++- .../CredentialHelperCredentials.java | 93 +++++++------------ .../credentialhelper/CredentialModule.java | 43 +++++++++ .../com/google/devtools/build/lib/bazel/BUILD | 2 + .../devtools/build/lib/bazel/Bazel.java | 3 + .../build/lib/buildeventservice/BUILD | 2 + .../BazelBuildEventServiceModule.java | 16 ++++ .../google/devtools/build/lib/remote/BUILD | 1 + .../build/lib/remote/RemoteModule.java | 93 +++++++++---------- .../build/lib/buildeventservice/BUILD | 1 + .../BazelBuildEventServiceModuleTest.java | 2 + .../google/devtools/build/lib/buildtool/BUILD | 1 + .../buildtool/TargetCompleteEventTest.java | 2 + .../google/devtools/build/lib/remote/BUILD | 3 + .../BuildWithoutTheBytesIntegrationTest.java | 2 + .../build/lib/remote/RemoteModuleTest.java | 34 ++++--- .../bazel/remote/remote_execution_test.sh | 65 +++++++++++-- 19 files changed, 264 insertions(+), 132 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialModule.java 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 252e6587c23c43..d220892d066b1b 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 fa4b81da46e7e3..2210e86c5daa08 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,15 @@ package com.google.devtools.build.lib.authandtls; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; 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; @@ -48,6 +52,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; @@ -248,6 +253,7 @@ public static CallCredentialsProvider newCallCredentialsProvider(@Nullable Crede */ public static Credentials newCredentials( CredentialHelperEnvironment credentialHelperEnvironment, + Cache>> credentialCache, CommandLinePathFactory commandLinePathFactory, FileSystem fileSystem, AuthAndTLSOptions authAndTlsOptions) @@ -257,12 +263,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. @@ -276,8 +282,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 b14a03cf3e437d..275c159b60087c 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 @@ -8,9 +8,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/CredentialHelperCredentials.java b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperCredentials.java index ecc40e15707d8d..21193cfbf72575 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,48 +14,36 @@ 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; -import javax.annotation.Nullable; /** * Implementation of {@link Credentials} which fetches credentials by invoking a {@code credential * 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; - 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 @@ -71,9 +59,14 @@ public String getAuthenticationType() { 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 (Exception e) { + throw new IOException(e); + } + if (credentials != null) { + return (Map) credentials; } if (fallbackCredentials.isPresent()) { @@ -83,13 +76,27 @@ public Map> getRequestMetadata(URI uri) throws IOException return ImmutableMap.of(); } - @SuppressWarnings("unchecked") // Map> to Map> - private Optional>> getRequestMetadataFromCredentialHelper(URI uri) { + 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(); - return Optional.ofNullable(response).map(value -> (Map) value.getHeaders()); + GetCredentialsResponse response; + try { + response = credentialHelper.getCredentials(credentialHelperEnvironment, uri); + } catch (Exception e) { + throw new RuntimeException(e); + } + if (response == null) { + return null; + } + + return response.getHeaders(); } @Override @@ -110,32 +117,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..32c6742b4b5c11 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialModule.java @@ -0,0 +1,43 @@ +// 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); + } +} 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 499a038e030eb7..6ff035d95f5d49 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/BUILD @@ -26,6 +26,7 @@ java_library( "//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_configuration", + "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:common", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:inspection", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:inspection_impl", @@ -136,6 +137,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 fb10eddbea35af..e35b11a8386f5d 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 @@ -16,6 +16,7 @@ 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.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.runtime.MemoryPressureModule.class, com.google.devtools.build.lib.platform.SleepPreventionModule.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 bde58e545a8739..6c61f671a71326 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventservice/BUILD +++ b/src/main/java/com/google/devtools/build/lib/buildeventservice/BUILD @@ -38,9 +38,11 @@ java_library( ":buildeventservice-options", "//src/main/java/com/google/devtools/build/lib:build-request-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 ac876b0399819f..773d7da131ce5b 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,16 @@ 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,10 +107,12 @@ protected BuildEventServiceClient getBesClient( .setClientEnvironment(env.getClientEnv()) .setHelperExecutionTimeout(authAndTLSOptions.credentialHelperTimeout) .build(), + credentialModule.getCredentialCache(), env.getCommandLinePathFactory(), env.getRuntime().getFileSystem(), newConfig.authAndTLSOptions()); + config = newConfig; client = new BuildEventServiceGrpcClient( 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 6a3292448d9b83..6a844d33dd3378 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -63,6 +63,7 @@ java_library( "//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 b383a3796c6fa1..7601ac1eb5b7b7 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 @@ -21,6 +21,7 @@ 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; @@ -28,6 +29,7 @@ import com.google.common.base.Strings; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; +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; @@ -42,6 +44,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.bazel.repository.downloader.HttpDownloader; import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; @@ -159,6 +162,8 @@ public ManagedChannel newChannel( private final MutableSupplier remoteDownloaderSupplier = new MutableSupplier<>(); + private CredentialModule credentialModule; + @Override public void serverInit(OptionsParsingResult startupOptions, ServerBuilder builder) { builder.addBuildEventArtifactUploaderFactory( @@ -167,7 +172,9 @@ 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); } @@ -211,33 +218,16 @@ private static ServerCapabilities getAndVerifyServerCapabilities( 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, authAndTlsOptions, Preconditions.checkNotNull(env.getWorkingDirectory(), "workingDirectory"), digestUtil); @@ -256,8 +246,11 @@ private void initHttpAndDiskCache( public void workspaceInit( BlazeRuntime runtime, BlazeDirectories directories, WorkspaceBuilder builder) { Preconditions.checkState(blockWaitingModule == null, "blockWaitingModule must be null"); + Preconditions.checkState(credentialModule == null, "credentialModule must be null"); blockWaitingModule = Preconditions.checkNotNull(runtime.getBlazeModule(BlockWaitingModule.class)); + credentialModule = + Preconditions.checkNotNull(runtime.getBlazeModule(CredentialModule.class)); } @Override @@ -358,8 +351,25 @@ 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; } @@ -456,27 +466,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( @@ -500,7 +492,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { getAndVerifyServerCapabilities( remoteOptions, execChannel, - credentials, + callCredentials, retrier, env, digestUtil, @@ -509,7 +501,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { getAndVerifyServerCapabilities( remoteOptions, cacheChannel, - credentials, + callCredentials, retrier, env, digestUtil, @@ -519,7 +511,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { getAndVerifyServerCapabilities( remoteOptions, execChannel, - credentials, + callCredentials, retrier, env, digestUtil, @@ -530,7 +522,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { getAndVerifyServerCapabilities( remoteOptions, cacheChannel, - credentials, + callCredentials, retrier, env, digestUtil, @@ -681,7 +673,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { buildRequestId, invocationId, downloaderChannel.retain(), - Optional.ofNullable(credentials), + Optional.ofNullable(callCredentials), retrier, cacheClient, remoteOptions, @@ -1082,8 +1074,10 @@ RemoteActionContextProvider getActionContextProvider() { return actionContextProvider; } - static Credentials newCredentials( + @VisibleForTesting + static Credentials createCredentials( CredentialHelperEnvironment credentialHelperEnvironment, + Cache>> credentialCache, CommandLinePathFactory commandLinePathFactory, FileSystem fileSystem, AuthAndTLSOptions authAndTlsOptions, @@ -1091,7 +1085,8 @@ 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 793238eb0f0c56..4aa7f3221587fa 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 50b5433da0c635..4124c431c2446b 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 @@ -32,6 +32,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.bugreport.Crash; import com.google.devtools.build.lib.bugreport.CrashContext; @@ -145,6 +146,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 0733ee95aa31e1..c604a800700a67 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/BUILD +++ b/src/test/java/com/google/devtools/build/lib/buildtool/BUILD @@ -530,6 +530,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//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", diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/TargetCompleteEventTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/TargetCompleteEventTest.java index ea810373900f2b..9ffd40e6a94498 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/TargetCompleteEventTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/TargetCompleteEventTest.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.analysis.TargetCompleteEvent; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.analysis.util.AnalysisMock; +import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule; import com.google.devtools.build.lib.buildeventservice.BazelBuildEventServiceModule; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEvent; @@ -71,6 +72,7 @@ public void stageEmbeddedTools() throws Exception { protected BlazeRuntime.Builder getRuntimeBuilder() throws Exception { return super.getRuntimeBuilder() .addBlazeModule(new NoSpawnCacheModule()) + .addBlazeModule(new CredentialModule()) .addBlazeModule(new BazelBuildEventServiceModule()); } 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 830c481ea452da..13e3b1dc4d8fbc 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -58,6 +58,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", @@ -100,6 +101,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", @@ -153,6 +155,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module", "//src/main/java/com/google/devtools/build/lib/dynamic", "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/standalone", diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java index 70e138cbc34ed3..8c49c36b9c7d6f 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.BuildFailedException; +import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule; import com.google.devtools.build.lib.dynamic.DynamicExecutionModule; import com.google.devtools.build.lib.remote.util.IntegrationTestUtils.WorkerInstance; import com.google.devtools.build.lib.runtime.BlazeModule; @@ -77,6 +78,7 @@ protected ImmutableList getSpawnModules() { return ImmutableList.builder() .addAll(super.getSpawnModules()) .add(new StandaloneModule()) + .add(new CredentialModule()) .add(new RemoteModule()) .add(new DynamicExecutionModule()) .build(); 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 2deeff89cae143..47b1c8a172fe4f 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 @@ -26,6 +26,8 @@ import build.bazel.remote.execution.v2.GetCapabilitiesRequest; import build.bazel.remote.execution.v2.ServerCapabilities; import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy; +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; @@ -35,6 +37,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; @@ -103,7 +106,8 @@ public final class RemoteModuleTest { CacheCapabilities.newBuilder().addDigestFunctions(Value.SHA256).build()) .build(); - 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); @@ -134,6 +138,8 @@ private static CommandEnvironment createTestCommandEnvironment(RemoteOptions rem .setServerDirectories(serverDirectories) .setStartupOptionsProvider( OptionsParser.builder().optionsClasses(BlazeServerStartupOptions.class).build()) + .addBlazeModule(new CredentialModule()) + .addBlazeModule(remoteModule) .addBlazeModule(new BlockWaitingModule()) .build(); @@ -198,7 +204,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); @@ -226,7 +232,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); @@ -272,7 +278,7 @@ public void testVerifyCapabilities_executionAndCacheForDifferentEndpoints() thro remoteOptions.remoteExecutor = executionServerName; remoteOptions.remoteCache = cacheServerName; - CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); remoteModule.beforeCommand(env); @@ -327,7 +333,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); @@ -365,7 +371,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 { @@ -398,7 +404,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 { @@ -430,7 +436,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); @@ -468,7 +474,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); @@ -492,14 +498,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, @@ -526,7 +536,7 @@ public void testCacheCapabilities_propagatedToRemoteCache() throws Exception { RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); remoteOptions.remoteCache = cacheServerName; - CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); remoteModule.beforeCommand(env); @@ -558,7 +568,7 @@ public void testCacheCapabilities_propagatedToRemoteExecutionCache() throws Exce RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); remoteOptions.remoteExecutor = executionServerName; - CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); remoteModule.beforeCommand(env); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index c3d4b645ecd8e2..bbddf74d00ffb4 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -77,24 +77,44 @@ function has_utf8_locale() { } function setup_credential_helper() { + # 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 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 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 @@ -105,10 +125,24 @@ 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 \ --remote_cache=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 4 distinct URIs. + expect_credential_helper_calls 4 + + 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" + + # Second build should have hit the credentials cache. + expect_credential_helper_calls 4 } function test_credential_helper_remote_execution() { @@ -117,11 +151,11 @@ function test_credential_helper_remote_execution() { 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 @@ -133,11 +167,26 @@ 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_remote_grpc_cache_with_protocol() { From 4e3caebcf0143afb3073e37788e60db94826d3d6 Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Wed, 23 Nov 2022 14:03:19 +0100 Subject: [PATCH 2/2] Clear the credential cache on `bazel clean`. --- .../lib/authandtls/AuthAndTLSOptions.java | 6 +- .../credentialhelper/CredentialModule.java | 8 ++- .../bazel/remote/remote_execution_test.sh | 65 +++++++++++-------- 3 files changed, 49 insertions(+), 30 deletions(-) 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 8f3aae8ed30980..7423b555a13e9c 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 @@ -175,7 +175,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/credentialhelper/CredentialModule.java b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialModule.java index 32c6742b4b5c11..0e256a44910c30 100644 --- 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 @@ -38,6 +38,12 @@ public Cache>> getCredentialCach 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); + credentialCache.policy().expireAfterWrite().get() + .setExpiresAfter(authAndTlsOptions.credentialHelperCacheTimeout); + + // Clear the cache on clean. + if (env.getCommand().name().equals("clean")) { + credentialCache.invalidateAll(); + } } } diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index bbddf74d00ffb4..26531744461be8 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -76,7 +76,7 @@ function has_utf8_locale() { [[ "${charmap}" == "UTF-8" ]] } -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" @@ -92,20 +92,7 @@ os.close(fd) print("""{"headers":{"Authorization":["Bearer secret_token"]}}""") EOF - chmod +x "${TEST_TMPDIR}/credhelper" -} - -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 mkdir -p a @@ -119,6 +106,18 @@ 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} \ @@ -146,20 +145,7 @@ EOF } function test_credential_helper_remote_execution() { - setup_credential_helper - - mkdir -p a - - cat > a/BUILD <<'EOF' -[genrule( - name = x, - outs = [x + ".txt"], - cmd = "touch $(OUTS)", -) for x in ["a", "b"]] -EOF - - stop_worker - start_worker --expected_authorization_token=secret_token + setup_credential_helper_test bazel build \ --spawn_strategy=remote \ @@ -189,6 +175,29 @@ EOF 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() { # Test that if 'grpc' is provided as a scheme for --remote_cache flag, remote cache works. mkdir -p a