You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/25 20:27:42 UTC
[GitHub] [airflow] potiuk opened a new pull request #10559: Untangle conf secrets deps
potiuk opened a new pull request #10559:
URL: https://github.com/apache/airflow/pull/10559
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #10559:
URL: https://github.com/apache/airflow/pull/10559
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#issuecomment-680775514
Taking a look at the changes and failed tests I would say that we may need to adjust mocking?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r477255782
##########
File path: airflow/secrets/__init__.py
##########
@@ -22,102 +22,12 @@
* Metastore database
* AWS SSM Parameter store
"""
-__all__ = ['BaseSecretsBackend', 'get_connections', 'get_variable', 'get_custom_secret_backend']
+__all__ = ['BaseSecretsBackend', 'DEFAULT_SECRETS_SEARCH_PATH', 'SECRETS_CONFIG_SECTION']
-import json
-from json import JSONDecodeError
-from typing import TYPE_CHECKING, List, Optional
-
-from airflow.configuration import conf
-from airflow.exceptions import AirflowNotFoundException
from airflow.secrets.base_secrets import BaseSecretsBackend
-from airflow.utils.module_loading import import_string
-
-if TYPE_CHECKING:
- from airflow.models.connection import Connection
-
-CONFIG_SECTION = "secrets"
+SECRETS_CONFIG_SECTION = "secrets"
Review comment:
```suggestion
```
##########
File path: airflow/secrets/__init__.py
##########
@@ -22,102 +22,12 @@
* Metastore database
* AWS SSM Parameter store
"""
-__all__ = ['BaseSecretsBackend', 'get_connections', 'get_variable', 'get_custom_secret_backend']
+__all__ = ['BaseSecretsBackend', 'DEFAULT_SECRETS_SEARCH_PATH', 'SECRETS_CONFIG_SECTION']
Review comment:
```suggestion
__all__ = ['BaseSecretsBackend', 'DEFAULT_SECRETS_SEARCH_PATH']
```
##########
File path: airflow/configuration.py
##########
@@ -951,3 +953,52 @@ def set(*args, **kwargs): # noqa pylint: disable=redefined-builtin
stacklevel=2
)
return conf.set(*args, **kwargs)
+
+
+def ensure_secrets_loaded() -> 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
+ if len(secrets_backend_list) == 2:
+ return initialize_secrets_backends()
+ return secrets_backend_list
+
+
+def get_custom_secret_backend() -> Optional[BaseSecretsBackend]:
+ """Get Secret Backend if defined in airflow.cfg"""
+ secrets_backend_cls = conf.getimport(section='secrets', key='backend')
+
+ if secrets_backend_cls:
+ try:
+ alternative_secrets_config_dict = json.loads(
+ conf.get(section=SECRETS_CONFIG_SECTION, key='backend_kwargs', fallback='{}')
Review comment:
```suggestion
conf.get(section='secrets', key='backend_kwargs', fallback='{}')
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r476752232
##########
File path: airflow/hooks/base_hook.py
##########
@@ -20,7 +20,8 @@
import random
from typing import Any, List
-from airflow import secrets
+import airflow.configuration
+import airflow.models.connection
Review comment:
Both comments are outdated so ignore 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#issuecomment-680301183
I think it shoudl be ratrher nice now :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r477254702
##########
File path: airflow/hooks/base_hook.py
##########
@@ -54,7 +53,7 @@ def get_connection(cls, conn_id: str) -> Connection:
:param conn_id: connection id
:return: connection
"""
- conn = random.choice(list(cls.get_connections(conn_id)))
+ conn = random.choice(cls.get_connections(conn_id))
Review comment:
Any reason for removing `list` from here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#issuecomment-680794656
> Taking a look at the changes and failed tests I would say that we may need to adjust mocking?
I think it's better to keep backward compatibility and restore "get_connections" method in BaseHook :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r476724420
##########
File path: airflow/hooks/base_hook.py
##########
@@ -20,7 +20,8 @@
import random
from typing import Any, List
-from airflow import secrets
+import airflow.configuration
Review comment:
Is this needed?
##########
File path: airflow/hooks/base_hook.py
##########
@@ -20,7 +20,8 @@
import random
from typing import Any, List
-from airflow import secrets
+import airflow.configuration
+import airflow.models.connection
Review comment:
We could just import `get_connections` method here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r476730492
##########
File path: airflow/models/connection.py
##########
@@ -380,3 +381,17 @@ def extra_dejson(self) -> Dict:
self.log.error("Failed parsing the json for conn_id %s", self.conn_id)
return obj
+
+
+def get_connections(conn_id: str) -> List[Connection]:
Review comment:
True .. They are only used internally and in tests :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r477266121
##########
File path: airflow/models/connection.py
##########
@@ -380,3 +381,17 @@ def extra_dejson(self) -> Dict:
self.log.error("Failed parsing the json for conn_id %s", self.conn_id)
return obj
+
+ @classmethod
+ def get_connections_from_secrets(cls, conn_id: str) -> List['Connection']:
Review comment:
Unfottunately It won't work - at this time "Connection" is not yet defined and mypy will complain (because it is inside the Connection definition - quoting is specifically foreseen for that case https://www.python.org/dev/peps/pep-0484/#forward-references
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#issuecomment-680796931
I think this should work now :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r476761637
##########
File path: airflow/hooks/base_hook.py
##########
@@ -20,7 +20,8 @@
import random
from typing import Any, List
-from airflow import secrets
+import airflow.configuration
+import airflow.models.connection
Review comment:
True. Moving.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#issuecomment-680268355
I think this way it is even nicer.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#issuecomment-680842651
Or rather this time - there was some bad refactor breaking other tests :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r476720004
##########
File path: airflow/models/connection.py
##########
@@ -380,3 +381,17 @@ def extra_dejson(self) -> Dict:
self.log.error("Failed parsing the json for conn_id %s", self.conn_id)
return obj
+
+
+def get_connections(conn_id: str) -> List[Connection]:
Review comment:
```suggestion
def _get_secret_connections(conn_id: str) -> List[Connection]:
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#issuecomment-680794656
> Taking a look at the changes and failed tests I would say that we may need to adjust mocking?
I think it's better to keep backward compatibility and restore "get_connections" method in Connection :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r476734337
##########
File path: airflow/models/connection.py
##########
@@ -380,3 +381,17 @@ def extra_dejson(self) -> Dict:
self.log.error("Failed parsing the json for conn_id %s", self.conn_id)
return obj
+
+
+def get_connections(conn_id: str) -> List[Connection]:
Review comment:
Well. It seems even more appropriate to make them static methods in Connection/Variable respectively
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r477254999
##########
File path: airflow/models/connection.py
##########
@@ -380,3 +381,17 @@ def extra_dejson(self) -> Dict:
self.log.error("Failed parsing the json for conn_id %s", self.conn_id)
return obj
+
+ @classmethod
+ def get_connections_from_secrets(cls, conn_id: str) -> List['Connection']:
Review comment:
```suggestion
def get_connections_from_secrets(cls, conn_id: str) -> List[Connection]:
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r476752027
##########
File path: airflow/hooks/base_hook.py
##########
@@ -20,7 +20,8 @@
import random
from typing import Any, List
-from airflow import secrets
+import airflow.configuration
+import airflow.models.connection
Review comment:
lol -- looks like I forgot to press Submit button after review :D
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r476720110
##########
File path: airflow/models/variable.py
##########
@@ -181,3 +181,17 @@ def rotate_fernet_key(self):
fernet = get_fernet()
if self._val and self.is_encrypted:
self._val = fernet.rotate(self._val.encode('utf-8')).decode()
+
+
+def get_variable(key: str) -> Optional[str]:
Review comment:
```suggestion
def _get_secret_variable(key: str) -> Optional[str]:
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r476732162
##########
File path: airflow/models/connection.py
##########
@@ -380,3 +381,17 @@ def extra_dejson(self) -> Dict:
self.log.error("Failed parsing the json for conn_id %s", self.conn_id)
return obj
+
+
+def get_connections(conn_id: str) -> List[Connection]:
Review comment:
Well this one is actually used in Base Hook. but I can incorporate it there.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r477267479
##########
File path: airflow/models/connection.py
##########
@@ -380,3 +381,17 @@ def extra_dejson(self) -> Dict:
self.log.error("Failed parsing the json for conn_id %s", self.conn_id)
return obj
+
+ @classmethod
+ def get_connections_from_secrets(cls, conn_id: str) -> List['Connection']:
Review comment:
Oh yes, you are right
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r477267303
##########
File path: airflow/secrets/__init__.py
##########
@@ -22,102 +22,12 @@
* Metastore database
* AWS SSM Parameter store
"""
-__all__ = ['BaseSecretsBackend', 'get_connections', 'get_variable', 'get_custom_secret_backend']
+__all__ = ['BaseSecretsBackend', 'DEFAULT_SECRETS_SEARCH_PATH', 'SECRETS_CONFIG_SECTION']
Review comment:
Indeed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10559: Untangle conf secrets deps
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10559:
URL: https://github.com/apache/airflow/pull/10559#discussion_r476761961
##########
File path: airflow/hooks/base_hook.py
##########
@@ -20,7 +20,8 @@
import random
from typing import Any, List
-from airflow import secrets
+import airflow.configuration
Review comment:
Not any more of we move the method to Connection
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org