You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "sungwy (via GitHub)" <gi...@apache.org> on 2025/06/01 16:33:21 UTC
[PR] Add Support for Custom AuthManager implementation [iceberg-python]
sungwy opened a new pull request, #2055:
URL: https://github.com/apache/iceberg-python/pull/2055
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace ${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${1960} -->
# Rationale for this change
Expose a way for users to use a custom AuthManager when they define their REST Catalog.
- [ ] Add property parsing for custom AuthManager
- [x] Add OAuth2AuthManager as an example Custom AuthManager
# Are these changes tested?
Unit and integration tests will be added
# Are there any user-facing changes?
New properties will be exposed to support adding a custom AuthManager to RestCatalog
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#discussion_r2231654180
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -247,7 +249,23 @@ def _create_session(self) -> Session:
elif ssl_client_cert := ssl_client.get(CERT):
session.cert = ssl_client_cert
- session.auth = AuthManagerAdapter(self._create_legacy_oauth2_auth_manager(session))
+ if auth_config := self.properties.get(AUTH):
+ auth_type = auth_config.get("type")
+ if auth_type is None:
+ raise ValueError("auth.type must be defined")
+ auth_type_config = auth_config.get(auth_type, {})
+ auth_impl = auth_config.get("impl")
+
+ if auth_type is CUSTOM and not auth_impl:
+ raise ValueError("auth.impl must be specified when using custom auth.type")
+
+ if auth_type is not CUSTOM and auth_impl:
Review Comment:
The comparison `auth_type is CUSTOM` uses identity comparison with a string literal. This should use equality comparison (`==`) instead since `auth_type` comes from dictionary input and may not be the same string object.
```suggestion
if auth_type == CUSTOM and not auth_impl:
raise ValueError("auth.impl must be specified when using custom auth.type")
if auth_type != CUSTOM and auth_impl:
```
##########
tests/catalog/test_rest.py:
##########
@@ -1519,6 +1520,132 @@ def test_request_session_with_ssl_client_cert() -> None:
assert "Could not find the TLS certificate file, invalid path: path_to_client_cert" in str(e.value)
+def test_rest_catalog_with_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "basic",
+ "basic": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] == expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "dummy.nonexistent.package",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "Could not load AuthManager class for 'dummy.nonexistent.package'" in str(e.value)
+
+
+def test_rest_catalog_with_custom_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "pyiceberg.catalog.rest.auth.BasicAuthManager",
+ "custom": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] == expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type_no_impl() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "auth.impl must be specified when using custom auth.type" in str(e.value)
+
+
+def test_rest_catalog_with_non_custom_auth_type_impl() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "basic",
+ "impl": "basic.package",
+ "basic": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "auth.impl can only be specified when using custom auth.type" in str(e.value)
+
+
+def test_rest_catalog_with_unsupported_auth_type() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "unsupported",
+ "unsupported": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
Review Comment:
The comment 'Missing namespace' is misleading. The test is actually checking for an unsupported auth type, not a missing namespace.
```suggestion
# Unsupported authentication type
```
##########
tests/catalog/test_rest.py:
##########
@@ -1519,6 +1520,132 @@ def test_request_session_with_ssl_client_cert() -> None:
assert "Could not find the TLS certificate file, invalid path: path_to_client_cert" in str(e.value)
+def test_rest_catalog_with_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "basic",
+ "basic": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] == expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "dummy.nonexistent.package",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "Could not load AuthManager class for 'dummy.nonexistent.package'" in str(e.value)
+
+
+def test_rest_catalog_with_custom_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "pyiceberg.catalog.rest.auth.BasicAuthManager",
+ "custom": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] == expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type_no_impl() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "auth.impl must be specified when using custom auth.type" in str(e.value)
+
+
+def test_rest_catalog_with_non_custom_auth_type_impl() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "basic",
+ "impl": "basic.package",
+ "basic": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
Review Comment:
The comment 'Missing namespace' is misleading. The test is actually checking for an invalid impl property usage, not a missing namespace.
```suggestion
# Invalid impl property usage in non-custom auth.type
```
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -247,7 +249,23 @@ def _create_session(self) -> Session:
elif ssl_client_cert := ssl_client.get(CERT):
session.cert = ssl_client_cert
- session.auth = AuthManagerAdapter(self._create_legacy_oauth2_auth_manager(session))
+ if auth_config := self.properties.get(AUTH):
+ auth_type = auth_config.get("type")
+ if auth_type is None:
+ raise ValueError("auth.type must be defined")
+ auth_type_config = auth_config.get(auth_type, {})
+ auth_impl = auth_config.get("impl")
+
+ if auth_type is CUSTOM and not auth_impl:
+ raise ValueError("auth.impl must be specified when using custom auth.type")
+
+ if auth_type is not CUSTOM and auth_impl:
Review Comment:
The comparison `auth_type is not CUSTOM` uses identity comparison with a string literal. This should use inequality comparison (`!=`) instead since `auth_type` comes from dictionary input and may not be the same string object.
```suggestion
if auth_type != CUSTOM and auth_impl:
```
##########
tests/catalog/test_rest.py:
##########
@@ -1519,6 +1520,132 @@ def test_request_session_with_ssl_client_cert() -> None:
assert "Could not find the TLS certificate file, invalid path: path_to_client_cert" in str(e.value)
+def test_rest_catalog_with_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "basic",
+ "basic": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] == expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "dummy.nonexistent.package",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "Could not load AuthManager class for 'dummy.nonexistent.package'" in str(e.value)
+
+
+def test_rest_catalog_with_custom_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "pyiceberg.catalog.rest.auth.BasicAuthManager",
+ "custom": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] == expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type_no_impl() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
Review Comment:
The comment 'Missing namespace' is misleading. The test is actually checking for a missing impl property when using custom auth type, not a missing namespace.
```suggestion
# Missing 'impl' property in custom auth configuration
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#discussion_r2216931460
##########
pyiceberg/catalog/rest/auth.py:
##########
@@ -109,6 +122,95 @@ def auth_header(self) -> str:
return f"Bearer {self._token}"
+class OAuth2TokenProvider:
+ """Thread-safe OAuth2 token provider with token refresh support."""
+
+ client_id: str
+ client_secret: str
+ token_url: str
+ scope: Optional[str]
+ refresh_margin: int
+ expires_in: Optional[int]
+
+ _token: Optional[str]
+ _expires_at: int
+ _lock: threading.Lock
+
+ def __init__(
+ self,
+ client_id: str,
+ client_secret: str,
Review Comment:
should we let users provide these 2 separately? or parse them out from `credential` based on https://github.com/apache/iceberg-python/blob/main/mkdocs/docs/configuration.md#oauth2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#discussion_r2224189527
##########
mkdocs/docs/configuration.md:
##########
@@ -374,6 +374,94 @@ Specific headers defined by the RESTCatalog spec include:
| ------------------------------------ | ------------------------------------- | -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `header.X-Iceberg-Access-Delegation` | `{vended-credentials,remote-signing}` | `vended-credentials` | Signal to the server that the client supports delegated access via a comma-separated list of access mechanisms. The server may choose to supply access via any or none of the requested mechanisms |
+#### Authentication in RESTCatalog
Review Comment:
separated out to: https://github.com/apache/iceberg-python/pull/2244
##########
pyiceberg/catalog/rest/auth.py:
##########
@@ -109,6 +122,95 @@ def auth_header(self) -> str:
return f"Bearer {self._token}"
+class OAuth2TokenProvider:
+ """Thread-safe OAuth2 token provider with token refresh support."""
+
+ client_id: str
+ client_secret: str
+ token_url: str
+ scope: Optional[str]
+ refresh_margin: int
+ expires_in: Optional[int]
+
+ _token: Optional[str]
+ _expires_at: int
+ _lock: threading.Lock
+
+ def __init__(
+ self,
+ client_id: str,
+ client_secret: str,
Review Comment:
separated out to: https://github.com/apache/iceberg-python/pull/2244
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "corleyma (via GitHub)" <gi...@apache.org>.
corleyma commented on code in PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#discussion_r2214822904
##########
tests/catalog/test_rest.py:
##########
@@ -1519,6 +1519,131 @@ def test_request_session_with_ssl_client_cert() -> None:
assert "Could not find the TLS certificate file, invalid path: path_to_client_cert" in str(e.value)
+def test_rest_catalog_with_basic_auth_type() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "basic",
+ "basic": {
+ "username": "one",
+ },
+ },
+ }
+ with pytest.raises(TypeError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "__init__() missing 1 required positional argument: 'password'" in str(e.value)
+
+
+def test_rest_catalog_with_custom_auth_type() -> None:
Review Comment:
could imagine doing a version of this test that specifies the basic auth as the implementation and just verifies that the custom implementation is actually used/that properties specified for custom are actually passed to the implementation.
##########
mkdocs/docs/configuration.md:
##########
@@ -374,6 +374,94 @@ Specific headers defined by the RESTCatalog spec include:
| ------------------------------------ | ------------------------------------- | -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `header.X-Iceberg-Access-Delegation` | `{vended-credentials,remote-signing}` | `vended-credentials` | Signal to the server that the client supports delegated access via a comma-separated list of access mechanisms. The server may choose to supply access via any or none of the requested mechanisms |
+#### Authentication in RESTCatalog
+
+The RESTCatalog supports pluggable authentication via the `auth` configuration block. This allows you to specify which how the access token will be fetched and managed for use with the HTTP requests to the RESTCatalog server. The authentication method is selected by setting the `auth.type` property, and additional configuration can be provided as needed for each method.
+
+##### Supported Authentication Types
+
+- `noop`: No authentication (no Authorization header sent).
+- `basic`: HTTP Basic authentication.
+- `oauth2`: OAuth2 client credentials flow.
+- `legacyoauth2`: Legacy OAuth2 client credentials flow (Deprecated and will be removed in PyIceberg 1.0.0)
+- `custom`: Custom authentication manager (requires `auth.impl`).
+
+##### Configuration Properties
+
+The `auth` block is structured as follows:
+
+```yaml
+catalog:
+ default:
+ type: rest
+ uri: http://rest-catalog/ws/
+ auth:
+ type: <auth_type>
+ <auth_type>:
+ # Type-specific configuration
+ impl: <custom_class_path> # Only for custom auth
+```
+
+**Property Reference:**
+
+| Property | Required | Description |
+|------------------|----------|-------------------------------------------------------------------------------------------------|
+| `auth.type` | Yes | The authentication type to use (`noop`, `basic`, `oauth2`, or `custom`). |
Review Comment:
on review I was debating the necessity of a separate type config given an impl option, but I can see the appeal (we are free to change the implementation referred to by a given type).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#discussion_r2216937726
##########
mkdocs/docs/configuration.md:
##########
@@ -374,6 +374,94 @@ Specific headers defined by the RESTCatalog spec include:
| ------------------------------------ | ------------------------------------- | -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `header.X-Iceberg-Access-Delegation` | `{vended-credentials,remote-signing}` | `vended-credentials` | Signal to the server that the client supports delegated access via a comma-separated list of access mechanisms. The server may choose to supply access via any or none of the requested mechanisms |
+#### Authentication in RESTCatalog
+
+The RESTCatalog supports pluggable authentication via the `auth` configuration block. This allows you to specify which how the access token will be fetched and managed for use with the HTTP requests to the RESTCatalog server. The authentication method is selected by setting the `auth.type` property, and additional configuration can be provided as needed for each method.
+
+##### Supported Authentication Types
+
+- `noop`: No authentication (no Authorization header sent).
+- `basic`: HTTP Basic authentication.
+- `oauth2`: OAuth2 client credentials flow.
+- `legacyoauth2`: Legacy OAuth2 client credentials flow (Deprecated and will be removed in PyIceberg 1.0.0)
+- `custom`: Custom authentication manager (requires `auth.impl`).
+
+##### Configuration Properties
+
+The `auth` block is structured as follows:
+
+```yaml
+catalog:
+ default:
+ type: rest
+ uri: http://rest-catalog/ws/
+ auth:
+ type: <auth_type>
+ <auth_type>:
+ # Type-specific configuration
+ impl: <custom_class_path> # Only for custom auth
+```
+
+**Property Reference:**
+
+| Property | Required | Description |
+|------------------|----------|-------------------------------------------------------------------------------------------------|
+| `auth.type` | Yes | The authentication type to use (`noop`, `basic`, `oauth2`, or `custom`). |
Review Comment:
we have similar pattern to load "custom" implementation for catalog/file_io/location_provider :)
https://grep.app/search?f.repo=apache%2Ficeberg-python&f.repo.pattern=iceberg&q=def+_import_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#issuecomment-3105384321
It looks like there's some more discussions to be had to introduce the OAuth2AuthManager into the repository, and I think there's a lot of value in introducing the custom AuthManager parsing in this release.
@kevinjqliu - let me separate that implementation out to a separate review
@corleyma - I will take your suggestion to use the basic auth manager for testing for this PR :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#issuecomment-3119631520
> @sungwy For absolute completeness, you might consider adding a simple test for type: noop that asserts that the Authorization header is not present in the request. This would confirm the "off switch" works as intended.
+1 to this. can we also test the fallback when the `auth:` block is not specified?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#issuecomment-3119873468
Merged, this should unblock #2244 and #2072
Thanks everyone for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#issuecomment-3119600292
first time trying to copilot, good bot!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#discussion_r2224189527
##########
mkdocs/docs/configuration.md:
##########
@@ -374,6 +374,94 @@ Specific headers defined by the RESTCatalog spec include:
| ------------------------------------ | ------------------------------------- | -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `header.X-Iceberg-Access-Delegation` | `{vended-credentials,remote-signing}` | `vended-credentials` | Signal to the server that the client supports delegated access via a comma-separated list of access mechanisms. The server may choose to supply access via any or none of the requested mechanisms |
+#### Authentication in RESTCatalog
Review Comment:
separated out to: https://github.com/apache/iceberg-python/pull/2244
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu merged PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add Support for Custom AuthManager implementation [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#discussion_r2216919410
##########
mkdocs/docs/configuration.md:
##########
@@ -374,6 +374,94 @@ Specific headers defined by the RESTCatalog spec include:
| ------------------------------------ | ------------------------------------- | -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `header.X-Iceberg-Access-Delegation` | `{vended-credentials,remote-signing}` | `vended-credentials` | Signal to the server that the client supports delegated access via a comma-separated list of access mechanisms. The server may choose to supply access via any or none of the requested mechanisms |
+#### Authentication in RESTCatalog
Review Comment:
This is great! A heads up, we recently merged a PR which added an "Authentication Options" section under the REST Catalog docs. Would be great to merge with what you have here.
See,
https://github.com/apache/iceberg-python/blob/main/mkdocs/docs/configuration.md#authentication-options
##########
pyiceberg/catalog/rest/auth.py:
##########
@@ -109,6 +122,95 @@ def auth_header(self) -> str:
return f"Bearer {self._token}"
+class OAuth2TokenProvider:
+ """Thread-safe OAuth2 token provider with token refresh support."""
+
+ client_id: str
+ client_secret: str
+ token_url: str
+ scope: Optional[str]
+ refresh_margin: int
+ expires_in: Optional[int]
+
+ _token: Optional[str]
+ _expires_at: int
+ _lock: threading.Lock
+
+ def __init__(
+ self,
+ client_id: str,
+ client_secret: str,
Review Comment:
there are a couple of options in the oauth section
token
credential
scope
resource
audience
`LegacyOAuth2AuthManager` currently supports this, so i think we should support them in the new implementation too
WDYT?
##########
pyiceberg/catalog/rest/auth.py:
##########
@@ -109,6 +122,95 @@ def auth_header(self) -> str:
return f"Bearer {self._token}"
+class OAuth2TokenProvider:
+ """Thread-safe OAuth2 token provider with token refresh support."""
+
+ client_id: str
+ client_secret: str
+ token_url: str
+ scope: Optional[str]
+ refresh_margin: int
+ expires_in: Optional[int]
+
+ _token: Optional[str]
+ _expires_at: int
+ _lock: threading.Lock
+
+ def __init__(
+ self,
+ client_id: str,
+ client_secret: str,
Review Comment:
should we let users provide these 2 separately? or parse them out from `credential` based on https://github.com/apache/iceberg-python/blob/main/mkdocs/docs/configuration.md#oauth2
##########
pyiceberg/catalog/rest/auth.py:
##########
@@ -109,6 +122,95 @@ def auth_header(self) -> str:
return f"Bearer {self._token}"
+class OAuth2TokenProvider:
+ """Thread-safe OAuth2 token provider with token refresh support."""
+
+ client_id: str
+ client_secret: str
+ token_url: str
+ scope: Optional[str]
+ refresh_margin: int
+ expires_in: Optional[int]
+
+ _token: Optional[str]
+ _expires_at: int
+ _lock: threading.Lock
+
+ def __init__(
+ self,
+ client_id: str,
+ client_secret: str,
Review Comment:
should we let users provide these 2 separately? or parse them out from `credential` based on https://github.com/apache/iceberg-python/blob/main/mkdocs/docs/configuration.md#oauth2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org