sandbox: Allow network access for builds by default.

This solves a performance issue that slowed down builds by about 40% at least on Linux, due to clone() with CLONE_NEWNET becoming extremely slow (>1 second) for highly parallel builds. See this thread for a discussion: https://lkml.org/lkml/2014/8/20/40

For the sake of consistency, we apply the same policy on OS X, too.

If we find a better way to block network access for processes on Linux that doesn't have this performance hit, we will revisit this.

RELNOTES: Sandboxed builds allow network access for builds by default. Tests will still be run without networking, unless "requires-network" is specified as a tag.

--
MOS_MIGRATED_REVID=131393514

5 files changed, 65 insertions, 32 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
index c248648ecf..cd1429fe6d 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
@@ -35,6 +35,7 @@ import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.analysis.AnalysisUtils;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
+import com.google.devtools.build.lib.buildtool.BuildRequest;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction;
 import com.google.devtools.build.lib.rules.fileset.FilesetActionContext;
@@ -75,9 +76,9 @@ public class DarwinSandboxedStrategy implements SpawnActionContext {
   private final ImmutableMap<String, String> clientEnv;
   private final BlazeDirectories blazeDirs;
   private final Path execRoot;
+  private final BuildRequest buildRequest;
   private final SandboxOptions sandboxOptions;
   private final boolean verboseFailures;
-  private final boolean unblockNetwork;
   private final String productName;
   private final ImmutableList<Path> confPaths;
 
@@ -85,32 +86,30 @@ public class DarwinSandboxedStrategy implements SpawnActionContext {
   private final AtomicInteger execCounter = new AtomicInteger();
 
   private DarwinSandboxedStrategy(
-      SandboxOptions options,
+      BuildRequest buildRequest,
       Map<String, String> clientEnv,
       BlazeDirectories blazeDirs,
       ExecutorService backgroundWorkers,
       boolean verboseFailures,
-      boolean unblockNetwork,
       String productName,
       ImmutableList<Path> confPaths) {
-    this.sandboxOptions = options;
+    this.buildRequest = buildRequest;
+    this.sandboxOptions = buildRequest.getOptions(SandboxOptions.class);
     this.clientEnv = ImmutableMap.copyOf(clientEnv);
     this.blazeDirs = blazeDirs;
     this.execRoot = blazeDirs.getExecRoot();
     this.backgroundWorkers = Preconditions.checkNotNull(backgroundWorkers);
     this.verboseFailures = verboseFailures;
-    this.unblockNetwork = unblockNetwork;
     this.productName = productName;
     this.confPaths = confPaths;
   }
 
   public static DarwinSandboxedStrategy create(
-      SandboxOptions options,
+      BuildRequest buildRequest,
       Map<String, String> clientEnv,
       BlazeDirectories blazeDirs,
       ExecutorService backgroundWorkers,
       boolean verboseFailures,
-      boolean unblockNetwork,
       String productName)
       throws IOException {
     // On OS X, in addition to what is specified in $TMPDIR, two other temporary directories may be
@@ -125,12 +124,11 @@ public class DarwinSandboxedStrategy implements SpawnActionContext {
     }
 
     return new DarwinSandboxedStrategy(
-        options,
+        buildRequest,
         clientEnv,
         blazeDirs,
         backgroundWorkers,
         verboseFailures,
-        unblockNetwork,
         productName,
         writablePaths.build());
   }
@@ -264,9 +262,7 @@ public class DarwinSandboxedStrategy implements SpawnActionContext {
     DarwinSandboxRunner runner;
     try {
       Path sandboxConfigPath =
-          generateScriptFile(
-              sandboxPath,
-              unblockNetwork || spawn.getExecutionInfo().containsKey("requires-network"));
+          generateScriptFile(sandboxPath, SandboxHelpers.shouldAllowNetwork(buildRequest, spawn));
       runner =
           new DarwinSandboxRunner(
               execRoot,
@@ -297,7 +293,7 @@ public class DarwinSandboxedStrategy implements SpawnActionContext {
     return dirs.build();
   }
 
-  private Path generateScriptFile(Path sandboxPath, boolean network) throws IOException {
+  private Path generateScriptFile(Path sandboxPath, boolean allowNetwork) throws IOException {
     FileSystemUtils.createDirectoryAndParents(sandboxPath);
     Path sandboxConfigPath =
         sandboxPath.getParentDirectory().getRelative(sandboxPath.getBaseName() + ".sb");
@@ -316,7 +312,7 @@ public class DarwinSandboxedStrategy implements SpawnActionContext {
       out.println("(allow network* (remote unix-socket (subpath \"/\")))");
       out.println("(allow network* (local unix-socket (subpath \"/\")))");
       // check network
-      if (network) {
+      if (allowNetwork) {
         out.println("(allow network*)");
       }
 
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxRunner.java
index 06c84de165..4a786100f3 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxRunner.java
@@ -122,7 +122,7 @@ public class LinuxSandboxRunner {
       Map<PathFragment, Path> inputs,
       Collection<PathFragment> outputs,
       int timeout,
-      boolean blockNetwork)
+      boolean allowNetwork)
       throws IOException, ExecException {
     createFileSystem(inputs, outputs);
 
@@ -159,7 +159,7 @@ public class LinuxSandboxRunner {
       fileArgs.add(inaccessiblePath.getPathString());
     }
 
-    if (blockNetwork) {
+    if (!allowNetwork) {
       // Block network access out of the namespace.
       fileArgs.add("-N");
     }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
index 4ee34e891e..d6e65c34d7 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
@@ -30,6 +30,7 @@ import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.analysis.AnalysisUtils;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
+import com.google.devtools.build.lib.buildtool.BuildRequest;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction;
 import com.google.devtools.build.lib.rules.fileset.FilesetActionContext;
@@ -69,28 +70,27 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
 
   private final ExecutorService backgroundWorkers;
 
+  private final BuildRequest buildRequest;
   private final SandboxOptions sandboxOptions;
   private final BlazeDirectories blazeDirs;
   private final Path execRoot;
   private final boolean verboseFailures;
-  private final boolean unblockNetwork;
   private final UUID uuid = UUID.randomUUID();
   private final AtomicInteger execCounter = new AtomicInteger();
   private final String productName;
 
   LinuxSandboxedStrategy(
-      SandboxOptions options,
+      BuildRequest buildRequest,
       BlazeDirectories blazeDirs,
       ExecutorService backgroundWorkers,
       boolean verboseFailures,
-      boolean unblockNetwork,
       String productName) {
-    this.sandboxOptions = options;
+    this.buildRequest = buildRequest;
+    this.sandboxOptions = buildRequest.getOptions(SandboxOptions.class);
     this.blazeDirs = blazeDirs;
     this.execRoot = blazeDirs.getExecRoot();
     this.backgroundWorkers = Preconditions.checkNotNull(backgroundWorkers);
     this.verboseFailures = verboseFailures;
-    this.unblockNetwork = unblockNetwork;
     this.productName = productName;
   }
 
@@ -170,7 +170,7 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
             mounts,
             outputFiles.build(),
             timeout,
-            !this.unblockNetwork && !spawn.getExecutionInfo().containsKey("requires-network"));
+            SandboxHelpers.shouldAllowNetwork(buildRequest, spawn));
       } finally {
         // Due to the Linux kernel behavior, if we try to remove the sandbox too quickly after the
         // process has exited, we get "Device busy" errors because some of the mounts have not yet
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
index 7bf38c901b..40f11aaf0e 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.sandbox;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.ActionContextProvider;
 import com.google.devtools.build.lib.actions.Executor.ActionContext;
-import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.buildtool.BuildRequest;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.exec.ExecutionOptions;
@@ -46,11 +45,6 @@ public class SandboxActionContextProvider extends ActionContextProvider {
       CommandEnvironment env, BuildRequest buildRequest, ExecutorService backgroundWorkers)
       throws IOException {
     boolean verboseFailures = buildRequest.getOptions(ExecutionOptions.class).verboseFailures;
-    boolean unblockNetwork =
-        buildRequest
-            .getOptions(BuildConfiguration.Options.class)
-            .testArguments
-            .contains("--wrapper_script_flag=--debug");
     ImmutableList.Builder<ActionContext> contexts = ImmutableList.builder();
 
     switch (OS.getCurrent()) {
@@ -58,11 +52,10 @@ public class SandboxActionContextProvider extends ActionContextProvider {
         if (LinuxSandboxedStrategy.isSupported(env)) {
           contexts.add(
               new LinuxSandboxedStrategy(
-                  buildRequest.getOptions(SandboxOptions.class),
+                  buildRequest,
                   env.getDirectories(),
                   backgroundWorkers,
                   verboseFailures,
-                  unblockNetwork,
                   env.getRuntime().getProductName()));
         } else if (!buildRequest.getOptions(SandboxOptions.class).ignoreUnsupportedSandboxing) {
           env.getReporter().handle(Event.warn(SANDBOX_NOT_SUPPORTED_MESSAGE));
@@ -72,12 +65,11 @@ public class SandboxActionContextProvider extends ActionContextProvider {
         if (DarwinSandboxRunner.isSupported()) {
           contexts.add(
               DarwinSandboxedStrategy.create(
-                  buildRequest.getOptions(SandboxOptions.class),
+                  buildRequest,
                   env.getClientEnv(),
                   env.getDirectories(),
                   backgroundWorkers,
                   verboseFailures,
-                  unblockNetwork,
                   env.getRuntime().getProductName()));
         }
         break;
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
new file mode 100644
index 0000000000..bd8c365c3c
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
@@ -0,0 +1,45 @@
+// Copyright 2016 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.sandbox;
+
+import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
+import com.google.devtools.build.lib.buildtool.BuildRequest;
+
+/** Helper methods that are shared by the different sandboxing strategies in this package. */
+final class SandboxHelpers {
+
+  static boolean shouldAllowNetwork(BuildRequest buildRequest, Spawn spawn) {
+    // If we don't run tests, allow network access.
+    if (!buildRequest.shouldRunTests()) {
+      return true;
+    }
+
+    // If the Spawn specifically requests network access, allow it.
+    if (spawn.getExecutionInfo().containsKey("requires-network")) {
+      return true;
+    }
+
+    // Allow network access, when --java_debug is specified, otherwise we can't connect to the
+    // remote debug server of the test.
+    if (buildRequest
+        .getOptions(BuildConfiguration.Options.class)
+        .testArguments
+        .contains("--wrapper_script_flag=--debug")) {
+      return true;
+    }
+
+    return false;
+  }
+}