Skip to content
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

azure key vault integration prefixes variables_prefix and connections_prefix incorrectly #485

Closed
rob-1126 opened this issue Jun 30, 2022 · 6 comments
Assignees
Labels
area/operator Kuberentes operator bug Something isn't working oss airflow pri/high High priority

Comments

@rob-1126
Copy link

Describe the bug
azure key vault adds a - between the key prefix and the secret name when it reads it from azure key vault. It does this even when the key prefix is "" so secrets end up named -my-secret instead of my-secret.

To Reproduce
Follow the instructions to set up an azure key vault integrated install with a connections_prefix of "".
Create a secret named -my-var in azure key vault.
Use the example dag provided in the docs with Variable.get("my-var")
You will see that it fetches the variable because it is incorrectly mapping my-var to -my-var instead of my-var.

Expected Behavior
connections_prefix and variables_prefix dont add a leading - when they are set to "".

Additional context
From my understanding null his historically not been supported as a value for that even though its passed as a JSON blob? Might be more idiomatic but if this is changed I'd think all providers should be changed. I suggest we don't do it and just continue to use "" but fix our prefixing as described in this ticket.

@rob-1126 rob-1126 added the bug Something isn't working label Jun 30, 2022
@rob-1126
Copy link
Author

Doc fix filed at https://github.com/astronomer/issues/issues/4796 intentionally updates docs to match the current (pre-fix) behavior of this provider. Once this ticket is done, the docs should be updated again to state the behavior changes with Airflow v. .

@bharanidharan14 bharanidharan14 self-assigned this Aug 1, 2022
@phanikumv phanikumv added the area/operator Kuberentes operator label Aug 17, 2022
@pankajkoti pankajkoti added the pri/high High priority label Sep 15, 2022
@rajaths010494
Copy link
Contributor

Secrets are used in OSS airflow and providers don't have that. So this would be a fix in OSS.

@rajaths010494
Copy link
Contributor

rajaths010494 commented Sep 22, 2022

Hi @rob-1126 Separator used to concatenate path_prefix and secret_id so when you make path_prefix="" it will take it empty string and add separator and then secret_id.
ex -my-var so if ur secret_id itself is -my-var the end key would be --my-var..
ideally we shouldn't use separator in the key itself.
https://github.com/apache/airflow/blob/main/airflow/providers/microsoft/azure/secrets/key_vault.py#L160-L172

@rajaths010494
Copy link
Contributor

apache/airflow#26749

@rajaths010494
Copy link
Contributor

PR is approved and waiting for it to be merged.

@rajaths010494
Copy link
Contributor

PR got merged in OSS airflow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/operator Kuberentes operator bug Something isn't working oss airflow pri/high High priority
Projects
None yet
Development

No branches or pull requests

5 participants