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 2020/07/03 15:41:46 UTC
[GitHub] [airflow] kaxil opened a new pull request #9645: Get Airflow configs with sensitive data from Secret Backends
kaxil opened a new pull request #9645:
URL: https://github.com/apache/airflow/pull/9645
We should be able to get the followings config containing passwords and other sensitive data from Secret Backends:
```
('core', 'sql_alchemy_conn'),
('core', 'fernet_key'),
('celery', 'broker_url'),
('celery', 'flower_basic_auth'),
('celery', 'result_backend'),
('atlas', 'password'),
('smtp', 'smtp_password'),
('ldap', 'bind_password'),
('kubernetes', 'git_password'),
```
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Target Github ISSUE in description if exists
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451463659
##########
File path: airflow/configuration.py
##########
@@ -85,6 +85,15 @@ def run_command(command):
return output
+def _get_config_value_from_secret_backend(config_key):
Review comment:
Just to keep it similar to `def run_command(command):`, no other reason
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r449726296
##########
File path: airflow/configuration.py
##########
@@ -267,17 +271,49 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
return run_command(os.environ[env_var_cmd])
def _get_cmd_option(self, section, key):
fallback_key = key + '_cmd'
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
if super().has_option(section, fallback_key):
command = super().get(section, fallback_key)
return run_command(command)
+ @cached_property
+ def _secrets_backend_client(self):
+ if super().has_option('secrets', 'backend') is False:
+ return None
+
+ secrets_backend_cls = super().get('secrets', 'backend')
+ if not secrets_backend_cls:
+ return None
+ print("secrets_backend_cls", secrets_backend_cls)
+ secrets_backend = import_string(secrets_backend_cls)
+ if secrets_backend:
+ try:
+ alternative_secrets_config_dict = json.loads(
+ super().get('secrets', 'backend_kwargs', fallback='{}')
+ )
+ except json.JSONDecodeError:
+ alternative_secrets_config_dict = {}
+ return secrets_backend(**alternative_secrets_config_dict)
Review comment:
Updated with TYPE_CHECKING in https://github.com/apache/airflow/pull/9645/commits/ed8cc1a6316ddbd0971d1d5e56ca8c61bdab08e7
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451196233
##########
File path: airflow/configuration.py
##########
@@ -267,17 +278,32 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.sensitive_config_values:
return run_command(os.environ[env_var_cmd])
+ # alternatively AIRFLOW__{SECTION}__{KEY}_SECRET (to get from Secrets Backend)
+ env_var_cmd = env_var + '_SECRET'
+ if env_var_cmd in os.environ:
+ # if this is a valid command key...
+ if (section, key) in self.sensitive_config_values:
+ return _get_config_value_from_secret_backend(os.environ[env_var_cmd])
Review comment:
Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r450716775
##########
File path: docs/howto/set-config.rst
##########
@@ -46,7 +46,18 @@ the key like this:
[core]
sql_alchemy_conn_cmd = bash_command_to_run
-The following config options support this ``_cmd`` version:
+You can also derive the connection string at run time by appending ``_secret`` to
+the key like this:
+
+.. code-block:: ini
+
+ [core]
+ sql_alchemy_conn_secret = sql_alchemy_conn
Review comment:
Hmmmmm, I wonder if this will be confusing for people running in Kube, who might expect this to come from a Kube secret.
(I don't have a suggestion on how to deal with this.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451464977
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -42,8 +42,11 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
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``.
- And if variables prefix is ``airflow/variables/hello``, this would be accessible
+ 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 configs_prefix is ``airflow/configs/sql_alchemy_conn``, this would be accessible
+ if you provide ``{"configs_prefix": "airflow/configs"}`` and request variable
Review comment:
```suggestion
if you provide ``{"configs_prefix": "airflow/configs"}`` and request config
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r450718657
##########
File path: docs/howto/set-config.rst
##########
@@ -73,6 +91,7 @@ The universal order of precedence for all configuration options is as follows:
#. set as a command environment variable
Review comment:
```suggestion
#. set as a command environment variable
#. set as a secret environment variable
```
Though the wording on this isn't that clear :(
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r449642561
##########
File path: airflow/configuration.py
##########
@@ -267,17 +272,49 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
return run_command(os.environ[env_var_cmd])
def _get_cmd_option(self, section, key):
fallback_key = key + '_cmd'
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
if super().has_option(section, fallback_key):
command = super().get(section, fallback_key)
return run_command(command)
+ @cached_property
+ def _secrets_backend_client(self):
+ if super().has_option('secrets', 'backend') is False:
+ return None
+
+ secrets_backend_cls = super().get('secrets', 'backend')
+ if not secrets_backend_cls:
+ return None
+ print("secrets_backend_cls", secrets_backend_cls)
+ secrets_backend = import_string(secrets_backend_cls)
+ if secrets_backend:
+ try:
+ alternative_secrets_config_dict = json.loads(
+ super().get('secrets', 'backend_kwargs', fallback='{}')
+ )
+ except JSONDecodeError:
Review comment:
Done in https://github.com/apache/airflow/pull/9645/commits/a3ceb574fcef9c6cd6baea0bbcc58d08d2becd9e
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r449647991
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -62,13 +67,15 @@ def __init__(
self,
connections_prefix: str = 'airflow/connections',
variables_prefix: str = 'airflow/variables',
+ configurations_prefix: str = 'airflow/configuration',
Review comment:
:D I went back and forth with this
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451199109
##########
File path: airflow/secrets/base_secrets.py
##########
@@ -73,3 +75,12 @@ def get_variable(self, key: str) -> Optional[str]:
:return: Variable Value
"""
raise NotImplementedError()
+
+ def get_config(self, key: str) -> Optional[str]: # pylint: disable=unused-argument
Review comment:
add docs in https://github.com/apache/airflow/pull/9645/commits/032d33f257681ac9d189357fe90fdba23a583da7
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r449663361
##########
File path: airflow/configuration.py
##########
@@ -267,17 +271,49 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
return run_command(os.environ[env_var_cmd])
def _get_cmd_option(self, section, key):
fallback_key = key + '_cmd'
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
if super().has_option(section, fallback_key):
command = super().get(section, fallback_key)
return run_command(command)
+ @cached_property
+ def _secrets_backend_client(self):
+ if super().has_option('secrets', 'backend') is False:
+ return None
+
+ secrets_backend_cls = super().get('secrets', 'backend')
+ if not secrets_backend_cls:
+ return None
+ print("secrets_backend_cls", secrets_backend_cls)
+ secrets_backend = import_string(secrets_backend_cls)
+ if secrets_backend:
+ try:
+ alternative_secrets_config_dict = json.loads(
+ super().get('secrets', 'backend_kwargs', fallback='{}')
+ )
+ except json.JSONDecodeError:
+ alternative_secrets_config_dict = {}
+ return secrets_backend(**alternative_secrets_config_dict)
Review comment:
```ini
sql_alchemy_conn_secret = conn/sql_alchemy_conn
```
should already allow that.
Whatever they pass to `sql_alchemy_conn_secret` will be directly used to get secret
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r450717535
##########
File path: airflow/secrets/base_secrets.py
##########
@@ -73,3 +75,12 @@ def get_variable(self, key: str) -> Optional[str]:
:return: Variable Value
"""
raise NotImplementedError()
+
+ def get_config(self, key: str) -> Optional[str]: # pylint: disable=unused-argument
Review comment:
Oh, except it comes from a single value in the config file. Hmmmm.
Perhaps update the example to show include a `/` in it?
```ini
[core]
sql_alchemy_conn_secret = core/sql_alchemy_conn
```
Dunno.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r449663507
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -62,13 +67,15 @@ def __init__(
self,
connections_prefix: str = 'airflow/connections',
variables_prefix: str = 'airflow/variables',
+ configurations_prefix: str = 'airflow/configuration',
Review comment:
fixed
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -42,8 +42,11 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
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``.
- And if variables prefix is ``airflow/variables/hello``, this would be accessible
+ 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 configurations_prefix is ``airflow/configurations/sql_alchemy_conn``, this would be accessible
+ if you provide ``{"configurations_prefix": "airflow/configurations"}`` and request variable
Review comment:
fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451466796
##########
File path: airflow/configuration.py
##########
@@ -495,6 +532,10 @@ def as_dict(
set (True, default), or should the _cmd options be left as the
command to run (False)
:type include_cmds: bool
+ :param include_secret: Should the result of calling any *_secret config be
+ set (True, default), or should the _secret options be left as the
Review comment:
`display_sensitive` is `False` by default, so by default it should show as `'< hidden >'`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r449642855
##########
File path: airflow/configuration.py
##########
@@ -115,7 +117,9 @@ class AirflowConfigParser(ConfigParser):
# These configuration elements can be fetched as the stdout of commands
# following the "{section}__{name}__cmd" pattern, the idea behind this
# is to not store password on boxes in text files.
- as_command_stdout = {
+ # These configs can also be fetched from Secrets backend
+ # following the "{section}__{name}__secret" pattern
+ configs_with_secret = {
Review comment:
```suggestion
senstive_config_values = {
```
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -62,13 +67,15 @@ def __init__(
self,
connections_prefix: str = 'airflow/connections',
variables_prefix: str = 'airflow/variables',
+ configurations_prefix: str = 'airflow/configuration',
Review comment:
```suggestion
config_prefix: str = 'airflow/config',
```
##########
File path: airflow/secrets/base_secrets.py
##########
@@ -73,3 +73,12 @@ def get_variable(self, key: str) -> Optional[str]:
:return: Variable Value
"""
raise NotImplementedError()
+
+ def get_configuration(self, key: str) -> Optional[str]:
Review comment:
```suggestion
def get_configuration(self, section: str, key: str) -> Optional[str]:
```
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -62,13 +67,15 @@ def __init__(
self,
connections_prefix: str = 'airflow/connections',
variables_prefix: str = 'airflow/variables',
+ configurations_prefix: str = 'airflow/configuration',
Review comment:
etc (haven't made this change everywhere)
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -42,8 +42,11 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
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``.
- And if variables prefix is ``airflow/variables/hello``, this would be accessible
+ 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 configurations_prefix is ``airflow/configurations/sql_alchemy_conn``, this would be accessible
Review comment:
```suggestion
And if configurations_prefix is ``airflow/config/core/sql_alchemy_conn``, this would be accessible
```
##########
File path: airflow/configuration.py
##########
@@ -267,17 +271,49 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
return run_command(os.environ[env_var_cmd])
def _get_cmd_option(self, section, key):
fallback_key = key + '_cmd'
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
if super().has_option(section, fallback_key):
command = super().get(section, fallback_key)
return run_command(command)
+ @cached_property
+ def _secrets_backend_client(self):
+ if super().has_option('secrets', 'backend') is False:
+ return None
+
+ secrets_backend_cls = super().get('secrets', 'backend')
+ if not secrets_backend_cls:
+ return None
+ print("secrets_backend_cls", secrets_backend_cls)
+ secrets_backend = import_string(secrets_backend_cls)
+ if secrets_backend:
+ try:
+ alternative_secrets_config_dict = json.loads(
+ super().get('secrets', 'backend_kwargs', fallback='{}')
+ )
+ except json.JSONDecodeError:
+ alternative_secrets_config_dict = {}
+ return secrets_backend(**alternative_secrets_config_dict)
Review comment:
Hmmmm, not a huge fan of the duplication here and in `airflow/secrets/__init__.py`.
Since this would only be called _after_ the class is constructed would we instead do
```suggestion
@cached_property
def _secrets_backend_client(self):
if super().has_option('secrets', 'backend') is False:
return None
from airflow.secrets import secrets_backend_list
return secrets_backend_list
```
(give or take).
By delaying the import to inside the function no import loop is created.
This probably also means adding a `get_config(section, key)` to airflow.secrets, and possibly changing the default implemenation from `NotImplementedError` to `return None`.
Note: we should pass they key and section in directly, and let the secret backends convert that to a single parameter _if needed_ (for instance we could have /config/core/sql_alchemy_conn, rather than config/core__sql_alchemy_conn)
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -42,8 +42,11 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
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``.
- And if variables prefix is ``airflow/variables/hello``, this would be accessible
+ 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 configurations_prefix is ``airflow/configurations/sql_alchemy_conn``, this would be accessible
+ if you provide ``{"configurations_prefix": "airflow/configurations"}`` and request variable
Review comment:
```suggestion
if you provide ``{"configurations_prefix": "airflow/config"}`` and request variable
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451465889
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -62,13 +67,15 @@ def __init__(
self,
connections_prefix: str = 'airflow/connections',
variables_prefix: str = 'airflow/variables',
+ configs_prefix: str = 'airflow/configs',
Review comment:
This should probably just be config, not configs -- as config is both singular and plural, at least in British English. (Configurations/configs) sounds odd to my ears.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r449648643
##########
File path: airflow/configuration.py
##########
@@ -267,17 +271,49 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
return run_command(os.environ[env_var_cmd])
def _get_cmd_option(self, section, key):
fallback_key = key + '_cmd'
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
if super().has_option(section, fallback_key):
command = super().get(section, fallback_key)
return run_command(command)
+ @cached_property
+ def _secrets_backend_client(self):
+ if super().has_option('secrets', 'backend') is False:
+ return None
+
+ secrets_backend_cls = super().get('secrets', 'backend')
+ if not secrets_backend_cls:
+ return None
+ print("secrets_backend_cls", secrets_backend_cls)
+ secrets_backend = import_string(secrets_backend_cls)
+ if secrets_backend:
+ try:
+ alternative_secrets_config_dict = json.loads(
+ super().get('secrets', 'backend_kwargs', fallback='{}')
+ )
+ except json.JSONDecodeError:
+ alternative_secrets_config_dict = {}
+ return secrets_backend(**alternative_secrets_config_dict)
Review comment:
The main reason there is some code duplication here is we can't use `airflow.secrets` as otherwise we would have infinite loop (even when the import is inside the loop) since the DEFAULT_SECRET_BACKEND ( Environment Variable and Connection ) needs `sql_achemy_conn` value.
I can separate the code to a different file and import that file at both places if the concern is just duplication
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r450715746
##########
File path: airflow/secrets/base_secrets.py
##########
@@ -73,3 +75,12 @@ def get_variable(self, key: str) -> Optional[str]:
:return: Variable Value
"""
raise NotImplementedError()
+
+ def get_config(self, key: str) -> Optional[str]: # pylint: disable=unused-argument
Review comment:
I think this should be
```suggestion
def get_config(self, section: str, key: str) -> Optional[str]: # pylint: disable=unused-argument
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451474418
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -42,8 +42,11 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
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``.
- And if variables prefix is ``airflow/variables/hello``, this would be accessible
+ 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 configs_prefix is ``airflow/configs/sql_alchemy_conn``, this would be accessible
+ if you provide ``{"configs_prefix": "airflow/configs"}`` and request variable
Review comment:
Updated in https://github.com/apache/airflow/pull/9645/commits/d8d5b8fed26e6f7e04411a43115b3dc4b7c008c8
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451199040
##########
File path: airflow/configuration.py
##########
@@ -267,17 +278,32 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.sensitive_config_values:
return run_command(os.environ[env_var_cmd])
+ # alternatively AIRFLOW__{SECTION}__{KEY}_SECRET (to get from Secrets Backend)
+ env_var_cmd = env_var + '_SECRET'
+ if env_var_cmd in os.environ:
+ # if this is a valid command key...
+ if (section, key) in self.sensitive_config_values:
+ return _get_config_value_from_secret_backend(os.environ[env_var_cmd])
Review comment:
Fixed in https://github.com/apache/airflow/pull/9645/commits/032d33f257681ac9d189357fe90fdba23a583da7
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] aneesh-joseph commented on pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
aneesh-joseph commented on pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#issuecomment-654673171
thank you for picking this one and sorting it quickly @kaxil - this is super useful, hoping to see this in the next 1.10.* release
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451481769
##########
File path: airflow/configuration.py
##########
@@ -495,6 +532,10 @@ def as_dict(
set (True, default), or should the _cmd options be left as the
command to run (False)
:type include_cmds: bool
+ :param include_secret: Should the result of calling any *_secret config be
+ set (True, default), or should the _secret options be left as the
Review comment:
Oh right, that's alright then.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r449726296
##########
File path: airflow/configuration.py
##########
@@ -267,17 +271,49 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
return run_command(os.environ[env_var_cmd])
def _get_cmd_option(self, section, key):
fallback_key = key + '_cmd'
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
if super().has_option(section, fallback_key):
command = super().get(section, fallback_key)
return run_command(command)
+ @cached_property
+ def _secrets_backend_client(self):
+ if super().has_option('secrets', 'backend') is False:
+ return None
+
+ secrets_backend_cls = super().get('secrets', 'backend')
+ if not secrets_backend_cls:
+ return None
+ print("secrets_backend_cls", secrets_backend_cls)
+ secrets_backend = import_string(secrets_backend_cls)
+ if secrets_backend:
+ try:
+ alternative_secrets_config_dict = json.loads(
+ super().get('secrets', 'backend_kwargs', fallback='{}')
+ )
+ except json.JSONDecodeError:
+ alternative_secrets_config_dict = {}
+ return secrets_backend(**alternative_secrets_config_dict)
Review comment:
Updated with TYPE_CHECKING
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451199109
##########
File path: airflow/secrets/base_secrets.py
##########
@@ -73,3 +75,12 @@ def get_variable(self, key: str) -> Optional[str]:
:return: Variable Value
"""
raise NotImplementedError()
+
+ def get_config(self, key: str) -> Optional[str]: # pylint: disable=unused-argument
Review comment:
added docs in https://github.com/apache/airflow/pull/9645/commits/032d33f257681ac9d189357fe90fdba23a583da7
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451199524
##########
File path: docs/howto/set-config.rst
##########
@@ -46,7 +46,18 @@ the key like this:
[core]
sql_alchemy_conn_cmd = bash_command_to_run
-The following config options support this ``_cmd`` version:
+You can also derive the connection string at run time by appending ``_secret`` to
+the key like this:
+
+.. code-block:: ini
+
+ [core]
+ sql_alchemy_conn_secret = sql_alchemy_conn
Review comment:
for Kubernetes, the section would be kubernetes_secrets:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] aneesh-joseph commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
aneesh-joseph commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r450685848
##########
File path: airflow/configuration.py
##########
@@ -267,17 +278,32 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.sensitive_config_values:
return run_command(os.environ[env_var_cmd])
+ # alternatively AIRFLOW__{SECTION}__{KEY}_SECRET (to get from Secrets Backend)
+ env_var_cmd = env_var + '_SECRET'
+ if env_var_cmd in os.environ:
+ # if this is a valid command key...
+ if (section, key) in self.sensitive_config_values:
+ return _get_config_value_from_secret_backend(os.environ[env_var_cmd])
Review comment:
```suggestion
env_var_secret_path = env_var + '_SECRET'
if env_var_secret_path in os.environ:
# if this is a valid secret key...
if (section, key) in self.sensitive_config_values:
return _get_config_value_from_secret_backend(os.environ[env_var_secret_path])
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r449648643
##########
File path: airflow/configuration.py
##########
@@ -267,17 +271,49 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
return run_command(os.environ[env_var_cmd])
def _get_cmd_option(self, section, key):
fallback_key = key + '_cmd'
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
if super().has_option(section, fallback_key):
command = super().get(section, fallback_key)
return run_command(command)
+ @cached_property
+ def _secrets_backend_client(self):
+ if super().has_option('secrets', 'backend') is False:
+ return None
+
+ secrets_backend_cls = super().get('secrets', 'backend')
+ if not secrets_backend_cls:
+ return None
+ print("secrets_backend_cls", secrets_backend_cls)
+ secrets_backend = import_string(secrets_backend_cls)
+ if secrets_backend:
+ try:
+ alternative_secrets_config_dict = json.loads(
+ super().get('secrets', 'backend_kwargs', fallback='{}')
+ )
+ except json.JSONDecodeError:
+ alternative_secrets_config_dict = {}
+ return secrets_backend(**alternative_secrets_config_dict)
Review comment:
The main reason there is some code duplication here is we can't use `airflow.secrets` as otherwise we would have infinite loop since the DEFAULT_SECRET_BACKEND ( Environment Variable and Connection ) needs `sql_achemy_conn` value.
I can separate the code to a different file and import that file at both places if the concern is just duplication
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r449648643
##########
File path: airflow/configuration.py
##########
@@ -267,17 +271,49 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
return run_command(os.environ[env_var_cmd])
def _get_cmd_option(self, section, key):
fallback_key = key + '_cmd'
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
if super().has_option(section, fallback_key):
command = super().get(section, fallback_key)
return run_command(command)
+ @cached_property
+ def _secrets_backend_client(self):
+ if super().has_option('secrets', 'backend') is False:
+ return None
+
+ secrets_backend_cls = super().get('secrets', 'backend')
+ if not secrets_backend_cls:
+ return None
+ print("secrets_backend_cls", secrets_backend_cls)
+ secrets_backend = import_string(secrets_backend_cls)
+ if secrets_backend:
+ try:
+ alternative_secrets_config_dict = json.loads(
+ super().get('secrets', 'backend_kwargs', fallback='{}')
+ )
+ except json.JSONDecodeError:
+ alternative_secrets_config_dict = {}
+ return secrets_backend(**alternative_secrets_config_dict)
Review comment:
The main there is some code duplication here is we can't use `airflow.secrets` as otherwise we would have infinite loop since the DEFAULT_SECRET_BACKEND ( Environment Variable and Connection ) needs `sql_achemy_conn` value
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r449639584
##########
File path: airflow/configuration.py
##########
@@ -267,17 +272,49 @@ def _get_env_var_option(self, section, key):
env_var_cmd = env_var + '_CMD'
if env_var_cmd in os.environ:
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
return run_command(os.environ[env_var_cmd])
def _get_cmd_option(self, section, key):
fallback_key = key + '_cmd'
# if this is a valid command key...
- if (section, key) in self.as_command_stdout:
+ if (section, key) in self.configs_with_secret:
if super().has_option(section, fallback_key):
command = super().get(section, fallback_key)
return run_command(command)
+ @cached_property
+ def _secrets_backend_client(self):
+ if super().has_option('secrets', 'backend') is False:
+ return None
+
+ secrets_backend_cls = super().get('secrets', 'backend')
+ if not secrets_backend_cls:
+ return None
+ print("secrets_backend_cls", secrets_backend_cls)
+ secrets_backend = import_string(secrets_backend_cls)
+ if secrets_backend:
+ try:
+ alternative_secrets_config_dict = json.loads(
+ super().get('secrets', 'backend_kwargs', fallback='{}')
+ )
+ except JSONDecodeError:
Review comment:
```suggestion
except json.JSONDecodeError:
```
Then we don't have to have two import statements.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451464684
##########
File path: airflow/configuration.py
##########
@@ -495,6 +532,10 @@ def as_dict(
set (True, default), or should the _cmd options be left as the
command to run (False)
:type include_cmds: bool
+ :param include_secret: Should the result of calling any *_secret config be
+ set (True, default), or should the _secret options be left as the
Review comment:
I'm not sure this should be on by default... Hmmm
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #9645:
URL: https://github.com/apache/airflow/pull/9645
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451199613
##########
File path: docs/howto/set-config.rst
##########
@@ -73,6 +91,7 @@ The universal order of precedence for all configuration options is as follows:
#. set as a command environment variable
Review comment:
fixed in 032d33f
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451474474
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -62,13 +67,15 @@ def __init__(
self,
connections_prefix: str = 'airflow/connections',
variables_prefix: str = 'airflow/variables',
+ configs_prefix: str = 'airflow/configs',
Review comment:
fixed in https://github.com/apache/airflow/pull/9645/commits/35d75e5a52d413ecb97d935060011472625bac01
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9645:
URL: https://github.com/apache/airflow/pull/9645#discussion_r451462635
##########
File path: airflow/configuration.py
##########
@@ -85,6 +85,15 @@ def run_command(command):
return output
+def _get_config_value_from_secret_backend(config_key):
Review comment:
Any reason why this is a function in the module, and not a method on the AirflowConfiguration class?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org