credentials: return Unavailable instead of Internal for per-RPC creds errors

[jeanbza]

Referencing #1704.

I'm a little confused on the part, "The oauth implementation should return UNAVAILABLE if the oauth server cannot be reached.". Does that mean, all implementers of TokenSource :: Token() (*Token, error) should return UNAVAILABLE if the server cannot be reached? If so I'm happy to try and go through and add them either in this PR or a separate PR.

[jeanbza]

I'm rather confused about the failing tests in travis ci; go test ./... -v works locally. Are these flaky tests?

[dfawley]

#1693 is about flakes in this test with this error. Here it failed 4/5 times, which is unusual. I restarted all the failing jobs, and we'll see what happens.

[jeanbza]

@dfawley 👍 thanks - if it fails again I'll dig and see what I can find

[dfawley]

I restarted all the failing jobs, and we'll see what happens.

Looks like Travis is green now. I guess it was just having a bad day when it ran the first time.

I'm a little confused on the part, "The oauth implementation should return UNAVAILABLE if the oauth server cannot be reached.". Does that mean, all implementers of TokenSource :: Token() (*Token, error) should return UNAVAILABLE if the server cannot be reached? If so I'm happy to try and go through and add them either in this PR or a separate PR.

GetRequestMetadata in the credentials/oauth package should ideally start returning status errors as part of this PR. If we can't distinguish between network errors and other errors, then we may not be able to return the appropriate code without going in and changing the golang.org/x/oauth2 package(s) to enable that. If that's the case, we can return Unauthenticated status errors always for this PR.

[dfawley]

This should be checking whether the error was produced by the status package instead:

if _, ok := status.FromError(err); ok {
  return nil, err
}

It's fine to keep returning streamErrorf(codes.Unauthenticated) below to avoid changing too much at once.

We also should update the comments in credentials/credentials.go for GetRequestMetadata to explain that if a status error is returned, it will be used as the status for the RPC.

[jeanbza]

@dfawley 👍 done and done; I'll take a closer look at the error structs, apologies for that

[jeanbza]

👍 thanks for these comments, I'll work on this tonight

[jeanbza]

@dfawley I took a look through some of the golang.org/x/oauth2 implementers (of TokenSource interface), and generally it looks like there's no consistent error statuses being returned. For example:

• jwtSource returns fmt.Errorf("oauth2: cannot fetch token: %v", err) for connection problems, as well as fmt.Errorf("oauth2: cannot fetch token: %v", err) for unmarshalling problems

• computeSource returns whatever error metadata.Get returns, which may be any client.Do / ioutil.ReadAll / etc error

Certainly nothing like the defined error structs in this library, unless I'm mistaken (which I may be!). Would you prefer me to continue by returning Unauthenticated for now?

[dfawley]

it looks like there's no consistent error statuses being returned.

That's what I was afraid of..

Would you prefer me to continue by returning Unauthenticated for now?

Yes, I think that's the best we can do without changes elsewhere - thanks!

[jeanbza]

👍 sounds good, give a shout if anything else I can do on this PR

[dfawley]

FYI: I filed golang/oauth2#264 as a feature request for oauth2.