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

ci(google-auth): use workload identity federation instead of json #10342

Merged

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Oct 21, 2024

No description provided.

@cpcloud cpcloud added this to the 10.0 milestone Oct 21, 2024
@cpcloud cpcloud added ci Continuous Integration issues or PRs bigquery The BigQuery backend labels Oct 21, 2024
@cpcloud cpcloud requested a review from tswast October 21, 2024 13:49
Copy link
Collaborator

@tswast tswast left a comment

Choose a reason for hiding this comment

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

Love it! Still working on getting approvals to enable this in our ibis-gbq test project.

@cpcloud
Copy link
Member Author

cpcloud commented Nov 12, 2024

@tswast Should we just give this a go and merge it?

@cpcloud cpcloud force-pushed the google-cloud-workload-id-federation branch from ba0cee5 to 620bd00 Compare November 12, 2024 10:37
@tswast
Copy link
Collaborator

tswast commented Nov 12, 2024

Sorry, I misspoke at PyData NYC. Looks like I'm still waiting for approval on one more internal change (issue 375475414 for Googlers' reference) before this can work.

@cpcloud
Copy link
Member Author

cpcloud commented Dec 17, 2024

@tswast Any updates here?

@tswast
Copy link
Collaborator

tswast commented Dec 17, 2024

Thanks for the reminder. I'm still waiting on a thumbs up from our security team. I think I went through the right process, but it's been stuck on the last LGTM for a couple months now. 😭 I'll ping them again today. Maybe I can get another reviewer assigned?

@cpcloud cpcloud removed this from the 10.0 milestone Dec 18, 2024
Copy link
Collaborator

@tswast tswast left a comment

Choose a reason for hiding this comment

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

Approving, but consider resolving the comments on .github/workflows/ci-data.yml before merging.

.github/workflows/ci-data.yml Outdated Show resolved Hide resolved
@cpcloud cpcloud force-pushed the google-cloud-workload-id-federation branch from 56427cb to 7d71163 Compare December 26, 2024 19:45
@cpcloud cpcloud added this to the 10.0 milestone Dec 26, 2024
@cpcloud cpcloud force-pushed the google-cloud-workload-id-federation branch from 7d71163 to 8229554 Compare December 26, 2024 19:47
@cpcloud
Copy link
Member Author

cpcloud commented Dec 26, 2024

@tswast Thanks for pushing this through, merging on green!

@cpcloud cpcloud enabled auto-merge (squash) December 26, 2024 19:48
@cpcloud cpcloud merged commit 8f94b15 into ibis-project:main Dec 26, 2024
86 checks passed
@cpcloud cpcloud deleted the google-cloud-workload-id-federation branch December 26, 2024 20:08
@cpcloud
Copy link
Member Author

cpcloud commented Dec 27, 2024

@tswast It looks like two tests are failing on main (which is still a win since everything else is passing 🥳) see here: https://github.com/ibis-project/ibis/actions/runs/12507938156/job/34896929010

Can you help debug the permissions here?

@cpcloud
Copy link
Member Author

cpcloud commented Dec 27, 2024

Looks like step 5 here might be required (giving permissions to specific cloud resources).

@tswast
Copy link
Collaborator

tswast commented Dec 27, 2024

Thanks for the heads up. Taking a look now. Step 5 does have a concerning warning, which I wonder if applies in this case?

However, not all Google Cloud resources support principalSet identities

For example, the failure to read gs://ibis-testing-libraries/lodash.min.js from BigQuery is concerning, as I have granted the GitHub provider access to that bucket in two different ways. I'm not seeing any notes about principalSet in the BigQuery + GCS or BigQuery + JS libraries docs. I'll do some experimentation and see if I can narrow down what's going on.

@cpcloud
Copy link
Member Author

cpcloud commented Dec 27, 2024

Thanks. If it's helpful, writing data to a bucket (the benchmark data upload) is working, so accessing a bucket directly is not a problem.

Just to clarify, from my POV I see two things that might be an issue:

  1. Reading a bucket from within BigQuery (perhaps specific to JS libraries)
  2. Accessing another project's data, even if it's publicly accessible, like the one we're testing against.

@cpcloud
Copy link
Member Author

cpcloud commented Dec 27, 2024

If you can't find anything in the next few business days, we can xfail those tests until we understand whether there's an issue.

@tswast
Copy link
Collaborator

tswast commented Dec 27, 2024

I tried out GitHub Actions on another repo (tswast/code-snippets#3) to see if I could reproduce the BigQuery + JS library failure, but I was able to call bqutil.fn.xml_to_json in https://github.com/tswast/code-snippets/actions/runs/12521512469/job/34928670087

I suspect it might be a permission issue rather than a BigQuery bug with such credentials. I'll try to take a look at the ibis setup a bit more next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bigquery The BigQuery backend ci Continuous Integration issues or PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants