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.

Sign up for GitHub

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

Fix instantiating Vault Secret Backend during configuration #17935

Merged
merged 1 commit into from
Aug 31, 2021
Merged

Fix instantiating Vault Secret Backend during configuration #17935

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions airflow/providers/hashicorp/secrets/vault.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@
# specific language governing permissions and limitations
# under the License.
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
from typing import TYPE_CHECKING, Optional

from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient
from airflow.secrets import BaseSecretsBackend
from airflow.utils.log.logging_mixin import LoggingMixin
Expand Down Expand Up @@ -206,7 +205,12 @@ def get_conn_uri(self, conn_id: str) -> Optional[str]:

return response.get("conn_uri") if response else None

def get_connection(self, conn_id: str) -> Optional[Connection]:
# Make sure connection is imported this way for type checking, otherwise when importing
# the backend it will get a circular dependency and fail
if TYPE_CHECKING:
from airflow.models.connection import Connection
Comment on lines +208 to +211
Copy link
Member

Choose a reason for hiding this comment

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

This can go at the top of the file

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually deliberately wanted to keep it in one place, to avoid any accidental removal. It's pretty "special" case (which eventually we might be able to remove) :)

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW. I did not even know that you can do such statements at the class level . But it worked.

Copy link
Member Author

@potiuk potiuk Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

TIL: https://docs.python.org/3/tutorial/classes.html#class-definition-syntax

In practice, the statements inside a class definition will usually be function definitions, but other statements are allowed, and sometimes useful — we’ll come back to this later. The function definitions inside a class normally have a peculiar form of argument list, dictated by the calling conventions for methods — again, this is explained later.

Perfecly legitimate use case IMHO :D ..

Copy link
Member

Choose a reason for hiding this comment

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

Yeah no strong opinion on top or bottom :D -- just won't make a difference from code view as it is inside TYPE_CHECKING conditional


def get_connection(self, conn_id: str) -> 'Optional[Connection]':
"""
Get connection from Vault as secret. Prioritize conn_uri if exists,
if not fall back to normal Connection creation.
Expand All @@ -215,6 +219,10 @@ def get_connection(self, conn_id: str) -> Optional[Connection]:
:rtype: Connection
:return: A Connection object constructed from Vault data
"""
# The Connection needs to be locally imported because otherwise we get into cyclic import
# problems when instantiating the backend during configuration
from airflow.models.connection import Connection

response = self.get_response(conn_id)
if response is None:
return None
Expand Down