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

Configure HttpTransport for proxied GoogleCredentials #17783

Merged
merged 3 commits into from
Jun 9, 2023

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Jun 7, 2023

This would allow proxying communication with oauth2.googleapis.com when default credentials are used.

@cla-bot cla-bot bot added the cla-signed label Jun 7, 2023
@wendigo wendigo requested review from ebyhr and hashhar June 7, 2023 09:04
@wendigo wendigo force-pushed the serafin/bq-credentials-proxy branch from b075587 to 468ef64 Compare June 7, 2023 09:11
@github-actions github-actions bot added the bigquery BigQuery connector label Jun 7, 2023
@wendigo wendigo force-pushed the serafin/bq-credentials-proxy branch from 468ef64 to c7be056 Compare June 7, 2023 10:51
if (staticCredentialsConfig.getCredentialsFile().isPresent() || staticCredentialsConfig.getCredentialsKey().isPresent()) {
credentialsSupplierBinder
.setBinding()
.to(StaticBigQueryCredentialsSupplier.class)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static one also needs the proxied transport. The service account key still requires to go talk to oauth2.googleapis.com to get an access token, the service account key itself is not an access token.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thanks for investigating that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I can indeed see extra proxied calls

wendigo added 2 commits June 8, 2023 13:55
Since this credentials could be obtained using proxied connections,
it's not possible to validate those in a config class.
@wendigo wendigo force-pushed the serafin/bq-credentials-proxy branch from c7be056 to 2d9a83b Compare June 8, 2023 11:57
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I'll merge this after a run with secrets finishes.

@hashhar
Copy link
Member

hashhar commented Jun 9, 2023

/test-with-secrets sha=2d9a83b5d569dbade01ea4ba145c09557f54949e

@github-actions
Copy link

github-actions bot commented Jun 9, 2023

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/5218792948

@hashhar
Copy link
Member

hashhar commented Jun 9, 2023

CI hit #16803

@hashhar hashhar merged commit 48ee2f1 into trinodb:master Jun 9, 2023
@hashhar hashhar mentioned this pull request Jun 9, 2023
@github-actions github-actions bot added this to the 420 milestone Jun 9, 2023
@wendigo wendigo deleted the serafin/bq-credentials-proxy branch January 21, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bigquery BigQuery connector cla-signed
Development

Successfully merging this pull request may close these issues.

2 participants