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

feat: add universe domain support to core, bigquery, storage, and pubsub #6850

Merged
merged 15 commits into from
Jan 9, 2024

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Dec 6, 2023

Add support for Universe Domain to BigQuery and Storage via modifications to RequestWrapper, RequestWrapperTrait, and RestTrait. Add support in PubSub via GrpcTrait.

Still needs:

  • verify behavior for when API Keys and Access Tokens supplied outside the GDU
    • Translate is the only Client that accepts a "key" argument for API Keys, but shouldSignRequest can bypass credential check
    • Both raw access tokens and API keys are allowed to bypass the check.
    • shouldSignRequest bypasses the check (and the sending of credentials)
    • In our implementation, we will not allow raw access tokens. API keys are allowed.
  • design approval (@vishwarajanand)
  • unit tests
  • manual tests in universe domain testing environment

Co-authored-by: Vishwaraj Anand <[email protected]>
@vishwarajanand
Copy link
Contributor

vishwarajanand commented Dec 29, 2023

I observed that ApplicationDefaultCredentials::fetchAuthToken() produces an array with just 1 key, access_token while using a universe domain, while in GDU, it produces an array with keys access_token, expires_in (comes from oAuth2 and almost always has value 3599), token_type (value is Bearer).

Upon discussion with @bshaffer, it seems that TPC uses jwt, so if I call $credentials->useJwtAccessWithScope(); in GDU, I get an array with just access_token key. I think we need to change this behavior here.

We can change it to something like:

        return [
            'access_token' => $access_token,
            'expires_in' => $this->auth->getExpiry(),
            'token_type' => 'Bearer',
        ];

@bshaffer
Copy link
Contributor Author

I observed that ApplicationDefaultCredentials::fetchAuthToken() produces an array with just 1 key, access_token

To be fair, this is a behavioral difference between SSJs and the oauth token endpoint, and is not related to the Universe Domain support (although it did help us uncover the discrepancy)

@vishwarajanand
Copy link
Contributor

Is my environment broken or is this an actual issue with the token?
https://paste.googleplex.com/5467801802768384
I am seeing an access_token in RequestWrapper but when its used in BQ or Storage client, I am seeing errors as mentioned in the log. This seems to be consistent with REST calls too so looks like not a library issue.

@bshaffer bshaffer changed the title feat: add universe domain support for bigquery and storage feat: add universe domain support to core, bigquery, storage, and pubsub Jan 2, 2024
@bshaffer
Copy link
Contributor Author

bshaffer commented Jan 2, 2024

Is my environment broken or is this an actual issue with the token?

You are using the GDU endpoint (bigquery.googleapis.com) instead of the URL for the Universe Domain you've provided (bigquery.apis-tpclp.goog), so the exception you're seeing is expected.

@vishwarajanand vishwarajanand force-pushed the universe-domain-for-bigquery-and-storage branch from 4de3589 to 339e761 Compare January 3, 2024 21:22
@bshaffer bshaffer marked this pull request as ready for review January 4, 2024 18:38
@bshaffer bshaffer requested review from a team as code owners January 4, 2024 18:38
Copy link
Contributor

@vishwarajanand vishwarajanand left a comment

Choose a reason for hiding this comment

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

There are some merge conflicts, otherwise LGTM

@bshaffer bshaffer merged commit 52bc721 into main Jan 9, 2024
27 checks passed
@bshaffer bshaffer deleted the universe-domain-for-bigquery-and-storage branch January 9, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants