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

[query] Unify CloudCredentials and Simplify BatchClient #14684

Merged
merged 19 commits into from
Dec 16, 2024

Conversation

ehigham
Copy link
Member

@ehigham ehigham commented Sep 13, 2024

This change combines cloud auth logic that was previously duplicated
between the various FS implementations and the BatchClient.

The main refactoring is to make the interface between the ServiceBackend more
high-level and leave json serialisation to the BatchClient. To do this, I've
added a bunch of case classes that resemble the python objects the batch service
expects (or a subset of the data). To simplify the interface, I've split batch
creation from job submission (update). For QoB, the python client creates the
batch before handing control to the query driver; batch creation is necessary
for testing only.

This change has low security impact as there are minor changes to the creation
and scoping of service account credentials. Note that for each FS, credentials
are scoped to the default storage oauth2 scopes for each service.

Copy link
Member Author

ehigham commented Sep 13, 2024

@ehigham ehigham marked this pull request as ready for review September 13, 2024 19:05
@ehigham ehigham force-pushed the ehigham/cloud-credentials branch from cd38e9b to 0ac3faa Compare September 16, 2024 17:45
@ehigham ehigham force-pushed the ehigham/cloud-credentials branch from 0ac3faa to 9c4151a Compare September 16, 2024 17:47
@ehigham ehigham force-pushed the ehigham/cloud-credentials branch from 9c4151a to 16b6635 Compare September 16, 2024 17:48
@ehigham ehigham force-pushed the ehigham/cloud-credentials branch from 9cff2fc to d14679d Compare September 19, 2024 20:52
@ehigham ehigham force-pushed the ehigham/enclosing branch from 2b028ba to 8c7b1c3 Compare October 1, 2024 19:44
@ehigham ehigham force-pushed the ehigham/cloud-credentials branch from d14679d to ed2e8e8 Compare October 1, 2024 19:44
@ehigham ehigham force-pushed the ehigham/enclosing branch from 8c7b1c3 to 3c702c0 Compare October 8, 2024 19:18
@ehigham ehigham force-pushed the ehigham/cloud-credentials branch 2 times, most recently from fc06f02 to a101b55 Compare October 8, 2024 20:30
@ehigham ehigham force-pushed the ehigham/cloud-credentials branch from a101b55 to 1682a0f Compare October 16, 2024 20:02
@ehigham ehigham force-pushed the ehigham/cloud-credentials branch from 1682a0f to 6e63e69 Compare October 16, 2024 21:30
Base automatically changed from ehigham/enclosing to main October 17, 2024 14:26
@ehigham ehigham force-pushed the ehigham/cloud-credentials branch from 6e36a7a to e121222 Compare November 19, 2024 17:08
Copy link
Collaborator

@patrick-schultz patrick-schultz left a comment

Choose a reason for hiding this comment

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

These are great changes! I'm very sorry again it took me so long to get through the review. I only have a few small questions/requests.

@ehigham ehigham force-pushed the ehigham/cloud-credentials branch 2 times, most recently from 0a14104 to 4b932f1 Compare December 12, 2024 22:00
@ehigham ehigham force-pushed the ehigham/cloud-credentials branch from 4b932f1 to 64042bc Compare December 12, 2024 22:02
@hail-ci-robot hail-ci-robot merged commit 48f2925 into main Dec 16, 2024
2 of 3 checks passed
@hail-ci-robot hail-ci-robot deleted the ehigham/cloud-credentials branch December 16, 2024 17:19
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.

4 participants