You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/08/10 20:24:39 UTC
[GitHub] [airflow] o-nikolas commented on a diff in pull request #25628: Allow AWS Secrets Backends use AWS Connection capabilities
o-nikolas commented on code in PR #25628:
URL: https://github.com/apache/airflow/pull/25628#discussion_r942801868
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -58,8 +50,17 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
if you provide ``{"config_prefix": "airflow/config"}`` and request config
key ``sql_alchemy_conn``.
- You can also pass additional keyword arguments like ``aws_secret_access_key``, ``aws_access_key_id``
- or ``region_name`` to this class and they would be passed on to Boto3 client.
+ You can also pass additional keyword arguments listed in AWS Connection Extra config
+ to this class, and they would be used for establish connection and passed on to Boto3 client.
Review Comment:
```suggestion
to this class, and they would be used for establishing a connection and passed on to Boto3 client.
```
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -154,14 +153,34 @@ def __init__(
)
self.are_secret_values_urlencoded = are_secret_values_urlencoded
self.extra_conn_words = extra_conn_words or {}
+
+ self.profile_name = kwargs.get("profile_name", None)
Review Comment:
You need to remove the doc string for this from above
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -127,13 +139,33 @@ def __post_init__(self, conn: "Connection"):
init_credentials = self._get_credentials(**extra)
self.aws_access_key_id, self.aws_secret_access_key, self.aws_session_token = init_credentials
- if not self.region_name and "region_name" in extra:
- self.region_name = extra["region_name"]
- self.log.info("Retrieving region_name=%s from %s extra.", self.region_name, self.conn_repr)
+ if not self.region_name:
+ if "region_name" in extra:
+ self.region_name = extra["region_name"]
+ self.log.info("Retrieving region_name=%s from %s extra.", self.region_name, self.conn_repr)
Review Comment:
Should this be debug level?
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -430,6 +423,11 @@ def config(self) -> Optional[Config]:
"""Configuration for botocore client read-only property."""
return self.conn_config.botocore_config
+ @property
+ def verify(self) -> Optional[str]:
+ """Verify or not SSL certificates boto3 client/resource read-only property."""
+ return self.conn_config.region_name
+
Review Comment:
I'm confused by this property. Why is this just returning the region?
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -127,13 +139,33 @@ def __post_init__(self, conn: "Connection"):
init_credentials = self._get_credentials(**extra)
self.aws_access_key_id, self.aws_secret_access_key, self.aws_session_token = init_credentials
- if not self.region_name and "region_name" in extra:
- self.region_name = extra["region_name"]
- self.log.info("Retrieving region_name=%s from %s extra.", self.region_name, self.conn_repr)
+ if not self.region_name:
+ if "region_name" in extra:
+ self.region_name = extra["region_name"]
+ self.log.info("Retrieving region_name=%s from %s extra.", self.region_name, self.conn_repr)
+ elif "region_name" in session_kwargs:
+ self.region_name = session_kwargs["region_name"]
+ self.log.info(
+ "Retrieving region_name=%s from %s extra['session_kwargs'].",
+ self.region_name,
+ self.conn_repr,
+ )
+
+ if not self.verify and "verify" in extra:
+ self.verify = extra["verify"]
+ self.log.debug("Retrieving verify=%s from %s extra.", self.verify, self.conn_repr)
- if "session_kwargs" in extra:
- self.session_kwargs = extra["session_kwargs"]
- self.log.info("Retrieving session_kwargs=%s from %s extra.", self.session_kwargs, self.conn_repr)
+ self.profile_name = extra.get("session_kwargs", {}).get("profile_name")
Review Comment:
You've already gotten `session_kwargs` on line 115, you don't need to fetch it again from `extra` right?
Also you fetch the value form session_kwargs on line 162 anyway, so I don't think you need this line at all.
##########
docs/apache-airflow-providers-amazon/connections/aws.rst:
##########
@@ -63,27 +64,37 @@ AWS Secret Access Key (optional)
Extra (optional)
Specify the extra parameters (as json dictionary) that can be used in AWS
- connection. The following parameters are all optional:
+ connection. All parameters are optional.
+
+ The following extra parameters used for create initial
+ `boto3.session.Session <https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html>`__:
* ``aws_access_key_id``: AWS access key ID used for the initial connection.
* ``aws_secret_access_key``: AWS secret access key used for the initial connection
* ``aws_session_token``: AWS session token used for the initial connection if you use external credentials.
You are responsible for renewing these.
* ``region_name``: AWS Region for the connection.
- * ``session_kwargs``: Additional **kwargs** passed to
- `boto3.session.Session <https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html>`__.
- * ``config_kwargs``: Additional **kwargs** used to construct a
- `botocore.config.Config <https://botocore.amazonaws.com/v1/documentation/api/latest/reference/config.html>`__
- passed to `boto3.client <https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html#boto3.session.Session.client>`__
- and `boto3.resource <https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html#boto3.session.Session.resource>`__.
+ * ``profile_name``: The name of a profile to use listed in
+ `configuration and credential file settings <https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html#cli-configure-files-settings>`__.
+
+ The following extra parameters used for `assume role <https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRole.html>`__:
+
* ``role_arn``: If specified, then assume this role, obtaining a set of temporary security credentials using the ``assume_role_method``.
* ``assume_role_method``: AWS STS client method, one of
`assume_role <https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRole.html>`__,
`assume_role_with_saml <https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRoleWithSAML.html>`__ or
`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``.
+
+ The following extra parameters used for passed to
Review Comment:
```suggestion
The following extra parameters used for
```
##########
docs/apache-airflow-providers-amazon/connections/aws.rst:
##########
@@ -63,27 +64,37 @@ AWS Secret Access Key (optional)
Extra (optional)
Specify the extra parameters (as json dictionary) that can be used in AWS
- connection. The following parameters are all optional:
+ connection. All parameters are optional.
+
+ The following extra parameters used for create initial
Review Comment:
```suggestion
The following extra parameters used to create an initial
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org