amoghrajesh commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2452090298
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -1323,6 +1323,20 @@ secrets:
sensitive: true
example: ~
default: ""
+ backends_order:
+ description: |
+ Comma-separated list of secret backends. These backends will be used
in the order they are specified.
+ Please note that the `environment_variable` and `metastore` are
required values and cannot be removed
+ from the list. Supported values are:
+
+ * ``custom``: Custom secret backend specified in the
``secrets[backend]`` configuration option.
+ * ``environment_variable``: Standard environment variable backend
+
``airflow.secrets.environment_variables.EnvironmentVariablesBackend``.
+ * ``metastore``: Standard metastore backend
``airflow.secrets.metastore.MetastoreBackend``.
+ version_added: 3.0.0
+ type: string
+ example: ~
+ default: "custom,environment_variable,metastore"
Review Comment:
On giving this some more thought, I am not very convinced that having the
config under "secrets" is the right thing to do.
For someone wanting to configure workers backend, they have to set this:
[secrets] backends_order, [workers] secrets_backend which is not at all
intuitive and is confusing.
We should consider having backends_order as a config for workers too, maybe:
[workers] backends_order which plays nice with workers backend and the separate
one that we have for secrets can be used there.
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -2210,28 +2208,73 @@ def get_custom_secret_backend(worker_mode: bool =
False) -> BaseSecretsBackend |
return secrets_backend_cls(**backend_kwargs)
+def get_importable_secret_backend(class_name: str | None) ->
BaseSecretsBackend | None:
+ """Get secret backend defined in the given class name."""
+ if class_name is not None:
+ secrets_backend_cls = import_string(class_name)
+ return secrets_backend_cls()
+ return None
+
+
def initialize_secrets_backends(
- default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH,
+ default_backends: list[str] | None = None,
) -> list[BaseSecretsBackend]:
"""
Initialize secrets backend.
* import secrets backend classes
* instantiate them and return them in a list
"""
- backend_list = []
worker_mode = False
- if default_backends != DEFAULT_SECRETS_SEARCH_PATH:
+ environment_variable_args: str | None = (
+ "airflow.secrets.environment_variables.EnvironmentVariablesBackend"
+ )
+ metastore_args: str | None = "airflow.secrets.metastore.MetastoreBackend"
+ if default_backends is not None:
worker_mode = True
+ environment_variable_args = (
+ environment_variable_args if environment_variable_args in
default_backends else None
+ )
+ metastore_args = metastore_args if metastore_args in default_backends
else None
+ backends_map: dict[str, dict[str, Any]] = {
+ "environment_variable": {
+ "callback": get_importable_secret_backend,
+ "args": (environment_variable_args,),
+ },
+ "metastore": {
+ "callback": get_importable_secret_backend,
+ "args": (metastore_args,),
+ },
+ "custom": {
+ "callback": get_custom_secret_backend,
+ "args": (worker_mode,),
+ },
+ }
- custom_secret_backend = get_custom_secret_backend(worker_mode)
+ backends_order = conf.getlist("secrets", "backends_order", delimiter=",")
- if custom_secret_backend is not None:
- backend_list.append(custom_secret_backend)
+ required_backends = ["environment_variable"] if worker_mode else
["metastore", "environment_variable"]
+ if missing_backends := [b for b in required_backends if b not in
backends_order]:
Review Comment:
Do you think defining an Enum here would make things cleaner?
```
class Backend(Enum):
ENVIRONMENT_VARIABLE = "environment_variable"
METASTORE = "metastore"
CUSTOM = "custom"
```
The hardcoded strings are error prone imo
##########
airflow-core/tests/unit/always/test_secrets.py:
##########
@@ -119,6 +120,38 @@ def test_backend_fallback_to_env_var(self,
mock_get_connection):
assert conn.get_uri() == "mysql://airflow:airflow@host:5432/airflow"
+ @conf_vars(
+ {
+ (
+ "secrets",
+ "backend",
+ ):
"airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend",
+ ("secrets", "backend_kwargs"): '{"connections_prefix": "/airflow",
"profile_name": null}',
+ ("secrets", "backends_order"):
"custom,environment_variable,metastore",
+ }
+ )
+ def test_backends_order(self):
+ backends = ensure_secrets_loaded()
+ backend_classes = [backend.__class__.__name__ for backend in backends]
+ assert backend_classes == [
+ "SystemsManagerParameterStoreBackend",
+ "EnvironmentVariablesBackend",
+ "MetastoreBackend",
+ ]
+
+ @pytest.mark.parametrize(
+ "backends_order",
+ [
+ pytest.param("custom,metastore",
id="no_environment_variable_backend"),
+ pytest.param("environment_variable", id="no_metastore_backend"),
+ pytest.param("metastore,environment_variable,unsupported",
id="unsupported_backend"),
+ ],
+ )
+ def test_backends_order_invalid_cases(self, backends_order):
+ with conf_vars({("secrets", "backends_order"): backends_order}):
+ with pytest.raises(AirflowConfigException):
+ ensure_secrets_loaded()
Review Comment:
Needs more test coverage to cover the cases for worker mode too. This only
checks for "non workers"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
