[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: ![image](https://user-images.githubusercontent.com/8811558/86855309-fcb9c980-c0b1-11ea-9e21-09a3a737196b.png) 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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9645: Get Airflow configs with sensitive data from Secret Backends
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: us...@infra.apache.org