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: falls back to subsequent proxies too early during package resolution #31785

Closed
heschi opened this issue May 1, 2019 · 8 comments
Closed
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@heschi
Copy link
Contributor

heschi commented May 1, 2019

What version of Go are you using (go version)?

$ go version
go version devel +f0c383b833 Wed May 1 16:53:19 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

No, the feature doesn't exist.

What did you do?

$ GOPROXY=https://proxy.golang.org,direct go get -x -v golang.org/x/tools/cmd/goimports

What did you expect to see?

Requests only to the proxy, then a successful build of goimports.

What did you see instead?

One request to the proxy, then a git clone, then alternating between the two until success.

Package to module resolution walks up from the package name looking for a module, and during that walk it's expected to encounter 404s until it finds the actual module path. Those 404s trigger fallback to the next fetch method.

I don't know this code very well but I think this would cause at least two problems: First, the name of the requested package leaks to the subsequent source. If the first source is a corporate proxy and the second is proxy.golang.org, that could be a problem. Second, if the second source is unavailable for some reason, like an offline origin, I suspect the entire go get command will fail.

Marking as a release blocker since this seems to seriously undercut the intended uses of the feature.

@rsc

@bcmills
Copy link
Contributor

bcmills commented May 8, 2019

See related discussion on CL 173441 (#26334).

@andybons andybons 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
@rsc rsc assigned rsc and bcmills and unassigned rsc May 9, 2019
@bcmills
Copy link
Contributor

bcmills commented May 15, 2019

We should also clarify what happens if direct appears earlier in the list: I could imagine someone wanting to prefer to fetch directly, but to fall back to (say) the public proxy if the origin is unavailable.

@bcmills
Copy link
Contributor

bcmills commented May 16, 2019

We should also clarify what happens if direct appears earlier in the list

I talked with Russ and we decided that the fallback stops at direct, but we won't reject that configuration in order to provide forward-compatibility if we decide to add some sort of “proceed past a broken origin” semantics later.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/177958 mentions this issue: cmd/go: when resolving packages, try all module paths before falling back to the next proxy

@heschi
Copy link
Contributor Author

heschi commented Aug 8, 2019

This appears to be happening at tip, either still or again -- I never verified the fix myself.

$ gotip version                                                       
go version devel +f4be93a Thu Aug 8 16:16:15 2019 +0000 linux/amd64
$ GOPATH=$(mktemp -d) gotip get -x -v golang.org/x/tools/cmd/goimports
# get https://proxy.golang.org/golang.org/x/tools/cmd/goimports/@v/list
# get https://proxy.golang.org/golang.org/x/tools/cmd/goimports/@v/list: 410 Gone (0.280s)
# get https://golang.org/x/tools/cmd/goimports?go-get=1
# get //golang.org/x/tools/cmd/goimports?go-get=1: 200 OK (0.057s)
get "golang.org/x/tools/cmd/goimports": found meta tag get.metaImport{Prefix:"golang.org/x/tools", VCS:"git", RepoRoot:"https://go.googlesource.com/tools"} at //golang.org/x/tools/cmd/goimports?go-get=1
get "golang.org/x/tools/cmd/goimports": verifying non-authoritative meta tag
# get https://golang.org/x/tools?go-get=1
# get //golang.org/x/tools?go-get=1: 200 OK (0.038s)
mkdir -p /tmp/tmp.3h1pRjCJVt/pkg/mod/cache/vcs # git3 https://go.googlesource.com/tools
# lock /tmp/tmp.3h1pRjCJVt/pkg/mod/cache/vcs/7d9b3b49b55db5b40e68a94007f21a05905d3fda866f685220de88f9c9bad98a.lockmkdir -p /tmp/tmp.3h1pRjCJVt/pkg/mod/cache/vcs/7d9b3b49b55db5b40e68a94007f21a05905d3fda866f685220de88f9c9bad98a # git3 https://go.googlesource.com/tools
cd /tmp/tmp.3h1pRjCJVt/pkg/mod/cache/vcs/7d9b3b49b55db5b40e68a94007f21a05905d3fda866f685220de88f9c9bad98a; git init --bare

@heschi heschi reopened this Aug 8, 2019
@bcmills
Copy link
Contributor

bcmills commented Aug 8, 2019

What's going on there is that we're looking for an exact hit for the module (from the proxy and then from the origin), and then searching all possible module paths in the proxy, and then all possible module paths from the origin.

This only affects go get, and (I believe) only affects newly-added package dependencies. It's probably a bug, but at this late stage of the cycle I'm not sure that it needs to be a release-blocker.

CC @jayconrod @rsc for thoughts.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/189517 mentions this issue: cmd/go: query each path only once in 'go get'

@bcmills
Copy link
Contributor

bcmills commented Aug 8, 2019

I have a candidate fix, although it does change the meaning of go get some/nested/module@some-version in a few (esoteric) cases where the requested package path coincides with a module that does not contain that package.

@golang golang locked and limited conversation to collaborators Aug 8, 2020
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants