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 2021/06/14 07:42:22 UTC
[GitHub] [airflow] fhoda opened a new pull request #16404: Secrets backend failover
fhoda opened a new pull request #16404:
URL: https://github.com/apache/airflow/pull/16404
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
1. Currently Airflow does not check the default secrets backends (`env` and `metastore db`) if there is any sort of connection related error to an Alternative Backend, causing related tasks to fail. The change proposed here allows it to fail over to checking the default backends when this happens.
2. Additionally GCP Secrets Manager causes the Airflow Webserver to crash at startup if credentials for the backend cannot be found. This behavior seems to be unique to GCP Secret Manager and this PR addresses that for parity in behavior regarding missing credentials across all backends.
closes: #14592
--
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] fhoda edited a comment on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
fhoda edited a comment on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-861797812
Ah yes, I understand now. Agree, was planning to add tests for all changes once I got feedback on the approach. I will implement your suggestion for `get_config` as well and then add 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] uranusjr commented on a change in pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#discussion_r660197969
##########
File path: airflow/models/connection.py
##########
@@ -35,6 +36,8 @@
from airflow.utils.log.secrets_masker import mask_secret
from airflow.utils.module_loading import import_string
+log = logging.getLogger()
Review comment:
Use `getLogger(__file__)` to give the logger a name. Otherwise it returns the root logger, making the log messages very difficult to manage.
##########
File path: airflow/models/connection.py
##########
@@ -35,6 +36,8 @@
from airflow.utils.log.secrets_masker import mask_secret
from airflow.utils.module_loading import import_string
+log = logging.getLogger()
Review comment:
Use `getLogger(__name__)` to give the logger a name. Otherwise it returns the root logger, making the log messages very difficult to manage.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-870093163
Thanks for the PR, can you fix the static checks and rebase on latest main @fhoda
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-870377858
> Currently Airflow does not check the default secrets backends (env and metastore db) if there is any sort of connection related error to an Alternative Backend, causing related tasks to fail. The change proposed here allows it to fail over to checking the default backends when this happens.
This leads to some other possible bugs -- if you have _different_ connection info in the DB to the external secrets store then _sometimes_/somewhat randomly you would end up with different crednetials returned.
I'm not sure that is a good behaviour...
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-860131588
I believe the conclusion of our discussion was that in case of `get_config` use where Secrets Backend is missing-in-action we should raise an Exception. I gues It should be added here (including 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] kaxil commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-870093163
Thanks for the PR, can you fix the static checks and rebase on latest main @fhoda
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] fhoda commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
fhoda commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-861797812
Ah yes, got it. Agree, was planning to add tests for all changes once I got feedback on the approach. I will implement your suggestion for `get_config` as well and then add 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 pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-870386738
> I'm not sure that is a good behaviour...
> If the goal is to protect against a misconfigured secrets backend only, then we should change it to test the settings somehow in get_custom_secret_backend, and if it's not working there, to not use it, rather than to put the try/except around every use of the secrets backends.
I quite agree. Just a bit of context @ashb
We had big discussion about it in https://github.com/apache/airflow/issues/14592 and my original "extreme" statement was that we should crash Airflow if secret backend is unreachable (I had same concern).
Finally we reached a bit "softer" approach (which might allow the tasks to survive secret manager temporary unavailability if the user decides that this is the case). We decideed the crash should only happen when you read configuration (because there are default fallback values which might change airflow behaviour), but there might be some valid cases of fallback for connections or variables.
The idea here is that the user **might** then implement scenarios where the fallback values will be OK. It requires a deliberate action from the user - of defining both variable and connection fallback. There are no "defaults" for neither connections nor variables, so unless the users explicitly defines them, the task will not proceed/fail in case secret backend is unreachable (there will be no connection nor variable to use). While this is not as "clean" as ('crash whenever the backend is not available'), it makes sense to be implemented this way.
I'd love to hear your thoughts about it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-870386738
> I'm not sure that is a good behaviour...
> If the goal is to protect against a misconfigured secrets backend only, then we should change it to test the settings somehow in get_custom_secret_backend, and if it's not working there, to not use it, rather than to put the try/except around every use of the secrets backends.
I quite agree. Just a bit of context @ashb
We had big discussion about it in https://github.com/apache/airflow/issues/14592 and my original "extreme" statement was that we should crash Airflow if secret backend is unreachable (I had same concern).
Finally we reached a bit "softer" approach (which might allow the tasks to survive secret manager temporary unavailability if the user decides that this is the case). We decideed the crash should only happen when you read configuration (because there are default fallback values which might change airflow behaviour), but there might be some valid cases of fallback for connections or variables.
The idea here is that the user **might** then implement scenarios where the fallback values will be OK. It requires a deliberate action from the user - of defining both variable and connection fallback. There are no "defaults" for neither connections nor variables, so unless the users explicitly defines them, the task will not proceed/fail in case secret backend is unreachable (there will be no connection nor variable to use). While this is not as "clean" as ('crash whenever the backend is not available'), it makes sense to be implemented this way.
I'd love to hear your thoughts about it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#discussion_r670028958
##########
File path: airflow/models/variable.py
##########
@@ -201,7 +201,13 @@ def get_variable_from_secrets(key: str) -> Optional[str]:
:return: Variable Value
"""
for secrets_backend in ensure_secrets_loaded():
- var_val = secrets_backend.get_variable(key=key)
- if var_val is not None:
- return var_val
+ try:
+ var_val = secrets_backend.get_variable(key=key)
+ if var_val is not None:
+ return var_val
+ except Exception: # pylint: disable=broad-except
+ log.exception(
+ 'Unable to retrieve variable from alternative secrets backend. '
Review comment:
This will raise the same error if there is any error getting variable from default secret backends (env var and db).
Let's separate this PR into 2:
1) GCP Change in airflow/providers/google/cloud/secrets/secret_manager.py
2) Broader Secrets Failover Change (with optional retries controlled via airflow.cfg 🤷 - that can even be a 3rd PR )
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#discussion_r660197362
##########
File path: airflow/configuration.py
##########
@@ -81,10 +81,18 @@ def run_command(command):
def _get_config_value_from_secret_backend(config_key):
"""Get Config option values from Secret Backend"""
- secrets_client = get_custom_secret_backend()
- if not secrets_client:
- return None
- return secrets_client.get_config(config_key)
+ try:
+ secrets_client = get_custom_secret_backend()
+ if not secrets_client:
+ return None
+ return secrets_client.get_config(config_key)
+ except Exception as e: # pylint: disable=broad-except
+ raise AirflowConfigException(
+ 'Cannot retrieve config from Alternative Secrets Backend. '
Review comment:
Why is Alternative Secrets Backend capitalised? [It is not in the documentation](https://airflow.apache.org/docs/apache-airflow/stable/security/secrets/secrets-backend/index.html).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-870377858
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] fhoda edited a comment on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
fhoda edited a comment on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-861797812
> But I think we should add a better message in this case and a test to cover this scenario explicitly. Then no-one will ever "fix it" thinking it is a bug.
Ah yes, I understand now. Agree, was planning to add tests for all changes once I got feedback on the approach. I will implement your suggestion for `get_config` as well and then add 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] ashb commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-870385305
If the goal is to protect against a _misconfigured_ secrets backend only, then we should change it to test the settings somehow in `get_custom_secret_backend`, and if it's not working there, to not use it, rather than to put the try/except around every use of the secrets backends.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] fhoda commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
fhoda commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-882755191
I will split this PR up and reference accordingly as @kaxil suggested.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#discussion_r650391513
##########
File path: airflow/models/connection.py
##########
@@ -18,6 +18,7 @@
import json
import warnings
+import logging
Review comment:
This won’t pass linters. Please run pre-commit locally and fix the errors first.
##########
File path: airflow/models/connection.py
##########
@@ -391,7 +394,14 @@ def get_connection_from_secrets(cls, conn_id: str) -> 'Connection':
:return: connection
"""
for secrets_backend in ensure_secrets_loaded():
- conn = secrets_backend.get_connection(conn_id=conn_id)
- if conn:
- return conn
+ try:
+ conn = secrets_backend.get_connection(conn_id=conn_id)
+ if conn:
+ return conn
+ except Exception as e:
+ log.warning('Unable to retrieve connection from alternative secret backend:'
+ f'\n{e}\n'
+ 'Checking default secrets backends'
+ )
Review comment:
Do not use f-strings to format log messages. [This blog post explains it well.](https://www.siegescape.com/blog/2019/10/3/be-careful-with-your-f-strings). In this particular instance, it’s best to use `logger.exception()` to also retain the stack trace, which is useful for debugging.
##########
File path: airflow/models/connection.py
##########
@@ -391,7 +394,14 @@ def get_connection_from_secrets(cls, conn_id: str) -> 'Connection':
:return: connection
"""
for secrets_backend in ensure_secrets_loaded():
- conn = secrets_backend.get_connection(conn_id=conn_id)
- if conn:
- return conn
+ try:
+ conn = secrets_backend.get_connection(conn_id=conn_id)
+ if conn:
+ return conn
+ except Exception as e:
+ log.warning('Unable to retrieve connection from alternative secret backend:'
+ f'\n{e}\n'
+ 'Checking default secrets backends'
+ )
Review comment:
Do not use f-strings to format log messages. [This blog post explains it well.](https://www.siegescape.com/blog/2019/10/3/be-careful-with-your-f-strings) In this particular instance, it’s best to use `logger.exception()` to also retain the stack trace, which is useful for debugging.
##########
File path: airflow/providers/google/cloud/secrets/secret_manager.py
##########
@@ -101,9 +104,13 @@ def __init__(
"`connections_prefix`, `variables_prefix` and `sep` should "
f"follows that pattern {SECRET_ID_PATTERN}"
)
- self.credentials, self.project_id = get_credentials_and_project_id(
- keyfile_dict=gcp_keyfile_dict, key_path=gcp_key_path, scopes=gcp_scopes
- )
+ try:
+ self.credentials, self.project_id = get_credentials_and_project_id(
+ keyfile_dict=gcp_keyfile_dict, key_path=gcp_key_path, scopes=gcp_scopes
+ )
+ except Exception as e:
+ log.exception(f'{e}')
Review comment:
`Logger.exception()` already logs the exception, so the `f'{e}'` message is adding nothing useful. You should provide a readable string explaining why the exception happens instead.
--
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 merged pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #16404:
URL: https://github.com/apache/airflow/pull/16404
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#discussion_r670028958
##########
File path: airflow/models/variable.py
##########
@@ -201,7 +201,13 @@ def get_variable_from_secrets(key: str) -> Optional[str]:
:return: Variable Value
"""
for secrets_backend in ensure_secrets_loaded():
- var_val = secrets_backend.get_variable(key=key)
- if var_val is not None:
- return var_val
+ try:
+ var_val = secrets_backend.get_variable(key=key)
+ if var_val is not None:
+ return var_val
+ except Exception: # pylint: disable=broad-except
+ log.exception(
+ 'Unable to retrieve variable from alternative secrets backend. '
Review comment:
This will raise the same error if there is any error getting variable from default secret backends (env var and db).
Let's separate this PR into 2:
1) GCP Change in airflow/providers/google/cloud/secrets/secret_manager.py
2) Broader Secrets Failover Change (with optional retries controlled via airflow.cfg 🤷 )
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#discussion_r660197362
##########
File path: airflow/configuration.py
##########
@@ -81,10 +81,18 @@ def run_command(command):
def _get_config_value_from_secret_backend(config_key):
"""Get Config option values from Secret Backend"""
- secrets_client = get_custom_secret_backend()
- if not secrets_client:
- return None
- return secrets_client.get_config(config_key)
+ try:
+ secrets_client = get_custom_secret_backend()
+ if not secrets_client:
+ return None
+ return secrets_client.get_config(config_key)
+ except Exception as e: # pylint: disable=broad-except
+ raise AirflowConfigException(
+ 'Cannot retrieve config from Alternative Secrets Backend. '
Review comment:
Why is Alternative Secrets Backend capitalised? [It is not in the documentation](https://airflow.apache.org/docs/apache-airflow/stable/security/secrets/secrets-backend/index.html).
##########
File path: airflow/models/connection.py
##########
@@ -35,6 +36,8 @@
from airflow.utils.log.secrets_masker import mask_secret
from airflow.utils.module_loading import import_string
+log = logging.getLogger()
Review comment:
Use `getLogger(__file__)` to give the logger a name. Otherwise it returns the root logger, making the log messages very difficult to manage.
##########
File path: airflow/models/connection.py
##########
@@ -35,6 +36,8 @@
from airflow.utils.log.secrets_masker import mask_secret
from airflow.utils.module_loading import import_string
+log = logging.getLogger()
Review comment:
Use `getLogger(__name__)` to give the logger a name. Otherwise it returns the root logger, making the log messages very difficult to manage.
##########
File path: airflow/providers/google/cloud/secrets/secret_manager.py
##########
@@ -101,9 +106,17 @@ def __init__(
"`connections_prefix`, `variables_prefix` and `sep` should "
f"follows that pattern {SECRET_ID_PATTERN}"
)
- self.credentials, self.project_id = get_credentials_and_project_id(
- keyfile_dict=gcp_keyfile_dict, key_path=gcp_key_path, scopes=gcp_scopes
- )
+ try:
+ self.credentials, self.project_id = get_credentials_and_project_id(
+ keyfile_dict=gcp_keyfile_dict, key_path=gcp_key_path, scopes=gcp_scopes
+ )
+ except (DefaultCredentialsError, FileNotFoundError):
Review comment:
Why is this catch block narrow while others are broard (except Exception)? Can those catching `Exception` be narrowed down?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] fhoda commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
fhoda commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-861764695
> I believe the conclusion of our discussion was that in case of `get_config` use where Secrets Backend is missing-in-action we should raise an Exception. I gues It should be added here (including tests?)
@potiuk For `get_config` currently it causes airflow to crash if it cannot reach the Secrets Backend. This happens when `get_config` is called. If I understood properly we considered this behavior as expected so I left it alone.
--
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] fhoda commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
fhoda commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-882755191
I will split this PR up and reference accordingly as @kaxil suggested.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] fhoda commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
fhoda commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-882755191
I will split this PR up and reference accordingly as @kaxil suggested.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-861789074
> > I believe the conclusion of our discussion was that in case of `get_config` use where Secrets Backend is missing-in-action we should raise an Exception. I gues It should be added here (including tests?)
>
> @potiuk For `get_config` currently it causes airflow to crash if it cannot reach the Secrets Backend. This happens when `get_config` is called. If I understood properly we considered this behavior as expected so I left it alone.
But I think we should add a better message in this case and a test to cover this scenario explicitly. Then no-one will ever "fix it" thinking it is a bug.
--
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] fhoda commented on pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
fhoda commented on pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#issuecomment-859937061
Submitting for initial review. Assuming I will need to make changes based on feedback :)
--
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] fhoda commented on a change in pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
fhoda commented on a change in pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#discussion_r660727668
##########
File path: airflow/providers/google/cloud/secrets/secret_manager.py
##########
@@ -101,9 +106,17 @@ def __init__(
"`connections_prefix`, `variables_prefix` and `sep` should "
f"follows that pattern {SECRET_ID_PATTERN}"
)
- self.credentials, self.project_id = get_credentials_and_project_id(
- keyfile_dict=gcp_keyfile_dict, key_path=gcp_key_path, scopes=gcp_scopes
- )
+ try:
+ self.credentials, self.project_id = get_credentials_and_project_id(
+ keyfile_dict=gcp_keyfile_dict, key_path=gcp_key_path, scopes=gcp_scopes
+ )
+ except (DefaultCredentialsError, FileNotFoundError):
Review comment:
This one is narrow because it is specifically for GCP Secrets Manager, while the others are implemented at at higher level and thus have to account for all alternative secrets backends including custom built ones.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16404: Secrets backend failover
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16404:
URL: https://github.com/apache/airflow/pull/16404#discussion_r660198533
##########
File path: airflow/providers/google/cloud/secrets/secret_manager.py
##########
@@ -101,9 +106,17 @@ def __init__(
"`connections_prefix`, `variables_prefix` and `sep` should "
f"follows that pattern {SECRET_ID_PATTERN}"
)
- self.credentials, self.project_id = get_credentials_and_project_id(
- keyfile_dict=gcp_keyfile_dict, key_path=gcp_key_path, scopes=gcp_scopes
- )
+ try:
+ self.credentials, self.project_id = get_credentials_and_project_id(
+ keyfile_dict=gcp_keyfile_dict, key_path=gcp_key_path, scopes=gcp_scopes
+ )
+ except (DefaultCredentialsError, FileNotFoundError):
Review comment:
Why is this catch block narrow while others are broard (except Exception)? Can those catching `Exception` be narrowed down?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org