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: should show error detail from module proxies #30748

Closed
heschik opened this issue Mar 11, 2019 · 18 comments
Assignees
Milestone

Comments

@heschik
Copy link
Contributor

@heschik heschik commented Mar 11, 2019

When a module proxy returns an error, the go command shows only the HTTP status code.

go get golang.org/x/net@latest: unexpected status (https://<proxy url>/golang.org/x/net/@v/list): 404 Not Found

In many cases the proxy will have more detail to report from the output of go mod download or go list. Proxy users should be able to see that; without it it's very hard to tell if the proxy is broken or if the user made a mistake.

One suggestion was simply to add the response body to the error message if it's text/plain. Maybe there should be some rules about it being one line, or maybe proxy authors should just use their judgement.

@bcmills @jayconrod @marwan-at-work @rsc

@bcmills bcmills added this to the Go1.13 milestone Mar 11, 2019
@marwan-at-work

This comment has been minimized.

Copy link
Contributor

@marwan-at-work marwan-at-work commented Mar 11, 2019

If cmd/go shows the status, then the Proxy can define what each status means.

For example, 404 means module not found while 400 means incorrect version etc.
If you want more details about an error, you can look at the GOPROXY server logs.

That said, I wouldn't mind it if Go exposed the response body, for pure debugging purposes although it might clutter the Go command output (maybe only behind -v, or maybe not)

@heschik

This comment has been minimized.

Copy link
Contributor Author

@heschik heschik commented Mar 11, 2019

That assumes that the go command can give very precise causes for an error, and even then I don't think it'd be enough. The reason I remembered to file this bug today is because Gerrit was having trouble on Friday. I think users are best served by an error more like the one they'd have seen going direct to the VCS:

git fetch -f origin refs/heads/*:refs/heads/* refs/tags/*:refs/tags/* in .../pkg/mod/cache/vcs/4a22365141bc4eea5d5ac4a1395e653f2669485db75ef119e7bbec8e19b12a21: exit status 128:
        fatal: remote error: Internal Server Error

than a generic "we think upstream is broken" error that provides no further detail for debugging.

you can look at the GOPROXY server logs

If you have access and know where to look, sure. But in a sufficiently large organization, or for a user of a central proxy service, that's not so easy.

@marwan-at-work

This comment has been minimized.

Copy link
Contributor

@marwan-at-work marwan-at-work commented Mar 11, 2019

That assumes that the go command can give very precise causes for an error, and even then I don't think it'd be enough

It's not the go command that gives the error code, it's the GOPROXY that returns a status code and the go command outputs it for the client (you). Therefore, if you know what those error codes mean on the proxy side, then you should be able to understand what they mean.

That said, it'd be nice to add the response body potentially behind a -v flag or some rule that doesn't clutter the go build too much.

@thepudds

This comment has been minimized.

Copy link

@thepudds thepudds commented Jul 2, 2019

Is this planned for 1.13? (Current milestone says 1.13, but not sure if that’s accurate).

More informative error messages would help with GOPROXY defaulting to enabled...

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 2, 2019

Probably not for 1.13, but we'll see. It might be small enough and useful enough.

@sirkon

This comment has been minimized.

Copy link

@sirkon sirkon commented Aug 2, 2019

This would be handy addition indeed.

For instance, we have a local go modules proxy at my job where it serves from proxy.golang.org for 3rd parties code, from our gitlab for our internal code and compiles proto files from gitlab.example.com/common/schema/service into valid go module gitlab.example.com/common/schema/service/vX.

A user needs to set up gitlab token in order to have an access to gitlab repository:

export GOPROXY=https://token@gitlab.example.com

And it would be nice to see sensible error output in this case in go get output: lack of token, invalid token, etc.

I guess this would work:

If go modules proxy response complies all these points

  1. Status code is 400 (Bad Request) or != 200
  2. Content type is application/json
  3. Returned body looks like
    {
        "errorMessage": "<message here>"
    }

then go get will print that <message here>

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Aug 2, 2019

@sirkon, I don't think we even need application/json support for this.

What I have in mind is more like: if the content type is text/plain and the charset is either us-ascii or utf-8, show the first N lines up to a total of M characters.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 9, 2019

Change https://golang.org/cl/189781 mentions this issue: cmd/go/internal/module: in VersionError, do not wrap an existing ModuleError

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 9, 2019

Change https://golang.org/cl/189782 mentions this issue: cmd/go/internal/modfetch: reduce path redundancy in checkMod error reporting

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 9, 2019

Change https://golang.org/cl/189778 mentions this issue: cmd/go/internal/get: propagate parse errors in parseMetaGoImports

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 9, 2019

Change https://golang.org/cl/189783 mentions this issue: cmd/go/internal/web: include snippets of plain-text server responses in error detail

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 9, 2019

Change https://golang.org/cl/189780 mentions this issue: cmd/go/internal/modload: propagate errors from Query for missing imports

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 9, 2019

Change https://golang.org/cl/189779 mentions this issue: cmd/go/internal/get: propagate server errors if no go-import tags are found

@bcmills bcmills self-assigned this Aug 12, 2019
@thepudds

This comment has been minimized.

Copy link

@thepudds thepudds commented Aug 27, 2019

@bcmills, @jayconrod It looks like the related CL series is not going to be able to make the cut for 1.13? (Not sure of right CL(s), but maybe for example CL 189783).

I can understand if the full solution might be too risky, but I wonder if there is some simpler type of solution, or at least mitigation?

For example:

  1. maybe cmd/go could emit some type of generic "consider re-running with GOPROXY=direct" message after a proxy returns a fatal error?

  2. or maybe cmd/go could emit some type of "please visit https://proxy.golang.org/the/bad/uri for additional details", where the uri corresponds to the one that returned the error status?

  3. or maybe go get -v could emit the raw output, and the generic error could instead suggest go get -v?

  4. or otherwise have cmd/go do something that helps point people towards some type of self-help step?

Here is a sample error message:

go: finding k8s.io/client-go v12.0.0+incompatible
go: k8s.io/client-go@v12.0.0+incompatible: unexpected status
(https://proxy.golang.org/k8s.io/client-go/@v/v12.0.0+incompatible.info): 410 Gone
go: error loading module requirements

Right now, I think that contains the error-causing uri? However, it doesn't suggest visiting there, and that line doesn't say "error", and the key message is often nestled between other messages such that people don't always focus on the most pertinent line or otherwise recognize it as the real error.

Also, is there something in the release notes currently on how to troubleshoot a proxy error?

I don't quite have a handle on what classes of errors people are likely to see, but it seems there might be problems that have better errors if running with GOPROXY=direct, but then another class of problems that only occur when using the proxy and hence don't occur when running with GOPROXY=direct.

CC @myitcv

@thepudds

This comment has been minimized.

Copy link

@thepudds thepudds commented Aug 27, 2019

One other option could be emitting a generic error that suggests visiting https://golang.org/s/troubleshooting-proxy-errors or similar (which could be an exciting pilot of doing more of that in the future ;-)

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Aug 27, 2019

it seems there might be problems that have better errors if running with GOPROXY=direct, but then another class of problems that only occur when using the proxy and hence don't occur when running with GOPROXY=direct.

Typically, proxy.golang.org serves the same error text as direct mode. Since the default in 1.13 is https://proxy.golang.org,direct, most users will end up with the direct error messages.

(We got a lot of reports of proxy-related confusion from users who had explicitly set GOPROXY on 1.12, but 1.12 does not support the fallback to direct.)

@thepudds

This comment has been minimized.

Copy link

@thepudds thepudds commented Aug 27, 2019

Ah, that makes sense. The instances of confusing errors I have seen have almost certainly been with Go 1.12.

gopherbot pushed a commit that referenced this issue Sep 11, 2019
The signature of parseMetaGoImports implies that it can return an error,
but it has not done so since CL 119675. Restore the missing error check,
and remove the named return-values to avoid reintroducing this bug in the
future.

Updates #30748
Updates #21291

Change-Id: Iab19ade5b1c23c282f3c385a55ed277465526515
Reviewed-on: https://go-review.googlesource.com/c/go/+/189778
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Sep 11, 2019
… found

Updates #30748

Change-Id: Ic93c68c1c4b2728f383edfdb06371ecc79a6f7b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/189779
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Sep 11, 2019
Updates #30748
Updates #28459

Change-Id: I1c34b3dae0bf9361dba0dae66bb868901ecafe29
Reviewed-on: https://go-review.googlesource.com/c/go/+/189780
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 11, 2019

Change https://golang.org/cl/194817 mentions this issue: cmd/go/internal/modload: add an Unwrap method on ImportMissingError

gopherbot pushed a commit that referenced this issue Sep 12, 2019
…leError

VersionError wraps the given error in a ModuleError struct.

If the given error is already a ModuleError for the same path and
version, we now return it directly instead of wrapping.
This makes it safer to call VersionError if we don't know whether
a given error is already wrapped.

Updates #30748

Change-Id: I41b23f6c3ead0ec382e848696da51f478da1ad35
Reviewed-on: https://go-review.googlesource.com/c/go/+/189781
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Sep 12, 2019
…porting

Updates #30748

Change-Id: I38a6cdc9c9b488fec579e6362a4284e26e0f526e
Reviewed-on: https://go-review.googlesource.com/c/go/+/189782
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot gopherbot closed this in 9a44023 Sep 12, 2019
gopherbot pushed a commit that referenced this issue Sep 12, 2019
Jay suggested this in CL 189780, and it seems semantically correct.

As far as I can tell this has no impact one way or the other right
now, but might prevent confusion (or at least give us more experience
with error handling!) in future changes.

Updates #30748
Updates #28459
Updates #30322

Change-Id: I5d7e9a08ea141628ed6a8fd03c62d0d3c2edf2bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/194817
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.