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

chore(spanner): source default opts from gapic client #3572

Merged
merged 3 commits into from
Jan 19, 2021

Conversation

olavloite
Copy link
Contributor

Sources the default client options from the generated client instead of the hand-written client. This makes sure that new default options that are introduced in the future will automatically be picked up by the Spanner client.

Fixes #3532

Sources the default client options from the generated client instead of
the hand-written client. This makes sure that new default options that
are introduced in the future will automatically be picked up by the
Spanner client.

Fixes #3532
@olavloite olavloite requested review from skuruppu and a team as code owners January 18, 2021 15:45
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 18, 2021
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Jan 18, 2021
@olavloite olavloite requested a review from codyoss January 18, 2021 16:09
@olavloite olavloite added the automerge Merge the pull request once unit tests and other checks pass. label Jan 19, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 549c351 into master Jan 19, 2021
@gcf-merge-on-green gcf-merge-on-green bot deleted the spanner-default-opts branch January 19, 2021 16:34
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jan 19, 2021
@olavloite
Copy link
Contributor Author

@codyoss @skuruppu While testing something else today, I ran into a slight difference in behavior that was introduced by this change, and I want to check with you whether you consider that a problem:

  1. Previously, the hand-written Spanner client would manage the default scopes itself, and set these when a new client was created. Now we source that from the default options of the generated client. The values were equal in both the hand-written and generated client. BUT: The hand-written client would set these by calling WithScopes, while the generated client calls WithDefaultScopes.
  2. That means that when a connection is set up the client will now use a self-signed JWT token if the credentials that are used is a service account. Previous to this change, the client would use an OAuth2 flow. This works fine in most cases.
  3. However if I use a different endpoint than the default endpoint, and my service account was generated for the default endpoint, I will get an Unauthenticated error. I can circumvent this by also calling WithScopes(...).

So this means that the following code snippet would work prior to this change, but will not work any more when using a service account that has been created for the default endpoint, but that is also authorized for the custom endpoint:

client, err := spanner.NewClient(
    context.Background(),
    "projects/my-project/instances/my-instance/databases/my-database",
    option.WithEndpoint("custom-endpoint.googleapis.com:443"),
)

The following is now needed to make it work:

client, err := spanner.NewClient(
    context.Background(),
    "projects/my-project/instances/my-instance/databases/my-database",
    option.WithEndpoint("custom-endpoint.googleapis.com:443"),
    option.WithScopes("https://www.googleapis.com/auth/spanner.data"),
)

Is this something that we need to do something about?

@codyoss
Copy link
Member

codyoss commented Jan 28, 2021

@olavloite This sound like it might be a bug. I will investigate and get back to you. Thanks for bringing this to my attention.

@codyoss
Copy link
Member

codyoss commented Feb 1, 2021

@olavloite This should no longer be an issue on HEAD. Thanks for the report!

@olavloite
Copy link
Contributor Author

Great, thanks for the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner: source defaultClientOptions for grpc.Pool
3 participants