From bcbd2da340ed24a026729104dc0ba2a71cf70463 Mon Sep 17 00:00:00 2001 From: buchgr Date: Fri, 14 Jul 2017 14:58:07 +0200 Subject: remote: Improve error handling for --remote_* cmd line flags. Fixes #3361, #3358 - Move flag handling into RemoteModule to fail as early as possible. - Make error messages from flag handling human readable. - Fix a bug where remote execution would only support TLS with a root certificate being specified. - If a remote executor without a remote cache is specified, assume the remote cache to be the same as the executor. PiperOrigin-RevId: 161946029 --- .../devtools/build/lib/remote/ChannelOptions.java | 96 +++++++++++++--------- .../devtools/build/lib/remote/GrpcUtils.java | 29 ++++--- .../lib/remote/RemoteActionContextProvider.java | 55 ++++--------- .../devtools/build/lib/remote/RemoteModule.java | 62 +++++++++++++- .../build/lib/remote/RemoteSpawnRunner.java | 15 +++- .../build/lib/remote/SimpleBlobStoreFactory.java | 4 +- .../build/lib/remote/blobstore/RestBlobStore.java | 15 +++- 7 files changed, 180 insertions(+), 96 deletions(-) (limited to 'src/main/java/com/google/devtools/build/lib/remote') diff --git a/src/main/java/com/google/devtools/build/lib/remote/ChannelOptions.java b/src/main/java/com/google/devtools/build/lib/remote/ChannelOptions.java index a78458d135..9a3f736455 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ChannelOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ChannelOptions.java @@ -19,17 +19,16 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; -import com.google.devtools.common.options.Options; import io.grpc.CallCredentials; import io.grpc.auth.MoreCallCredentials; import io.grpc.netty.GrpcSslContexts; import io.netty.handler.ssl.SslContext; import java.io.File; import java.io.FileInputStream; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import javax.annotation.Nullable; -import javax.net.ssl.SSLException; /** Instantiate all authentication helpers from build options. */ @ThreadSafe @@ -38,7 +37,6 @@ public final class ChannelOptions { private final SslContext sslContext; private final String tlsAuthorityOverride; private final CallCredentials credentials; - public static final ChannelOptions DEFAULT = create(Options.getDefaults(AuthAndTLSOptions.class)); private ChannelOptions( boolean tlsEnabled, @@ -67,52 +65,70 @@ public final class ChannelOptions { return sslContext; } - public static ChannelOptions create(AuthAndTLSOptions options) { - try { - return create( - options, - options.authCredentials != null - ? new FileInputStream(options.authCredentials) - : null); - } catch (IOException e) { - throw new IllegalArgumentException( - "Failed initializing auth credentials for remote cache/execution " + e); + public static ChannelOptions create(AuthAndTLSOptions options) throws IOException { + if (options.authCredentials != null) { + try (InputStream authFile = new FileInputStream(options.authCredentials)) { + return create(options, authFile); + } catch (FileNotFoundException e) { + String message = String.format("Could not open auth credentials file '%s': %s", + options.authCredentials, e.getMessage()); + throw new IOException(message, e); + } + } else { + return create(options, null); } } @VisibleForTesting - public static ChannelOptions create( + static ChannelOptions create( AuthAndTLSOptions options, - @Nullable InputStream credentialsInputStream) { - boolean tlsEnabled = options.tlsEnabled; - SslContext sslContext = null; - String tlsAuthorityOverride = options.tlsAuthorityOverride; - CallCredentials credentials = null; - if (options.tlsEnabled && options.tlsCertificate != null) { - try { - sslContext = - GrpcSslContexts.forClient().trustManager(new File(options.tlsCertificate)).build(); - } catch (SSLException e) { - throw new IllegalArgumentException( - "SSL error initializing cert " + options.tlsCertificate + " : " + e); + @Nullable InputStream credentialsFile) throws IOException { + final SslContext sslContext = + options.tlsEnabled ? createSSlContext(options.tlsCertificate) : null; + + final CallCredentials callCredentials = + options.authEnabled ? createCallCredentials(credentialsFile, options.authScope) : null; + + return new ChannelOptions( + sslContext != null, sslContext, options.tlsAuthorityOverride, callCredentials); + } + + private static CallCredentials createCallCredentials(@Nullable InputStream credentialsFile, + @Nullable String authScope) throws IOException { + try { + GoogleCredentials creds = + credentialsFile == null + ? GoogleCredentials.getApplicationDefault() + : GoogleCredentials.fromStream(credentialsFile); + if (authScope != null) { + creds = creds.createScoped(ImmutableList.of(authScope)); } + return MoreCallCredentials.from(creds); + } catch (IOException e) { + String message = "Failed to init auth credentials for remote caching/execution: " + + e.getMessage(); + throw new IOException(message, e); } - if (options.authEnabled) { + } + + private static SslContext createSSlContext(@Nullable String rootCert) throws IOException { + if (rootCert == null) { + try { + return GrpcSslContexts.forClient().build(); + } catch (Exception e) { + String message = "Failed to init TLS infrastructure for remote caching/execution: " + + e.getMessage(); + throw new IOException(message, e); + } + } else { try { - GoogleCredentials creds = - credentialsInputStream == null - ? GoogleCredentials.getApplicationDefault() - : GoogleCredentials.fromStream(credentialsInputStream); - if (options.authScope != null) { - creds = creds.createScoped(ImmutableList.of(options.authScope)); - } - credentials = MoreCallCredentials.from(creds); - } catch (IOException e) { - throw new IllegalArgumentException( - "Failed initializing auth credentials for remote cache/execution " + e); + return GrpcSslContexts.forClient().trustManager(new File(rootCert)).build(); + } catch (Exception e) { + String message = "Failed to init TLS infrastructure for remote caching/execution using " + + "'%s' as root certificate: %s"; + message = String.format(message, rootCert, e.getMessage()); + throw new IOException(message, e); } } - return new ChannelOptions( - tlsEnabled, sslContext, tlsAuthorityOverride, credentials); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcUtils.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcUtils.java index f6e0092822..8f91c51708 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcUtils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcUtils.java @@ -18,21 +18,30 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import io.grpc.ManagedChannel; import io.grpc.netty.NegotiationType; import io.grpc.netty.NettyChannelBuilder; +import java.io.IOException; /** Helper methods for gRPC calls */ @ThreadSafe final class GrpcUtils { - public static ManagedChannel createChannel(String target, ChannelOptions channelOptions) { - NettyChannelBuilder builder = - NettyChannelBuilder.forTarget(target) - .negotiationType( - channelOptions.tlsEnabled() ? NegotiationType.TLS : NegotiationType.PLAINTEXT); - if (channelOptions.getSslContext() != null) { - builder.sslContext(channelOptions.getSslContext()); - if (channelOptions.getTlsAuthorityOverride() != null) { - builder.overrideAuthority(channelOptions.getTlsAuthorityOverride()); + public static ManagedChannel createChannel(String target, ChannelOptions channelOptions) + throws IOException { + try { + NettyChannelBuilder builder = + NettyChannelBuilder.forTarget(target) + .negotiationType( + channelOptions.tlsEnabled() ? NegotiationType.TLS : NegotiationType.PLAINTEXT); + if (channelOptions.getSslContext() != null) { + builder.sslContext(channelOptions.getSslContext()); + if (channelOptions.getTlsAuthorityOverride() != null) { + builder.overrideAuthority(channelOptions.getTlsAuthorityOverride()); + } } + return builder.build(); + } catch (RuntimeException e) { + // gRPC might throw all kinds of RuntimeExceptions: StatusRuntimeException, + // IllegalStateException, NullPointerException, ... + String message = "Failed to connect to the remote cache/executor '%s': %s"; + throw new IOException(String.format(message, target, e.getMessage())); } - return builder.build(); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java index 9b6e42c35d..b68cc993d8 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java @@ -13,12 +13,12 @@ // limitations under the License. package com.google.devtools.build.lib.remote; -import com.google.common.base.Preconditions; +import static com.google.common.base.Preconditions.checkNotNull; + import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.ActionContext; import com.google.devtools.build.lib.actions.ActionInputFileCache; import com.google.devtools.build.lib.actions.ResourceManager; -import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.exec.ActionContextProvider; import com.google.devtools.build.lib.exec.ActionInputPrefetcher; import com.google.devtools.build.lib.exec.ExecutionOptions; @@ -29,61 +29,40 @@ import com.google.devtools.build.lib.exec.local.LocalExecutionOptions; import com.google.devtools.build.lib.exec.local.LocalSpawnRunner; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.util.OS; +import javax.annotation.Nullable; /** * Provide a remote execution context. */ final class RemoteActionContextProvider extends ActionContextProvider { private final CommandEnvironment env; + private final RemoteActionCache cache; + private final GrpcRemoteExecutor executor; + + private RemoteSpawnRunner spawnRunner; private RemoteSpawnStrategy spawnStrategy; - RemoteActionContextProvider(CommandEnvironment env) { + RemoteActionContextProvider(CommandEnvironment env, @Nullable RemoteActionCache cache, + @Nullable GrpcRemoteExecutor executor) { this.env = env; + this.executor = executor; + this.cache = cache; } @Override public void init( ActionInputFileCache actionInputFileCache, ActionInputPrefetcher actionInputPrefetcher) { - ExecutionOptions executionOptions = env.getOptions().getOptions(ExecutionOptions.class); - RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class); - AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class); - ChannelOptions channelOptions = ChannelOptions.create(authAndTlsOptions); - - Retrier retrier = new Retrier(remoteOptions); + ExecutionOptions executionOptions = + checkNotNull(env.getOptions().getOptions(ExecutionOptions.class)); + RemoteOptions remoteOptions = checkNotNull(env.getOptions().getOptions(RemoteOptions.class)); - RemoteActionCache remoteCache; - if (SimpleBlobStoreFactory.isRemoteCacheOptions(remoteOptions)) { - remoteCache = new SimpleBlobStoreActionCache(SimpleBlobStoreFactory.create(remoteOptions)); - } else if (GrpcRemoteCache.isRemoteCacheOptions(remoteOptions)) { - remoteCache = - new GrpcRemoteCache( - GrpcUtils.createChannel(remoteOptions.remoteCache, channelOptions), - channelOptions, - remoteOptions, - retrier); - } else { - remoteCache = null; - } - - // Otherwise remoteCache remains null and remote caching/execution are disabled. - GrpcRemoteExecutor remoteExecutor; - if (remoteCache != null && remoteOptions.remoteExecutor != null) { - remoteExecutor = - new GrpcRemoteExecutor( - GrpcUtils.createChannel(remoteOptions.remoteExecutor, channelOptions), - channelOptions.getCallCredentials(), - remoteOptions.remoteTimeout, - retrier); - } else { - remoteExecutor = null; - } spawnRunner = new RemoteSpawnRunner( env.getExecRoot(), remoteOptions, createFallbackRunner(actionInputPrefetcher), - remoteCache, - remoteExecutor); + cache, + executor); spawnStrategy = new RemoteSpawnStrategy( "remote", @@ -109,7 +88,7 @@ final class RemoteActionContextProvider extends ActionContextProvider { @Override public Iterable getActionContexts() { - return ImmutableList.of(Preconditions.checkNotNull(spawnStrategy)); + return ImmutableList.of(checkNotNull(spawnStrategy)); } @Override 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 63ef0b6337..d5c0575115 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 @@ -14,17 +14,21 @@ package com.google.devtools.build.lib.remote; +import static com.google.common.base.Preconditions.checkState; + import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.buildeventstream.PathConverter; import com.google.devtools.build.lib.buildtool.BuildRequest; +import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.exec.ExecutorBuilder; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.Command; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.runtime.ServerBuilder; import com.google.devtools.build.lib.util.AbruptExitException; +import com.google.devtools.build.lib.util.ExitCode; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsProvider; @@ -73,6 +77,9 @@ public final class RemoteModule extends BlazeModule { private final CasPathConverter converter = new CasPathConverter(); + private CommandEnvironment env; + private RemoteActionContextProvider actionContextProvider; + @Override public void serverInit(OptionsProvider startupOptions, ServerBuilder builder) throws AbruptExitException { @@ -82,16 +89,67 @@ public final class RemoteModule extends BlazeModule { @Override public void beforeCommand(CommandEnvironment env) { env.getEventBus().register(this); + this.env = env; } @Override public void handleOptions(OptionsProvider optionsProvider) { - converter.options = optionsProvider.getOptions(RemoteOptions.class); + checkState(env != null); + + RemoteOptions remoteOptions = optionsProvider.getOptions(RemoteOptions.class); + AuthAndTLSOptions authAndTlsOptions = optionsProvider.getOptions(AuthAndTLSOptions.class); + converter.options = remoteOptions; + + // Quit if no remote options specified. + if (remoteOptions == null) { + return; + } + + try { + ChannelOptions channelOpts = ChannelOptions.create(authAndTlsOptions); + + boolean restCache = SimpleBlobStoreFactory.isRemoteCacheOptions(remoteOptions); + boolean grpcCache = GrpcRemoteCache.isRemoteCacheOptions(remoteOptions); + + Retrier retrier = new Retrier(remoteOptions); + final RemoteActionCache cache; + if (restCache) { + cache = new SimpleBlobStoreActionCache(SimpleBlobStoreFactory.create(remoteOptions)); + } else if (grpcCache) { + cache = new GrpcRemoteCache(GrpcUtils.createChannel(remoteOptions.remoteCache, channelOpts), + channelOpts, remoteOptions, retrier); + } else if (remoteOptions.remoteExecutor != null) { + // If a remote executor but no remote cache is specified, assume both at the same target. + cache = + new GrpcRemoteCache(GrpcUtils.createChannel(remoteOptions.remoteExecutor, channelOpts), + channelOpts, remoteOptions, retrier); + } else { + cache = null; + } + + final GrpcRemoteExecutor executor; + if (remoteOptions.remoteExecutor != null) { + executor = new GrpcRemoteExecutor( + GrpcUtils.createChannel(remoteOptions.remoteExecutor, channelOpts), + channelOpts.getCallCredentials(), + remoteOptions.remoteTimeout, + retrier); + } else { + executor = null; + } + + actionContextProvider = new RemoteActionContextProvider(env, cache, executor); + } catch (IOException e) { + env.getReporter().handle(Event.error(e.getMessage())); + env.getBlazeModuleEnvironment().exit(new AbruptExitException(ExitCode.COMMAND_LINE_ERROR)); + } } @Override public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) { - builder.addActionContextProvider(new RemoteActionContextProvider(env)); + if (actionContextProvider != null) { + builder.addActionContextProvider(actionContextProvider); + } } @Override diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index 58c6c668a6..5b5c56eeb0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -17,6 +17,7 @@ package com.google.devtools.build.lib.remote; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputFileCache; +import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.Spawns; @@ -143,12 +144,22 @@ final class RemoteSpawnRunner implements SpawnRunner { if (options.remoteLocalFallback) { return execLocally(spawn, policy, remoteCache, actionKey); } - throw e; + + io.grpc.Status grpcStatus = io.grpc.Status.fromThrowable(e); + final String message; + if (io.grpc.Status.UNAVAILABLE.getCode().equals(grpcStatus.getCode())) { + message = "The remote executor/cache is unavailable: " + grpcStatus.getDescription(); + } else { + message = "I/O Error in remote cache/executor: " + e.getMessage(); + } + throw new EnvironmentalExecException(message, true); } catch (CacheNotFoundException e) { if (options.remoteLocalFallback) { return execLocally(spawn, policy, remoteCache, actionKey); } - throw new IOException(e); + + String message = "Failed to download from remote cache: " + e.getMessage(); + throw new EnvironmentalExecException(message, true); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java index fd05ef84d3..170e22db96 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java @@ -70,11 +70,11 @@ public final class SimpleBlobStoreFactory { return new ConcurrentMapBlobStore(instance.getMap(HAZELCAST_CACHE_NAME)); } - public static SimpleBlobStore createRest(RemoteOptions options) { + public static SimpleBlobStore createRest(RemoteOptions options) throws IOException { return new RestBlobStore(options.remoteRestCache, options.restCachePoolSize); } - public static SimpleBlobStore create(RemoteOptions options) { + public static SimpleBlobStore create(RemoteOptions options) throws IOException { if (isHazelcastOptions(options)) { return createHazelcast(options); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/blobstore/RestBlobStore.java b/src/main/java/com/google/devtools/build/lib/remote/blobstore/RestBlobStore.java index 60ff77b7bd..82e9b47bad 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/blobstore/RestBlobStore.java +++ b/src/main/java/com/google/devtools/build/lib/remote/blobstore/RestBlobStore.java @@ -17,6 +17,8 @@ import com.google.common.io.ByteStreams; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.net.URI; +import java.net.URISyntaxException; import org.apache.http.HttpStatus; import org.apache.http.client.HttpClient; import org.apache.http.client.methods.HttpGet; @@ -53,7 +55,8 @@ public final class RestBlobStore implements SimpleBlobStore { * @param baseUrl base URL for the remote cache * @param poolSize maximum number of simultaneous connections */ - public RestBlobStore(String baseUrl, int poolSize) { + public RestBlobStore(String baseUrl, int poolSize) throws IOException { + validateUrl(baseUrl); this.baseUrl = baseUrl; connMan = new PoolingHttpClientConnectionManager(); connMan.setDefaultMaxPerRoute(poolSize); @@ -122,4 +125,12 @@ public final class RestBlobStore implements SimpleBlobStore { return null; }); } -} \ No newline at end of file + + private void validateUrl(String url) throws IOException { + try { + new URI(url); + } catch (URISyntaxException e) { + throw new IOException("Failed to parse remote REST cache URL: " + baseUrl, e); + } + } +} -- cgit v1.2.3