-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Python: Add support for providing SSL config for REST Catalog client. #6019
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
Python: Add support for providing SSL config for REST Catalog client. #6019
Conversation
|
High level summary of changes done as part of the PR:
|
…aders as well as it no longer needs to be a property.
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.
Thanks for working on this @dhruv-pratap! I like it a lot. Another approach we could also take is to have a function _create_session() -> Session that will set up the session. This would both set the SSL and the headers, and in the constructor, we would do:
self.session = self._create_session()
This way we have a single place where we set up the session, instead of two methods. WDYT?
python/pyiceberg/catalog/rest.py
Outdated
| "X-Client-Version": ICEBERG_REST_SPEC_VERSION, | ||
| "User-Agent": f"PyIceberg/{__version__}", | ||
| } | ||
| def _set_session_headers(self): |
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 like this a lot, much nicer than having to pass in the headers every time.
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.
Thanks for working on this @dhruv-pratap! I like it a lot. Another approach we could also take is to have a function
_create_session() -> Sessionthat will set up the session. This would both set the SSL and the headers, and in the constructor, we would do:self.session = self._create_session()This way we have a single place where we set up the session, instead of two methods. WDYT?
I did think about that initially, but the self._fetch_access_token(credential) REST call poses a kind-of chicken-and-egg problem. You need the session SSL config to be configured to make that REST call, and then the session's header has to be enriched with the Auth Token. Let me try to give it a shot to see what that looks like and send an amendment.
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.
Suggested changes pushed in recent commit. Please review.
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.
@dhruv-pratapI didn't think of the chicken-and-egg problem. The current version isn't working:
RESTError: HttpMediaTypeNotSupportedException: Content type 'application/json' not supported
It already sets the application/json header, but we need to set the application/x-www-form-urlencoded header for the signer:
https://github.com/apache/iceberg/blob/master/python/pyiceberg/catalog/rest.py#L237
We either need to set the headers after we fetch the token, or revert to the previous version.
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.
Sorry, I didn't have an OAuth environment to integration test it against.
Changed the order in recent commit, now setting the HTTP headers after fetching the OAuth token. Also, enforcing the content-type check to application/x-www-form-urlencoded for OAuth calls in unit tests.
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.
No problem at all. I've just checked and it works 👍🏻
Co-authored-by: Fokko Driesprong <fokko@apache.org>
Co-authored-by: Fokko Driesprong <fokko@apache.org>
…uth token call expects the Content-Type application/x-www-form-urlencoded. Enforce the content-type check for oauth calls in unit tests as well.
|
This is awesome, thanks @dhruv-pratap 👍🏻 |
Context: https://apache-iceberg.slack.com/archives/C029EE6HQ5D/p1666112648002419
We at Netflix here are trying to integrate PyIceberg 0.1.0 with our Iceberg Rest Catalog Service and realized there is a gap in the PyIceberg rest client that we need to address.
For background, at Netflix all client-server interaction happens over TLS and is client side auth enforced for security purposes. The rest client that sits inside PyIceberg at present uses requests module for interaction with rest catalog service. Although this module allows the CA trust bundle to be set via an environment variable, but it does not allow a similar mechanism for setting client side certificates via environment variable and has to be done programmatically when setting up a requests client.
The below two approaches were discussed on the Slack thread:
After discussion with @samredai and @Fokko we agreed on approach #2, and this PR is to address the same.