[6.0.0] [remote] Respect whether the server supports action cache upd…

…ates (#16724)

Only a subset of users may be allowed to update the action cache (e.g., only CI but not devs).

Today, there are 2 ways to achive the desired behavior:
  - `GetCapabilities` returning that all users are allowed to update, and `UpdateActionResult` returning an error that Bazel prints and ignores, or
  - have the users that are not allowed to update the action cache set `--remote_upload_local_results=false`.

Why don't we instead respect whether the remote cache supports uploading action results?

Note that this requires support from the remote system to fully work (i.e., it needs to return `update_enabled = false` for users that don't have permission). Otherwise, Bazel's behavior will be the same as before this change: failed `UpdateActionResult` do not cause the build to fail. The only change this introduces is that Bazel will no longer error if `--remote_upload_local_results=true` and `GetCapabilities` returning `update_enabled = false`.

Closes #16624.

PiperOrigin-RevId: 486901751
Change-Id: I0991f6891e21711df1e23ae0998a8bc95e2389bc

src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java

@@ -122,6 +122,11 @@ public ListenableFuture<ImmutableSet<Digest>> findMissingDigests(
     return cacheProtocol.findMissingDigests(context, digests);
   }
 
+  /** Returns whether the action cache supports updating action results. */
+  public boolean actionCacheSupportsUpdate() {
+    return cacheCapabilities.getActionCacheUpdateCapabilities().getUpdateEnabled();
+  }
+
   /** Upload the action result to the remote cache. */
   public ListenableFuture<Void> uploadActionResult(
       RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) {

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

@@ -305,7 +305,8 @@ public CachePolicy getWriteCachePolicy(Spawn spawn) {
 
     if (useRemoteCache(remoteOptions)) {
       allowRemoteCache =
-          shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn.getExecutionInfo());
+          shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn.getExecutionInfo())
+              && remoteCache.actionCacheSupportsUpdate();
       if (useDiskCache(remoteOptions)) {
         // Combined cache
         if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) {

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

@@ -16,6 +16,7 @@
 
 import static java.util.concurrent.TimeUnit.SECONDS;
 
+import build.bazel.remote.execution.v2.ActionCacheUpdateCapabilities;
 import build.bazel.remote.execution.v2.CacheCapabilities;
 import build.bazel.remote.execution.v2.DigestFunction;
 import build.bazel.remote.execution.v2.ServerCapabilities;
@@ -27,7 +28,6 @@
 import com.google.common.base.Strings;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
-import com.google.common.flogger.GoogleLogger;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
@@ -112,11 +112,10 @@
 
 /** RemoteModule provides distributed cache and remote execution for Bazel. */
 public final class RemoteModule extends BlazeModule {
-
-  private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
-
-  private static final CacheCapabilities DISK_CACHE_CAPABILITIES =
+  private static final CacheCapabilities HTTP_AND_DISK_CACHE_CAPABILITIES =
       CacheCapabilities.newBuilder()
+          .setActionCacheUpdateCapabilities(
+              ActionCacheUpdateCapabilities.newBuilder().setUpdateEnabled(true).build())
           .setSymlinkAbsolutePathStrategy(SymlinkAbsolutePathStrategy.Value.ALLOWED)
           .build();
 
@@ -247,7 +246,7 @@ private void initHttpAndDiskCache(
       return;
     }
     RemoteCache remoteCache =
-        new RemoteCache(DISK_CACHE_CAPABILITIES, cacheClient, remoteOptions, digestUtil);
+        new RemoteCache(HTTP_AND_DISK_CACHE_CAPABILITIES, cacheClient, remoteOptions, digestUtil);
     actionContextProvider =
         RemoteActionContextProvider.createForRemoteCaching(
             executorService, env, remoteCache, /* retryScheduler= */ null, digestUtil);

src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java

@@ -25,7 +25,6 @@
 import build.bazel.remote.execution.v2.PriorityCapabilities.PriorityRange;
 import build.bazel.remote.execution.v2.RequestMetadata;
 import build.bazel.remote.execution.v2.ServerCapabilities;
-import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
@@ -234,25 +233,12 @@ public static ClientServerCompatibilityStatus checkClientServerCompatibility(
                 digestFunction, cacheCap.getDigestFunctionsList()));
       }
 
-      // Check updating remote cache is allowed, if we ever need to do that.
-      boolean remoteExecution = !Strings.isNullOrEmpty(remoteOptions.remoteExecutor);
-      if (remoteExecution) {
-        if (remoteOptions.remoteLocalFallback
-            && remoteOptions.remoteUploadLocalResults
-            && !cacheCap.getActionCacheUpdateCapabilities().getUpdateEnabled()) {
-          result.addError(
-              "--remote_local_fallback and --remote_upload_local_results are set, "
-                  + "but the current account is not authorized to write local results "
-                  + "to the remote cache.");
-        }
-      } else {
-        // Local execution: check updating remote cache is allowed.
-        if (remoteOptions.remoteUploadLocalResults
-            && !cacheCap.getActionCacheUpdateCapabilities().getUpdateEnabled()) {
-          result.addError(
-              "--remote_upload_local_results is set, but the current account is not authorized "
-                  + "to write local results to the remote cache.");
-        }
+      if (remoteOptions.remoteUploadLocalResults
+          && !cacheCap.getActionCacheUpdateCapabilities().getUpdateEnabled()) {
+        result.addWarning(
+            "--remote_upload_local_results is set, but the remote cache does not support uploading "
+                + "action results or the current account is not authorized to write local results "
+                + "to the remote cache.");
       }
 
       if (remoteOptions.cacheCompression

src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java

@@ -273,7 +273,9 @@ public String getTypeDescription() {
       defaultValue = "true",
       documentationCategory = OptionDocumentationCategory.REMOTE,
       effectTags = {OptionEffectTag.UNKNOWN},
-      help = "Whether to upload locally executed action results to the remote cache.")
+      help =
+          "Whether to upload locally executed action results to the remote cache if the remote "
+              + "cache supports it and the user is authorized to do so.")
   public boolean remoteUploadLocalResults;
 
   @Deprecated

src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java

@@ -308,8 +308,7 @@ public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportDigestFu
   }
 
   @Test
-  public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportUpdate()
-      throws Exception {
+  public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportUpdate() {
     ServerCapabilities caps =
         ServerCapabilities.newBuilder()
             .setLowApiVersion(ApiVersion.current.toSemVer())
@@ -324,9 +323,10 @@ public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportUpdate()
     RemoteServerCapabilities.ClientServerCompatibilityStatus st =
         RemoteServerCapabilities.checkClientServerCompatibility(
             caps, remoteOptions, DigestFunction.Value.SHA256, ServerCapabilitiesRequirement.CACHE);
-    assertThat(st.getErrors()).hasSize(1);
-    assertThat(st.getErrors().get(0))
-        .containsMatch("not authorized to write local results to the remote cache");
+    assertThat(st.getErrors()).isEmpty();
+    assertThat(st.getWarnings()).hasSize(1);
+    assertThat(st.getWarnings().get(0))
+        .contains("remote cache does not support uploading action results");
 
     // Ignored when no local upload.
     remoteOptions.remoteUploadLocalResults = false;
@@ -398,8 +398,7 @@ public void testCheckClientServerCompatibility_remoteExecutionDoesNotSupportDige
   }
 
   @Test
-  public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate()
-      throws Exception {
+  public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate() {
     ServerCapabilities caps =
         ServerCapabilities.newBuilder()
             .setLowApiVersion(ApiVersion.current.toSemVer())
@@ -423,9 +422,10 @@ public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate(
             remoteOptions,
             DigestFunction.Value.SHA256,
             ServerCapabilitiesRequirement.EXECUTION_AND_CACHE);
-    assertThat(st.getErrors()).hasSize(1);
-    assertThat(st.getErrors().get(0))
-        .containsMatch("not authorized to write local results to the remote cache");
+    assertThat(st.getErrors()).isEmpty();
+    assertThat(st.getWarnings()).hasSize(1);
+    assertThat(st.getWarnings().get(0))
+        .contains("remote cache does not support uploading action results");
 
     // Ignored when no fallback.
     remoteOptions.remoteLocalFallback = false;
@@ -435,7 +435,10 @@ public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate(
             remoteOptions,
             DigestFunction.Value.SHA256,
             ServerCapabilitiesRequirement.EXECUTION_AND_CACHE);
-    assertThat(st.isOk()).isTrue();
+    assertThat(st.getErrors()).isEmpty();
+    assertThat(st.getWarnings()).hasSize(1);
+    assertThat(st.getWarnings().get(0))
+        .contains("remote cache does not support uploading action results");
 
     // Ignored when no uploading local results.
     remoteOptions.remoteLocalFallback = true;

src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java

@@ -15,6 +15,7 @@
 
 import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture;
 
+import build.bazel.remote.execution.v2.ActionCacheUpdateCapabilities;
 import build.bazel.remote.execution.v2.CacheCapabilities;
 import build.bazel.remote.execution.v2.Digest;
 import build.bazel.remote.execution.v2.Directory;
@@ -33,9 +34,10 @@
 
 /** A {@link RemoteCache} backed by an {@link DiskCacheClient}. */
 class OnDiskBlobStoreCache extends RemoteCache {
-
   private static final CacheCapabilities CAPABILITIES =
       CacheCapabilities.newBuilder()
+          .setActionCacheUpdateCapabilities(
+              ActionCacheUpdateCapabilities.newBuilder().setUpdateEnabled(true).build())
           .setSymlinkAbsolutePathStrategy(SymlinkAbsolutePathStrategy.Value.ALLOWED)
           .build();