Handling yanked module version in the BCR

[meteorcloudy]

In the original Bzlmod design doc: Bazel External Dependencies Overhaul, we planned to implement the yanked version feature in the BCR. Currently, this is still a no-op even if we add a yanked version into the metadata.json file.

In the original design, we basically put "Bazel throws an error when the yanked version ends up in the resolved graph. If a lock file is used, Bazel will still be able to fetch the source archive." But as brought up in #81 (comment), there are more design choices and details to explore for this feature.

In general, we have to balance between "preventing users from depending on the yanked version" vs "build reproducibility".

I think there are two potentially viable solutions:

Solution 1 (updated with input from @alexeagle )

1. Add the yanked version and yanked reason in the metadata.json file of the module. Eg. "yanked_versions": {"1.2.11": "zlib 1.2.11 is yanked due to CVE-2018-25032, please upgrade to 1.2.12"}

2. If a yanked version appears in the resolved dependency graph, Bazel throws an error, which contains the yanked reason and the way to resolve/suppress the error.

3. Then users can upgrade the module to a new version by adding a bazel_dep(name = "foo", version = "<new_version>") statement in the MODULE.bazel file.

4. In case the module is not a direct dependency, users can add bazel_dep(name = "foo", version = "<new_version>", repo_name=None), which upgrades the module version but doesn't give any repo visibility from the root module.

5. Users can also suppress the error by adding --allow_yanked_version=<module_name>@<version> or set BZLMOD_ALLOW_YANKED_VERSION=<module_name>@<version> env var, then Bazel prints a warning about the suppression, and the build can continue.

Pros

• The dependency resolution is still deterministic even after adding a yanked version.
• Users are not forced to upgrade the module to a new version which could contain breaking changes.

Cons

• Users have to manually add bazel_dep to upgrade the module. Mitigate: In case the module is a direct dependency, update the bazel_dep directive is what the expected solution anyway, in case it's not, using repo_name=None with bazel_dep can still update the module and still follow strict deps.
• The build will break if an error (instead of a warning) is thrown because a resolved version is yanked. Mitigate: The build error can be easily suppressed by a flag or an env var.

Solution 2

1. Add the yanked version in the metadata.json file of the module, also record the alternative version users should upgrade to. Eg. "yanked_versions": {"1.2.11": "1.2.12"}
2. During dependency resolution, we replace the yanked version with its alternative version.

Pros

• Rerunning the dependency resolution automatically upgrades the module to a suitable version to avoid the yanked version.

Cons

• The dependency resolution is not reproducible after adding a yanked version
• There is still a chance that the alternative version will break users.

In both solutions, if a lock file is used, users would still be able to download the source archive of the yanked dependency. This is similar to cargo: https://doc.rust-lang.org/cargo/commands/cargo-yank.html Basically, lock file can prevent any breakages caused by the BCR content change because dependency resolution is skipped.

Feel free to share your opinions on this or propose other possible solutions!

[jsharpe]

The dependency resolution is not reproducible after adding a yanked version

This is only the case if we don't pin the commit of the registry that is used. If the commit from the registry is pinned then this issue goes away?

[Wyverald]

First off, just to be clear, BCR at HEAD contains all the versions of all the modules. Using BCR at HEAD does not mean using all modules in there at HEAD.

everything else in bazel is designed to be pinned to a give commit; why shouldn't the BCR also be the same?

"Everything else" right now is all the dependency versions; there is a very good reason to want them to be pinned. However, pinning the registry itself just adds to the user burden -- if you wanted to update a module, you'd need to first check which commit of the registry you need, change that, and then change the version of the module you're asking for. That is very cumbersome and is actually part of what we looked to solve with Bzlmod.

So there is inherently an implicit coupling between bazel version and BCR HEAD.

That's true, mostly because we inline the MODULE.bazel content. We're looking at having a branched model for BCR (a branch for each LTS version) to support changes in the syntax/semantics of MODULE.bazel files.

[jsharpe]

First off, just to be clear, BCR at HEAD contains all the versions of all the modules. Using BCR at HEAD does not mean using all modules in there at HEAD.

everything else in bazel is designed to be pinned to a give commit; why shouldn't the BCR also be the same?

"Everything else" right now is all the dependency versions; there is a very good reason to want them to be pinned. However, pinning the registry itself just adds to the user burden -- if you wanted to update a module, you'd need to first check which commit of the registry you need, change that, and then change the version of the module you're asking for. That is very cumbersome and is actually part of what we looked to solve with Bzlmod.

I guess many package managers solve this with a .lock file. They provide tooling to generate the lockfile and then the user checks this. in to the lock in the dependency. I guess the reason I'm in favour of pinning the registry commit is because this removes the burden of responsibility on the maintainers to maintain old bzlmod entries in the registry. With the current model you can never change any historic modules in the registry because you may break someone.

I envisage that most users would simply pickup the current HEAD of the BCR and then pick their dependencies based on what is available at that commit. This is similar to what most package managers do - have an update which pulls down the latest metadata. I'm imagining a tool similar to yarn's upgrade-interactive tool which automatically offers all available upgrades to a user for versions.

I think having the user have to specify a single commit sha vs one per dependency is a much better position than we are currently in.

So there is inherently an implicit coupling between bazel version and BCR HEAD.

That's true, mostly because we inline the MODULE.bazel content. We're looking at having a branched model for BCR (a branch for each LTS version) to support changes in the syntax/semantics of MODULE.bazel files.

Its not just MODULE.bazel but the entire syntax / semantics of a given bazel version. I'm not sure whether a branched model is necesssarily the correct solution unless it can be automated as this creates additional burden on a maintainer to test and merge to multiple branches. Maybe the registry needs a 'overlay' type structure in subdirectories per bazel version so that optionally an overriden version may be provided. I've seen KitWare do similar things with CMake Superbuilds - you can provide a macos or windows or linux specific build recipe that overrides the generic one. Something similar could work for providing backwards compatibility patches for LTS versions because not every module will require specific changes.

[Wyverald]

With the current model you can never change any historic modules in the registry because you may break someone.

What kind of changes do you have in mind? Are you saying that you might want to make some changes to existing module versions, so you'd prefer if users pinned the registry commit? But that's sounding much more like a Debian/Nixpkgs type of registry, where the whole registry is a "snapshot" of a lot of modules working together and there's no version resolution, as opposed to the Maven/npm/Cargo type of registry, which BCR is more similar to.

I think having the user have to specify a single commit sha vs one per dependency is a much better position than we are currently in.

By "currently", you mean with WORKSPACE or Bzlmod? Since with the latter, we don't need any commit SHAs -- just versions are enough.

Its not just MODULE.bazel but the entire syntax / semantics of a given bazel version.

If foo@1.0 only works with Bazel 4 and foo@2.0 only works with Bazel 5, they can totally exist in the same registry, as long as the MODULE.bazel semantics remain the same. Bazel 4 can grab foo@1.0 from BCR HEAD without any problems.

as this creates additional burden on a maintainer to test and merge to multiple branches.

Module maintainers don't need to test different branches if they don't care about old versions of Bazel. But anyway, we should leave the BCR-branching discussion to another time.

[jsharpe]

With the current model you can never change any historic modules in the registry because you may break someone.

What kind of changes do you have in mind? Are you saying that you might want to make some changes to existing module versions, so you'd prefer if users pinned the registry commit? But that's sounding much more like a Debian/Nixpkgs type of registry, where the whole registry is a "snapshot" of a lot of modules working together and there's no version resolution, as opposed to the Maven/npm/Cargo type of registry, which BCR is more similar to.

A couiple of examples I can think of:
Bug fixes to version dependencies - a maintainer may have got a minimum dependency wrong.
Updates to dependency hashes / download urls; these can and do change overtime in upstream repos. Sure the mirror will mitigate this to some extent but this assumes someone wants to trust the mirror.

Exposing additional functionality into the bzlmod ecosystem. For example with rules_foreign_cc my initial implementation of support is heavily based on patches to the formal release. It is currently supporting only a subset of the features when used via bzlmod. As I mature the features that I expose, I may want to backport these to the older releases in the bzlmod BCR.

I think having the user have to specify a single commit sha vs one per dependency is a much better position than we are currently in.

By "currently", you mean with WORKSPACE or Bzlmod? Since with the latter, we don't need any commit SHAs -- just versions are enough.

Currently I mean with WORKSPACE. Reducing N shas to one per registry means that this sha is essentially representing the set of all possible package resolutions at that point in time. With the current proposed model with no pin on the registry then to vendor you need a lockfile that describes each module you are depending on. So in pinning the registry you reduce the lockfile / vendoring problem to a single sha where the registry has known resolutions. I can imagine security teams being happier with this as they can vet a point in time snapshot of the registry, rather than relying on the immutability policies that are enforced by the presubmits in the BCR.

Its not just MODULE.bazel but the entire syntax / semantics of a given bazel version.

If foo@1.0 only works with Bazel 4 and foo@2.0 only works with Bazel 5, they can totally exist in the same registry, as long as the MODULE.bazel semantics remain the same. Bazel 4 can grab foo@1.0 from BCR HEAD without any problems.

as this creates additional burden on a maintainer to test and merge to multiple branches.

Module maintainers don't need to test different branches if they don't care about old versions of Bazel. But anyway, we should leave the BCR-branching discussion to another time.

On the contrary, the branching model very much is linked to the behaviour of yanked_versions; with my security hat on I want to be able to yank a given version of a package across all bazel versions, not just the ones that happen to be maintained...

[fmeum]

@jsharpe The BCR supports more than three segments in a version string. If you only change some of the patches checked into the BCR, you could use the same source archive of your upstream ruleset and add a fourth segment to the version string, creating a new module version.

[jsharpe]

Solution 2

1. Add the yanked version in the metadata.json file of the module, also record the alternative version users should upgrade to. Eg. "yanked_versions": {"1.2.11": "1.2.12"}

The problem with this approach is this creates a forward maintainance burden; when 1.2.12 gets yanked in the future, this requires the maintainers to then update the resolution for all previously yanked versions too. This will not scale.

[Wyverald]

we could very simply just follow the chain, no? It's all in the same file.

[jsharpe]

I guess; but at some version bumps I assume there will be API / ABI breaks which will break users and so its possible that a 'transparent' upgrade isn't possible.

[Wyverald]

That's always going to be a problem with Solution 2, with or without yank chains. I'm just saying it shouldn't be a scalability concern.

[meteorcloudy]

@jsharpe Can you summarise your ideas as solution 3 so that we can have more clear comparisons here?

[jsharpe]

@meteorcloudy I'll try to put something cohesive together; just thinking out loud here at the moment and haven't quite fully formulated a full idea

[alexeagle]

I think the reproducibility is essential, and also users can't be forced to upgrade immediately. I'd prefer a variation on Solution 1:

1. version gets marked in bcr as yanked
2. Bazel prints an error when this version is used, failing the build. The error message explains why it was yanked (do we allow for the BCR to also store the reason for yanking??) and prints the remaining steps:
3. User can suppress the error, so that they're not forced to upgrade anything. Essentially they ACK the yank by adding sth like allow_yanked="1.2.3" in their MODULE.bazel
4. An identical suppression can also be added with an environment variable and a bazel command-line flag, so that you can green up your CI without having to land a new commit (so that a yank doesn't have to be a pageable oncall emergency for buildcops)
5. With the suppression added, users are unblocked, maybe Bazel still prints a warning reminding them that the suppression is in-place

[meteorcloudy]

I like this solution, updated solution 1 with your input. But about suppressing the yanked version error, I think providing a build flag should be enough. This is meant to be a temporary solution, so I prefer it doesn't end up in the MODULE.bazel file. @Wyverald WDYT?

[Wyverald]

yeah I think having the yanked reason is very helpful, thanks for that. I'd also prefer that suppression be done in a flag.

[alexeagle]

I agree the suppression doesn't need to be permitted in the MODULE.bazel file. I do think it might be handy to allow an environment variable too - some buildcops will be oncall for a CI system where they can't easily pass flags to Bazel without code changes, but can add env variables to the CI pipeline. (Imagine they're building on a cherry-pick branch and now they can't release)
How hard would that be?

[meteorcloudy]

Hmm, probably should be easy with the ClientEnvironmentFunction SkyFunction.