Skip to content

Deprecate rest.authorization-url in favor of oauth2-server-uri #962

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

Merged
merged 2 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions mkdocs/docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -186,19 +186,19 @@ catalog:

<!-- markdown-link-check-disable -->

| Key | Example | Description |
| ---------------------- | -------------------------------- | -------------------------------------------------------------------------------------------------- |
| uri | https://rest-catalog/ws | URI identifying the REST Server |
| ugi | t-1234:secret | Hadoop UGI for Hive client. |
| credential | t-1234:secret | Credential to use for OAuth2 credential flow when initializing the catalog |
| token | FEW23.DFSDF.FSDF | Bearer token value to use for `Authorization` header |
| scope | openid offline corpds:ds:profile | Desired scope of the requested security token (default : catalog) |
| resource | rest_catalog.iceberg.com | URI for the target resource or service |
| audience | rest_catalog | Logical name of target resource or service |
| rest.sigv4-enabled | true | Sign requests to the REST Server using AWS SigV4 protocol |
| rest.signing-region | us-east-1 | The region to use when SigV4 signing a request |
| rest.signing-name | execute-api | The service signing name to use when SigV4 signing a request |
| rest.authorization-url | https://auth-service/cc | Authentication URL to use for client credentials authentication (default: uri + 'v1/oauth/tokens') |
| Key | Example | Description |
| ------------------- | -------------------------------- | -------------------------------------------------------------------------------------------------- |
| uri | https://rest-catalog/ws | URI identifying the REST Server |
| ugi | t-1234:secret | Hadoop UGI for Hive client. |
| credential | t-1234:secret | Credential to use for OAuth2 credential flow when initializing the catalog |
| token | FEW23.DFSDF.FSDF | Bearer token value to use for `Authorization` header |
| scope | openid offline corpds:ds:profile | Desired scope of the requested security token (default : catalog) |
| resource | rest_catalog.iceberg.com | URI for the target resource or service |
| audience | rest_catalog | Logical name of target resource or service |
| rest.sigv4-enabled | true | Sign requests to the REST Server using AWS SigV4 protocol |
| rest.signing-region | us-east-1 | The region to use when SigV4 signing a request |
| rest.signing-name | execute-api | The service signing name to use when SigV4 signing a request |
| oauth2-server-uri | https://auth-service/cc | Authentication URL to use for client credentials authentication (default: uri + 'v1/oauth/tokens') |

<!-- markdown-link-check-enable-->

Expand Down
18 changes: 18 additions & 0 deletions mkdocs/docs/contributing.md
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,24 @@ Which will warn:
Call to load_something, deprecated in 0.1.0, will be removed in 0.2.0. Please use load_something_else() instead.
```

If you want to remove a property or notify about a behavior change, please add a deprecation notice by calling the deprecation_message function:

```python
from pyiceberg.utils.deprecated import deprecation_message

deprecation_message(
deprecated_in="0.1.0",
removed_in="0.2.0",
help_message="The old_property is deprecated. Please use the something_else property instead.",
)
```

Which will warn:

```
Deprecated in 0.1.0, will be removed in 0.2.0. The old_property is deprecated. Please use the something_else property instead.
```

## Type annotations

For the type annotation the types from the `Typing` package are used.
Expand Down
10 changes: 10 additions & 0 deletions mkdocs/docs/how-to-release.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ For example, the API with the following deprecation tag should be removed when p
)
```

We also have the `deprecation_message` function. We need to change the behavior according to what is noted in the message of that deprecation.

```python
deprecation_message(
deprecated_in="0.1.0",
removed_in="0.2.0",
help_message="The old_property is deprecated. Please use the something_else property instead.",
)
```

## Running a release candidate

Make sure that the version is correct in `pyproject.toml` and `pyiceberg/__init__.py`. Correct means that it reflects the version that you want to release.
Expand Down
6 changes: 3 additions & 3 deletions pyiceberg/catalog/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
RecursiveDict,
)
from pyiceberg.utils.config import Config, merge_config
from pyiceberg.utils.deprecated import deprecated
from pyiceberg.utils.deprecated import deprecation_message

if TYPE_CHECKING:
import pyarrow as pa
Expand Down Expand Up @@ -742,11 +742,11 @@ def __init__(self, name: str, **properties: str):

for property_name in DEPRECATED_PROPERTY_NAMES:
if self.properties.get(property_name):
deprecated(
deprecation_message(
deprecated_in="0.7.0",
removed_in="0.8.0",
help_message=f"The property {property_name} is deprecated. Please use properties that start with client., glue., and dynamo. instead",
)(lambda: None)()
)

def create_table_transaction(
self,
Expand Down
33 changes: 31 additions & 2 deletions pyiceberg/catalog/rest.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@
from pyiceberg.table.sorting import UNSORTED_SORT_ORDER, SortOrder, assign_fresh_sort_order_ids
from pyiceberg.typedef import EMPTY_DICT, UTF8, IcebergBaseModel, Identifier, Properties
from pyiceberg.types import transform_dict_value_to_str
from pyiceberg.utils.properties import property_as_bool
from pyiceberg.utils.deprecated import deprecation_message
from pyiceberg.utils.properties import get_first_property_value, property_as_bool

if TYPE_CHECKING:
import pyarrow as pa
Expand Down Expand Up @@ -120,6 +121,7 @@ class Endpoints:
SIGV4_REGION = "rest.signing-region"
SIGV4_SERVICE = "rest.signing-name"
AUTH_URL = "rest.authorization-url"
OAUTH2_SERVER_URI = "oauth2-server-uri"
HEADER_PREFIX = "header."

NAMESPACE_SEPARATOR = b"\x1f".decode(UTF8)
Expand Down Expand Up @@ -291,11 +293,38 @@ def url(self, endpoint: str, prefixed: bool = True, **kwargs: Any) -> str:

@property
def auth_url(self) -> str:
if url := self.properties.get(AUTH_URL):
if self.properties.get(AUTH_URL):
deprecation_message(
deprecated_in="0.8.0",
removed_in="0.9.0",
help_message=f"The property {AUTH_URL} is deprecated. Please use {OAUTH2_SERVER_URI} instead",
)

self._warn_oauth_tokens_deprecation()

if url := get_first_property_value(self.properties, AUTH_URL, OAUTH2_SERVER_URI):
return url
else:
return self.url(Endpoints.get_token, prefixed=False)

def _warn_oauth_tokens_deprecation(self) -> None:
has_oauth_server_uri = OAUTH2_SERVER_URI in self.properties
has_credential = CREDENTIAL in self.properties
has_init_token = TOKEN in self.properties
has_sigv4_enabled = property_as_bool(self.properties, SIGV4, False)

if not has_oauth_server_uri and (has_init_token or has_credential) and not has_sigv4_enabled:
deprecation_message(
deprecated_in="0.8.0",
removed_in="1.0.0",
help_message="Iceberg REST client is missing the OAuth2 server URI "
f"configuration and defaults to {self.uri}{Endpoints.get_token}. "
"This automatic fallback will be removed in a future Iceberg release."
f"It is recommended to configure the OAuth2 endpoint using the '{OAUTH2_SERVER_URI}'"
"property to be prepared. This warning will disappear if the OAuth2"
"endpoint is explicitly configured. See https://github.com/apache/iceberg/issues/10537",
)

def _extract_optional_oauth_params(self) -> Dict[str, str]:
optional_oauth_param = {SCOPE: self.properties.get(SCOPE) or CATALOG_SCOPE}
set_of_optional_params = {AUDIENCE, RESOURCE}
Expand Down
33 changes: 25 additions & 8 deletions pyiceberg/utils/deprecated.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,33 @@ def deprecated(deprecated_in: str, removed_in: str, help_message: Optional[str]
def decorator(func: Callable): # type: ignore
@functools.wraps(func)
def new_func(*args: Any, **kwargs: Any) -> Any:
warnings.simplefilter("always", DeprecationWarning) # turn off filter

warnings.warn(
f"Call to {func.__name__}, deprecated in {deprecated_in}, will be removed in {removed_in}.{help_message}",
category=DeprecationWarning,
stacklevel=2,
)
warnings.simplefilter("default", DeprecationWarning) # reset filter
message = f"Call to {func.__name__}, deprecated in {deprecated_in}, will be removed in {removed_in}.{help_message}"

_deprecation_warning(message)

return func(*args, **kwargs)

return new_func

return decorator


def deprecation_message(deprecated_in: str, removed_in: str, help_message: Optional[str]) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice addition, I like it a lot 👍

"""Mark properties or behaviors as deprecated.

Adding this will result in a warning being emitted.
"""
message = f"Deprecated in {deprecated_in}, will be removed in {removed_in}. {help_message}"

_deprecation_warning(message)


def _deprecation_warning(message: str) -> None:
warnings.simplefilter("always", DeprecationWarning) # turn off filter

warnings.warn(
message,
category=DeprecationWarning,
stacklevel=2,
)
warnings.simplefilter("default", DeprecationWarning) # reset filter
4 changes: 2 additions & 2 deletions tests/catalog/test_rest.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

import pyiceberg
from pyiceberg.catalog import PropertiesUpdateSummary, load_catalog
from pyiceberg.catalog.rest import AUTH_URL, RestCatalog
from pyiceberg.catalog.rest import OAUTH2_SERVER_URI, RestCatalog
from pyiceberg.exceptions import (
AuthorizationExpiredError,
NamespaceAlreadyExistsError,
Expand Down Expand Up @@ -235,7 +235,7 @@ def test_token_200_w_auth_url(rest_mock: Mocker) -> None:
)
# pylint: disable=W0212
assert (
RestCatalog("rest", uri=TEST_URI, credential=TEST_CREDENTIALS, **{AUTH_URL: TEST_AUTH_URL})._session.headers[
RestCatalog("rest", uri=TEST_URI, credential=TEST_CREDENTIALS, **{OAUTH2_SERVER_URI: TEST_AUTH_URL})._session.headers[
"Authorization"
]
== f"Bearer {TEST_TOKEN}"
Expand Down
14 changes: 14 additions & 0 deletions tests/utils/test_deprecated.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,17 @@ def deprecated_method() -> None:
assert warn.call_args[0] == (
"Call to deprecated_method, deprecated in 0.1.0, will be removed in 0.2.0. Please use load_something_else() instead.",
)


@patch("warnings.warn")
def test_deprecation_message(warn: Mock) -> None:
from pyiceberg.utils.deprecated import deprecation_message

deprecation_message(
deprecated_in="0.1.0",
removed_in="0.2.0",
help_message="Please use something_else instead",
)

assert warn.called
assert warn.call_args[0] == ("Deprecated in 0.1.0, will be removed in 0.2.0. Please use something_else instead",)