dimberman commented on code in PR #29580:
URL: https://github.com/apache/airflow/pull/29580#discussion_r1111314542
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -84,12 +88,24 @@ class SecretsManagerBackend(BaseSecretsBackend,
LoggingMixin):
:param connections_prefix: Specifies the prefix of the secret to read to
get Connections.
If set to None (null value in the configuration), requests for
connections will not be
sent to AWS Secrets Manager. If you don't want a connections_prefix,
set it as an empty string
+ :param connections_lookup_pattern: Specifies a pattern the connection ID
needs to match to be looked up in
+ AWS Secrets Manager. Applies only if `connections_prefix` is not None.
+ If set to None (null value in the configuration), all connections will
be looked up first in
+ AWS Secrets Manager.
Review Comment:
I really like this idea. Cool feature add on!
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -264,14 +286,14 @@ def get_conn_uri(self, conn_id: str) -> str | None:
def get_variable(self, key: str) -> str | None:
"""
- Get Airflow Variable from Environment Variable
+ Get Airflow Variable
Review Comment:
Can you add some more description here. If we're getting this variable from
multiple places maybe we should be clear about the options?
##########
airflow/providers/amazon/aws/secrets/systems_manager.py:
##########
@@ -166,16 +185,26 @@ def get_config(self, key: str) -> str | None:
if self.config_prefix is None:
return None
- return self._get_secret(self.config_prefix, key)
+ return self._get_secret(self.config_prefix, key,
self.config_lookup_pattern)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _get_secret(self, path_prefix: str, secret_id: str, lookup_pattern:
str | None) -> str | None:
"""
Get secret value from Parameter Store.
:param path_prefix: Prefix for the Path to get Secret
:param secret_id: Secret Key
+ :param lookup_pattern: If provided, `secret_id` must match this
pattern to look up the secret in
+ Systems Manager
"""
+ if lookup_pattern is not None and not re.match(lookup_pattern,
secret_id, re.IGNORECASE):
Review Comment:
```suggestion
if lookup_pattern and not re.match(lookup_pattern, secret_id,
re.IGNORECASE):
```
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -282,14 +304,19 @@ def get_config(self, key: str) -> str | None:
if self.config_prefix is None:
return None
- return self._get_secret(self.config_prefix, key)
+ return self._get_secret(self.config_prefix, key,
self.config_lookup_pattern)
- def _get_secret(self, path_prefix, secret_id: str) -> str | None:
+ def _get_secret(self, path_prefix, secret_id: str, lookup_pattern: str |
None) -> str | None:
"""
Get secret value from Secrets Manager
:param path_prefix: Prefix for the Path to get Secret
:param secret_id: Secret Key
+ :param lookup_pattern: If provided, `secret_id` must match this
pattern to look up the secret in
+ Secrets Manager
"""
+ if lookup_pattern is not None and not re.match(lookup_pattern,
secret_id, re.IGNORECASE):
Review Comment:
```suggestion
if lookup_pattern and not re.match(lookup_pattern, secret_id,
re.IGNORECASE):
```
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -41,13 +42,16 @@ class SecretsManagerBackend(BaseSecretsBackend,
LoggingMixin):
backend =
airflow.providers.amazon.aws.secrets.secrets_manager.SecretsManagerBackend
backend_kwargs = {"connections_prefix": "airflow/connections"}
- For example, if secrets prefix is ``airflow/connections/smtp_default``,
this would be accessible
- if you provide ``{"connections_prefix": "airflow/connections"}`` and
request conn_id ``smtp_default``.
- If variables prefix is ``airflow/variables/hello``, this would be
accessible
- if you provide ``{"variables_prefix": "airflow/variables"}`` and request
variable key ``hello``.
- And if config_prefix is ``airflow/config/sql_alchemy_conn``, this would be
accessible
- if you provide ``{"config_prefix": "airflow/config"}`` and request config
- key ``sql_alchemy_conn``.
+ For example, when ``{"connections_prefix": "airflow/connections"}`` is
set, if a secret is defined with
+ the path ``airflow/connections/smtp_default``, the connection with conn_id
``smtp_default`` would be
+ accessible.
+
+ When ``{"variables_prefix": "airflow/variables"}`` is set, if a secret is
defined with
+ the path ``airflow/variables/hello``, the variable with they ``hello``
would be accessible.
Review Comment:
```suggestion
the path ``airflow/variables/hello``, the variable with the
name``hello`` would be accessible.
```
##########
airflow/providers/amazon/aws/secrets/systems_manager.py:
##########
@@ -166,16 +185,26 @@ def get_config(self, key: str) -> str | None:
if self.config_prefix is None:
return None
- return self._get_secret(self.config_prefix, key)
+ return self._get_secret(self.config_prefix, key,
self.config_lookup_pattern)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _get_secret(self, path_prefix: str, secret_id: str, lookup_pattern:
str | None) -> str | None:
"""
Get secret value from Parameter Store.
:param path_prefix: Prefix for the Path to get Secret
:param secret_id: Secret Key
+ :param lookup_pattern: If provided, `secret_id` must match this
pattern to look up the secret in
+ Systems Manager
"""
+ if lookup_pattern is not None and not re.match(lookup_pattern,
secret_id, re.IGNORECASE):
+ return None
+
ssm_path = self.build_path(path_prefix, secret_id)
+
+ # AWS Systems Manager mandate to have a leading "/". Adding it
dynamically if not there
+ if not ssm_path.startswith("/"):
+ ssm_path = f"/{ssm_path}"
Review Comment:
Maybe break this out into a function? I know it's a small nit, but would be
a bit cleaner.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
