Access delegation #1033
Access delegation #1033
Conversation
Co-authored-by: Fokko Driesprong <[email protected]>
Co-authored-by: Fokko Driesprong <[email protected]>
Co-authored-by: Fokko Driesprong <[email protected]>
|
@guitcastro Can you rebase this one, this looks good to me 👍 |
|
@Fokko done :) |
|
@Fokko Can you please take a look in this? |
No problem at all. I have renamed the property to use dash instead of |
There was a problem hiding this comment.
Hi @guitcastro thank you for raising the issue and working on this PR. I've left some comments regarding the approach - please let me know what you think!
pyiceberg/catalog/rest.py
Outdated
| @@ -532,7 +534,7 @@ def _config_headers(self, session: Session) -> None: | |||
| session.headers["Content-type"] = "application/json" | |||
| session.headers["X-Client-Version"] = ICEBERG_REST_SPEC_VERSION | |||
| session.headers["User-Agent"] = f"PyIceberg/{__version__}" | |||
| session.headers["X-Iceberg-Access-Delegation"] = "vended-credentials" | |||
| session.headers["X-Iceberg-Access-Delegation"] = self.properties.get(ACCESS_DELEGATION, ACCESS_DELEGATION_DEFAULT) | |||
There was a problem hiding this comment.
It looks like we already have a way of setting custom headers in _extract_headers_from_properties. If I understand it correctly, if we set a property like: header.X-Iceberg-Access-Delegation = remote-signing then this should set the header "X-Iceberg-Access-Delegation" as remote-signing.
I think we could achieve this by setting the default header values, and then setting the property based values after the default values are set:
session.headers["Content-type"] = "application/json"
session.headers["X-Client-Version"] = ICEBERG_REST_SPEC_VERSION
session.headers["User-Agent"] = f"PyIceberg/{__version__}"
session.headers["X-Iceberg-Access-Delegation"] = "vended-credentials"
header_properties = self._extract_headers_from_properties()
session.headers.update(header_properties)
What do you think of this approach over introducing a different property?
There was a problem hiding this comment.
IMHO having an explicit and documented property is better than setting custom header. But I can change if you think it's better.
There was a problem hiding this comment.
@sungwy I tried your code, but some tests failed with:
assert (
catalog._session.headers.get("Content-type") == "application/json"
), "Expected 'Content-Type' default header not to be overwritten"
I guess the idea is not allow default header override. Thus I guess having a dedicate property still the best option. What are you thoughts on this?
There was a problem hiding this comment.
Hi @guitcastro - I appreciate you giving it a go. That error message is interesting because its specific to the Content-Type property, Did you intend to overwrite the Content-Type as well?
There was a problem hiding this comment.
@guitcastro I was able to make @sungwy's suggestion work in guitcastro#1, without breaking existing tests.
Co-authored-by: Sung Yun <[email protected]>
Suggestions for apache#1033
This PR fix the hardcoded
X-Iceberg-Access-Delegationheader, the second point of #1028.It's based on the #1029 , and must be merged after.
Closes #1028