Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3841511233 Can you please resolve conflicts and fix the remaining static check? -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3819963246 Hello @potiuk @eladkal Thank you for your comments. I added experimental tag to the feature. Here are screenshots of how backends order looks on UI: https://github.com/user-attachments/assets/59fc7fc0-061f-40ba-ac73-21ad9e83952d"; /> https://github.com/user-attachments/assets/abc3d7e5-1b76-4757-a805-6c677d59fdd3"; /> -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3784567693 > I advise to tag this as experimental feature so community will use this feature with judgment. Good idea @eladkal -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3784275267 I don't have time to re-review but I still have some concerns. I advise to tag this as experimental feature so community will use this feature with judgment. I will however ask to please present a UI image of how the indication of backend order is shown to users. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3784010613 @amoghrajesh @uranusjr -> any comments here? I think there were some important questions/comments from you and I would love to merge that one to make sure it makes it for 3.2 (it looks good in general). -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3738772066 Hello @amoghrajesh @uranusjr I responded to your comments in the PR. Could you please help with the review? -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2627268679 ## airflow-core/src/airflow/configuration.py: ## @@ -2348,28 +2334,101 @@ 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 + + +class Backends(Enum): +"""Type of the secrets backend.""" + +ENVIRONMENT_VARIABLE = "environment_variable" +EXECUTION_API = "execution_api" +CUSTOM = "custom" +METASTORE = "metastore" + + 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: +search_section = "secrets" +environment_variable_args: str | None = ( +"airflow.secrets.environment_variables.EnvironmentVariablesBackend" +) +metastore_args: str | None = "airflow.secrets.metastore.MetastoreBackend" +execution_args: str | None = None +if default_backends is not None: worker_mode = True +search_section = "workers" +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 +execution_args = ( + "airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend" +if "airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend" +in default_backends +else None +) Review Comment: Could you expand a little bit on this? ExecutionAPISecretsBackend exists only in task-sdk and applies for the workers. Where else could it be imported? -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2627247913 ## airflow-core/src/airflow/configuration.py: ## @@ -2301,9 +2288,8 @@ def ensure_secrets_loaded( """ # Check if the secrets_backend_list contains only 2 default backends. -# Check if we are loading the backends for worker too by checking if the default_backends is equal -# to DEFAULT_SECRETS_SEARCH_PATH. -if len(secrets_backend_list) == 2 or default_backends != DEFAULT_SECRETS_SEARCH_PATH: +# Check if we are loading the backends for worker too by checking if the default_backends is not None +if len(secrets_backend_list) == 2 or default_backends is not None: return initialize_secrets_backends(default_backends=default_backends) return secrets_backend_list Review Comment: Hello @amoghrajesh ! Thank you for mentioning the PR, I updated the code. Please check when you have time! -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2623754538 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -89,13 +100,21 @@ configure separate secrets backend for workers, you can do that using: [workers] secrets_backend = secrets_backend_kwargs = - +backends_order = Review Comment: Hello @uranusjr ! Thank you for your review and comments! No, actually it should be under both sections: [secrets] and [workers]. Because after talking with @amoghrajesh We agreed that implementation should be extended on workers too. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2558826591 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -89,13 +100,21 @@ configure separate secrets backend for workers, you can do that using: [workers] secrets_backend = secrets_backend_kwargs = - +backends_order = Set ``secrets_backend`` to the fully qualified class name of the backend you want to enable. You can provide ``secrets_backend_kwargs`` with json and it will be passed as kwargs to the ``__init__`` method of your secrets backend for the workers. +``backends_order`` comma-separated list of secret backends for workers. These backends will be used in the order they are specified. Review Comment: ```suggestion ``backends_order`` is a comma-separated list of secret backends for workers. These backends will be used in the order they are specified. ``` ## airflow-core/src/airflow/configuration.py: ## @@ -2348,28 +2334,101 @@ 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 + + +class Backends(Enum): +"""Type of the secrets backend.""" + +ENVIRONMENT_VARIABLE = "environment_variable" +EXECUTION_API = "execution_api" +CUSTOM = "custom" +METASTORE = "metastore" + + 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: +search_section = "secrets" +environment_variable_args: str | None = ( +"airflow.secrets.environment_variables.EnvironmentVariablesBackend" +) +metastore_args: str | None = "airflow.secrets.metastore.MetastoreBackend" +execution_args: str | None = None +if default_backends is not None: worker_mode = True +search_section = "workers" +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 +execution_args = ( + "airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend" +if "airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend" +in default_backends +else None +) Review Comment: We are trying to avoid importing sdk in airflow core when possible ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -64,12 +66,21 @@ The ``[secrets]`` section has the following options: [secrets] backend = backend_kwargs = +backends_order = Set ``backend`` to the fully qualified class name of the backend you want to enable. You can provide ``backend_kwargs`` with json and it will be passed as kwargs to the ``__init__`` method of your secrets backend. +``backends_order`` comma-separated list of secret backends. These backends will be used in the order they are specified. Review Comment: ```suggestion ``backends_order`` is a comma-separated list of secret backends. These backends will be used in the order they are specified. ``` ## airflow-core/src/airflow/configuration.py: ## @@ -2301,9 +2288,8 @@ def ensure_secrets_loaded( """ # Check if the secrets_backend_list contains only 2 default backends. -# Check if we are loading the backends for worker too by checking if the default_backends is equal -# to DEFAULT_SECRETS_SEARCH_PATH. -if len(secrets_backend_list) == 2 or default_backends != DEFAULT_SECRETS_SEARCH_PATH: +# Check if we are loading the backends for worker too by checking if the default_backends is not None +if len(secrets_backend_list) == 2 or default_backends is not None: return initialize_secrets_backends(default_backends=default_backends) return secrets_backend_list Review Comment: With: https://github.com/apache/airflow/pull/57744 merged, there is a shared config parser now. You will have to update this in sdk/configuration.py too -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.
Re: [PR] Add config option [secrets]backends_order [airflow]
uranusjr commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2548867570 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -89,13 +100,21 @@ configure separate secrets backend for workers, you can do that using: [workers] secrets_backend = secrets_backend_kwargs = - +backends_order = Review Comment: This doesn’t read right? The key implemented here is not under `[workders]`. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
uranusjr commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3561862735 I wonder if this should just be called `backends` and have the description explain how the ordering is significant. The implementation is fine to me. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
uranusjr commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2548860808 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -64,12 +66,21 @@ The ``[secrets]`` section has the following options: [secrets] backend = backend_kwargs = +backends_order = Set ``backend`` to the fully qualified class name of the backend you want to enable. You can provide ``backend_kwargs`` with json and it will be passed as kwargs to the ``__init__`` method of your secrets backend. +``backends_order`` 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 Review Comment: OK then I think this is fine. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2545346813 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -64,12 +66,21 @@ The ``[secrets]`` section has the following options: [secrets] backend = backend_kwargs = +backends_order = Set ``backend`` to the fully qualified class name of the backend you want to enable. You can provide ``backend_kwargs`` with json and it will be passed as kwargs to the ``__init__`` method of your secrets backend. +``backends_order`` 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 Review Comment: Hello @uranusjr as it listed in the code here: https://github.com/apache/airflow/pull/45931/files#diff-12a2a29eab84b723e9342e89877c875d3e979220a53884a9103d8c067ffdc286R2409 Exception will be raised in this case. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
uranusjr commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2544542438 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -64,12 +66,21 @@ The ``[secrets]`` section has the following options: [secrets] backend = backend_kwargs = +backends_order = Set ``backend`` to the fully qualified class name of the backend you want to enable. You can provide ``backend_kwargs`` with json and it will be passed as kwargs to the ``__init__`` method of your secrets backend. +``backends_order`` 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 Review Comment: What happens if the user only list some of the backends? ``` [secrets] backends_order = custom ``` -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3556160258 @VladaZakharova @moiseenkov @Crowiant I will take a look at this one soon. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
VladaZakharova commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3552210262 hi @amoghrajesh :) can you please check if we need to add additional changes here in PR? 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3546744163 Hello @amoghrajesh Could you please take a look at the PR? I addressed all your comments. Thank you! -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3541671716 Hello @amoghrajesh Could you please review this PR? I responded to your comments. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2487177214 ## 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: Yeah, I agree. Added [workers]backends_order as a separate option that could be configured. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2487166122
##
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:
Yes, I agree, added to the PR.
--
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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2487167401
##
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",
+]
+
[email protected](
+"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:
Added
--
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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3481487585 Hello @amoghrajesh Thank you for your review! > I have some comments re the design of the config. > > I would also not add the UI portion in the same PR, maybe a orthogonal one would be the place to add it. Regarding UI: as it was mentioned here that UI should be introduced in this PR: https://github.com/apache/airflow/pull/45931#issuecomment-3023344132 -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
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):
+bac
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2452064529 ## airflow-core/src/airflow/configuration.py: ## @@ -2138,7 +2137,7 @@ def make_group_other_inaccessible(file_path: str): def ensure_secrets_loaded( -default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH, +default_backends: list[str] | None = None, Review Comment: In the previous version, a value was always passed to default_backends. Either `DEFAULT_SECRETS_SEARCH_PATH` or `DEFAULT_SECRETS_SEARCH_PATH_WORKERS` which imo is far more intuitive than None... -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3432248321 @VladaZakharova I meant I am looking at it. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
VladaZakharova commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3432236972 > @moiseenkov looking at it He is not, because he doesn't work in our team anymore, @Crowiant is the one responsible for it, so that's why he is requesting review from you -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3432226543 @moiseenkov looking at it -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3422033604 Hello @amoghrajesh can you please take a look at the PR? -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3330266822 Hello @amoghrajesh can you please take a look at the PR? -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3219370503 @moiseenkov 3.1 is fast approaching, could you take a look at the comments if we want to make it for Airflow 3.1? -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2275843984
##
airflow-core/src/airflow/configuration.py:
##
@@ -2194,28 +2192,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 = ["metastore", "environment_variable"]
+if missing_backends := [b for b in required_backends if b not in
backends_order]:
+raise AirflowConfigException(
+"The configuration option [secrets]backends_order is
misconfigured. "
+"The following backend types are missing: %s",
+missing_backends,
+)
Review Comment:
This check would behave badly for workers?
Example:
```
required_backends = ["metastore", "environment_variable"]
backends_order = ["custom", "environment_variable"]
```
`required_backends` need not have `metastore` for workers.
##
airflow-core/src/airflow/configuration.py:
##
@@ -2138,7 +2137,7 @@ def make_group_other_inaccessible(file_path: str):
def ensure_secrets_loaded(
-default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH,
+default_backends: list[str] | None = None,
Review Comment:
The convention here now is:
- If None, its called from someplace other than worker
- If not None, its called from worker (for `[workers][secrets_backend]`
config)
Its not very intuitive, could we go back with the previous behaviour?
##
airflow-core/docs/security/secrets/secrets-backend/index.rst:
##
@@ -64,12 +66,21 @@ The ``[secrets]`` section has the following options:
[secrets]
backend =
backend_kwargs =
+backends_order =
Set ``backend`` to the fully qualified class name of the backend you want to
enable.
You can provide ``backend_kwargs`` with json and it will be passed as kwargs
to the ``__init__`` method of
your secrets backend.
+``backends_order`` 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``.
Review Comment:
I also request you to have some user guidelines in the docs to configure it
properly.
For example:
Lets say DB lookup has higher precedence than that of say ENV backend.
This would be shooting ourselves in the foot by compromising the performance
here? DB lookup will be more expensive than DB.
##
airflow-core/src/airflow/configuration.py:
##
@@ -2194,28 +2192,73 @@ def g
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2259651486
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,40 @@ 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",
+]
+
+@conf_vars({("secrets", "backends_order"): "custom,metastore"})
+def test_backends_order_no_environment_variable_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"): "environment_variable"})
+def test_backends_order_no_metastore_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"):
"metastore,environment_variable,unsupported"})
+def test_backends_order_unsupported(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
Review Comment:
Thank you very much @jason810496 ! Added to the PR
--
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]
Re: [PR] Add config option [secrets]backends_order [airflow]
jason810496 commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2257466928
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,40 @@ 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",
+]
+
+@conf_vars({("secrets", "backends_order"): "custom,metastore"})
+def test_backends_order_no_environment_variable_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"): "environment_variable"})
+def test_backends_order_no_metastore_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"):
"metastore,environment_variable,unsupported"})
+def test_backends_order_unsupported(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
Review Comment:
The usage of `parametrize` will be somehow like the following change (the
indent and format might still need fix).
```suggestion
@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(backends_order):
with conf_vars({("secrets", "backends_order"): backends_order}):
with pytest.raises(AirflowConfigException):
ensure_secrets_loaded()
```
--
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]
Re: [PR] Add config option [secrets]backends_order [airflow]
jason810496 commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2257466928
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,40 @@ 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",
+]
+
+@conf_vars({("secrets", "backends_order"): "custom,metastore"})
+def test_backends_order_no_environment_variable_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"): "environment_variable"})
+def test_backends_order_no_metastore_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"):
"metastore,environment_variable,unsupported"})
+def test_backends_order_unsupported(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
Review Comment:
The usage of `parametrize` will be like the following change (the indent and
format might still need fix).
```suggestion
@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(backends_order):
with conf_vars({("secrets", "backends_order"): backends_order}):
with pytest.raises(AirflowConfigException):
ensure_secrets_loaded()
```
--
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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2257280351
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,40 @@ 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",
+]
+
+@conf_vars({("secrets", "backends_order"): "custom,metastore"})
+def test_backends_order_no_environment_variable_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"): "environment_variable"})
+def test_backends_order_no_metastore_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"):
"metastore,environment_variable,unsupported"})
+def test_backends_order_unsupported(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
Review Comment:
I cannot find usage of this decorator with pytest.mark.parametrize. So if it
is not so important can we leave it as it is?
--
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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2257242842
##
airflow-core/src/airflow/api_fastapi/core_api/routes/public/config.py:
##
@@ -155,3 +155,36 @@ def get_config_value(
config = Config(sections=[ConfigSection(name=section,
options=[ConfigOption(key=option, value=value)])])
return _response_based_on_accept(accept, config)
+
+
+@config_router.get(
+"/backends_order",
+responses={
+**create_openapi_http_exception_doc(
+[
+status.HTTP_404_NOT_FOUND,
+status.HTTP_406_NOT_ACCEPTABLE,
+]
+),
+status.HTTP_200_OK: {
+"description": "Successful Response",
+"content": text_example_response_for_get_config_value,
+},
+},
+response_model=Config,
+dependencies=[Depends(requires_access_configuration("GET"))],
+)
+def get_backends_order_value(
Review Comment:
Hello @jason810496 agree. Moved.
--
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]
Re: [PR] Add config option [secrets]backends_order [airflow]
jason810496 commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2252043506
##
airflow-core/src/airflow/api_fastapi/core_api/routes/public/config.py:
##
@@ -155,3 +155,36 @@ def get_config_value(
config = Config(sections=[ConfigSection(name=section,
options=[ConfigOption(key=option, value=value)])])
return _response_based_on_accept(accept, config)
+
+
+@config_router.get(
+"/backends_order",
+responses={
+**create_openapi_http_exception_doc(
+[
+status.HTTP_404_NOT_FOUND,
+status.HTTP_406_NOT_ACCEPTABLE,
+]
+),
+status.HTTP_200_OK: {
+"description": "Successful Response",
+"content": text_example_response_for_get_config_value,
+},
+},
+response_model=Config,
+dependencies=[Depends(requires_access_configuration("GET"))],
+)
+def get_backends_order_value(
Review Comment:
No problem! That sounds reasonable. If that's the case, I think we can move
on to the UI-specific routes.
--
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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2251761667
##
airflow-core/src/airflow/api_fastapi/core_api/routes/public/config.py:
##
@@ -155,3 +155,36 @@ def get_config_value(
config = Config(sections=[ConfigSection(name=section,
options=[ConfigOption(key=option, value=value)])])
return _response_based_on_accept(accept, config)
+
+
+@config_router.get(
+"/backends_order",
+responses={
+**create_openapi_http_exception_doc(
+[
+status.HTTP_404_NOT_FOUND,
+status.HTTP_406_NOT_ACCEPTABLE,
+]
+),
+status.HTTP_200_OK: {
+"description": "Successful Response",
+"content": text_example_response_for_get_config_value,
+},
+},
+response_model=Config,
+dependencies=[Depends(requires_access_configuration("GET"))],
+)
+def get_backends_order_value(
Review Comment:
Hello @jason810496 ! Thank you for your review. Regarding the new endpoint.
I decided to do so because get_config_value can be restricted to all users
including admin by deployment managers and not allow to see any option.
_check_expose_config() prevents from doing so. In order not to change the
logic of this endpoint I added get_backends_order_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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
jason810496 commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2251688093
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,40 @@ 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",
+]
+
+@conf_vars({("secrets", "backends_order"): "custom,metastore"})
+def test_backends_order_no_environment_variable_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"): "environment_variable"})
+def test_backends_order_no_metastore_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"):
"metastore,environment_variable,unsupported"})
+def test_backends_order_unsupported(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
Review Comment:
Small nits, no strong opnion.
Maybe we can combine the test cases with `pytest.mark.parameterize`.
##
airflow-core/src/airflow/api_fastapi/core_api/routes/public/config.py:
##
@@ -155,3 +155,36 @@ def get_config_value(
config = Config(sections=[ConfigSection(name=section,
options=[ConfigOption(key=option, value=value)])])
return _response_based_on_accept(accept, config)
+
+
+@config_router.get(
+"/backends_order",
+responses={
+**create_openapi_http_exception_doc(
+[
+status.HTTP_404_NOT_FOUND,
+status.HTTP_406_NOT_ACCEPTABLE,
+]
+),
+status.HTTP_200_OK: {
+"description": "Successful Response",
+"content": text_example_response_for_get_config_value,
+},
+},
+response_model=Config,
+dependencies=[Depends(requires_access_configuration("GET"))],
+)
+def get_backends_order_value(
Review Comment:
It seems we can reuse with `get_config_value` route.
Or if the `get_backends_order_value` is necessary, it will be better to move
the route under `airflow-core/src/airflow/api_fastapi/core_api/routes/ui`
##
airflow-core/src/airflow/api_fastapi/core_api/routes/public/config.py:
##
@@ -155,3 +155,36 @@ def get_config_value(
config = Config(sections=[ConfigSection(name=section,
options=[ConfigOption(key=option, value=value)])])
return _response_based_on_accept(accept, config)
+
+
+@config_router.get(
+"/backends_order",
+responses={
+**create_openapi_http_exception_doc(
+[
+status.HTTP_404_NOT_FOUND,
+status.HTTP_406_NOT_ACCEPTABLE,
+]
+),
+status.HTTP_200_OK: {
+"description": "Successful Response",
+"content": text_example_response_for_get_config_value,
+},
+},
+response_model=Config,
+dependencies=[Depends(requires_access_configuration("GET"))],
+)
+def get_backends_order_value(
Review Comment:
May I ask why do we need "backends_order" specific new endpoint?
--
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]
Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3023344132 > I think it would be good to raise a discussion asking what peopel think about it and try to reach consensus. I agree. I am not comfortable with making this change without the UI indication / other mechanisem that allows dag authors to see the cluster admin setup for backend order -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3022748078 This looks good to me - but I think it might be worth to raise a devlist discussion for it @VladaZakharova -> there were past discussions about changing the sequence of resolving configurations, and I know people have strong opinion about "fixed" vs. "confifurable" sequence - and there are arguments pro / against each of those options. I think it would be good to raise a discussion asking what peopel think about it and try to reach consensus. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
VladaZakharova commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2174662212 ## airflow-core/src/airflow/config_templates/config.yml: ## @@ -1265,6 +1265,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: Hi there! I see that we are close to release Af3.1, so maybe we can merge this change? 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2023517222 ## airflow-core/src/airflow/config_templates/config.yml: ## @@ -1265,6 +1265,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: right now it's not configurable so the Airflow docs is good enough but after this PR is accepted the Airflow docs can't help you... You'll need to check the deployment to understand the priority order. In many deployments dag authors don't have access to the deployment code. My thoughts on the UI is an improvement (I think crucial but that is just my own perspective) I welcome others to share their thoughts. This feature is already targeting 3.1+ so regardless if we do the UI part or not we can't merge it now. We need to wait for main branch to become 3.1 (probably only after we cut RC1 for Airflow 3( -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2022943159 ## airflow-core/src/airflow/config_templates/config.yml: ## @@ -1265,6 +1265,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: Hello @eladkal ! I don't see any info regarding the secrets section the UI is using right now. So maybe we could skip it for now? What do you think? Regarding the naming. As stated in the doc it is responsible for _secrets backends search ordering_. So maybe `backends_search_order` is more convenient? -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2021018760 ## airflow-core/src/airflow/config_templates/config.yml: ## @@ -1265,6 +1265,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: i am a bit concerned making it just a "hidden" setting. I think we better to come up with a way to expose the chosen order in the UI. that way DAG authors can verify and use it for debug for questions like (why I see wrong value in variable). Also, correct me if I am wrong the order affect only read, not write. maybe the setting should be `backends_read_order?` -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2661094214 Yeah. I think that might be 3.1 -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2656887152 We are on feature freeze for Airflow 3. https://lists.apache.org/thread/r26htzl0w3th7pw0l1y31g6s14qbtwwt -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
VladaZakharova commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2656190070 Hi @potiuk @eladkal ! Are there some other changes we need to make here? Or we can merge this one? -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
VladaZakharova commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2639426046 hi there! @potiuk Can we merge this one please? -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2614079232 @eladkal ? -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2614079156 I was initially against making it configurable, but seeing the simplicity and flexibility, I am in. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
moiseenkov commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2609151622 @eladkal , please take a look at the updates. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
moiseenkov commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r1925348243 ## airflow/config_templates/config.yml: ## @@ -1296,6 +1296,15 @@ 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`, `environment_variable`, `metastore`. + version_added: 2.10.5 + type: string + example: ~ + default: "custom,environment_variable,metastore" Review Comment: Thanks for noticing that - I updated the documentation and the new option description. Speaking of breaking changes, I'm pretty sure that the default behavior is identical the current one, because the existing implementation used to check any custom backend first, and then load `EnvironmentVariablesBackend` and `MetastoreBackend`. ```python DEFAULT_SECRETS_SEARCH_PATH = [ "airflow.secrets.environment_variables.EnvironmentVariablesBackend", "airflow.secrets.metastore.MetastoreBackend", ] ... def initialize_secrets_backends() -> list[BaseSecretsBackend]: ... backend_list = [] custom_secret_backend = get_custom_secret_backend() if custom_secret_backend is not None: backend_list.append(custom_secret_backend) for class_name in DEFAULT_SECRETS_SEARCH_PATH: secrets_backend_cls = import_string(class_name) backend_list.append(secrets_backend_cls()) return backend_list ``` -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
moiseenkov commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r1925339026 ## airflow/config_templates/config.yml: ## @@ -1296,6 +1296,15 @@ 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`, `environment_variable`, `metastore`. + version_added: 2.10.5 Review Comment: Thanks for pointing that out. Changed to the version 3.0.0. -- 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]
Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r1925299794 ## airflow/config_templates/config.yml: ## @@ -1296,6 +1296,15 @@ 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`, `environment_variable`, `metastore`. + version_added: 2.10.5 + type: string + example: ~ + default: "custom,environment_variable,metastore" Review Comment: isn't this breaking change? What is custom? Is it for rolling secret backend? The current order is not the same as you listed: > If you enable an alternative secrets backend, it will be searched first, followed by environment variables, then metastore. This search ordering is not configurable. Also you need to make doc changes to explain about this new functionality https://airflow.apache.org/docs/apache-airflow/stable/security/secrets/secrets-backend/index.html#search-path ## airflow/config_templates/config.yml: ## @@ -1296,6 +1296,15 @@ 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`, `environment_variable`, `metastore`. + version_added: 2.10.5 Review Comment: This can not be in 2.10.5 nor in 2.11 it's a new feature. Will have to wait for Airflow 3. -- 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]
