Add an optional allowlist for external network in tests.
PiperOrigin-RevId: 387118640
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Allowlist.java b/src/main/java/com/google/devtools/build/lib/analysis/Allowlist.java
index b6ff313..0f50913 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/Allowlist.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/Allowlist.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.analysis;
+import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
import static java.util.Objects.requireNonNull;
@@ -25,6 +26,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
+import javax.annotation.Nullable;
/**
* Class used for implementing allowlists using package groups.
@@ -97,6 +99,15 @@
public static PackageSpecificationProvider fetchPackageSpecificationProvider(
RuleContext ruleContext, String allowlistName) {
+ return checkNotNull(
+ fetchPackageSpecificationProviderOrNull(ruleContext, allowlistName),
+ "Allowlist argument for %s not found",
+ allowlistName);
+ }
+
+ @Nullable
+ public static PackageSpecificationProvider fetchPackageSpecificationProviderOrNull(
+ RuleContext ruleContext, String allowlistName) {
for (String attributeName : getAttributeNameFromAllowlistName(allowlistName)) {
if (!ruleContext.isAttrDefined(attributeName, LABEL)) {
continue;
@@ -107,7 +118,7 @@
packageGroup.getProvider(PackageSpecificationProvider.class);
return requireNonNull(packageSpecificationProvider, packageGroup.getLabel().toString());
}
- throw new AssertionError();
+ return null;
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
index bba6923..e804edb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
@@ -186,7 +186,7 @@
public static final class TestBaseRule implements RuleDefinition {
@Override
public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
- return builder
+ builder
.addExecGroup(TEST_RUNNER_EXEC_GROUP)
.requiresConfigurationFragments(TestConfiguration.class)
// TestConfiguration only needed to create TestAction and TestProvider
@@ -259,8 +259,15 @@
coverageReportGeneratorAttribute(
env.getToolsLabel(DEFAULT_COVERAGE_REPORT_GENERATOR_VALUE))))
// The target itself and run_under both run on the same machine.
- .add(attr(":run_under", LABEL).value(RUN_UNDER).skipPrereqValidatorCheck())
- .build();
+ .add(attr(":run_under", LABEL).value(RUN_UNDER).skipPrereqValidatorCheck());
+
+ env.getNetworkAllowlistForTests()
+ .ifPresent(
+ label ->
+ builder.add(
+ Allowlist.getAttributeFromAllowlistName("$network_allowlist").value(label)));
+
+ return builder.build();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
index 0c93f7c..3dffb6a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
@@ -74,6 +74,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.TreeSet;
import java.util.zip.ZipEntry;
@@ -171,6 +172,9 @@
private ThirdPartyLicenseExistencePolicy thirdPartyLicenseExistencePolicy =
ThirdPartyLicenseExistencePolicy.USER_CONTROLLABLE;
+ // TODO(b/192694287): Remove once we migrate all tests from the allowlist
+ @Nullable private Label networkAllowlistForTests;
+
public Builder addWorkspaceFilePrefix(String contents) {
defaultWorkspaceFilePrefix.append(contents);
return this;
@@ -572,6 +576,16 @@
public String getToolsRepository() {
return toolsRepository;
}
+
+ public Builder setNetworkAllowlistForTests(Label allowlist) {
+ networkAllowlistForTests = allowlist;
+ return this;
+ }
+
+ @Override
+ public Optional<Label> getNetworkAllowlistForTests() {
+ return Optional.ofNullable(networkAllowlistForTests);
+ }
}
/** Default content that should be added at the beginning of the WORKSPACE file. */
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleDefinitionEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleDefinitionEnvironment.java
index f9f646b..6db1e4a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleDefinitionEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleDefinitionEnvironment.java
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.analysis;
import com.google.devtools.build.lib.cmdline.Label;
+import java.util.Optional;
/**
* A minimal context available during rule definition, for both native and starlark rules.
@@ -33,7 +34,16 @@
* <p>TODO(brandjon,twigg): Require override to handle repositoryMapping? Note that
* Label.parseAbsoluteUnchecked itself is deprecated because of repositoryMapping!
*/
- public default Label getToolsLabel(String labelValue) {
+ default Label getToolsLabel(String labelValue) {
return Label.parseAbsoluteUnchecked(getToolsRepository() + labelValue);
}
+
+ /**
+ * Returns a label for network allowlist for tests if one should be added.
+ *
+ * <p>TODO(b/192694287): Remove once we migrate all tests from the allowlist.
+ */
+ default Optional<Label> getNetworkAllowlistForTests() {
+ return Optional.empty();
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
index 9349cbc..f2549c0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
@@ -34,6 +34,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.Allowlist;
import com.google.devtools.build.lib.analysis.BaseRuleClasses;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.analysis.TemplateVariableInfo;
@@ -172,87 +173,95 @@
.build();
/** Parent rule class for test Starlark rules. */
- public static final RuleClass getTestBaseRule(RuleDefinitionEnvironment env) {
+ public static RuleClass getTestBaseRule(RuleDefinitionEnvironment env) {
String toolsRepository = env.getToolsRepository();
- return new RuleClass.Builder("$test_base_rule", RuleClassType.ABSTRACT, true, baseRule)
- .requiresConfigurationFragments(TestConfiguration.class)
- // TestConfiguration only needed to create TestAction and TestProvider
- // Only necessary at top-level and can be skipped if trimmed.
- .setMissingFragmentPolicy(TestConfiguration.class, MissingFragmentPolicy.IGNORE)
- .add(
- attr("size", STRING)
- .value("medium")
- .taggable()
- .nonconfigurable("used in loading phase rule validation logic"))
- .add(
- attr("timeout", STRING)
- .taggable()
- .nonconfigurable("policy decision: should be consistent across configurations")
- .value(TIMEOUT_DEFAULT))
- .add(
- attr("flaky", BOOLEAN)
- .value(false)
- .taggable()
- .nonconfigurable("taggable - called in Rule.getRuleTags"))
- .add(attr("shard_count", INTEGER).value(StarlarkInt.of(-1)))
- .add(
- attr("local", BOOLEAN)
- .value(false)
- .taggable()
- .nonconfigurable(
- "policy decision: this should be consistent across configurations"))
- .add(attr("args", STRING_LIST))
- // Input files for every test action
- .add(
- attr("$test_wrapper", LABEL)
- .cfg(HostTransition.createFactory())
- .singleArtifact()
- .value(labelCache.get(toolsRepository + "//tools/test:test_wrapper")))
- .add(
- attr("$xml_writer", LABEL)
- .cfg(HostTransition.createFactory())
- .singleArtifact()
- .value(labelCache.get(toolsRepository + "//tools/test:xml_writer")))
- .add(
- attr("$test_runtime", LABEL_LIST)
- .cfg(HostTransition.createFactory())
- // Getting this default value through the getTestRuntimeLabelList helper ensures we
- // reuse the same ImmutableList<Label> instance for each $test_runtime attr.
- .value(getTestRuntimeLabelList(env)))
- .add(
- attr("$test_setup_script", LABEL)
- .cfg(HostTransition.createFactory())
- .singleArtifact()
- .value(labelCache.get(toolsRepository + "//tools/test:test_setup")))
- .add(
- attr("$xml_generator_script", LABEL)
- .cfg(HostTransition.createFactory())
- .singleArtifact()
- .value(labelCache.get(toolsRepository + "//tools/test:test_xml_generator")))
- .add(
- attr("$collect_coverage_script", LABEL)
- .cfg(HostTransition.createFactory())
- .singleArtifact()
- .value(labelCache.get(toolsRepository + "//tools/test:collect_coverage")))
- // Input files for test actions collecting code coverage
- .add(
- attr(":coverage_support", LABEL)
- .cfg(HostTransition.createFactory())
- .value(
- BaseRuleClasses.coverageSupportAttribute(
- labelCache.get(
- toolsRepository + BaseRuleClasses.DEFAULT_COVERAGE_SUPPORT_VALUE))))
- // Used in the one-per-build coverage report generation action.
- .add(
- attr(":coverage_report_generator", LABEL)
- .cfg(HostTransition.createFactory())
- .value(
- BaseRuleClasses.coverageReportGeneratorAttribute(
- labelCache.get(
- toolsRepository
- + BaseRuleClasses.DEFAULT_COVERAGE_REPORT_GENERATOR_VALUE))))
- .add(attr(":run_under", LABEL).value(RUN_UNDER))
- .build();
+ RuleClass.Builder builder =
+ new RuleClass.Builder("$test_base_rule", RuleClassType.ABSTRACT, true, baseRule)
+ .requiresConfigurationFragments(TestConfiguration.class)
+ // TestConfiguration only needed to create TestAction and TestProvider
+ // Only necessary at top-level and can be skipped if trimmed.
+ .setMissingFragmentPolicy(TestConfiguration.class, MissingFragmentPolicy.IGNORE)
+ .add(
+ attr("size", STRING)
+ .value("medium")
+ .taggable()
+ .nonconfigurable("used in loading phase rule validation logic"))
+ .add(
+ attr("timeout", STRING)
+ .taggable()
+ .nonconfigurable("policy decision: should be consistent across configurations")
+ .value(TIMEOUT_DEFAULT))
+ .add(
+ attr("flaky", BOOLEAN)
+ .value(false)
+ .taggable()
+ .nonconfigurable("taggable - called in Rule.getRuleTags"))
+ .add(attr("shard_count", INTEGER).value(StarlarkInt.of(-1)))
+ .add(
+ attr("local", BOOLEAN)
+ .value(false)
+ .taggable()
+ .nonconfigurable(
+ "policy decision: this should be consistent across configurations"))
+ .add(attr("args", STRING_LIST))
+ // Input files for every test action
+ .add(
+ attr("$test_wrapper", LABEL)
+ .cfg(HostTransition.createFactory())
+ .singleArtifact()
+ .value(labelCache.get(toolsRepository + "//tools/test:test_wrapper")))
+ .add(
+ attr("$xml_writer", LABEL)
+ .cfg(HostTransition.createFactory())
+ .singleArtifact()
+ .value(labelCache.get(toolsRepository + "//tools/test:xml_writer")))
+ .add(
+ attr("$test_runtime", LABEL_LIST)
+ .cfg(HostTransition.createFactory())
+ // Getting this default value through the getTestRuntimeLabelList helper ensures
+ // we reuse the same ImmutableList<Label> instance for each $test_runtime attr.
+ .value(getTestRuntimeLabelList(env)))
+ .add(
+ attr("$test_setup_script", LABEL)
+ .cfg(HostTransition.createFactory())
+ .singleArtifact()
+ .value(labelCache.get(toolsRepository + "//tools/test:test_setup")))
+ .add(
+ attr("$xml_generator_script", LABEL)
+ .cfg(HostTransition.createFactory())
+ .singleArtifact()
+ .value(labelCache.get(toolsRepository + "//tools/test:test_xml_generator")))
+ .add(
+ attr("$collect_coverage_script", LABEL)
+ .cfg(HostTransition.createFactory())
+ .singleArtifact()
+ .value(labelCache.get(toolsRepository + "//tools/test:collect_coverage")))
+ // Input files for test actions collecting code coverage
+ .add(
+ attr(":coverage_support", LABEL)
+ .cfg(HostTransition.createFactory())
+ .value(
+ BaseRuleClasses.coverageSupportAttribute(
+ labelCache.get(
+ toolsRepository + BaseRuleClasses.DEFAULT_COVERAGE_SUPPORT_VALUE))))
+ // Used in the one-per-build coverage report generation action.
+ .add(
+ attr(":coverage_report_generator", LABEL)
+ .cfg(HostTransition.createFactory())
+ .value(
+ BaseRuleClasses.coverageReportGeneratorAttribute(
+ labelCache.get(
+ toolsRepository
+ + BaseRuleClasses.DEFAULT_COVERAGE_REPORT_GENERATOR_VALUE))))
+ .add(attr(":run_under", LABEL).value(RUN_UNDER));
+
+ env.getNetworkAllowlistForTests()
+ .ifPresent(
+ label ->
+ builder.add(
+ Allowlist.getAttributeFromAllowlistName("$network_allowlist").value(label)));
+
+ return builder.build();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
index ecb2f35..c63c53c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
@@ -17,6 +17,7 @@
import static com.google.devtools.build.lib.analysis.BaseRuleClasses.TEST_RUNNER_EXEC_GROUP;
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
+import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
@@ -25,9 +26,11 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
+import com.google.devtools.build.lib.analysis.Allowlist;
import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
import com.google.devtools.build.lib.analysis.FileProvider;
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
+import com.google.devtools.build.lib.analysis.PackageSpecificationProvider;
import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.Runfiles;
@@ -43,6 +46,8 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.packages.TestTimeout;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -66,6 +71,10 @@
private static final String COVERAGE_REPORTED_TO_ACTUAL_SOURCES_FILE =
"COVERAGE_REPORTED_TO_ACTUAL_SOURCES_FILE";
+ @VisibleForSerialization @SerializationConstant
+ static final PackageSpecificationProvider EMPTY_PACKAGES_PROVIDER =
+ () -> NestedSetBuilder.emptySet(Order.STABLE_ORDER);
+
private final RuleContext ruleContext;
private final ImmutableList.Builder<Artifact> additionalTools;
private RunfilesSupport runfilesSupport;
@@ -435,7 +444,13 @@
tools.build(),
splitCoveragePostProcessing,
lcovMergerFilesToRun,
- lcovMergerRunfilesSupplier);
+ lcovMergerRunfilesSupplier,
+ // Network allowlist only makes sense in workspaces which explicitly add it, use an
+ // empty one as a fallback.
+ MoreObjects.firstNonNull(
+ Allowlist.fetchPackageSpecificationProviderOrNull(
+ ruleContext, "$network_allowlist"),
+ EMPTY_PACKAGES_PROVIDER));
testOutputs.addAll(testRunnerAction.getSpawnOutputs());
testOutputs.addAll(testRunnerAction.getOutputs());
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
index 0b21880..b23d95a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
@@ -48,6 +48,7 @@
import com.google.devtools.build.lib.actions.SpawnExecutedEvent;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.TestExecException;
+import com.google.devtools.build.lib.analysis.PackageSpecificationProvider;
import com.google.devtools.build.lib.analysis.SingleRunfilesSupplier;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.RunUnder;
@@ -157,6 +158,9 @@
private final NestedSetBuilder<Artifact> lcovMergerFilesToRun;
private final RunfilesSupplier lcovMergerRunfilesSupplier;
+ // TODO(b/192694287): Remove once we migrate all tests from the allowlist.
+ private final PackageSpecificationProvider networkAllowlist;
+
private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
ImmutableSet.Builder<Artifact> builder = ImmutableSet.builder();
for (Artifact artifact : artifacts) {
@@ -198,7 +202,8 @@
Iterable<Artifact> tools,
boolean splitCoveragePostProcessing,
NestedSetBuilder<Artifact> lcovMergerFilesToRun,
- RunfilesSupplier lcovMergerRunfilesSupplier) {
+ RunfilesSupplier lcovMergerRunfilesSupplier,
+ PackageSpecificationProvider networkAllowlist) {
super(
owner,
NestedSetBuilder.wrap(Order.STABLE_ORDER, tools),
@@ -256,6 +261,7 @@
this.splitCoveragePostProcessing = splitCoveragePostProcessing;
this.lcovMergerFilesToRun = lcovMergerFilesToRun;
this.lcovMergerRunfilesSupplier = lcovMergerRunfilesSupplier;
+ this.networkAllowlist = networkAllowlist;
// Mark all possible test outputs for deletion before test execution.
// TestRunnerAction potentially can create many more non-declared outputs - xml output, coverage
@@ -824,6 +830,10 @@
return testLog;
}
+ public PackageSpecificationProvider getNetworkAllowlist() {
+ return networkAllowlist;
+ }
+
@Override
public ActionContinuationOrResult beginExecution(ActionExecutionContext actionExecutionContext)
throws InterruptedException, ActionExecutionException {
