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

cmd/go: modules with buggy releases #26829

Open
mattfarina opened this Issue Aug 6, 2018 · 32 comments

Comments

Projects
None yet
9 participants
@mattfarina
Member

mattfarina commented Aug 6, 2018

Consider the following case where you have an app with a dependency on a module (modA) that has a dependency on another module (modB).

App --> modA --> modB

modB is released with a bug. For the examples sake, the bug causes goroutines to not be garbage collected and the number to continue to grow. This bug has been experienced in a real world application.

The developer of modA realizes this bug exists and sets the minimum version to the last stable release that does not contain the bug.

The developer of the App decides to update the dependencies to the latest releases (e.g., go get -u ./...). Alternatively, the apps developer could import a second dependency (modC) that also depends on modB but at the latest release because they did not catch the bug. Some bugs don't show up on tests but rather in long running situations which is why I choose the goroutine example.

There is currently no way for the author of modA to pass along information that modB has a bad releases. We need a way to communicate that information for those periods where that release is in the wild and one to use.

Existing tools like glide and dep let you say not to use a release. This is idea shows up in most other languages dependency management as well (e.g., !=1.2.3).

An idea was suggested, rather quickly and off the cuff, by @rsc to setup a web service to hold the information on buggy versions that should not be used. There are a few constraints that make this idea difficult:

  • Who can post details on what versions of what tools are incompatible? There is an opportunity to troll through manipulation of the data
  • How is access handled when packages can be hosted by VCS and you can't always count on GitHub
  • There is an information leakage problem where data on the dependencies of a proprietary app could be leaked to this services host. For example, a rival to Google is working on an app but leaking the dependencies they use to Google as a host of the service which gives lots of detail. We shouldn't have a case where data is leaked like that

This is a problem because of two things:

  • When not vendoring you lose the ability to easily diff the changes to dependencies
  • A mass number of developers never really check the changes to their dependencies when they pull in updates
  • A bug like this could be easily missed when updating, even for the trained eye, which I posit few will do

How will the Go toolchain handle this situation?

@ucirello

This comment has been minimized.

Contributor

ucirello commented Aug 6, 2018

Existing tools like glide and dep let you say not to use a release. This is idea shows up in most other languages dependency management as well (e.g., !=1.2.3).

Isn't what the exclude clause is?

@mattfarina

This comment has been minimized.

Member

mattfarina commented Aug 6, 2018

@ucirello great question. The answer is no according to the docs. Specifically, it says:

exclude and replace directives only operate on the current (“main”) module. exclude and replace directives in modules other than the main module are ignored when building the main module. The replace and exclude statements therefore allow the main module complete control over its own build, without also being subject to complete control by dependencies.

@OneOfOne

This comment has been minimized.

Contributor

OneOfOne commented Aug 6, 2018

@gopherbot, please add labels modules

@gopherbot gopherbot added the modules label Aug 6, 2018

@ucirello

This comment has been minimized.

Contributor

ucirello commented Aug 6, 2018

@mattfarina OK - I am not the one equipped to debate on this. But it does seem exclude is the right primitive. You are right in the sense that exclude does not cascade down to the dependency tree, but you could still state the exclusion in the main module. In the latest version you can even see when packages are imported indirectly, and thus take action accordingly.

Again, I am not the one equipped to debate on this, so I could be missing something important.

@mattfarina

This comment has been minimized.

Member

mattfarina commented Aug 6, 2018

@ucirello You bring up some valid questions that are worth discussing and going deeper on. I'm happy to expand for future readers.

To expand on the example... if you had...

App -> modD -> modA -> modB

If modB had the issue and modA had the exclude it would not automatically be honored. Since App imports modD the author of App may very likely not be aware of the details of modA or modB.

And, since it only works for the current main module, if modA or modB were libraries without a main it would not work for their testing situations, either.

@kardianos

This comment has been minimized.

Contributor

kardianos commented Aug 6, 2018

If this issue is just about marking releases as insecure / buggy, then this is a duplicate of #24031 (comment) .

If this issue is about code review and diffing changes, then I have a few thoughts. I'd like make a to address some of the issues you mentioned with code review, but I don't it should be part of cmd/go. Maybe we could discuss something over in Athens tracker or offline first.

If this issues is about the lack of a way for go.mod to transitively exclude module versions, then I think this is working as designed as you noted. One tool you could build could read all the mod files from dependencies and inform the user of any excludes constraints they request locally, and optionally include them locally. As you noted, this isn't what you designed in glide and Russ has acknowledged this situation a number of times and it seems fundamental to the principal of the design.

@flibustenet

This comment has been minimized.

flibustenet commented Aug 6, 2018

When modB has a bug and make a release to correct it, it will increment at least the patch number. Then go list -m -u all will show that, it's then to maintainer of the App to take care of this and go to look if one of theses dependencies need to be upgraded for it.

@mattfarina

This comment has been minimized.

Member

mattfarina commented Aug 6, 2018

@kardianos This is similar to #24031 (comment) but not the same.

In this case the latest release has a problem with it. There is no newer release to include information about a previous buggy release. There are numerous reasons this could happen. For example,

  1. While a module is useful the author is generally not a great maintainer and is slow to respond to issues
  2. The module maintainers went on vacation or are otherwise temporarily unavailable
  3. The module maintainers don't see the bug as something they immediately need to issue a release for

The design in #24031 (comment) is one where a module, in this case modB, describes information about its previous releases. The modules modA and modD in the examples here have no way to communicate information themselves.

The design in #24031 (comment) would work if module maintainers were always on the ball and got releases for issues out quickly. In practice people simply don't do that all the time which is where the problem comes in.

Don't we need to be fault tolerant of people? If so, how would that work? How can the ecosystem route around an issue in a release where the maintainer(s) of the module aren't handling it?

I believe any solution to this problem should be part of cmd/go because it's a real world situation and people need the default tooling to handle this well.

@mattfarina

This comment has been minimized.

Member

mattfarina commented Aug 6, 2018

@flibustenet How do people handle a long time before a bug fix release to modB? How do they work around that situation?

@ucirello

This comment has been minimized.

Contributor

ucirello commented Aug 6, 2018

@mattfarina If I understand you correctly, I think it is not a goal to protect against every possible way bad imports can land in your code, rather the idea is to provide the necessary primitives to handle them (in this case, exclude clauses), and data structures on which the ecosystem supporting modules can evolve around.

Although the modules implementation does not address this concern you put forward directly, it does offer the necessary primitives and the go.mod has the necessary information to dig further your dependency tree through tooling.

Thus, if this case you put forward is important, other use cases are also important (like having a tool that indicates that an update is available for both transitive and intransitive dependencies) - but neither are necessarily dealt by cmd/go.

Personally, I am working on a tool that crystalizes into the main's go.dep all the dependencies - even the intransitive ones, mostly giving me control of what lands or not in my dependency tree.

Edit: Added last paragraph.

@thepudds

This comment has been minimized.

thepudds commented Aug 6, 2018

I'm pretty sure @mattfarina would have seen all these given his role in glide, etc., but for other people that might land here, briefly adding some more specific URLs to some conversations about related possible future directions (either previously considered or still being considered):

  • declaring deprecated versions as described in the initial vgo blog series
  • declaring pair-wise incompatibility between modules in an external system (maybe a renamed godoc.org?) as discussed for example here during the proposal process
  • declaring buggy, insecure, or potentially incompatible versions of a module after a release has been published in #24031 (which was mentioned above already)

As far as I am aware, there is a strong desire to do something here, but still an open question as to the exact mechanism(s).

It seems like there is very likely a successful path forward that is possible here, but given its significance, this seems like an important area for the community to discuss, analyze, offer possible solutions, etc. (My 2 cents, anyway...)

@mattfarina

This comment has been minimized.

Member

mattfarina commented Aug 7, 2018

@thepudds I'd like to add some more detail to your possible future directions...

  • declaring deprecated versions as described in the initial vgo blog series

That's already touched on over at #24031 (comment) and in the blog series. That setup requires the author of modB to take an action. The problem here is when the maintainer of modB isn't taking an action.

  • declaring pair-wise incompatibility between modules in an external system (maybe a renamed godoc.org?) as discussed for example here during the proposal process

As I noted in the original issue, a problem arises here with information disclosure. If Google operates this site they are in the business of many things. A competitor to Google would be leaking the sets of dependencies they use to Google by using Go. That's not going to be acceptable for many.

  • declaring buggy, insecure, or potentially incompatible versions of a module after a release has been published in #24031 (which was mentioned above already)

Another case where the author of modB takes an action.

The case here is when the author of modB does not take an action or has not taken one, yet. The ecosystem needs to route around the issue they face until action is taken.

@thepudds

This comment has been minimized.

thepudds commented Aug 7, 2018

@mattfarina Regarding your last point, I had added a comment #24031 (comment) that at least attempted to outline how the author of modB wouldn’t be required to take an action in this scenario you outlined here?

But I could very easily be off base here…

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Aug 7, 2018

I agree that it would be nice if Google didn't run this service. But even if they did we could design it so that the complete set of information was downloaded to the local system, and then queried. Although it would grow over time I can't imagine that it would ever be very large.

@thepudds

This comment has been minimized.

thepudds commented Aug 7, 2018

@mattfarina and more than any other point I might make, I do strongly agree with your points here around this being an extremely important use case to handle well.

@mattfarina

This comment has been minimized.

Member

mattfarina commented Aug 7, 2018

@ianlancetaylor I've learned a little about this...

we could design it so that the complete set of information was downloaded to the local system, and then queried. Although it would grow over time I can't imagine that it would ever be very large.

This is a good idea and would likely scale in data for some time. There are other problems in a system like this and I'm curious how those would be solved. Here are a couple that immediately come to mind:

  • How would this system prevent trolls? For example, someone on purpose marking someone eases package as broken when it isn't.
  • How would private packages be handled so that their information never made it into the public registry but were still supported?
@flibustenet

This comment has been minimized.

flibustenet commented Aug 7, 2018

Even not trolls, how about a situation where modA vX as a part broken by modB vY but in an other situation maybe an App use only a little part of modA and modB and will not be affected with this incompatibility ?

That said, how often do we meet this kind of situation ? And how was it handled ? Do you have some examples with other languages ?

@kardianos

This comment has been minimized.

Contributor

kardianos commented Aug 7, 2018

@mattfarina

It is my understand you are asking "How do we communicate broken releases for transitive dependencies?" Is this correct?

Don't we need to be fault tolerant of people? ...

I don't know what you mean by this. Or rather, in my mind the current design is fault tolerant of people, so what do you mean by this?

I believe any solution to this problem should be part of cmd/go because it's a real world situation and people need the default tooling to handle this well.

When you say this, are you implying that displaying broken releases to the developer should be a go mod X sub-command or that it should be part of the existing go build, go get, go mod sync commands?

You are well aware of the Go modules design. I'm assuming you aren't saying that modA can automatically prevent App from getting the latest modB if requested. I'm assuming you want to determine how to best record and communicate breakage as an advisory notice. But please correct me if I'm wrong so we can be on the same page.

@mattfarina

This comment has been minimized.

Member

mattfarina commented Aug 7, 2018

@kardianos I think part of the problem is there is wiggle room on some of this an opinions. I will try to limit those to be more clear.

in my mind the current design is fault tolerant of people

I think we have different opinions on this. It's the details below or surrounding this we should discuss to be explicit. I will try to be more clear on that.

We need to deal with lazy developers. Specifically those who do not look at the details of their transitive dependencies and may not look at changes to their explicit ones often after they are included. This includes when they perform updates to them.

The case can be when a chain of lazy developers are involved. For example,

App -> modD -> modA -> modB

In this situation imagine you had the case where App and modD developers were these lazy developers who didn't look at the details. modA may know about the issue and even document it. But, the developers of modD and App may not read the docs or changelogs. They may not even look at a diff either. This way of updating dependencies is common.

I'm assuming you aren't saying that modA can automatically prevent App from getting the latest modB if requested.

I'm attempting to not suggest an implementation. Rather, I am trying to share the details on the need. Others can design a solution. There are no assumptions on how this should be handled in what I'm saying. If anything, I will try to ferret out more details on a proposed solution but those are from others and I'm trying not to propose one.

When you say this, are you implying that displaying broken releases to the developer should be a go mod X sub-command or that it should be part of the existing go build, go get, go mod sync commands?

This is a good question. This is really targeted at the user experience so the information should be displayed in a way that helps to prevent them from getting into a bad state. With that in mind, I would suggest displaying it for commands like go build, go run, and so forth as well as go mod commands. If someone does a go build in CI to generate a binary that's deployed it would be nice to have that in the logs.

The goal would be to aide the end user in situations where they would need it.

@bcmills

This comment has been minimized.

Member

bcmills commented Aug 7, 2018

If we suppose that exclude directives in checked-in modules exist for good reason (and aren't just there, say, because someone accidentally leaked debug code into the repo), the proposed go release subcommand could report when a module ends up using some version that is explicitly excluded (or replaced) by one of its dependencies.

Then it suffices for the author of “App” to run go release on their module before cutting a release (and, perhaps, periodically thereafter).

(I've already suggested that in #26420 (comment).)

@mattfarina

This comment has been minimized.

Member

mattfarina commented Aug 7, 2018

@bcmills What about apps that do not have releases.

For example, say one has a SaaS and the tip of master is what is running in production. A CI/CD pipeline could even auto-deploy.

If I read #26420 right, it's about releasing software that others can consume. For example, looking for API level changes. In a SaaS (especially a private codebase) these don't matter.

Would it be better to catch these issues at build time since the running build is the things being affected? That way someone running a dev build of a releasable codebase would also have the notifications.

@kardianos

This comment has been minimized.

Contributor

kardianos commented Aug 7, 2018

I'm sure you could have a go release -check-only and CI/CD could run that before go build.

No, it would not be better to catch this during build time. Building is for, well, building and debugging, and running, not going out to the network and checking.

@thepudds

This comment has been minimized.

thepudds commented Aug 7, 2018

@mattfarina mentioned elsewhere that this issue #26829 is about how a consumer of modules route around where module authors have yet to fix bugs, and that #24031 is really a separate issue.

At the risk of committing a large faux pas, I am therefore moving some of my commentary from #24031 to here and updating my comments here to more clearly target the #26829 usecase outlined in the opening comment above.

Setting all that aside, using the start of @mattfarina’s example from #26829:

Consider the following case where you have an app with a dependency on a module (modA) that has a dependency on another module (modB).

App --> modA --> modB

modB is released with a bug.

My understanding of part of the goal here in #26829 is how a consumer of modules can easily be notified of an incompatibility where module authors have yet to fix bugs, or in particular using this example:

  • the author of modA should be able to declare certain versions of modA do not want to use the buggy v.1.2.3 of modB
  • the author of "App" should be able to easily become aware of that action by the author of modA
  • and all of that should be able to happen without requiring any action on the part of the author of modB (which if I’m following is the key aspect to enable “routing around” inaction by the author of modB).

... which I think is targeting the use case outlined here in #26829?

Mechanism still TBD, but perhaps the mechanism (if I've followed the discussion in #24031) to target the use case here in #26829 might be something like:

  • the author of modA produces a new go.mod in a later release of modA that declares that certain versions of modA are incompatible with the buggy v1.2.3 of modB
  • the author of 'App' issues a 'go get' (where perhaps that 'go get' is not directly related to modA or modB, or perhaps a different go command), which as part of its work reads the latest go.mod from modA (even if a version of modA is not being requested), and the author of 'App' is warned about the pair-wise incompatibility between the versions of modA and modB now in use in 'App' (or perhaps it is something other than a warning).
  • and all of that is done without needing to predict the future (by declaring future incompatibilities that don't exist yet at the moment of publishing a release), and also without needing to update an immutable published release, and no action required by the author of the buggy modB.

But sorry if this is off base or just noise...

edit: made slightly more explicit comment (now in bold above) about where the mechanism discussed in #24031 (comment) could kick in for the initial problem scenario outlined here in #26829 by @mattfarina.

@thepudds

This comment has been minimized.

thepudds commented Aug 8, 2018

In other words, in that scenario outlined in my immediately prior comment:

By checking in and releasing a new modA with a new go.mod, the author of modA is able to communicate to consumers of modA (in this case, the author of 'App') that already released versions of modA are incompatible with the buggy modB, without the author of modB needing to take any action, and without the author of 'App' needing to know they should try to get a new modA (because go tooling automatically checked the latest modA's go.mod to discover the newly published incompatibility information wherein modA's author is declaring modB to be buggy or declaring pair-wise incompatibility between certain versions of modA/modB).

With this mechanism, this incompatibility information could be communicated using the same core mechanisms of releasing code (and hence the information moves with the same strengths and weaknesses of how the code itself is communicated).

In this example, it is the author of modA who both determines there is a problem and communicates the details of the problem, which is good because it is modA that directly depends on modB, and hence the author of modA is much better positioned to determine there is a bug in modB than the less-directly-connected author of 'App' (who is the integrator in this scenario, and who is integrating modB as an indirect dependency and as such the author of 'App' is likely not very aware of exactly how modA interacts with modB. Of all the different actors here, the author of 'App' is likely least well positioned to determine modB is buggy, though the author of 'App' might be the first actor here to experience the pain induced by a buggy modB when 'App' doesn't work). The author of modA and the author of 'App' do not need to rely on any immediate action by the author of the buggy modB, which is good because maintainers can be busy or otherwise might not be able to act for days or weeks or months.

Ultimately, hopefully the story has a happy ending when the buggy modB is eventually fixed and released, but no one needed to wait for that to occur.

In any event, it seems the mechanism outlined in #24031 (comment) could be used in the scenario outlined by @mattfarina here, but perhaps there is a much better or different solution that would be more appropriate.

edit: couple typos

@mattfarina

This comment has been minimized.

Member

mattfarina commented Aug 8, 2018

Building is for, well, building and debugging, and running, not going out to the network and checking.

@kardianos This is all brought on by bugs in dependencies. If someone discovers the effects of a bug, does work trying to debug it, and the information is captured but it's not exposed until you use a releaser tool the information isn't being used in a needed context.

When go build is run and a dependency/version isn't available locally won't the tooling go out to the network to get the dependency? Isn't it already hitting the network?

@bcmills

This comment has been minimized.

Member

bcmills commented Aug 8, 2018

When go build is run and a dependency/version isn't available locally won't the tooling go out to the network to get the dependency? Isn't it already hitting the network?

Most builds won't require dependencies from outside the local module cache, so either you'd be adding a lot more network latency to typical go build operations, or you'd be providing a false sense of security.

Seems better to have an explicit “yes, check the network for warnings” step, so that you can run it and be confident that there really aren't any warnings (rather than “well, if I added something new maybe it will warn me...”).

@mattfarina

This comment has been minimized.

Member

mattfarina commented Aug 8, 2018

@bcmills doesn't this depend on implementation detail?

A few thoughts, quickly thrown together and off the top of my head:

  • The "If-None-Match" header could be used when checking for updates to a file pulled down locally with the data (as previously suggested). If there are no changes a 304 response is returned. If that downloaded file does not change often (maybe even once or twice a day) this is a much faster option when checking for updates over the network
  • Does every go build need to check for an update on the network? Can it only happen periodically (e.g., every hour but be tunable with a flag or environment variable)? Maybe the release command (suggested elsewhere) would always check for updates
  • Could checking for updates and looking for issues to display be done concurrently from the rest of the build? It was previously suggested to provide warnings on the information. Could the check happen concurrently and then display any warnings after the build is done?

Just some ideas around the experience.

@kardianos

This comment has been minimized.

Contributor

kardianos commented Aug 8, 2018

proposal: create a mechanism to report issues discovered in package dependencies

This proposal is similar to #24031 (comment) but rather then for reporting a module's own bugs, this reports problems with the current module interacting with dependent module versions.

There are roughly two general ways we could solve this:

  1. Read dependencies go.mod files "excludes" directive and report them up to the developer.
  2. Create some type of web service that these issues could be reported to. This could be totally separate from the Go modules download specification or it could be an extension to it.

While I don't have any concrete design specs, I do have a few ideas. They are: ...

Edit: to be clear, I'm trying to re-cap what I understand from the above conversation. This is not my proposal.

@kardianos

This comment has been minimized.

Contributor

kardianos commented Aug 8, 2018

proposal: inform the developer of any stated version exclusion used by dependencies at build time

This relies on the previous proposal XXX for fetching and reporting the list of modules. This pertains to the UX.

Whenever a go build or go install is invoked, make a network request to determine if there has been any updates to all the dependencies in use exclusion list (implementation TBD). Alternatively, make the go build or go install command only periodically perform this check.

One advantage of bundling it in with go build is that CI servers would contain this information in the log output, unless the CI server was isolated from the network or the implementation was periodic then it may or may not perform this check.

Edit: to be clear, I'm trying to re-cap what I understand from the above conversation. This is not my proposal.

@bcmills

This comment has been minimized.

Member

bcmills commented Aug 8, 2018

The workflow I'm envisioning for CI tools is, roughly:

go get -u=patch && go release -test=all && go install $PKG

The go get -u=patch step should fetch any updated exclusions and deprecations. The go release -test=all step (or however it ends up spelled) would run the tests and also report any detected incompatibilities.

That doesn't require any annotation mechanism beyond issuing a patch release with an exclusion in the go.mod file, and doesn't require any extra network access during go build or go test.

@mattfarina

This comment has been minimized.

Member

mattfarina commented Aug 8, 2018

@bcmills If I'm running a SaaS where the internals of the app code are private why do I want to use go release and be interested in things like "detected incompatibilities" for the Go API?

@bcmills

This comment has been minimized.

Member

bcmills commented Aug 8, 2018

If I'm running a SaaS where the internals of the app code are private why do I want to use go release and be interested in things like "detected incompatibilities" for the Go API?

If you're running SaaS where the internals of the app code are private, then API incompatibilities should be nonexistent: you should have only binaries and internal packages anyway. You still care about detecting incompatibilities among the other modules you use.

That said, perhaps there should be a way to tell go release to skip the API check entirely, so that you can omit the top-level /internal/ path component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment