bzlmod: Enforce strict deps for Bzlmod generated repos

================================================================= Bzlmod generated repos can only access repos of its declared direct dependencies. To implement this, new attribute containing information about the owner repo is added to `RepositoryMapping` and `RepositoryName`. * In `RepositoryMapping`, if `ownerRepo` is not null, then it means strict dependency is enforced. * In `RepositoryName`, if `ownerRepoIfInvisible` is not null, then it means the repository name is actually not visible after repo mapping and should fail in `RepositoryDelegatorFunction` when trying to fetch the repository. Working towards: bazelbuild#13793 Related: bazelbuild#13316 RELNOTES: None PiperOrigin-RevId: 395109190
ulrfa · Sep 6, 2021 · 9cb5936 · 9cb5936
1 parent 11f7d80
commit 9cb5936
Show file tree

Hide file tree

Showing 16 changed files with 214 additions and 45 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Module.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Module.java
@@ -129,8 +129,20 @@ public enum WhichRepoMappings {
   }
 
   /** Returns the {@link RepositoryMapping} for the repo corresponding to this module. */
-  public final RepositoryMapping getRepoMapping(WhichRepoMappings whichRepoMappings) {
+  public final RepositoryMapping getRepoMapping(
+      WhichRepoMappings whichRepoMappings, ModuleKey key) {
     ImmutableMap.Builder<RepositoryName, RepositoryName> mapping = ImmutableMap.builder();
+    // If this is the root module, then the main repository should be visible as `@`.
+    if (key == ModuleKey.ROOT) {
+      mapping.put(RepositoryName.MAIN, RepositoryName.MAIN);
+    }
+    // Every module should be able to reference itself as @<module name>.
+    // If this is the root module, this perfectly falls into @<module name> => @
+    if (!getName().isEmpty()) {
+      mapping.put(
+          RepositoryName.createFromValidStrippedName(getName()),
+          RepositoryName.createFromValidStrippedName(key.getCanonicalRepoName()));
+    }
     for (Map.Entry<String, ModuleKey> dep : getDeps().entrySet()) {
       // Special note: if `dep` is actually the root module, its ModuleKey would be ROOT whose
       // canonicalRepoName is the empty string. This perfectly maps to the main repo ("@").
@@ -150,9 +162,7 @@ public final RepositoryMapping getRepoMapping(WhichRepoMappings whichRepoMapping
         }
       }
     }
-    // TODO(wyv): disallow fallback. (we can't do that cleanly right now because we need visibility
-    //   into stuff like @bazel_tools implicitly.)
-    return RepositoryMapping.createAllowingFallback(mapping.build());
+    return RepositoryMapping.create(mapping.build(), key.getCanonicalRepoName());
   }
 
   /**

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java
@@ -100,7 +100,7 @@ public static StarlarkBazelModule create(
     LabelConversionContext labelConversionContext =
         new LabelConversionContext(
             moduleRootLabel,
-            module.getRepoMapping(WhichRepoMappings.BAZEL_DEPS_ONLY),
+            module.getRepoMapping(WhichRepoMappings.BAZEL_DEPS_ONLY, moduleKey),
             /* convertedLabelsInPackage= */ new HashMap<>());
     ImmutableListMultimap.Builder<String, TypeCheckedTag> typeCheckedTags =
         ImmutableListMultimap.builder();

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
@@ -147,13 +147,12 @@ public static PackageIdentifier parse(
       throw new LabelSyntaxException(error);
     }
 
-    if (repositoryMapping != null) {
-      RepositoryName mappedRepo = repositoryMapping.get(RepositoryName.create(repo));
-      if (mappedRepo != null) {
-        return create(mappedRepo, PathFragment.create(packageName));
-      }
+    if (repositoryMapping == null) {
+      return create(repo, PathFragment.create(packageName));
     }
-    return create(repo, PathFragment.create(packageName));
+
+    return create(
+        repositoryMapping.get(RepositoryName.create(repo)), PathFragment.create(packageName));
   }
 
   public RepositoryName getRepository() {

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java
@@ -36,25 +36,39 @@ public abstract class RepositoryMapping {
 
   abstract ImmutableMap<RepositoryName, RepositoryName> repositoryMapping();
 
-  abstract boolean fallback();
+  /**
+   * The owner repo of this repository mapping. It is for providing useful debug information when
+   * repository mapping fails due to enforcing strict dependency, therefore it's only recorded when
+   * we don't fallback to the requested repo name.
+   */
+  @Nullable
+  abstract String ownerRepo();
 
-  public static RepositoryMapping create(Map<RepositoryName, RepositoryName> repositoryMapping) {
+  public static RepositoryMapping create(
+      Map<RepositoryName, RepositoryName> repositoryMapping, String ownerRepo) {
     return new AutoValue_RepositoryMapping(
-        ImmutableMap.copyOf(Preconditions.checkNotNull(repositoryMapping)), /* fallback= */ false);
+        ImmutableMap.copyOf(Preconditions.checkNotNull(repositoryMapping)), ownerRepo);
   }
 
   public static RepositoryMapping createAllowingFallback(
       Map<RepositoryName, RepositoryName> repositoryMapping) {
     return new AutoValue_RepositoryMapping(
-        ImmutableMap.copyOf(Preconditions.checkNotNull(repositoryMapping)), /* fallback= */ true);
+        ImmutableMap.copyOf(Preconditions.checkNotNull(repositoryMapping)), null);
   }
 
-  @Nullable
   public RepositoryName get(RepositoryName repositoryName) {
-    if (fallback()) {
+    // 1. Default repo ("") should always map to default repo
+    // 2. @bazel_tools is a special repo that should be visible to all repositories.
+    if (repositoryName.equals(RepositoryName.DEFAULT)
+        || repositoryName.equals(RepositoryName.BAZEL_TOOLS)) {
+      return repositoryName;
+    }
+    // If the owner repo is not present, that means we should fallback to the requested repo name.
+    if (ownerRepo() == null) {
       return repositoryMapping().getOrDefault(repositoryName, repositoryName);
     } else {
-      return repositoryMapping().get(repositoryName);
+      return repositoryMapping()
+          .getOrDefault(repositoryName, repositoryName.toNonVisible(ownerRepo()));
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
@@ -16,6 +16,7 @@
 
 import com.github.benmanes.caffeine.cache.Caffeine;
 import com.github.benmanes.caffeine.cache.LoadingCache;
+import com.google.common.base.Preconditions;
 import com.google.common.base.Throwables;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
@@ -29,18 +30,25 @@
 import java.io.ObjectOutputStream;
 import java.io.ObjectStreamException;
 import java.io.Serializable;
+import java.util.Objects;
 import java.util.concurrent.CompletionException;
 import java.util.regex.Pattern;
+import javax.annotation.Nullable;
 
 /** A human-readable name for the repository. */
 @AutoCodec
 public final class RepositoryName implements Serializable {
 
   static final String DEFAULT_REPOSITORY = "";
 
+  static final String BAZEL_TOOLS_REPO_NAME = "@bazel_tools";
+
   @SerializationConstant
   public static final RepositoryName DEFAULT = new RepositoryName(DEFAULT_REPOSITORY);
 
+  @SerializationConstant
+  public static final RepositoryName BAZEL_TOOLS = new RepositoryName(BAZEL_TOOLS_REPO_NAME);
+
   @SerializationConstant public static final RepositoryName MAIN = new RepositoryName("@");
 
   private static final Pattern VALID_REPO_NAME = Pattern.compile("@[\\w\\-.]*");
@@ -162,8 +170,21 @@ public static Pair<RepositoryName, PathFragment> fromPathFragment(
 
   private final String name;
 
-  private RepositoryName(String name) {
+  /**
+   * Store the name if the owner repository where this repository name is requested. If this field
+   * is not null, it means this instance represents the requested repository name that is actually
+   * not visible from the owner repository and should fail in {@link RepositoryDelegatorFunction}
+   * when fetching the repository.
+   */
+  private final String ownerRepoIfNotVisible;
+
+  private RepositoryName(String name, String ownerRepoIfNotVisible) {
     this.name = name;
+    this.ownerRepoIfNotVisible = ownerRepoIfNotVisible;
+  }
+
+  private RepositoryName(String name) {
+    this(name, null);
   }
 
   /** Performs validity checking. Returns null on success, an error message otherwise. */
@@ -201,6 +222,25 @@ public String strippedName() {
     return name.substring(1);
   }
 
+  /**
+   * Create a {@link RepositoryName} instance that indicates the requested repository name is
+   * actually not visible from the owner repository and should fail in {@link
+   * RepositoryDelegatorFunction} when fetching with this {@link RepositoryName} instance.
+   */
+  public RepositoryName toNonVisible(String ownerRepo) {
+    Preconditions.checkNotNull(ownerRepo);
+    return new RepositoryName(name, ownerRepo);
+  }
+
+  public boolean isVisible() {
+    return ownerRepoIfNotVisible == null;
+  }
+
+  @Nullable
+  public String getOwnerRepoIfNotVisible() {
+    return ownerRepoIfNotVisible;
+  }
+
   /**
    * Returns the repository name without the leading "{@literal @}". For the default repository,
    * returns "".
@@ -282,11 +322,15 @@ public boolean equals(Object object) {
     if (!(object instanceof RepositoryName)) {
       return false;
     }
-    return OsPathPolicy.getFilePathOs().equals(name, ((RepositoryName) object).name);
+    RepositoryName other = (RepositoryName) object;
+    return OsPathPolicy.getFilePathOs().equals(name, other.name)
+        && OsPathPolicy.getFilePathOs().equals(ownerRepoIfNotVisible, other.ownerRepoIfNotVisible);
   }
 
   @Override
   public int hashCode() {
-    return OsPathPolicy.getFilePathOs().hash(name);
+    return Objects.hash(
+        OsPathPolicy.getFilePathOs().hash(name),
+        OsPathPolicy.getFilePathOs().hash(ownerRepoIfNotVisible));
   }
 }
diff --git a/...main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/...main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
@@ -237,6 +237,13 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       throws InterruptedException, RepositoryFunctionException {
     RepositoryName repositoryName = (RepositoryName) skyKey.argument();
 
+    if (!repositoryName.isVisible()) {
+      return new NoRepositoryDirectoryValue(
+          String.format(
+              "Repository '%s' is not visible from repository '@%s'",
+              repositoryName.getCanonicalForm(), repositoryName.getOwnerRepoIfNotVisible()));
+    }
+
     Map<RepositoryName, PathFragment> overrides = REPOSITORY_OVERRIDES.get(env);
     boolean doNotFetchUnconditionally =
         DONT_FETCH_UNCONDITIONALLY.equals(DEPENDENCY_FOR_UNCONDITIONAL_FETCHING.get(env));

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -864,7 +864,7 @@ private static RepositoryMapping getRepositoryMapping(BzlLoadValue.Key key, Envi
       return selectionValue
           .getDepGraph()
           .get(moduleKey)
-          .getRepoMapping(WhichRepoMappings.BAZEL_DEPS_ONLY);
+          .getRepoMapping(WhichRepoMappings.BAZEL_DEPS_ONLY, moduleKey);
     }
 
     // We are fully done with workspace evaluation so we should get the mappings from the

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java
@@ -81,7 +81,8 @@ private Optional<RepositoryMapping> computeFromBzlmod(
       return Optional.empty();
     }
     Module module = selectionValue.getDepGraph().get(moduleKey);
-    return Optional.of(module.getRepoMapping(WhichRepoMappings.WITH_MODULE_EXTENSIONS_TOO));
+    return Optional.of(
+        module.getRepoMapping(WhichRepoMappings.WITH_MODULE_EXTENSIONS_TOO, moduleKey));
   }
 
   private SkyValue computeFromWorkspace(

diff --git a/src/main/java/com/google/devtools/build/lib/vfs/UnixOsPathPolicy.java b/src/main/java/com/google/devtools/build/lib/vfs/UnixOsPathPolicy.java
@@ -16,6 +16,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Splitter;
 import com.google.common.collect.Iterables;
+import java.util.Objects;
 
 @VisibleForTesting
 class UnixOsPathPolicy implements OsPathPolicy {
@@ -103,11 +104,14 @@ public int compare(char c1, char c2) {
 
   @Override
   public boolean equals(String s1, String s2) {
-    return s1.equals(s2);
+    return Objects.equals(s1, s2);
   }
 
   @Override
   public int hash(String s) {
+    if (s == null) {
+      return 0;
+    }
     return s.hashCode();
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/vfs/WindowsOsPathPolicy.java b/src/main/java/com/google/devtools/build/lib/vfs/WindowsOsPathPolicy.java
@@ -182,12 +182,15 @@ public int compare(char c1, char c2) {
 
   @Override
   public boolean equals(String s1, String s2) {
-    return s1.equalsIgnoreCase(s2);
+    return (s1 == null && s2 == null) || (s1 != null && s1.equalsIgnoreCase(s2));
   }
 
   @Override
   public int hash(String s) {
     // Windows is case-insensitive
+    if (s == null) {
+      return 0;
+    }
     return s.toLowerCase().hashCode();
   }
 

diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodTestUtil.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodTestUtil.java
@@ -36,14 +36,14 @@ public static ModuleKey createModuleKey(String name, String version) {
     }
   }
 
-  public static RepositoryMapping createRepositoryMapping(String... names) {
+  public static RepositoryMapping createRepositoryMapping(ModuleKey key, String... names) {
     ImmutableMap.Builder<RepositoryName, RepositoryName> mappingBuilder = ImmutableMap.builder();
     for (int i = 0; i < names.length; i += 2) {
       mappingBuilder.put(
           RepositoryName.createFromValidStrippedName(names[i]),
           RepositoryName.createFromValidStrippedName(names[i + 1]));
     }
-    return RepositoryMapping.createAllowingFallback(mappingBuilder.build());
+    return RepositoryMapping.create(mappingBuilder.build(), key.getCanonicalRepoName());
   }
 
   public static TagClass createTagClass(Attribute... attrs) {

diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleTest.java
@@ -86,8 +86,11 @@ public void withDepKeysTransformed() throws Exception {
 
   @Test
   public void getRepoMapping() throws Exception {
+    ModuleKey key = createModuleKey("test_module", "1.0");
     Module module =
         Module.builder()
+            .setName(key.getName())
+            .setVersion(key.getVersion())
             .addDep("my_foo", createModuleKey("foo", "1.0"))
             .addDep("my_bar", createModuleKey("bar", "2.0"))
             .addDep("my_root", ModuleKey.ROOT)
@@ -99,12 +102,24 @@ public void getRepoMapping() throws Exception {
                     .setImports(ImmutableBiMap.of("my_guava", "guava"))
                     .build())
             .build();
-    assertThat(module.getRepoMapping(WhichRepoMappings.BAZEL_DEPS_ONLY))
+    assertThat(module.getRepoMapping(WhichRepoMappings.BAZEL_DEPS_ONLY, key))
         .isEqualTo(
-            createRepositoryMapping("my_foo", "foo.1.0", "my_bar", "bar.2.0", "my_root", ""));
-    assertThat(module.getRepoMapping(WhichRepoMappings.WITH_MODULE_EXTENSIONS_TOO))
+            createRepositoryMapping(
+                key,
+                "test_module",
+                "test_module.1.0",
+                "my_foo",
+                "foo.1.0",
+                "my_bar",
+                "bar.2.0",
+                "my_root",
+                ""));
+    assertThat(module.getRepoMapping(WhichRepoMappings.WITH_MODULE_EXTENSIONS_TOO, key))
         .isEqualTo(
             createRepositoryMapping(
+                key,
+                "test_module",
+                "test_module.1.0",
                 "my_foo",
                 "foo.1.0",
                 "my_bar",
@@ -114,4 +129,20 @@ public void getRepoMapping() throws Exception {
                 "my_guava",
                 "maven.guava"));
   }
+
+  @Test
+  public void getRepoMapping_asMainModule() throws Exception {
+    ModuleKey key = ModuleKey.ROOT;
+    Module module =
+        Module.builder()
+            .setName("test_module")
+            .setVersion(Version.parse("1.0"))
+            .addDep("my_foo", createModuleKey("foo", "1.0"))
+            .addDep("my_bar", createModuleKey("bar", "2.0"))
+            .build();
+    assertThat(module.getRepoMapping(WhichRepoMappings.BAZEL_DEPS_ONLY, key))
+        .isEqualTo(
+            createRepositoryMapping(
+                key, "", "", "test_module", "", "my_foo", "foo.1.0", "my_bar", "bar.2.0"));
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTagTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTagTest.java
@@ -17,6 +17,7 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.bazel.bzlmod.BzlmodTestUtil.buildTag;
+import static com.google.devtools.build.lib.bazel.bzlmod.BzlmodTestUtil.createModuleKey;
 import static com.google.devtools.build.lib.bazel.bzlmod.BzlmodTestUtil.createRepositoryMapping;
 import static com.google.devtools.build.lib.bazel.bzlmod.BzlmodTestUtil.createTagClass;
 import static com.google.devtools.build.lib.packages.Attribute.attr;
@@ -62,7 +63,7 @@ public void label() throws Exception {
                 .build(),
             new LabelConversionContext(
                 Label.parseAbsoluteUnchecked("@myrepo//mypkg:defs.bzl"),
-                createRepositoryMapping("repo", "other_repo"),
+                createRepositoryMapping(createModuleKey("test", "1.0"), "repo", "other_repo"),
                 new HashMap<>()));
     assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
     assertThat(typeCheckedTag.getValue("foo"))

diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryMappingTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryMappingTest.java
@@ -39,9 +39,11 @@ public void maybeFallback() throws Exception {
   public void neverFallback() throws Exception {
     RepositoryMapping mapping =
         RepositoryMapping.create(
-            ImmutableMap.of(RepositoryName.create("@A"), RepositoryName.create("@com_foo_bar_a")));
+            ImmutableMap.of(RepositoryName.create("@A"), RepositoryName.create("@com_foo_bar_a")),
+            "fake_owner_repo");
     assertThat(mapping.get(RepositoryName.create("@A")))
         .isEqualTo(RepositoryName.create("@com_foo_bar_a"));
-    assertThat(mapping.get(RepositoryName.create("@B"))).isNull();
+    assertThat(mapping.get(RepositoryName.create("@B")))
+        .isEqualTo(RepositoryName.create("@B").toNonVisible("fake_owner_repo"));
   }
 }