From ed9f74becb0c4385ad976ab46a4ad2ce989154c0 Mon Sep 17 00:00:00 2001 From: Amogh Date: Tue, 15 Oct 2024 20:38:04 +0530 Subject: [PATCH 01/13] Masking configuration values irrelevant to DAG author --- airflow/configuration.py | 7 +++++++ airflow/settings.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/airflow/configuration.py b/airflow/configuration.py index 81dc18365392e..95f8751614997 100644 --- a/airflow/configuration.py +++ b/airflow/configuration.py @@ -775,6 +775,13 @@ def _create_future_warning(name: str, section: str, current_value: Any, new_valu def _env_var_name(self, section: str, key: str) -> str: return f"{ENV_VAR_PREFIX}{section.replace('.', '_').upper()}__{key.upper()}" + def get_section_and_key_for_env(self, env: str): + if env in os.environ: + _, section, key = env.split("__", 2) + return section, key + else: + return None, None + def _get_env_var_option(self, section: str, key: str): # must have format AIRFLOW__{SECTION}__{KEY} (note double underscore) env_var = self._env_var_name(section, key) diff --git a/airflow/settings.py b/airflow/settings.py index a6adbbcf9ff77..61beb174f8503 100644 --- a/airflow/settings.py +++ b/airflow/settings.py @@ -118,6 +118,27 @@ "upstream_failed": "orange", } +# list of the environment variables that need to be masked from the logs +# some of these could be sensitive and there's no reason to log those +ENV_VARIABLES_TO_MASK = [ + "AIRFLOW__CORE__FERNET_KEY", + "AIRFLOW__SMTP__SMTP_PORT", + "AIRFLOW__SMTP__SMTP_PASSWORD", + "AIRFLOW__SMTP__SMTP_PASSWORD_SECRET", + "AIRFLOW__SMTP__SMTP_USER", + "AIRFLOW__SMTP__SMTP_SSL", + "AIRFLOW__SMTP__SMTP_HOST", + "AIRFLOW__WEBSERVER__SECRET_KEY", + "AIRFLOW__WEBSERVER__SECRET_KEY_SECRET", + "AIRFLOW__SENTRY__SENTRY_DSN_SECRET", + "AIRFLOW__DATABASE__SQL_ALCHEMY_CONN_SECRET", + "AIRFLOW__DATABASE__SQL_ALCHEMY_ENGINE_ARGS_SECRET", + "AIRFLOW__CORE__INTERNAL_API_SECRET_KEY", + "AIRFLOW__CORE__INTERNAL_API_SECRET_KEY_SECRET", + "AIRFLOW__CORE__DATASET_MANAGER_KWARGS_SECRET", + "AIRFLOW__LOGGING__REMOTE_TASK_HANDLER_KWARGS_SECRET", +] + @functools.lru_cache(maxsize=None) def _get_rich_console(file): @@ -722,6 +743,16 @@ def import_local_settings(): log.info("Loaded airflow_local_settings from %s .", airflow_local_settings.__file__) +def mask_conf_values(): + from airflow.utils.log.secrets_masker import mask_secret + + for env in ENV_VARIABLES_TO_MASK: + section, key = conf.get_section_and_key_for_env(env) + if section is None and key is None: + continue + mask_secret(conf.get(section, key)) + + def initialize(): """Initialize Airflow with all the settings from this file.""" configure_vars() @@ -741,6 +772,9 @@ def initialize(): configure_orm() configure_action_logging() + # mask the configuration irrelevant to DAG author + mask_conf_values() + # Run any custom runtime checks that needs to be executed for providers run_providers_custom_runtime_checks() From ad52d25c4cd27f904efd3445a5b593a32a629d38 Mon Sep 17 00:00:00 2001 From: adesai Date: Wed, 16 Oct 2024 16:35:29 +0530 Subject: [PATCH 02/13] example UT --- tests/core/test_settings.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/core/test_settings.py b/tests/core/test_settings.py index 27b40ad7d1fae..e6c92595c2419 100644 --- a/tests/core/test_settings.py +++ b/tests/core/test_settings.py @@ -31,7 +31,8 @@ from airflow.api_internal.internal_api_call import InternalApiConfig from airflow.configuration import conf from airflow.exceptions import AirflowClusterPolicyViolation, AirflowConfigException -from airflow.settings import _ENABLE_AIP_44, TracebackSession, is_usage_data_collection_enabled +from airflow.settings import _ENABLE_AIP_44, TracebackSession, is_usage_data_collection_enabled, \ + mask_conf_values from airflow.utils.session import create_session from tests_common.test_utils.config import conf_vars @@ -385,3 +386,20 @@ def test_usage_data_collection_disabled(env_var, conf_setting, is_enabled, clear else: with conf_patch: assert is_usage_data_collection_enabled() == is_enabled + + +@patch('airflow.utils.log.secret_masker.mask_secret') +@patch('airflow.configuration.conf.get') +@patch('airflow.configuration.get_section_and_key_for_env') +@patch('airflow.settings.ENV_VARIABLES_TO_MASK', ['AIRFLOW__CORE__FERNET_KEY', 'AIRFLOW__NONMASK_ENV']) +def test_mask_conf_values(self, mock_get_section_and_key, mock_get, mock_mask_secret): + mock_get_section_and_key.side_effect = [ + ('core', 'fernet_key'), + (None, None), + ] + + mock_get.side_effect = ['my_fernet_key'] + mask_conf_values() + mock_mask_secret.assert_called_once_with('my_fernet_key') + self.assertEqual(mock_get_section_and_key.call_count, 2) + mock_get.assert_called_once_with('core', 'fernet_key') From e5c46cf8f49f0c025ede9f6300abb82727d6246a Mon Sep 17 00:00:00 2001 From: adesai Date: Wed, 16 Oct 2024 16:37:59 +0530 Subject: [PATCH 03/13] writing UTs --- tests/core/test_settings.py | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/tests/core/test_settings.py b/tests/core/test_settings.py index e6c92595c2419..55bc9a92448f4 100644 --- a/tests/core/test_settings.py +++ b/tests/core/test_settings.py @@ -31,8 +31,12 @@ from airflow.api_internal.internal_api_call import InternalApiConfig from airflow.configuration import conf from airflow.exceptions import AirflowClusterPolicyViolation, AirflowConfigException -from airflow.settings import _ENABLE_AIP_44, TracebackSession, is_usage_data_collection_enabled, \ - mask_conf_values +from airflow.settings import ( + _ENABLE_AIP_44, + TracebackSession, + is_usage_data_collection_enabled, + mask_conf_values, +) from airflow.utils.session import create_session from tests_common.test_utils.config import conf_vars @@ -388,18 +392,19 @@ def test_usage_data_collection_disabled(env_var, conf_setting, is_enabled, clear assert is_usage_data_collection_enabled() == is_enabled -@patch('airflow.utils.log.secret_masker.mask_secret') -@patch('airflow.configuration.conf.get') -@patch('airflow.configuration.get_section_and_key_for_env') -@patch('airflow.settings.ENV_VARIABLES_TO_MASK', ['AIRFLOW__CORE__FERNET_KEY', 'AIRFLOW__NONMASK_ENV']) +@patch("airflow.utils.log.secret_masker.mask_secret") +@patch("airflow.configuration.conf.get") +@patch("airflow.configuration.get_section_and_key_for_env") +@patch("airflow.settings.ENV_VARIABLES_TO_MASK", ["AIRFLOW__CORE__FERNET_KEY", "AIRFLOW__NONMASK_ENV"]) def test_mask_conf_values(self, mock_get_section_and_key, mock_get, mock_mask_secret): mock_get_section_and_key.side_effect = [ - ('core', 'fernet_key'), + ("core", "fernet_key"), (None, None), ] - mock_get.side_effect = ['my_fernet_key'] + mock_get.side_effect = ["my_fernet_key"] mask_conf_values() - mock_mask_secret.assert_called_once_with('my_fernet_key') - self.assertEqual(mock_get_section_and_key.call_count, 2) - mock_get.assert_called_once_with('core', 'fernet_key') + + mock_mask_secret.assert_called_once_with("my_fernet_key") + assert mock_get_section_and_key.call_count == 2 + mock_get.assert_called_once_with("core", "fernet_key") From ead1bf34edcef7c033b5545f7e00df9ed8fa1df5 Mon Sep 17 00:00:00 2001 From: Amogh Date: Wed, 16 Oct 2024 17:32:55 +0530 Subject: [PATCH 04/13] writing UTs --- tests/core/test_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/core/test_settings.py b/tests/core/test_settings.py index 55bc9a92448f4..0a359f77a39fe 100644 --- a/tests/core/test_settings.py +++ b/tests/core/test_settings.py @@ -396,7 +396,7 @@ def test_usage_data_collection_disabled(env_var, conf_setting, is_enabled, clear @patch("airflow.configuration.conf.get") @patch("airflow.configuration.get_section_and_key_for_env") @patch("airflow.settings.ENV_VARIABLES_TO_MASK", ["AIRFLOW__CORE__FERNET_KEY", "AIRFLOW__NONMASK_ENV"]) -def test_mask_conf_values(self, mock_get_section_and_key, mock_get, mock_mask_secret): +def test_mask_conf_values(mock_mask_secret, mock_get_section_and_key, mock_get): mock_get_section_and_key.side_effect = [ ("core", "fernet_key"), (None, None), From c03ccfc0716fdbf88eb778c920767b4d3953ac5b Mon Sep 17 00:00:00 2001 From: Amogh Date: Wed, 16 Oct 2024 17:53:52 +0530 Subject: [PATCH 05/13] removing UTs --- tests/core/test_settings.py | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/tests/core/test_settings.py b/tests/core/test_settings.py index 0a359f77a39fe..b24d094c95711 100644 --- a/tests/core/test_settings.py +++ b/tests/core/test_settings.py @@ -35,7 +35,6 @@ _ENABLE_AIP_44, TracebackSession, is_usage_data_collection_enabled, - mask_conf_values, ) from airflow.utils.session import create_session from tests_common.test_utils.config import conf_vars @@ -390,21 +389,3 @@ def test_usage_data_collection_disabled(env_var, conf_setting, is_enabled, clear else: with conf_patch: assert is_usage_data_collection_enabled() == is_enabled - - -@patch("airflow.utils.log.secret_masker.mask_secret") -@patch("airflow.configuration.conf.get") -@patch("airflow.configuration.get_section_and_key_for_env") -@patch("airflow.settings.ENV_VARIABLES_TO_MASK", ["AIRFLOW__CORE__FERNET_KEY", "AIRFLOW__NONMASK_ENV"]) -def test_mask_conf_values(mock_mask_secret, mock_get_section_and_key, mock_get): - mock_get_section_and_key.side_effect = [ - ("core", "fernet_key"), - (None, None), - ] - - mock_get.side_effect = ["my_fernet_key"] - mask_conf_values() - - mock_mask_secret.assert_called_once_with("my_fernet_key") - assert mock_get_section_and_key.call_count == 2 - mock_get.assert_called_once_with("core", "fernet_key") From 5d2313ef388cf749b19ede35b07a5cdc7423c03f Mon Sep 17 00:00:00 2001 From: Amogh Date: Wed, 16 Oct 2024 19:03:44 +0530 Subject: [PATCH 06/13] temp commit --- airflow/settings.py | 38 +++++++++----------------------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/airflow/settings.py b/airflow/settings.py index 61beb174f8503..aa3b0e2b9f325 100644 --- a/airflow/settings.py +++ b/airflow/settings.py @@ -36,7 +36,7 @@ from airflow import __version__ as airflow_version, policies from airflow.configuration import AIRFLOW_HOME, WEBSERVER_CONFIG, conf # noqa: F401 -from airflow.exceptions import AirflowInternalRuntimeError +from airflow.exceptions import AirflowInternalRuntimeError, AirflowConfigException from airflow.executors import executor_constants from airflow.logging_config import configure_logging from airflow.utils.orm_event_handlers import setup_event_handlers @@ -118,28 +118,6 @@ "upstream_failed": "orange", } -# list of the environment variables that need to be masked from the logs -# some of these could be sensitive and there's no reason to log those -ENV_VARIABLES_TO_MASK = [ - "AIRFLOW__CORE__FERNET_KEY", - "AIRFLOW__SMTP__SMTP_PORT", - "AIRFLOW__SMTP__SMTP_PASSWORD", - "AIRFLOW__SMTP__SMTP_PASSWORD_SECRET", - "AIRFLOW__SMTP__SMTP_USER", - "AIRFLOW__SMTP__SMTP_SSL", - "AIRFLOW__SMTP__SMTP_HOST", - "AIRFLOW__WEBSERVER__SECRET_KEY", - "AIRFLOW__WEBSERVER__SECRET_KEY_SECRET", - "AIRFLOW__SENTRY__SENTRY_DSN_SECRET", - "AIRFLOW__DATABASE__SQL_ALCHEMY_CONN_SECRET", - "AIRFLOW__DATABASE__SQL_ALCHEMY_ENGINE_ARGS_SECRET", - "AIRFLOW__CORE__INTERNAL_API_SECRET_KEY", - "AIRFLOW__CORE__INTERNAL_API_SECRET_KEY_SECRET", - "AIRFLOW__CORE__DATASET_MANAGER_KWARGS_SECRET", - "AIRFLOW__LOGGING__REMOTE_TASK_HANDLER_KWARGS_SECRET", -] - - @functools.lru_cache(maxsize=None) def _get_rich_console(file): # Delay imports until we need it @@ -745,12 +723,14 @@ def import_local_settings(): def mask_conf_values(): from airflow.utils.log.secrets_masker import mask_secret - - for env in ENV_VARIABLES_TO_MASK: - section, key = conf.get_section_and_key_for_env(env) - if section is None and key is None: + for section, key in conf.sensitive_config_values: + try: + value = conf.get(section, key) + mask_secret(value) + except AirflowConfigException: + log.warning(f"AirflowConfigException encountered for section: {section}, key: {key}. " + f"Skipping...") continue - mask_secret(conf.get(section, key)) def initialize(): @@ -772,7 +752,7 @@ def initialize(): configure_orm() configure_action_logging() - # mask the configuration irrelevant to DAG author + # mask the sensitive_config_values mask_conf_values() # Run any custom runtime checks that needs to be executed for providers From 3acbe7f7d3c77d87237c921ad66fbe80a22a8793 Mon Sep 17 00:00:00 2001 From: Amogh Date: Wed, 16 Oct 2024 19:32:47 +0530 Subject: [PATCH 07/13] changing the logic to mask from conf.sensitive_config_values instead --- airflow/configuration.py | 7 ------- airflow/settings.py | 9 ++++++--- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/airflow/configuration.py b/airflow/configuration.py index 95f8751614997..81dc18365392e 100644 --- a/airflow/configuration.py +++ b/airflow/configuration.py @@ -775,13 +775,6 @@ def _create_future_warning(name: str, section: str, current_value: Any, new_valu def _env_var_name(self, section: str, key: str) -> str: return f"{ENV_VAR_PREFIX}{section.replace('.', '_').upper()}__{key.upper()}" - def get_section_and_key_for_env(self, env: str): - if env in os.environ: - _, section, key = env.split("__", 2) - return section, key - else: - return None, None - def _get_env_var_option(self, section: str, key: str): # must have format AIRFLOW__{SECTION}__{KEY} (note double underscore) env_var = self._env_var_name(section, key) diff --git a/airflow/settings.py b/airflow/settings.py index aa3b0e2b9f325..c0337914e481b 100644 --- a/airflow/settings.py +++ b/airflow/settings.py @@ -36,7 +36,7 @@ from airflow import __version__ as airflow_version, policies from airflow.configuration import AIRFLOW_HOME, WEBSERVER_CONFIG, conf # noqa: F401 -from airflow.exceptions import AirflowInternalRuntimeError, AirflowConfigException +from airflow.exceptions import AirflowConfigException, AirflowInternalRuntimeError from airflow.executors import executor_constants from airflow.logging_config import configure_logging from airflow.utils.orm_event_handlers import setup_event_handlers @@ -118,6 +118,7 @@ "upstream_failed": "orange", } + @functools.lru_cache(maxsize=None) def _get_rich_console(file): # Delay imports until we need it @@ -723,13 +724,15 @@ def import_local_settings(): def mask_conf_values(): from airflow.utils.log.secrets_masker import mask_secret + for section, key in conf.sensitive_config_values: try: value = conf.get(section, key) mask_secret(value) except AirflowConfigException: - log.warning(f"AirflowConfigException encountered for section: {section}, key: {key}. " - f"Skipping...") + log.warning( + "AirflowConfigException encountered for section:", section, "key: ", key, "Skipping..." + ) continue From ab6309cd32faa4ef41d113782de2900a85e2b24d Mon Sep 17 00:00:00 2001 From: Amogh Date: Wed, 16 Oct 2024 20:42:40 +0530 Subject: [PATCH 08/13] adding UT --- tests/core/test_settings.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/core/test_settings.py b/tests/core/test_settings.py index b24d094c95711..dcb73dab930d1 100644 --- a/tests/core/test_settings.py +++ b/tests/core/test_settings.py @@ -35,6 +35,7 @@ _ENABLE_AIP_44, TracebackSession, is_usage_data_collection_enabled, + mask_conf_values, ) from airflow.utils.session import create_session from tests_common.test_utils.config import conf_vars @@ -389,3 +390,21 @@ def test_usage_data_collection_disabled(env_var, conf_setting, is_enabled, clear else: with conf_patch: assert is_usage_data_collection_enabled() == is_enabled + + +@patch.object( + conf, "sensitive_config_values", new_callable=lambda: [("mysection1", "mykey1"), ("mysection2", "mykey2")] +) +@patch("airflow.utils.log.secrets_masker.mask_secret") +@patch("airflow.configuration.conf.get") +def test_mask_conf_values(mock_get, mock_mask_secret, mock_sensitive_config_values): + mock_get.side_effect = ["supersecret1", "supersecret2"] + mask_conf_values() + + mock_get.assert_any_call("mysection1", "mykey1") + mock_get.assert_any_call("mysection2", "mykey2") + mock_mask_secret.assert_any_call("supersecret1") + mock_mask_secret.assert_any_call("supersecret2") + + assert mock_get.call_count == 2 + assert mock_mask_secret.call_count == 2 From 459709f1f4a4b58f6941c7bed584bfdf3eddb349 Mon Sep 17 00:00:00 2001 From: Amogh Date: Wed, 16 Oct 2024 21:14:09 +0530 Subject: [PATCH 09/13] updating log --- airflow/settings.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/airflow/settings.py b/airflow/settings.py index c0337914e481b..8de91ec6f348a 100644 --- a/airflow/settings.py +++ b/airflow/settings.py @@ -36,7 +36,7 @@ from airflow import __version__ as airflow_version, policies from airflow.configuration import AIRFLOW_HOME, WEBSERVER_CONFIG, conf # noqa: F401 -from airflow.exceptions import AirflowConfigException, AirflowInternalRuntimeError +from airflow.exceptions import AirflowInternalRuntimeError from airflow.executors import executor_constants from airflow.logging_config import configure_logging from airflow.utils.orm_event_handlers import setup_event_handlers @@ -729,10 +729,8 @@ def mask_conf_values(): try: value = conf.get(section, key) mask_secret(value) - except AirflowConfigException: - log.warning( - "AirflowConfigException encountered for section:", section, "key: ", key, "Skipping..." - ) + except ValueError: + log.warning("ValueError encountered for section:", section, "key: ", key, ". Skipping...") continue From afe2ac3081b5a1abd63da1ca4d5001a8c613163c Mon Sep 17 00:00:00 2001 From: Amogh Date: Wed, 16 Oct 2024 21:23:30 +0530 Subject: [PATCH 10/13] updating log and test cases --- airflow/settings.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/airflow/settings.py b/airflow/settings.py index 8de91ec6f348a..8a9caabd74f6a 100644 --- a/airflow/settings.py +++ b/airflow/settings.py @@ -36,7 +36,7 @@ from airflow import __version__ as airflow_version, policies from airflow.configuration import AIRFLOW_HOME, WEBSERVER_CONFIG, conf # noqa: F401 -from airflow.exceptions import AirflowInternalRuntimeError +from airflow.exceptions import AirflowConfigException, AirflowInternalRuntimeError from airflow.executors import executor_constants from airflow.logging_config import configure_logging from airflow.utils.orm_event_handlers import setup_event_handlers @@ -728,10 +728,14 @@ def mask_conf_values(): for section, key in conf.sensitive_config_values: try: value = conf.get(section, key) - mask_secret(value) - except ValueError: - log.warning("ValueError encountered for section:", section, "key: ", key, ". Skipping...") + except AirflowConfigException: + log.warning( + "Could not retrieve value from section %s, for key %s. Skipping redaction of this conf.", + section, + key, + ) continue + mask_secret(value) def initialize(): From 5e975c9a246570300f4a0491775aaac44f58ba4f Mon Sep 17 00:00:00 2001 From: Amogh Date: Tue, 22 Oct 2024 10:28:11 +0530 Subject: [PATCH 11/13] review comments from ash --- airflow/configuration.py | 15 +++++++++++++++ airflow/settings.py | 20 ++------------------ tests/core/test_configuration.py | 23 +++++++++++++++++++++-- tests/core/test_settings.py | 20 +------------------- 4 files changed, 39 insertions(+), 39 deletions(-) diff --git a/airflow/configuration.py b/airflow/configuration.py index 81dc18365392e..7e4b92f613cf8 100644 --- a/airflow/configuration.py +++ b/airflow/configuration.py @@ -772,6 +772,21 @@ def _create_future_warning(name: str, section: str, current_value: Any, new_valu stacklevel=3, ) + def mask_secrets(self): + from airflow.utils.log.secrets_masker import mask_secret + + for section, key in conf.sensitive_config_values: + try: + value = conf.get(section, key) + except AirflowConfigException: + log.debug( + "Could not retrieve value from section %s, for key %s. Skipping redaction of this conf.", + section, + key, + ) + continue + mask_secret(value) + def _env_var_name(self, section: str, key: str) -> str: return f"{ENV_VAR_PREFIX}{section.replace('.', '_').upper()}__{key.upper()}" diff --git a/airflow/settings.py b/airflow/settings.py index 8a9caabd74f6a..57c382e2a1a1c 100644 --- a/airflow/settings.py +++ b/airflow/settings.py @@ -36,7 +36,7 @@ from airflow import __version__ as airflow_version, policies from airflow.configuration import AIRFLOW_HOME, WEBSERVER_CONFIG, conf # noqa: F401 -from airflow.exceptions import AirflowConfigException, AirflowInternalRuntimeError +from airflow.exceptions import AirflowInternalRuntimeError from airflow.executors import executor_constants from airflow.logging_config import configure_logging from airflow.utils.orm_event_handlers import setup_event_handlers @@ -722,22 +722,6 @@ def import_local_settings(): log.info("Loaded airflow_local_settings from %s .", airflow_local_settings.__file__) -def mask_conf_values(): - from airflow.utils.log.secrets_masker import mask_secret - - for section, key in conf.sensitive_config_values: - try: - value = conf.get(section, key) - except AirflowConfigException: - log.warning( - "Could not retrieve value from section %s, for key %s. Skipping redaction of this conf.", - section, - key, - ) - continue - mask_secret(value) - - def initialize(): """Initialize Airflow with all the settings from this file.""" configure_vars() @@ -758,7 +742,7 @@ def initialize(): configure_action_logging() # mask the sensitive_config_values - mask_conf_values() + conf.mask_secrets() # Run any custom runtime checks that needs to be executed for providers run_providers_custom_runtime_checks() diff --git a/tests/core/test_configuration.py b/tests/core/test_configuration.py index 1f376c955f503..e12b17f5d7b19 100644 --- a/tests/core/test_configuration.py +++ b/tests/core/test_configuration.py @@ -42,8 +42,6 @@ write_default_airflow_configuration_if_needed, ) from airflow.providers_manager import ProvidersManager -from tests_common.test_utils.config import conf_vars -from tests_common.test_utils.reset_warning_registry import reset_warning_registry from tests.utils.test_config import ( remove_all_configurations, @@ -51,6 +49,8 @@ set_sensitive_config_values, use_config, ) +from tests_common.test_utils.config import conf_vars +from tests_common.test_utils.reset_warning_registry import reset_warning_registry HOME_DIR = os.path.expanduser("~") @@ -1763,3 +1763,22 @@ def test_config_paths_is_directory(self): with pytest.raises(IsADirectoryError, match="configuration file, but got a directory"): write_default_airflow_configuration_if_needed() + + @patch.object( + conf, + "sensitive_config_values", + new_callable=lambda: [("mysection1", "mykey1"), ("mysection2", "mykey2")], + ) + @patch("airflow.utils.log.secrets_masker.mask_secret") + @patch("airflow.configuration.conf.get") + def test_mask_conf_values(self, mock_get, mock_mask_secret, mock_sensitive_config_values): + mock_get.side_effect = ["supersecret1", "supersecret2"] + conf.mask_secrets() + + mock_get.assert_any_call("mysection1", "mykey1") + mock_get.assert_any_call("mysection2", "mykey2") + mock_mask_secret.assert_any_call("supersecret1") + mock_mask_secret.assert_any_call("supersecret2") + + assert mock_get.call_count == 2 + assert mock_mask_secret.call_count == 2 diff --git a/tests/core/test_settings.py b/tests/core/test_settings.py index dcb73dab930d1..3e0e869a76aaf 100644 --- a/tests/core/test_settings.py +++ b/tests/core/test_settings.py @@ -35,9 +35,9 @@ _ENABLE_AIP_44, TracebackSession, is_usage_data_collection_enabled, - mask_conf_values, ) from airflow.utils.session import create_session + from tests_common.test_utils.config import conf_vars SETTINGS_FILE_POLICY = """ @@ -390,21 +390,3 @@ def test_usage_data_collection_disabled(env_var, conf_setting, is_enabled, clear else: with conf_patch: assert is_usage_data_collection_enabled() == is_enabled - - -@patch.object( - conf, "sensitive_config_values", new_callable=lambda: [("mysection1", "mykey1"), ("mysection2", "mykey2")] -) -@patch("airflow.utils.log.secrets_masker.mask_secret") -@patch("airflow.configuration.conf.get") -def test_mask_conf_values(mock_get, mock_mask_secret, mock_sensitive_config_values): - mock_get.side_effect = ["supersecret1", "supersecret2"] - mask_conf_values() - - mock_get.assert_any_call("mysection1", "mykey1") - mock_get.assert_any_call("mysection2", "mykey2") - mock_mask_secret.assert_any_call("supersecret1") - mock_mask_secret.assert_any_call("supersecret2") - - assert mock_get.call_count == 2 - assert mock_mask_secret.call_count == 2 From af80e4eefb09d31d4fe9a99954564955c62923ab Mon Sep 17 00:00:00 2001 From: Ash Berlin-Taylor Date: Tue, 22 Oct 2024 11:25:48 +0100 Subject: [PATCH 12/13] Apply suggestions from code review --- airflow/configuration.py | 4 ++-- tests/core/test_settings.py | 6 +----- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/airflow/configuration.py b/airflow/configuration.py index f96f137193e5c..461723f374994 100644 --- a/airflow/configuration.py +++ b/airflow/configuration.py @@ -775,9 +775,9 @@ def _create_future_warning(name: str, section: str, current_value: Any, new_valu def mask_secrets(self): from airflow.utils.log.secrets_masker import mask_secret - for section, key in conf.sensitive_config_values: + for section, key in self.sensitive_config_values: try: - value = conf.get(section, key) + value = self.get(section, key) except AirflowConfigException: log.debug( "Could not retrieve value from section %s, for key %s. Skipping redaction of this conf.", diff --git a/tests/core/test_settings.py b/tests/core/test_settings.py index 3e0e869a76aaf..00f0d14b6d519 100644 --- a/tests/core/test_settings.py +++ b/tests/core/test_settings.py @@ -31,11 +31,7 @@ from airflow.api_internal.internal_api_call import InternalApiConfig from airflow.configuration import conf from airflow.exceptions import AirflowClusterPolicyViolation, AirflowConfigException -from airflow.settings import ( - _ENABLE_AIP_44, - TracebackSession, - is_usage_data_collection_enabled, -) +from airflow.settings import _ENABLE_AIP_44, TracebackSession, is_usage_data_collection_enabled from airflow.utils.session import create_session from tests_common.test_utils.config import conf_vars From e7b9a36e93d311266711d87cb18d4b086f91d38b Mon Sep 17 00:00:00 2001 From: Amogh Date: Tue, 22 Oct 2024 16:15:32 +0530 Subject: [PATCH 13/13] using @conf_vars --- tests/core/test_configuration.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/core/test_configuration.py b/tests/core/test_configuration.py index 056fb655d6d14..583472eb0a64d 100644 --- a/tests/core/test_configuration.py +++ b/tests/core/test_configuration.py @@ -1764,21 +1764,17 @@ def test_config_paths_is_directory(self): with pytest.raises(IsADirectoryError, match="configuration file, but got a directory"): write_default_airflow_configuration_if_needed() + @conf_vars({("mysection1", "mykey1"): "supersecret1", ("mysection2", "mykey2"): "supersecret2"}) @patch.object( conf, "sensitive_config_values", new_callable=lambda: [("mysection1", "mykey1"), ("mysection2", "mykey2")], ) @patch("airflow.utils.log.secrets_masker.mask_secret") - @patch("airflow.configuration.conf.get") - def test_mask_conf_values(self, mock_get, mock_mask_secret, mock_sensitive_config_values): - mock_get.side_effect = ["supersecret1", "supersecret2"] + def test_mask_conf_values(self, mock_mask_secret, mock_sensitive_config_values): conf.mask_secrets() - mock_get.assert_any_call("mysection1", "mykey1") - mock_get.assert_any_call("mysection2", "mykey2") mock_mask_secret.assert_any_call("supersecret1") mock_mask_secret.assert_any_call("supersecret2") - assert mock_get.call_count == 2 assert mock_mask_secret.call_count == 2