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

Jump to bottom

proposal: cmd/go: deprecate direct fallback in GOPROXY #40580

Closed
urandom2 opened this issue Aug 5, 2020 · 12 comments
Closed

proposal: cmd/go: deprecate direct fallback in GOPROXY #40580

urandom2 opened this issue Aug 5, 2020 · 12 comments
Labels
FrozenDueToAge Proposal
Projects
Proposals (old)
Milestone
Proposal

Comments

@urandom2
Copy link
Contributor

urandom2 commented Aug 5, 2020

background

As noted in #40358, the ,direct suffix to the default value of GOPROXY is confusing. In isolation it makes sense, but with the added requirement of a sumdb, it seems broken.

If you cannot access sum.golang.org, the toolchain fails:

[user@localhost ~]$ go get golang.org/x/exp
go: downloading golang.org/x/exp v0.0.0-20200513190911-00229845015e
go get golang.org/x/exp: golang.org/x/exp@v0.0.0-20200513190911-00229845015e: verifying module: golang.org/x/exp@v0.0.0-20200513190911-00229845015e: Get "https://sum.golang.org/lookup/golang.org/x/exp@v0.0.0-20200513190911-00229845015e": dial tcp [::1]:443: connect: connection refused

If you fallback to direct, this implies that proxy.golang.org cannot fetch your source, thus sum.golang.org cannot either. As such, even if you manage to fetch a module directly, the sumdb verification will fail:

[user@localhost ~]$ go get gitern.com/urandom/errors
go: downloading gitern.com/urandom/errors.git v0.0.0-20200723020514-49681dcbcc21
go get gitern.com/urandom/errors.git: gitern.com/urandom/errors.git@v0.0.0-20200723020514-49681dcbcc21: verifying module: gitern.com/urandom/errors.git@v0.0.0-20200723020514-49681dcbcc21: reading https://sum.golang.org/lookup/gitern.com/urandom/errors.git@v0.0.0-20200723020514-49681dcbcc21: 410 Gone
        server response:
        not found: gitern.com/urandom/errors.git@v0.0.0-20200723020514-49681dcbcc21: invalid version: git fetch -f origin refs/heads/*:refs/heads/* refs/tags/*:refs/tags/* in /tmp/gopath/pkg/mod/cache/vcs/c58e8953608705577f340d5d716eb03f031f4f09a3698dde27ce667a4baf661a: exit status 128:
                fatal: unable to connect to gitern.com:
                gitern.com[0: 100.20.209.4]: errno=Connection refused

The only use case I can conceive for this feature, is if you wanted to fetch from vcs, and still retain the benefits of sumdb verification, but let me know if there is something I am missing.

Based on this understanding, it seems like there are two type of packages: accessible and inaccessible (wrt the proxy/sumbd). The former case are things that can be served by proxy.golang.org, or your (private) module proxy of choice. The latter are things that must be fetched from vcs directly. Documentation suggests using GOPRIVATE for the latter, and we have shown that GOPROXY=direct will not work.

description

I propose we remove the ,direct suffix from the GOPROXY default value. We should keep the keyword around to prevent breaking changes, but modify its behaviour so that it is a nop and prints a warning to the user.

While this may seems like we are forcing people to use proxy.golang.org exclusively, this was already done with GOSUMDB=sum.golang.org.

alternatives

We could actually make direct work by having it skip the sumdb verification, however there were concerns about it "destroying the security model" in #40358.

While we could also leave it alone, but I hope that I have shown that ,direct cannot work the way one expects it to today, thus is just noise and confusion to most users.
@gopherbot gopherbot added this to the Proposal milestone Aug 5, 2020
@martisch
Copy link
Contributor

martisch commented Aug 5, 2020

Isnt there the possibility of companies running their own internal sum db with internal url set in GOSUMDB? This does not require that the external proxy has the same visibility to packages as the internal sum db server.

@urandom2
Copy link
Contributor Author

urandom2 commented Aug 5, 2020

Running a sumbd without a proxy would be another use case, but it is pretty weak, since there are few sumdb only implementations, and most are both proxy and sumdb.

Furthermore, my main suggestion is to remove ,direct as a default, since once we start talking about custom setups, anything is possible. I will fixup the description to reflect this preference.

@martisch
Copy link
Contributor

martisch commented Aug 5, 2020 •
edited

There is still the possibility of the go proxy being unavailble while the sumdb is not. And the proxy might not have the module or be able to fetch the module while the sumdb already knows about it. Its nice to have a usable fallback to improve overall reliability.

Letting clients fetch directly also allows the proxy to loadshed requests without immediatly impacting the clients ability to get modules. Otherwise the default behaviour of clients would force them to continue to reach out to the module proxy continuing the load.

@urandom2
Copy link
Contributor Author

urandom2 commented Aug 5, 2020

If we are assuming that general network instability needs to be solved by the toolchain, then why do we hard fail when we cannot reach sum.golang.org?

Furthermore, it is inconcevable that, for the default values, one could reach sum.golang.org, but not proxy.golang.org, since they are the same service. This reflects all internal module deployments that i have seen too.

@martisch
Copy link
Contributor

martisch commented Aug 5, 2020 •
edited

Furthermore, it is inconcevable that, for the default values, one could reach sum.golang.org, but not proxy.golang.org.

Im certain if given the access controls or due to the right set of circumstances to disable traffic to proxy.golang.org while sum.golang.org will be fine.

since they are the same service.

They may or may not be currently. They dont have to be in the future and they dont have to use the same load balancer config.

@heschi
Copy link
Contributor

heschi commented Aug 5, 2020

The go command does not need to consult sum.golang.org if there is a fully-populated go.sum file in the project. In general, only operations that change dependencies need to access sum.golang.org.

As I said in #40358, there may be room for a usability improvement in the error messages, but I don't think there's much chance at all of this proposal being accepted.

@urandom2
Copy link
Contributor Author

urandom2 commented Aug 6, 2020

@ianlancetaylor: can you add this to the proposals project?

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 6, 2020
@ianlancetaylor
Copy link
Contributor

Done. (You could also just e-mail me a list of issues.)

CC @bcmills @jayconrod

@urandom2
Copy link
Contributor Author

urandom2 commented Aug 6, 2020

Sorry, that would have been easier, but thanks a bunch; if I get bored, I may look through for open tickets with the proposal label and milestone without the project, and I will email that list to you.

@rsc
Copy link
Contributor

rsc commented Jul 21, 2021

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

@rsc rsc moved this from Incoming to Active in Proposals (old) Jul 21, 2021
@rsc rsc mentioned this issue Jul 21, 2021
Open
@rsc
Copy link
Contributor

rsc commented Jul 28, 2021

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

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jul 28, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Aug 4, 2021
@rsc
Copy link
Contributor

rsc commented Aug 4, 2021

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

@rsc rsc closed this as completed Aug 4, 2021
@golang golang locked and limited conversation to collaborators Aug 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
No open projects
Proposals (old)
Declined
Milestone
Proposal
Development

No branches or pull requests
6 participants
@rsc @martisch @urandom2 @ianlancetaylor @gopherbot @heschi