Allow banning symlink action outputs from being uploaded to a remote cache.

This is mostly a roll-forward of 4465dae23de989f1452e93d0a88ac2a289103dd9, which
was reverted by fa36d2f48965b127e8fd397348d16e991135bfb6. The main difference is
that the new behavior is now gated behind the --noremote_allow_symlink_upload
flag.

https://docs.google.com/document/d/1gnOYszitgrLVet3sQk-TKGqIcpkkDsc6aw-izoo-d64
is a design proposal to support symlinks in the remote cache, which would render
this change moot. I'd like to be able to prevent incorrect cache behavior until
that change is implemented, though.

This fixes https://github.com/bazelbuild/bazel/issues/4840 (again).

Closes #5122.

Change-Id: I2136cfe82c2e1a8a9f5856e12a37d42cabd0e299
PiperOrigin-RevId: 195261827

4 files changed, 114 insertions, 17 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
index ab3bc4286e..9b9f85e42c 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
@@ -44,7 +44,6 @@ import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.Root;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
-import java.util.Collection;
 import java.util.List;
 import org.junit.Before;
 import org.junit.Test;
@@ -161,7 +160,7 @@ public class AbstractSpawnStrategyTest {
 
     // Must only be called exactly once.
     verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionContext.class));
-    verify(entry).store(eq(spawnResult), any(Collection.class));
+    verify(entry).store(eq(spawnResult));
   }
 
   @SuppressWarnings("unchecked")
@@ -192,7 +191,7 @@ public class AbstractSpawnStrategyTest {
     }
     // Must only be called exactly once.
     verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionContext.class));
-    verify(entry).store(eq(result), any(Collection.class));
+    verify(entry).store(eq(result));
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java
new file mode 100644
index 0000000000..821bb17a94
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java
@@ -0,0 +1,95 @@
+// Copyright 2018 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.remote;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.clock.JavaClock;
+import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.UploadManifest;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import com.google.devtools.remoteexecution.v1test.ActionResult;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link AbstractRemoteActionCache}. */
+@RunWith(JUnit4.class)
+public class AbstractRemoteActionCacheTests {
+
+  private FileSystem fs;
+  private Path execRoot;
+  private final DigestUtil digestUtil = new DigestUtil(HashFunction.SHA256);
+
+  @Before
+  public void setUp() throws Exception {
+    fs = new InMemoryFileSystem(new JavaClock(), HashFunction.SHA256);
+    execRoot = fs.getPath("/execroot");
+    execRoot.createDirectory();
+  }
+
+  @Test
+  public void uploadSymlinkAsFile() throws Exception {
+    ActionResult.Builder result = ActionResult.newBuilder();
+    Path link = fs.getPath("/execroot/link");
+    Path target = fs.getPath("/execroot/target");
+    FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5});
+    link.createSymbolicLink(target);
+
+    UploadManifest um = new UploadManifest(digestUtil, result, execRoot, true);
+    um.addFiles(ImmutableList.of(link));
+    assertThat(um.getDigestToFile()).containsExactly(digestUtil.compute(target), link);
+
+    assertThat(
+            assertThrows(
+                ExecException.class,
+                () ->
+                    new UploadManifest(digestUtil, result, execRoot, false)
+                        .addFiles(ImmutableList.of(link))))
+        .hasMessageThat()
+        .contains("Only regular files and directories may be uploaded to a remote cache.");
+  }
+
+  @Test
+  public void uploadSymlinkInDirectory() throws Exception {
+    ActionResult.Builder result = ActionResult.newBuilder();
+    Path dir = fs.getPath("/execroot/dir");
+    dir.createDirectory();
+    Path target = fs.getPath("/execroot/target");
+    FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5});
+    Path link = fs.getPath("/execroot/dir/link");
+    link.createSymbolicLink(fs.getPath("/execroot/target"));
+
+    UploadManifest um = new UploadManifest(digestUtil, result, fs.getPath("/execroot"), true);
+    um.addFiles(ImmutableList.of(link));
+    assertThat(um.getDigestToFile()).containsExactly(digestUtil.compute(target), link);
+
+    assertThat(
+            assertThrows(
+                ExecException.class,
+                () ->
+                    new UploadManifest(digestUtil, result, execRoot, false)
+                        .addFiles(ImmutableList.of(link))))
+        .hasMessageThat()
+        .contains("Only regular files and directories may be uploaded to a remote cache.");
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
index 8baaa26564..cae9840216 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -169,7 +169,7 @@ public class RemoteSpawnCacheTest {
             ImmutableMap.of("VARIABLE", "value"),
             /*executionInfo=*/ ImmutableMap.<String, String>of(),
             /*inputs=*/ ImmutableList.of(ActionInputHelper.fromPath("input")),
-            /*outputs=*/ ImmutableList.<ActionInput>of(),
+            /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath("/random/file")),
             ResourceSet.ZERO);
 
     Path stdout = fs.getPath("/tmp/stdout");
@@ -273,7 +273,7 @@ public class RemoteSpawnCacheTest {
             })
         .when(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
-    entry.store(result, outputFiles);
+    entry.store(result);
     verify(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
     assertThat(progressUpdates)
@@ -285,14 +285,15 @@ public class RemoteSpawnCacheTest {
     // Checks that spawns that have mayBeCached false are not looked up in the remote cache,
     // and also that their result is not uploaded to the remote cache. The artifacts, however,
     // are uploaded.
-    SimpleSpawn uncacheableSpawn = new SimpleSpawn(
-        new FakeOwner("foo", "bar"),
-        /*arguments=*/ ImmutableList.of(),
-        /*environment=*/ ImmutableMap.of(),
-        ImmutableMap.of(ExecutionRequirements.NO_CACHE, ""),
-        /*inputs=*/ ImmutableList.of(),
-        /*outputs=*/ ImmutableList.<ActionInput>of(),
-        ResourceSet.ZERO);
+    SimpleSpawn uncacheableSpawn =
+        new SimpleSpawn(
+            new FakeOwner("foo", "bar"),
+            /*arguments=*/ ImmutableList.of(),
+            /*environment=*/ ImmutableMap.of(),
+            ImmutableMap.of(ExecutionRequirements.NO_CACHE, ""),
+            /*inputs=*/ ImmutableList.of(),
+            /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath("/random/file")),
+            ResourceSet.ZERO);
     CacheHandle entry = cache.lookup(uncacheableSpawn, simplePolicy);
     verify(remoteCache, never())
         .getCachedActionResult(any(ActionKey.class));
@@ -303,8 +304,8 @@ public class RemoteSpawnCacheTest {
             .setStatus(Status.SUCCESS)
             .setRunnerName("test")
             .build();
+    entry.store(result);
     ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
-    entry.store(result, outputFiles);
     verify(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false));
     assertThat(progressUpdates).containsExactly();
@@ -324,7 +325,7 @@ public class RemoteSpawnCacheTest {
             .setRunnerName("test")
             .build();
     ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
-    entry.store(result, outputFiles);
+    entry.store(result);
     verify(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false));
     assertThat(progressUpdates)
@@ -347,7 +348,7 @@ public class RemoteSpawnCacheTest {
         .when(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
 
-    entry.store(result, outputFiles);
+    entry.store(result);
     verify(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
 
diff --git a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
index 0f241649cb..9ef1add565 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import com.google.devtools.common.options.Options;
 import com.google.devtools.remoteexecution.v1test.ActionResult;
 import com.google.devtools.remoteexecution.v1test.Digest;
 import com.google.devtools.remoteexecution.v1test.Directory;
@@ -74,7 +75,8 @@ public class SimpleBlobStoreActionCacheTest {
   }
 
   private SimpleBlobStoreActionCache newClient(ConcurrentMap<String, byte[]> map) {
-    return new SimpleBlobStoreActionCache(new ConcurrentMapBlobStore(map), DIGEST_UTIL);
+    return new SimpleBlobStoreActionCache(
+        Options.getDefaults(RemoteOptions.class), new ConcurrentMapBlobStore(map), DIGEST_UTIL);
   }
 
   @Test