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

cmd/go: GOPROXY list should not terminate on non 404/410 errors #31913

Closed
amnonbc opened this issue May 8, 2019 · 4 comments
Closed

cmd/go: GOPROXY list should not terminate on non 404/410 errors #31913

amnonbc opened this issue May 8, 2019 · 4 comments
Labels
FeatureRequest FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@amnonbc
Copy link

amnonbc commented May 8, 2019

The new GOPROXY list feature allows the specification of a series of proxies. If the first one returns a 404/410 status, we will try to fetch from the next one.

But if any other error is returned, e.g. a 500, then the search will terminate and the download will fail.
This limits our ability to use multiple proxies to improve make importing more robust and allow downloads to succeed even if a subset of the proxies are down or misbehaving.

We also can not rely on proxies to be well behaved in the errors they return. The athens proxy returns a 500 status if it can not authenticate against the upstream repository. This should of course be fixed in athens. But it would be useful if this feature allows us to work around such problems.

@bcmills bcmills changed the title Feature request: [modules] GOPROXY list should not terminate on non 404/410 errors cmd/go: GOPROXY list should not terminate on non 404/410 errors May 8, 2019
@bcmills bcmills added this to the Go1.13 milestone May 8, 2019
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 8, 2019
@marwan-at-work
Copy link
Contributor

marwan-at-work commented May 8, 2019

It might be a good idea that on 500 errors, the Go command can "report" and try the next one.
But we should definitely have a status code that blocks the Go command from choosing the next GOPROXY value.

This is so that initial proxies take precedent on what they might deem as unacceptable modules, whether for vulnerability reasons or others.

For blocking a module (aka failing a build), I suggest either keeping the current behavior or choosing the Forbidden status code (403)

@bcmills
Copy link
Contributor

bcmills commented May 8, 2019

See related discussion on CL 173441 (#26334).

@bcmills
Copy link
Contributor

bcmills commented May 8, 2019

A 500 error indicates that the server for some reason didn't know how to handle the request. We have no way to determine whether it would have rejected the module or simply didn't know about it. In that case, there is a conservative (privacy-preserving) option, which is to assume that it would have rejected it, and an aggressive (error-tolerant) option, which is to assume that the proxy simply didn't know about it.

For a comma-separated list of proxies in GOPROXY, we have chosen the option that biases toward preserving the privacy of paths. That was discussed and decided on #26334, and absent new information (such as empirical observations of the reliability of generally-available proxy implementations), I don't think we should revisit that decision.

If you want to compose multiple proxies using some other policy, it should be fairly trivial to write a local GOPROXY server that implements whatever error behavior you like.

@bcmills bcmills closed this as completed May 8, 2019
@beoran
Copy link

beoran commented May 9, 2019

In our proxy I made it so that it returns 404 if the upstream repo couldn't be checked out, exactly because of this way go works. So the current behavior of go helps proxy developers to do the Right Thing (tm). We should be writing proxies that are well behaved, we are due at least this much to our users. I think Athens should make the same fix, it's really normally just small a 15 minutes fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants