-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Support BigQuery OAuth using a refresh token and client secrets #2805
Conversation
5a13613
to
1eefced
Compare
0af9a34
to
1cf87c6
Compare
@@ -165,6 +179,16 @@ def get_bigquery_credentials(cls, profile_credentials): | |||
details = profile_credentials.keyfile_json | |||
return creds.from_service_account_info(details, scopes=cls.SCOPE) | |||
|
|||
elif method == BigQueryConnectionMethod.BEARER: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtcohen6 do you have any thoughts / opinions on what we call this auth method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind bearer
as long as it reflects what BQ calls this (right?). I prefer it to oauth-secrets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ it's actually different than the method described in that link, which is kind of why i do not want to call this bearer! Bearer auth actually feels more to me like the change described in #2802 :D
After some more thought, I think we should roll with refresh-token
on this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QED. Let's call this something else. oauth-refresh-token
?
Edit: refresh-token
also works, we both got there
I'm pretty excited about this feature! Let me know if you need any help with an integration test case (whether that's feasible). Currently, the BIGQUERY_SERVICE_ACCOUNT_JSON is passed in as an environment variable, so maybe a similar variable could be something like BIGQUERY_REFRESH_TOKEN_JSON, which would contain the parameters to the GoogleCredentials.Credentials method. |
@@ -51,19 +54,30 @@ class BigQueryConnectionMethod(StrEnum): | |||
OAUTH = 'oauth' | |||
SERVICE_ACCOUNT = 'service-account' | |||
SERVICE_ACCOUNT_JSON = 'service-account-json' | |||
OAUTH_SECRETS = 'oauth-secrets' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you going to rename this to refresh-token
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i decided not to! I actually updated this PR to also accept an access token directly via the token
configuration. So, it's kind of just a grab bag of oauth auth methods. Google lets you supply:
- a client id / client secret / refresh token / token uri OR
- a standalone access token
The access token is going to expire after ~60 mins (not well defined), but it was a one-line change and feels like a reasonable enough use case to support. You buy it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha — does that mean that this resolves #2802 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat!
resolves #2344
resolves #2802
Description
This PR adds support for the
bearer
connection method on BQ. When this method is specified, aclient_id
,client_secret
,refresh_token
andtoken_uri
must also be provided. Given these secrets, dbt will mint access tokens to authorize BigQuery queries.TODO
bearer
tooauth-secrets
as the auth method?Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.