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

spanner: Application Default Credentials fail to work if you cancel the context used for spanner.NewClient() #945

Closed
lhecker opened this issue Mar 27, 2018 · 7 comments
Assignees
Labels
🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@lhecker
Copy link
Contributor

lhecker commented Mar 27, 2018

Client

Spanner

Describe Your Environment

Local development using Windows or macOS and the gcloud SDK tool.

Expected Behavior

I expect the spanner package to correctly pick up my local "Application Default Credentials", which have been set up using gcloud auth application-default login.

Actual Behavior

The following code should be quite standard for creating spanner clients:

ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
client, err := spanner.NewClient(ctx, databaseName)

But if you call "cancel()" before you use the client (for instance due to the needed defer cancel()) all further queries and operations using the client will fail.

Cause

(The cause for this issue as far as I understand it)

The context used in spanner.NewClient(WithConfig) is declared here
https://github.com/GoogleCloudPlatform/google-cloud-go/blob/01c1d9b2a0ed098a4e1a7656984d3730e4b56224/spanner/client.go#L115

and used with gtransport.Dial here
https://github.com/GoogleCloudPlatform/google-cloud-go/blob/01c1d9b2a0ed098a4e1a7656984d3730e4b56224/spanner/client.go#L152

This will result in the context being used for internal.Creds here
https://github.com/google/google-api-go-client/blob/e4126357c891acdef6dcd7805daa4c6533be6544/transport/grpc/dial.go#L65

which will one way or the other finally call credentialsFromJSON
https://github.com/golang/oauth2/blob/fdc9e635145ae97e6c2cb777c48305600cf515cb/google/default.go#L85-L99

This will cause a TokenSource to be created using this function
https://github.com/golang/oauth2/blob/fdc9e635145ae97e6c2cb777c48305600cf515cb/oauth2.go#L211-L223

The issue is - as far as I understand - that this function holds onto the context and when it gets canceled it will cause client side connections to fail with Unauthenticated errors since the oauth2 package's token refresher will refuse to work that way.

@lhecker lhecker changed the title spanner: Only the first gRPC channel is successfully using the Application Default Credentials spanner: If you cancel the context used for spanner.NewClient() further queries will fail with "Unauthenticated" errors Mar 27, 2018
@lhecker lhecker changed the title spanner: If you cancel the context used for spanner.NewClient() further queries will fail with "Unauthenticated" errors spanner: Application Default Credentials fail to work if you cancel the context used for spanner.NewClient() Mar 27, 2018
@jba
Copy link
Contributor

jba commented Mar 27, 2018

If you're trying to add a deadline to the connection dial, you'll need to do it another way. Use grpc.WithTimeout. Pass it in using option.WithGRPCDialOption.

@lhecker
Copy link
Contributor Author

lhecker commented Mar 27, 2018

@jba But according to the docs you linked, this function is deprecated (and understandably so!). Should I still use it despite that?

The recommended alternative DialContext of course is what we've been using for our services for a long time as well, but the spanner package does not provide such an option.
Instead its dial function (namely NewClientWithConfig) and probably other packages as well keep the given context past the point of function return, which you've got to agree is... a bit unusual in Go, right?

@jba
Copy link
Contributor

jba commented Mar 27, 2018

Sorry, my mistake. You should probably dial your own gRPC connection (using a different context) and pass it in with option.WithGRPCConn. You could also create the creds yourself with google.CredentialsFromJSON.

Yes, it's unusual to hold on to a context, but it does happen sometimes. But we shouldn't be doing it here. I filed #946 to track.

@lhecker
Copy link
Contributor Author

lhecker commented Mar 27, 2018

Thanks @jba. 🙂

@jba jba closed this as completed Mar 27, 2018
@jba
Copy link
Contributor

jba commented Mar 27, 2018

As #946 says, you shouldn't try to set a timeout for the dial at all, because it may (will?) happen lazily. Set timeouts on the RPCs.

@majelbstoat
Copy link

I have a similar situation, which may or may not be worthy of a separate issue. I'm also using application default credentials. I'm not explicitly setting a timeout, but in development i have services which use spanner clients that run for a long time. If I close the laptop for a while, for example, over night, the first request in the morning gives:

Failed to prepare session, error: spanner: code = "Unauthenticated", desc = "transport: context deadline exceeded"

Like the original poster, I expected the client to automatically recover from severed connections (for whatever reason). It looks like it's trying to do that, but failing to re-read or refresh the credentials.

@jba jba reopened this Jun 25, 2018
@JustinBeckwith JustinBeckwith added 🚨 This issue needs some love. triage me I really want to be triaged. labels Jun 25, 2018
@jba
Copy link
Contributor

jba commented Jun 25, 2018

Actually, @majelbstoat, could you open a separate issue for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

No branches or pull requests

4 participants