-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Add config option [secrets]backends_order #45931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
61b5ab6 to
4489808
Compare
4489808 to
58d80f3
Compare
|
@eladkal , please take a look at the updates. |
|
I was initially against making it configurable, but seeing the simplicity and flexibility, I am in. |
|
@eladkal ? |
|
hi there! |
|
We are on feature freeze for Airflow 3. |
|
Yeah. I think that might be 3.1 |
8c516f8 to
347e2f1
Compare
eladkal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prevent accidental merge. We are on feature freeze for Airflow 3.
PR can not be merged till main branch is for 3.1
| 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there!
I see that we are close to release Af3.1, so maybe we can merge this change?
Thanks :)
347e2f1 to
2b750de
Compare
2b750de to
4c0d958
Compare
|
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. |
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 |
|
@moiseenkov looking at it |
|
|
@VladaZakharova I meant I am looking at it. |
amoghrajesh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, added to the PR.
| @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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs more test coverage to cover the cases for worker mode too. This only checks for "non workers"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
| 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree. Added [workers]backends_order as a separate option that could be configured.
8aae4af to
780bbd2
Compare
|
Hello @amoghrajesh Thank you for your review!
Regarding UI: as it was mentioned here: #45931 (comment) UI should be introduced in this PR |
368a70c to
1ce52cc
Compare
159dd6e to
64d53e7
Compare
|
Hello @amoghrajesh Could you please review this PR? I responded to your comments. |
|
hi @amoghrajesh :) |
|
@VladaZakharova @moiseenkov @Crowiant I will take a look at this one soon. |
|
I wonder if this should just be called |
| secrets_backend = | ||
| secrets_backend_kwargs = | ||
| backends_order = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn’t read right? The key implemented here is not under [workders].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
amoghrajesh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ``backends_order`` comma-separated list of secret backends. These backends will be used in the order they are specified. | |
| ``backends_order`` is a comma-separated list of secret backends. These backends will be used in the order they are specified. |
| 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ``backends_order`` comma-separated list of secret backends for workers. These backends will be used in the order they are specified. | |
| ``backends_order`` is a comma-separated list of secret backends for workers. These backends will be used in the order they are specified. |
| def ensure_secrets_loaded( | ||
| default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH, | ||
| default_backends: list[str] | None = None, | ||
| ) -> list[BaseSecretsBackend]: | ||
| """ | ||
| Ensure that all secrets backends are loaded. | ||
| If the secrets_backend_list contains only 2 default backends, reload it. | ||
| """ | ||
| # 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With: #57744 merged, there is a shared config parser now. You will have to update this in sdk/configuration.py too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @amoghrajesh ! Thank you for mentioning the PR, I updated the code. Please check when you have time!
| execution_args = ( | ||
| "airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend" | ||
| if "airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend" | ||
| in default_backends | ||
| else None | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to avoid importing sdk in airflow core when possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
64d53e7 to
437208f
Compare
785af81 to
2015d8b
Compare
2015d8b to
a5092ed
Compare
Introduce a new configuration option for specifying secret backends load order:
The default value represents current behavior, thus nothing will change for existing users.