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/internal/modfetch: treat malformed versions as “not found” on the proxy path #32955

Open
zx2c4 opened this issue Jul 5, 2019 · 36 comments
Labels
modules NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Jul 5, 2019

For Go 1.11 and 1.12, I have in my go.mod something like:

replace (
        github.com/lxn/walk => golang.zx2c4.com/wireguard/windows pkg/walk
        github.com/lxn/win => golang.zx2c4.com/wireguard/windows pkg/walk-win
)

I use this to indicate that usage of github.com/lxn/walk should be replaced with the latest contents of a branch called pkg/walk from golang.zx2c4.com/wireguard/windows.

With 1.13beta1, this now breaks with a message:

/home/zx2c4/Projects/wireguard-windows/go.mod:16: replace golang.zx2c4.com/wireguard/windows: version "pkg/walk" invalid: version "pkg/walk" invalid: disallowed version string
/home/zx2c4/Projects/wireguard-windows/go.mod:17: replace golang.zx2c4.com/wireguard/windows: version "pkg/walk-win" invalid: version "pkg/walk-win" invalid: disallowed version string

This poses a significant problem.

@oiooj oiooj added the modules label Jul 5, 2019
@cespare
Copy link
Contributor

cespare commented Jul 5, 2019

/cc @bcmills

@ALTree ALTree changed the title modules in 1.13beta1 break backwards compatibility with branch name specifier cmd/go: modules in 1.13beta1 break backwards compatibility with branch name specifier Jul 6, 2019
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 6, 2019
@ALTree ALTree added this to the Go1.13 milestone Jul 6, 2019
@zx2c4
Copy link
Contributor Author

zx2c4 commented Jul 8, 2019

Should this have the release-blocker tag? It's certainly a regression from 1.11 and 1.12.

@hyangah
Copy link
Contributor

hyangah commented Jul 8, 2019

@bcmills @jayconrod I think this is a side-effect of changing GOPROXY.

$ GOOS=windows GOPROXY=direct GONOSUM=.* go1.13beta get golang.zx2c4.com/wireguard/windows@pkg/walk
go: finding golang.zx2c4.com/wireguard/windows pkg/walk

The problem can be reproducible with go1.12 if GOPROXY is set.

GOOS=windows GOPROXY=https://proxy.golang.org GONOSUM=.* go get golang.zx2c4.com/wireguard/windows@pkg/walk
go: finding golang.zx2c4.com/wireguard/windows pkg/walk
go: finding golang.zx2c4.com/wireguard pkg/walk
go: finding golang.zx2c4.com pkg/walk
go get golang.zx2c4.com/wireguard/windows@pkg/walk: disallowed version string "pkg/walk"

@hyangah
Copy link
Contributor

hyangah commented Jul 8, 2019

Such version strings are not accepted by golang.org/x/mod/module package that is used by both go command and proxy.golang.org as well. The module proxy protocol doc (go help goproxy) does not specify an acceptable set of version strings.

@bcmills
Copy link
Contributor

bcmills commented Jul 8, 2019

@zx2c4, can you confirm that setting GOPROXY=direct continues to allow the branch-names in question? (That probably determines whether this gets the release-blocker label.)

@jayconrod
Copy link
Contributor

This might be a limitation in the proxy, rather than the Go command.

With go1.13beta1, the command below succeeds:

$ GOPROXY=direct go mod download -json golang.zx2c4.com/wireguard/windows@pkg/walk

However, it fails with GOPROXY=https://proxy.golang.org/. Specifically, this query fails:

$ curl https://proxy.golang.org/golang.zx2c4.com/wireguard/windows/@v/pkg/walk.info
bad request: invalid escaped version "pkg/walk": invalid char '/'

When the proxy is enabled, this is the first request the Go command will send in order to find out the canonical version for pkg/walk.

@cuonglm
Copy link
Member

cuonglm commented Jul 8, 2019

@zx2c4, can you confirm that setting GOPROXY=direct continues to allow the branch-names in question? (That probably determines whether this gets the release-blocker label.)

Set GOPROXY=direct works for me:

cuonglm :: /tmp/test » GOPROXY=direct go mod tidy
golang.zx2c4.com/wireguard/windows pkg/walk
go: finding golang.zx2c4.com/wireguard/windows pkg/walk
golang.zx2c4.com/wireguard/windows pkg/walk-win
go: finding golang.zx2c4.com/wireguard/windows pkg/walk-win

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@bcmills bcmills modified the milestones: Go1.14, Go1.13 Jul 8, 2019
@bcmills bcmills changed the title cmd/go: modules in 1.13beta1 break backwards compatibility with branch name specifier proxy.golang.org: proxy rejects branch names that successfully resolve in 'direct' mode Jul 8, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@bcmills
Copy link
Contributor

bcmills commented Jul 8, 2019

@hyangah

Such version strings are not accepted by golang.org/x/mod/module package

The general solution here may be for the proxy to treat any (non-pseudo-) version that fails module.Check as a potential branch name.

@hyangah
Copy link
Contributor

hyangah commented Jul 8, 2019

@bcmills @jayconrod

Note that the same code is being used in the go command.
The golang.org/x/mod/module package is a copy of src/cmd/go/internal/module package. The error message users observed is probably before hitting the network. (src/cmd/go/internal/modfetch/proxy.go)

@rsc

@heschi
Copy link
Contributor

heschi commented Jul 8, 2019

It wasn't clear to me, so for the record: the problem is that the module.Escape/UnescapeVersion functions, which are only used on the proxy code paths in the go command, don't allow slashes. So, yes, proxy.golang.org doesn't support them, but more importantly, the go command never even gets that far because it can't encode the request properly.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Jul 8, 2019

@zx2c4, can you confirm that setting GOPROXY=direct continues to allow the branch-names in question?

Confirmed.

@bcmills bcmills changed the title proxy.golang.org: proxy rejects branch names that successfully resolve in 'direct' mode cmd/go/internal/modfetch: proxy path rejects branch names that successfully resolve in 'direct' mode Jul 8, 2019
@bcmills bcmills modified the milestones: Go1.14, Go1.13 Jul 8, 2019
@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 8, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 8, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@zx2c4
Copy link
Contributor Author

zx2c4 commented Jul 16, 2019

Are slashes in git branch names very common?

Yes, extremely common. For example, some projects use it to denote the owner of the branch jd/something. Others use it for particular version partions. One of the most common ways of serving git repos to groups is a piece of software called gitolite, perhaps you've heard of it. One of its key features is that it can give granular permissions to branches based on a / hierarchy. This in turn matches the actual way git stores tags: in a directory hierarchy, with / being the directory separator.

@bcmills bcmills modified the milestones: Go1.13, Go1.14 Jul 16, 2019
@hyangah
Copy link
Contributor

hyangah commented Jul 31, 2019

fyi - proxy.golang.org now serves 404/410 for this

$ curl -i  https://proxy.golang.org/golang.zx2c4.com/wireguard/windows/@v/pkg/walk.info
HTTP/1.1 410 Gone
Content-Type: text/plain; charset=UTF-8
...
bad request: invalid escaped version "pkg/walk": invalid char '/'

pchampio added a commit to go-flutter-desktop/hover that referenced this issue Sep 26, 2019
Go 1.13 introduced a proxy for go modules versioning.
This proxy doesn't allow to `go get` package that have '/' in the
version string.

eg `go get github.com/go-flutter-desktop/go-flutter@v0.30.0` works.
But `go get get github.com/go-flutter-desktop/go-flutter@feature/test`
wont because of the '/' after the '@'.

The issue is reported in golang/go#32955
A fix is to by-pass the PROXY by using the `GOPROXY=direct` venv.

Go 1.13 also doesn't fetch '@latest' when the tag is already valid.
This commit sets '@latest' to the version string when needed (hover upgrade)
pchampio added a commit to go-flutter-desktop/hover that referenced this issue Sep 26, 2019
Go 1.13 introduced a proxy for go modules versioning.
This proxy doesn't allow to `go get` package that have '/' in the
version string.

eg `go get github.com/go-flutter-desktop/go-flutter@v0.30.0` works.
But `go get get github.com/go-flutter-desktop/go-flutter@feature/test`
wont because of the '/' after the '@'.

The issue is reported in golang/go#32955
A fix is to by-pass the PROXY by using the `GOPROXY=direct` venv.

Go 1.13 also doesn't fetch '@latest' when the tag is already valid.
This commit sets '@latest' to the version string when needed (hover upgrade)
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@utrack
Copy link

utrack commented Feb 7, 2020

This bug also affects module imports from a subdirectory of another module.

Here's an example tree for a repo:

github.com/foo/bar
├── pkg
│       └── baz
│           └── go.mod
└── go.mod

If I were to import package github.com/foo/bar/pkg/baz then go would try to fetch tag pkg/baz/v1.0.0 automatically (that would fail).

What're the next steps for that one - do we want to fix go or proxy (athens)?

@bcmills
Copy link
Contributor

bcmills commented Feb 7, 2020

@utrack, the versions in the proxy protocol are not the same as the tags in the underlying repo. The tag for the module in the pkg/baz subdirectory is indeed pkg/baz/v1.0.0, but once the module is extracted into the module cache that version is encoded as simply v1.0.0 (because the pkg/baz portion is already encoded in the module cache).

@hyangah
Copy link
Contributor

hyangah commented Feb 7, 2020

@utrack try the -x flag to see how the go command works with the proxy. That will show the behavior @bcmills described above. go get github.com/foo/bar/pkg/baz@v1.0.0 or go get github.com/foo/bar/pkg/baz will work as expected.

go get github.com/foo/bar/pkg/baz@pkg/baz/v1.0.0 won't due to this issue but not sure how often ppl want to fetch a module/package in that way.

@4F2E4A2E
Copy link

Using the hash of the branch git rev-parse did it for me, see the comment: #34175 (comment)

@mvdan
Copy link
Member

mvdan commented Mar 5, 2021

Are slashes in git branch names very common?

As much as I don't personally use them, I'm afraid they are common. Where I work, pretty much everyone works in branches like feat/some-feature or fix/some-bug. For example: https://github.com/ipfs/go-ipfs/branches

@mvdan
Copy link
Member

mvdan commented Mar 10, 2021

To add to my last comment, the convention is even documented here: https://github.com/ipfs/community/blob/master/CONTRIBUTING_GO.md#branch-names

If you are working on a new feature, prefix your branch name with feat/. If you are fixing an issue, fix/. If you are simply adding tests, test/. If you are adding documentation, doc/. If your changeset doesn't fall into one of these categories, use your best judgement and come up with your own short prefix.

@LockedThread

This comment was marked as spam.

Sandeepyadav93 added a commit to ci-framework/openstack-k8s-operators-ci that referenced this issue Nov 17, 2022
Before this patch, We were replacing go.mod in openstack-operator with
PR commit using branch name.

ex:
~~
go mod edit --replace
github.com/openstack-k8s-operators/keystone-operator/api@$version=
github.com/ysandeep93/keystone-operator/api@dependabot/go_modules/
sigs.k8s.io/controller-runtime-0.13.1
~~~

After Go 1.13, there is an known issue if we have slashes in branch name.[1]

~~~
$go mod tidy
go: errors parsing go.mod:
/tmp/openstack-operator/go.mod:97:
replace github.com/ysandeep93/keystone-operator/api: version
"dependabot/go_modules/sigs.k8s.io/controller-runtime-0.13.1"
invalid: version "dependabot/go_modules/sigs.k8s.io/controller-runtime-0.13.1"
invalid: disallowed version string
~~~

With this patch, updating our logic to use hash of branch instead of branch
name, same workaround/fix is mentioned here[2].

[1] golang/go#32955
[2] golang/go#34175 (comment)
@stevekuznetsov
Copy link

Any chance that this gets fixed at any point? Slashes in branch names are very common and it's frustrating to have to work around this limitation all the time.

@elgs
Copy link

elgs commented Mar 4, 2023

I am building a main app that will optionally support one or more database drivers. I wonder if I could branch each type of database driver separately and allow users to go get/install from respective branches if they only need one database type.

package main

import (
        // ...

	_ "github.com/go-sql-driver/mysql"
	_ "github.com/jackc/pgx/v5/stdlib"
	_ "github.com/mattn/go-sqlite3"
	_ "github.com/microsoft/go-mssqldb"
	_ "github.com/sijms/go-ora/v2"
	_ "modernc.org/sqlite"
)

@elgs
Copy link

elgs commented Mar 4, 2023

It seems it works as of go 1.20.1.
I created several branches, all, mysql, sqlite3, and etc., loading only corresponding drivers. And I was able to install the different branches by:

$ go install github.com/elgs/gosqlapi@mysql
$ go install github.com/elgs/gosqlapi@sqlite3
$ go install github.com/elgs/gosqlapi@all

I am quite satisfied as it is. But I am not sure if I overlooked anything.

@stevekuznetsov
Copy link

@elgs this issue is about interacting with branches that have a forward slash (/) in them - e.g. branch/name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modules NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests