Skip to content

Deprecate usage of extra[host] in AWS's connection #25494

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.

Sign up for GitHub

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 1 commit into from
Aug 8, 2022
Merged

Deprecate usage of extra[host] in AWS's connection #25494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion airflow/providers/amazon/aws/hooks/base_aws.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,7 @@ def get_ui_field_behaviour() -> Dict[str, Any]:
"assume_role_method": "assume_role",
"assume_role_kwargs": {"RoleSessionName": "airflow"},
"aws_session_token": "AQoDYXdzEJr...EXAMPLETOKEN",
"host": "http://localhost:4566",
"endpoint_url": "http://localhost:4566",
},
indent=2,
),
Expand Down
17 changes: 17 additions & 0 deletions airflow/providers/amazon/aws/utils/connection_wrapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,24 @@ def __post_init__(self, conn: "Connection"):
self.log.info("Retrieving botocore config=%s from %s extra.", config_kwargs, self.conn_repr)
self.botocore_config = Config(**config_kwargs)

if conn.host:
warnings.warn(
f"Host {conn.host} specified in the connection is not used."
" Please, set it on extra['endpoint_url'] instead",
DeprecationWarning,
stacklevel=2,
)

self.endpoint_url = extra.get("host")
if self.endpoint_url:
warnings.warn(
"extra['host'] is deprecated and will be removed in a future release."
Copy link
Contributor

Choose a reason for hiding this comment

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

@potuik If the Amazon provider is going to be a major version bump this month, is the deprecation message even required? The provider could just stop supporting the host connection field.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. That's a bit aggressive, but we could raise plain error here. I do not see this as being a major problem. But removing it altogether is not a good idea because it would likely stop doing that it was doing before without even a warning in some cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood. So more of a release flow of feature -> deprecate/message about not using feature -> remove feature regardless of major version bump along the way.

" Please set extra['endpoint_url'] instead",
DeprecationWarning,
stacklevel=2,
)
else:
self.endpoint_url = extra.get("endpoint_url")

# Retrieve Assume Role Configuration
assume_role_configs = self._get_assume_role_configs(**extra)
Expand Down
3 changes: 2 additions & 1 deletion docs/apache-airflow-providers-amazon/connections/aws.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ Extra (optional)
`assume_role_with_web_identity <https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRoleWithWebIdentity.html>`__
if not specified then **assume_role** is used.
* ``assume_role_kwargs``: Additional **kwargs** passed to ``assume_role_method``.
* ``host``: Endpoint URL for the connection.
* ``endpoint_url``: Endpoint URL for the connection.

.. warning:: Extra parameters below are deprecated and will be removed in a future version of this provider.

Expand All @@ -98,6 +98,7 @@ Extra (optional)
`s3cmd <https://s3tools.org/kb/item14.htm>`_ if not specified then **boto** is used.
* ``profile``: If you are getting your credentials from the ``s3_config_file``
you can specify the profile with this parameter.
* ``host``: Used as connection's URL. Use ``endpoint_url`` instead.

If you are configuring the connection via a URI, ensure that all components of the URI are URL-encoded.

Expand Down
40 changes: 35 additions & 5 deletions tests/providers/amazon/aws/utils/test_connection_wrapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,11 +201,30 @@ def test_get_botocore_config(self, mock_botocore_config, botocore_config, botoco
assert mock_botocore_config.assert_called_once
assert mock.call(**botocore_config_kwargs) in mock_botocore_config.mock_calls

@pytest.mark.parametrize("endpoint_url", [None, "https://example.org"])
def test_get_endpoint_url(self, endpoint_url):
mock_conn = mock_connection_factory(extra={"host": endpoint_url} if endpoint_url else None)
wrap_conn = AwsConnectionWrapper(conn=mock_conn)
assert wrap_conn.endpoint_url == endpoint_url
@pytest.mark.parametrize(
"extra, expected",
[
({"host": "https://host.aws"}, "https://host.aws"),
({"endpoint_url": "https://endpoint.aws"}, "https://endpoint.aws"),
({"host": "https://host.aws", "endpoint_url": "https://endpoint.aws"}, "https://host.aws"),
],
ids=["'host' is used", "'endpoint_url' is used", "'host' preferred over 'endpoint_url'"],
)
def test_get_endpoint_url_from_extra(self, extra, expected):
mock_conn = mock_connection_factory(extra=extra)
expected_deprecation_message = (
"extra['host'] is deprecated and will be removed in a future release."
" Please set extra['endpoint_url'] instead"
)

with pytest.warns(None) as records:
wrap_conn = AwsConnectionWrapper(conn=mock_conn)

if extra.get("host"):
assert len(records) == 1
assert str(records[0].message) == expected_deprecation_message

assert wrap_conn.endpoint_url == expected

@pytest.mark.parametrize("aws_account_id, aws_iam_role", [(None, None), ("111111111111", "another-role")])
def test_get_role_arn(self, aws_account_id, aws_iam_role):
Expand Down Expand Up @@ -351,3 +370,14 @@ def test_wrap_wrapper(self, orig_wrapper, region_name, botocore_config):
# Test overwrite/inherit init fields
assert wrap_conn.region_name == (region_name or orig_wrapper.region_name)
assert wrap_conn.botocore_config == (botocore_config or orig_wrapper.botocore_config)

def test_connection_host_raises_deprecation(self):
mock_conn = mock_connection_factory(host="https://aws.com")
expected_deprecation_message = (
f"Host {mock_conn.host} specified in the connection is not used."
" Please, set it on extra['endpoint_url'] instead"
)
with pytest.warns(DeprecationWarning) as record:
AwsConnectionWrapper(conn=mock_conn)

assert str(record[0].message) == expected_deprecation_message