Skip to content

Commit

Permalink
bzlmod: Enforce strict deps for Bzlmod generated repos
Browse files Browse the repository at this point in the history
=================================================================

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: #13793

Related: #13316

RELNOTES: None
PiperOrigin-RevId: 395109190
  • Loading branch information
meteorcloudy authored and copybara-github committed Sep 6, 2021
1 parent 11f7d80 commit 9cb5936
Show file tree
Hide file tree
Showing 16 changed files with 214 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 ("@").
Expand All @@ -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());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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\\-.]*");
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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 "".
Expand Down Expand Up @@ -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));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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",
Expand All @@ -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"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
}
}
Loading

0 comments on commit 9cb5936

Please sign in to comment.