Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: cmd/go: go mod: limit version resolution to packages that are consumed #52296

Closed
thaJeztah opened this issue Apr 12, 2022 · 23 comments
Closed

Comments

@thaJeztah
Copy link
Contributor

go modules resolve the minimum required version of dependencies based on the go.mod of modules used by a project. This works well for small modules where the list of dependencies in go.mod is representative for the code, but is problematic for larger modules that provide many packages.

Let's illustrate with an example.

Example: project "foobar"

This is our "foobar" project. It uses logrus to print "Hello foobar":

mkdir foobar && cd foobar

cat > main.go <<EOF
package main

import (
    "github.com/sirupsen/logrus"
)

func main() {
    logrus.Info("Hello foobar")
}
EOF

go mod init foobar

Project foobar requires logrus 1.7.0 - it can't currently use a newer version of this dependency, because it has change in behavior that causes foobar to break (of course, SemVer should guard us against breaking changes, but the world isn't perfect, so we specify we want v1.7.0):

go mod edit -require github.com/sirupsen/[email protected]
go mod tidy

cat go.mod
module foobar

go 1.18

require github.com/sirupsen/logrus v1.7.0

require golang.org/x/sys v0.0.0-20191026070338-33540a1f6037 // indirect

No project would be complete without an AppArmor check, and containerd provides an implementation for this. It's a small package, and pkg/apparmor has no dependencies, other than Go stdlib (apparmor.go, apparmor_linux.go, apparmor_unsupported.go).

cat > main.go <<EOF
package main

import (
    "github.com/containerd/containerd/pkg/apparmor"
    "github.com/sirupsen/logrus"
)

func main() {
    if apparmor.HostSupports() {
        logrus.Infof("Running Foobar Deluxe, with AppArmor")
    } else {
        logrus.Infof("Running Foobar Basic")
    }
}
EOF

So we add the containerd v1.6.2 dependency:

go mod edit -require github.com/containerd/[email protected]
go mod tidy

However, checking our go.mod;

Adding containerd as a dependency forced us to also updates the logrus dependency to a newer (for us "incompatible") version (as well as updates the golang.org/x/sys dependency), even though none of the files in containerd's pkg/apparmor package use this dependency

$ cat go.mod
module foobar

go 1.18

require github.com/sirupsen/logrus v1.8.1

require (
	github.com/containerd/containerd v1.6.2
	golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e // indirect
)

While the project "foobar" example is of course just to illustrate the problem, this issue is problematic for many real-life situations (some more details below).

Current "solutions"

There are various "solutions" for this problem, but they're not for the faint of heart.

A. Use replace rules

Projects can add a replace rule to force go modules to use a fixed version. While this helps "us" (the "foobar project" maintainers) build and ship our project, it's a different story for consumers of the "foobar" module; replace rules are not transitional, and because of this, all projects depending on our module will (out of the box) be "forced" to use the newer version, unless they copy the replace rules.

Various (sometimes "high profile") projects currently use replace rules (read them, and weep! 😭😭😭), e.g.: containerd and kubernetes. Worst of all, using replace rules (especially when use to the extend as the kubernetes example) throws out one of the biggest advantages of go modules; version resolution / management.

B. Separate modules (multi-module repository)

We can ask the containerd maintainers to provide pkg/apparmor as a separate module. While this may be an option in some cases, maintaining a multi-module repository gets complicated fast;

  • modules become separate entities (need to be tested separately)
  • if there's dependencies between packages (now separate modules), replace rules may be needed to make sure code it tested against the version in the repository (not the latest released version of the module)
  • if these modules are expected to be used externally, each of them has to be tagged/released separately
  • which, in combination with "inter-module dependencies" also means tagging and releasing MUST be performed in the correct order (to make sure all modules use the latest release)

In short; unless "you're Google", or have a dedicated team of engineers to set up automation to perform these actions (e.g., the complicated release procedures for the kubernetes project), maintaining a multi-module repository is complicated, and in many a heavy burden for project maintainers.

C. Separate modules (multiple repositories)

We can ask the containerd maintainers to provide pkg/apparmor as a separate module in a separate repository.

While this gives a clearer separation between the modules, it shares the same (if not more) problems as the previous solution. Maintaining a separate repository can add significant overhead for project maintainers (and in some cases may be restricted due to (company) policies). In addition, not all packages may be suitable to become a module / project of their own (let's not encourage creating another "leftpad").

D. Just copy the code! (It's open source, y'all!)

Unfortunately, this solution has been chosen on many occasions. I don't think this needs explaining why this should not be a preferred solution.

What did you expect to see? (proposed solution)

I'd like to see go modules to only consider version resolution based on the packages that are actually consumed from a module. Go modules conflates all packages in a repository, resulting in the (main) go module / go.mod to become a collection of all possible dependencies that may be needed (depending on which packages are consumed from the module).

While go modules won't use dependencies if they're not used by any code, version resolution is still be influenced by them (see the example above). I'm not very familiar with the internals of go module's tooling, but I think go has all the "building blocks" available to make this possible;

It's able to provide which imports a package needs:

go list -json ./pkg/apparmor/ | jq .Imports
[
  "os",
  "sync"
]

With that information, it could;

  • take all dependencies listed in the module's go.mod
  • remove all direct dependencies that are not used by the packages that are consumed
  • use the remaining dependencies to perform version resolution

And, if in future containerd's pkg/apparmor would introduce a new dependency, that's the moment it gets its "right to vote" in the version-resolution for that dependency.

@gopherbot gopherbot added this to the Proposal milestone Apr 12, 2022
@thaJeztah
Copy link
Contributor Author

/cc @dims @kzys @dmcgowan @tonistiigi @cpuguy83 @tianon (had this idea as a "draft" / "note" let me know if this idea makes sense to you 😅)

@dims
Copy link

dims commented Apr 12, 2022

cc @liggitt @thockin (our veteran go module wranglers)

@thaJeztah I love this! frankly ANYTHING other than status quo would be very welcome change. Thanks for taking the time and drafting this as a proposal

@liggitt
Copy link
Contributor

liggitt commented Apr 12, 2022

Various (sometimes "high profile") projects currently use replace rules (read them, and weep! 😭😭😭), e.g.: containerd and kubernetes. Worst of all, using replace rules (especially when use to the extend as the kubernetes example) throws out one of the biggest advantages of go modules; version resolution / management.

not to detract from the main point of the issue, but kubernetes/kubernetes does not use replace directives to force downlevel versions. This is enforced in presubmit, and there are no replace directives that select versions other than the require versions in the submodules published out of kubernetes that are consumed as libraries (for example, client-go).

@seankhliao
Copy link
Member

This sounds like it would make version resolution dramatically slower (everything from go get/list/tidy), requiring downloading and scanning the source code for every potential dependency revision.

@thaJeztah
Copy link
Contributor Author

This sounds like it would make version resolution dramatically slower (everything from go get/list/tidy), requiring downloading and scanning the source code for every potential dependency revision.

I guess that's something to look into, if it's possible to optimize. (As mentioned, I'm not deeply familiar with who all version resolution is performed); I can imagine this being mostly an issue when adding / removing / updating the list of dependencies in go.mod. After that's done, the resolved versions are "fixated", so a go get of the main module would already have performed all resolution.

Perhaps there's other solutions possible, and I'm definitely open to alternatives. I opened this proposal because there's a real issue that quite some projects that I'm (directly/indirectly) involved in struggle with this on a daily base, and I'd like to see if we can improve the status quo, without such projects digging themselves in further with hacks and workarounds trying to fix issues with go modules.

@thaJeztah
Copy link
Contributor Author

there are no replace directives that select versions other than the require versions in the submodules published out of kubernetes that are consumed as libraries (for example, client-go).

😅 perhaps it was a bad example; even then, I wonder if kubernetes would've needed the "please, don't use kubernetes/kubernetes as a module" situation (where modules are extracted from the main repository) if things were different? Also, if (e.g.) go mod (tidy|vendor) --recursive and/or tooling was available to manage multi-module repositories (updating inter-module versions), to not having to script all of that.

(but going way off-topic now 😂)

@bcmills
Copy link
Contributor

bcmills commented Apr 12, 2022

What you are describing is very similar in both motivation and effect to the module graph pruning added in Go 1.17 (proposed in #36460).

Since most modules in the wild have not yet upgraded their go.mod files to list go 1.17 or higher, I think it's premature to propose follow-on changes on top of that — we should get more modules upgraded to go 1.17 first (so that pruning can be more effective), and only then evaluate whether there is more that we can usefully do to trim down the dependency graph.

@thaJeztah
Copy link
Contributor Author

To my understanding, module graph pruning helps with transitive dependencies, but not with the issues described in this ticket (I may be wrong though), which is why I was hoping this could trigger a conversation on how to solve this.

@bcmills
Copy link
Contributor

bcmills commented Apr 12, 2022

Ah, I see. You actually do depend on containerd, and that module has a direct dependency on logrus.

Right, so module graph pruning doesn't help there, but there also isn't an inexpensive alternative. If we restricted the analysis to the specific packages imported by your module, we would have to load the entire package import graph just to know which dependencies to use. With the Go 1.17 module graph pruning, we don't need to do that — all of the upfront analysis is based purely on go.mod files, and there are typically far fewer of those than .go source files for packages.

(It took a fair amount of thinking for us to come up with a design with that property! It's important for efficiency, and the graph-pruning invariants are designed very carefully to support it — it isn't something we can discard lightly. 😅)

@bcmills
Copy link
Contributor

bcmills commented Apr 12, 2022

So, in general you have two options.

One is to avoid upgrading containerd until you have identified and fixed the logrus incompatibility (either by upstreaming a fix to logrus itself, or by upstreaming a fix to whatever other dependency is not compatible with the current logrus release).

The other is to apply exclude (not replace!) directives to notch out the dependencies that you know to be spurious. For example:

exclude github.com/sirupsen/logrus v1.8.1

would notch out the transitive dependency from containerd, essentially asserting “yeah, it's required by my dependencies but I know it isn't relevant”. Due to module graph pruning, your own exclude directive would also prune out that dependency for users who depend on your module, although of course they could still end up with it in their own dependency graphs (for example, if they end up transitively importing containerd).

IMO exclude is best used only temporarily, to buy you some time for the aforementioned upstream fixes. At some point, there isn't really a viable alternative to maintaining compatibility with the latest releases of your dependencies.

@thaJeztah
Copy link
Contributor Author

thaJeztah commented Apr 12, 2022

Thanks for taking the time to write up those options (very much appreciated!)

Right, so module graph pruning doesn't help there, but there also isn't an inexpensive alternative

I appreciate "performance". That said (as I mentioned above); version resolution only is needed when updating a dependency. It's a "one-time" performance penalty. On projects where a single run of CI can take hours (or literally days in total compute time), I think that's acceptable.

The "logrus" and "containerd" situation is of course "fictive". In projects with a large dependency tree, things are usually more complicated (a more practical example could be cobra, which through 3 levels of indirection also depends on grpc, and etcd, and, well, "the world").

These issues often can arise from some indirect dependency deep in the dependency tree. More often than not, some small module that's (e.g.) following "best practice", and enables dependabot to always keep its dependencies on the most current version. That module itself may not be affected at all by new updates (it may only be using a small bit of it, or perhaps only as part of a tools.go (those are fun!)). But as a result, that tiny dependency forces the whole dependency chain to update to the latest version (effectively nullifying the "minimal version selection" of go modules).

Now, these things can be solved in "my project" (say, containerd); we can add an exclude rule (probably those will be "many" if one is needed for each version you don't want?) or use a replace rule (pin it to a specific version), but besides having to maintain the list of exclude (which can be potentially fun job), none of those help any of the consumers of my module. Effectively, it's kicking the can down the road, making it "whoever's next's problem".

In the ideal situation, things "explode", and their code fails to build, helping them notify "something's wrong!". Often, things are more subtle than that, and consumers may be living on a ticking timebomb (not theoretical; this happened on more than one occasion, sometimes even resulting in security issues).

If replace and/or exclude rules would be transitive, that would somewhat help (preventing to kick things down the road), but a "smarter" version resolution, even at the cost of a performance penalty (at time of version resolution) would be great.

I realise this is a tough problem to solve, and (to some extend), go modules may not have been designed for large projects, but it's the situation we're in, so trying to find what options are possible. As mentioned; some of these problems can be "solved" or hacked/worked around by large projects themselves, but these projects also have a responsibility towards their respective consumers (of which there may be many), so I'm also trying to explore if there's a solution that would address that side.

@liggitt
Copy link
Contributor

liggitt commented Apr 12, 2022

version resolution only is needed when updating a dependency. It's a "one-time" performance penalty

I thought it ran on pretty much every go invocation (run, test, build, list, etc, etc)

@bcmills
Copy link
Contributor

bcmills commented Apr 12, 2022

version resolution only is needed when updating a dependency. It's a "one-time" performance penalty

I thought it ran on pretty much every go invocation (run, test, build, list, etc, etc)

When lazy module loading is in effect (that is, if the main module is at go 1.17 or higher), we only load the full module graph when a package to be imported isn't found directly in the go.mod file (such as when running go test on some transitively imported package).

So, pretty much every go invocation can trigger a full load, but at 1.17 and above it's not as pervasive as it used to be.

@thaJeztah
Copy link
Contributor Author

I thought it ran on pretty much every go invocation (run, test, build, list, etc, etc)
When lazy module loading is in effect (that is, if the main module is at go 1.17 or higher), ...

Thx! Yes, so this is what I had in mind when I wrote that (but I'm sure there's gonna be situations I missed, where the performance impact can be an issue - let's see what/if we can come to something that works and satisfies most use-cases).

@ianlancetaylor ianlancetaylor added the GoCommand cmd/go label Apr 13, 2022
@rsc
Copy link
Contributor

rsc commented Apr 13, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@thaJeztah
Copy link
Contributor Author

Thanks @rsc !

FWIW, if you or the other maintainers think it may be useful to have a synchronous call with some project maintainers to brainstorm, let me know: happy to reach out to them to make that happen.

@thepudds
Copy link
Contributor

thepudds commented Apr 14, 2022

Hi @bcmills

The other is to apply exclude (not replace!) directives to notch out the dependencies that you know to be spurious. For example:

exclude github.com/sirupsen/logrus v1.8.1

would notch out the transitive dependency from containerd, essentially asserting “yeah, it's required by my dependencies but I know it isn't relevant”. Due to module graph pruning, your own exclude directive would also prune out that dependency for users who depend on your module, although of course they could still end up with it in their own dependency graphs (for example, if they end up transitively importing containerd).

I think this might be implied by your last parenthetical, but just to make it explicit -- my understanding is that would not help the original example here?

In other words, for the foobar example in the first comment above, I suspect that if the foobar author added exclusions for the "bad" versions of logrus, it would not immediately help any clients of foobar avoid those "bad" versions?

FWIW, I put together a quick example here, including to possibly help the conversation with something more concrete:

https://github.com/thepudds/test-go-mod-52296-a/releases/tag/v0.3.0

The foobar module there has exclusions of the "bad" logrus versions, but a client still ends up with a "bad" logrus version.


There might be multiple reasons for that, but I suspect part of it might be (from go mod ref):

If all imported packages can be found without loading the module graph, the go command then loads the go.mod files for only the modules containing those packages, and their requirements are checked against the requirements of the main module to ensure that they are locally consistent. (Inconsistencies can arise due to version-control merges, hand-edits, and changes in modules that have been replaced using local filesystem paths.)

... where the client of foobar has loaded the go.mod of containerd and seen the logrus v1.8.1 requirement in the v1.6.2 containerd go.mod, and hence ends up with the "bad" logrus v1.8.1.

And related, the client of foobar has a require for containerd because containerd is part of the client's package-level import graph (ref):

At go 1.17 and above, the go command adds an indirect requirement for each module that provides any package imported (even indirectly) by a package or test in the main module

Sorry if any of this is off base.

Finally, that's not to say exclude never helps downstream consumers -- rather, it doesn't seem to help the original example.

@thepudds
Copy link
Contributor

thepudds commented Apr 14, 2022

Hi @thaJeztah

we can add an exclude rule (probably those will be "many" if one is needed for each version you don't want?) or use a replace rule (pin it to a specific version), but besides having to maintain the list of exclude (which can be potentially fun job), none of those help any of the consumers of my module. Effectively, it's kicking the can down the road, making it "whoever's next's problem".

I suspect that is not quite right.

FWIW, I also put together another hypothetical with a different package-level import graph (compared to your original example above):

https://github.com/thepudds/test-go-mod-52296-a/releases/tag/v0.4.0

In short:

  • apparmor from containerd is still in the module-level graph, but not necessarily in the package-level import graph.
  • foobar excludes the undesirable versions of logrus in the foobar go.mod.
  • a new client that does not have apparmor in its package-level import graph avoids the undesirable versions of logrus.

In other words, it is an example where exclude is helpful to the consumers in a Go 1.17+ world.

There are more details in that link, including an additional example showing how the exact sequence of events can influence whether or not exclude helps consumers by default.

probably those will be "many" [excludes] if one is needed for each version you don't want?

I think it is correct that multiple excludes can be needed, which is what I did here.

@bcmills
Copy link
Contributor

bcmills commented Apr 18, 2022

@thaJeztah

But as a result, that tiny dependency forces the whole dependency chain to update to the latest version

That is correct, and it is an intentional property of the design. (See the section titled Upgrade Speed in the original MVS blog post.)

As a counterbalance, though, note that you don't have to upgrade that tiny dependency in your own module until you are ready.

@bcmills
Copy link
Contributor

bcmills commented Apr 18, 2022

In principle it would be possible to load only the parts of the module graph corresponding to the main module's import graph, and that could indeed avoid a few unwanted upgrades. That's a benefit.

But we would need to trade that benefit against the costs, and the costs as I see them are numerous:

  • go mod why and go mod graph would become more complex to use and interpret. (They would have to report not only the module graph, but the reasons for the module graph containing — and excluding — the nodes that it does.)

  • The implementations of the module loader and package loader would become much more tightly coupled. (They're already quite complex, but at least today we can have the package loader call into the module graph and not the other way 'round.)

  • Several operations that are fairly inexpensive today (such as running go test on a package imported by the main module) would become approximately as expensive as running go mod tidy.

I just don't see the cost/benefit tradeoff working out — those are significant costs, and the marginal benefit of putting off upgrades of dependencies just doesn't seem that high in comparison, especially given the available workarounds. (If your project is large enough to have a lot of dependencies, you'll probably have to upgrade them eventually anyway!)

@rsc
Copy link
Contributor

rsc commented May 4, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented May 11, 2022

No change in consensus, so declined.
— rsc for the proposal review group

@thepudds
Copy link
Contributor

@thaJeztah wrote::

These issues often can arise from some indirect dependency deep in the dependency tree. More often than not, some small module that's (e.g.) following "best practice", and enables dependabot to always keep its dependencies on the most current version. That module itself may not be affected at all by new updates (it may only be using a small bit of it, or perhaps only as part of a tools.go (those are fun!)). But as a result, that tiny dependency forces the whole dependency chain to update to the latest version

FYI, if anyone is still following this issue, there is some related conversation in #48429 (comment) and in other comments in that "cmd/go: track tool dependencies in go.mod" proposal.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants