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

7 files changed, 180 insertions, 96 deletions

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<? extends ActionContext> 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.<String, byte[]>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);
+    }
+  }
+}