Skip to content

Comments

Avoid circular imports in AWS Secrets Backends if obtain secrets from config#26784

Merged
potiuk merged 1 commit intoapache:mainfrom
Taragolis:fix-circular-imports-amazon-sb
Oct 6, 2022
Merged

Avoid circular imports in AWS Secrets Backends if obtain secrets from config#26784
potiuk merged 1 commit intoapache:mainfrom
Taragolis:fix-circular-imports-amazon-sb

Conversation

@Taragolis
Copy link
Contributor

@Taragolis Taragolis commented Sep 29, 2022

PR #25628 add new possibility with circular imports in Secrets Backends.
It affect both AWS secrets backends in case if user want to retrieve secrets configs from SB.

How to reproduce

❯ export AIRFLOW__SECRETS__BACKEND=airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend
❯ export AIRFLOW__SECRETS__BACKEND_KWARGS='{"config_prefix": "/airflow/config", "region_name": "eu-west-1"}'
❯ export AIRFLOW__DATABASE__SQL_ALCHEMY_CONN_SECRET=boom
❯ airflow config get-value database sql_alchemy_conn

...

airflow.exceptions.AirflowConfigException: Cannot retrieve config from alternative secrets backend. Make sure it is configured properly and that the Backend is accessible.
Cannot retrieve config from alternative secrets backend. Make sure it is configured properly and that the Backend is accessible.
cannot import name 'SessionFactory' from partially initialized module 'airflow.providers.amazon.aws.hooks.base_aws' (most likely due to a circular import) (/Users/taragolis/Projects/common/airflow/airflow/providers/amazon/aws/hooks/base_aws.py)

After this changes

❯ export AIRFLOW__SECRETS__BACKEND=airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend
❯ export AIRFLOW__SECRETS__BACKEND_KWARGS='{"config_prefix": "/airflow/config", "region_name": "eu-west-1"}'
❯ export AIRFLOW__DATABASE__SQL_ALCHEMY_CONN_SECRET=boom
❯ airflow config get-value database sql_alchemy_conn

[2022-09-29 22:27:59,204] {credentials.py:1180} INFO - Found credentials in environment variables.
[2022-09-29 22:27:59,665] {credentials.py:1180} INFO - Found credentials in environment variables.
sqlite:////Users/taragolis/airflow/airflow-from-ssm.db

closes: #26754

@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Sep 29, 2022
vincbeck
vincbeck approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

uranusjr
uranusjr reviewed Oct 5, 2022
View reviewed changes
airflow/providers/amazon/aws/utils/connection_wrapper.py
"""

conn: InitVar[Connection | AwsConnectionWrapper | None]
conn: InitVar[Connection | AwsConnectionWrapper | _ConnectionMetadata | None]
Copy link
Member

@uranusjr uranusjr Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consolidate these types a bit, perhaps with a Protocol.

Copy link
Contributor Author

@Taragolis Taragolis Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree with you. Main problem with this class that when I initially create this helper I added InitVar and add some limitation with define attributes for this class. I have a plan to refactor it later and also get rid of _ConnectionMetadata.

@potiuk potiuk merged commit 8a1bbcf into apache:main Oct 6, 2022
@potiuk potiuk mentioned this pull request Nov 15, 2022
Closed
@Taragolis Taragolis mentioned this pull request Nov 22, 2022
Merged
@Taragolis Taragolis deleted the fix-circular-imports-amazon-sb branch January 14, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk approved these changes

@uranusjr uranusjr uranusjr approved these changes

@vincbeck vincbeck vincbeck approved these changes

Assignees

No one assigned

Labels

area:providers provider:amazon AWS/Amazon - related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cannot retrieve config from alternative secrets backend.

4 participants

@Taragolis @potiuk @uranusjr @vincbeck