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/10/22 15:35:57 UTC
[GitHub] [airflow] keze opened a new pull request #19164: Add support in GCP connection for reading key from Secret Manager
keze opened a new pull request #19164:
URL: https://github.com/apache/airflow/pull/19164
Next to two existing ways (key file path and key content) for defining key for GCP connection add support for reading key from Secret Manager. Documentation for creating GCP connection is also updated.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/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/main/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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r736481265
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -283,6 +289,31 @@ def _get_credentials_using_key_path(self):
project_id = credentials.project_id
return credentials, project_id
+ def _get_credentials_using_key_secret_name(self):
+ self._log_debug('Getting connection using JSON key data from GCP secret: %s', self.key_secret_name)
+
+ # Use ADC to access GCP Secret Manager.
+ adc_credentials, adc_project_id = google.auth.default(scopes=self.scopes)
+ secret_manager_client = _SecretManagerClient(credentials=adc_credentials)
Review comment:
Should this user not write their own [custom secret backend](http://airflow.apache.org/docs/apache-airflow/stable/security/secrets/secrets-backend/index.html#roll-your-own-secrets-backend) if they have very specific requirements? Simply put, I'm not sure we need another secret management method. For now, we are supporting the backend secret and why is that not enough?
--
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 a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r736671295
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -283,6 +289,31 @@ def _get_credentials_using_key_path(self):
project_id = credentials.project_id
return credentials, project_id
+ def _get_credentials_using_key_secret_name(self):
+ self._log_debug('Getting connection using JSON key data from GCP secret: %s', self.key_secret_name)
+
+ # Use ADC to access GCP Secret Manager.
+ adc_credentials, adc_project_id = google.auth.default(scopes=self.scopes)
+ secret_manager_client = _SecretManagerClient(credentials=adc_credentials)
Review comment:
I kind of agree with @keze - that this is rather valid use case, I think. Indeed specific to a GCP with metaserver, and there can be in this case some cases where impersonation is aither impossible or not yet implemented. Not the most secure way but I agree that it's a bit more secure than just stroring the SA key directly.
--
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] mik-laj commented on a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r735418664
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -283,6 +289,31 @@ def _get_credentials_using_key_path(self):
project_id = credentials.project_id
return credentials, project_id
+ def _get_credentials_using_key_secret_name(self):
+ self._log_debug('Getting connection using JSON key data from GCP secret: %s', self.key_secret_name)
+
+ # Use ADC to access GCP Secret Manager.
+ adc_credentials, adc_project_id = google.auth.default(scopes=self.scopes)
+ secret_manager_client = _SecretManagerClient(credentials=adc_credentials)
Review comment:
I'm not sure if it's a good idea to store a service account file in Secret Manager. Have you considered using impersonation to change service accounts?
http://airflow.apache.org/docs/apache-airflow/stable/production-deployment.html#impersonate-service-accounts
--
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 merged pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #19164:
URL: https://github.com/apache/airflow/pull/19164
--
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] mik-laj commented on a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r736481265
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -283,6 +289,31 @@ def _get_credentials_using_key_path(self):
project_id = credentials.project_id
return credentials, project_id
+ def _get_credentials_using_key_secret_name(self):
+ self._log_debug('Getting connection using JSON key data from GCP secret: %s', self.key_secret_name)
+
+ # Use ADC to access GCP Secret Manager.
+ adc_credentials, adc_project_id = google.auth.default(scopes=self.scopes)
+ secret_manager_client = _SecretManagerClient(credentials=adc_credentials)
Review comment:
Should this customer write their own [custom secret backend](http://airflow.apache.org/docs/apache-airflow/stable/security/secrets/secrets-backend/index.html#roll-your-own-secrets-backend) if they have very specific requirements? Simply put, I'm not sure we need another secret management method. For now, we are supporting the backend secret and why is that not enough?
--
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 #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r735729920
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -202,20 +204,22 @@ def __init__(
self,
key_path: Optional[str] = None,
keyfile_dict: Optional[Dict[str, str]] = None,
+ key_secret_name: Optional[str] = None,
scopes: Optional[Collection[str]] = None,
delegate_to: Optional[str] = None,
disable_logging: bool = False,
target_principal: Optional[str] = None,
delegates: Optional[Sequence[str]] = None,
) -> None:
super().__init__()
- if key_path and keyfile_dict:
- raise AirflowException(
- "The `keyfile_dict` and `key_path` fields are mutually exclusive. "
- "Please provide only one value."
+ key_options = [key_path, key_secret_name, keyfile_dict]
+ if len([x for x in key_options if x]) > 1:
+ raise AirflowException(f"The `keyfile_dict`, `key_path`, and `key_secret_name` fields"
+ "are all mutually exclusive. Please provide only one value."
)
Review comment:
Use `bool()` instead of `is None` if empty string is a concern
--
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 #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r736119651
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -202,20 +204,22 @@ def __init__(
self,
key_path: Optional[str] = None,
keyfile_dict: Optional[Dict[str, str]] = None,
+ key_secret_name: Optional[str] = None,
scopes: Optional[Collection[str]] = None,
delegate_to: Optional[str] = None,
disable_logging: bool = False,
target_principal: Optional[str] = None,
delegates: Optional[Sequence[str]] = None,
) -> None:
super().__init__()
- if key_path and keyfile_dict:
- raise AirflowException(
- "The `keyfile_dict` and `key_path` fields are mutually exclusive. "
- "Please provide only one value."
+ key_options = [key_path, key_secret_name, keyfile_dict]
+ if len([x for x in key_options if x]) > 1:
+ raise AirflowException(f"The `keyfile_dict`, `key_path`, and `key_secret_name` fields"
+ "are all mutually exclusive. Please provide only one value."
)
Review comment:
Sorry I was probably not clear about the suggestion. False is when I want the exception to be raised. This leaves `True ^ True ^ True` as a special case, where we can catch separately.
But thisking about this again, that entire expression is probably too complex to understand, so this is better.
--
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] keze commented on a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
keze commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r735700176
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -202,20 +204,22 @@ def __init__(
self,
key_path: Optional[str] = None,
keyfile_dict: Optional[Dict[str, str]] = None,
+ key_secret_name: Optional[str] = None,
scopes: Optional[Collection[str]] = None,
delegate_to: Optional[str] = None,
disable_logging: bool = False,
target_principal: Optional[str] = None,
delegates: Optional[Sequence[str]] = None,
) -> None:
super().__init__()
- if key_path and keyfile_dict:
- raise AirflowException(
- "The `keyfile_dict` and `key_path` fields are mutually exclusive. "
- "Please provide only one value."
+ key_options = [key_path, key_secret_name, keyfile_dict]
+ if len([x for x in key_options if x]) > 1:
+ raise AirflowException(f"The `keyfile_dict`, `key_path`, and `key_secret_name` fields"
+ "are all mutually exclusive. Please provide only one value."
)
Review comment:
Actually, provided change is not logically equal to the mine. Zero or exactly one var can be specified and non-empty
--
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 #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r735350989
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -202,20 +204,22 @@ def __init__(
self,
key_path: Optional[str] = None,
keyfile_dict: Optional[Dict[str, str]] = None,
+ key_secret_name: Optional[str] = None,
scopes: Optional[Collection[str]] = None,
delegate_to: Optional[str] = None,
disable_logging: bool = False,
target_principal: Optional[str] = None,
delegates: Optional[Sequence[str]] = None,
) -> None:
super().__init__()
- if key_path and keyfile_dict:
- raise AirflowException(
- "The `keyfile_dict` and `key_path` fields are mutually exclusive. "
- "Please provide only one value."
+ key_options = [key_path, key_secret_name, keyfile_dict]
+ if len([x for x in key_options if x]) > 1:
+ raise AirflowException(f"The `keyfile_dict`, `key_path`, and `key_secret_name` fields"
+ "are all mutually exclusive. Please provide only one value."
)
Review comment:
`(key_path is None) ^ (key_secret_name is None) ^ (keyfile_dict is None)` is probably both easier to understand and _sllightly_ faster to execute.
--
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 #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r735729920
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -202,20 +204,22 @@ def __init__(
self,
key_path: Optional[str] = None,
keyfile_dict: Optional[Dict[str, str]] = None,
+ key_secret_name: Optional[str] = None,
scopes: Optional[Collection[str]] = None,
delegate_to: Optional[str] = None,
disable_logging: bool = False,
target_principal: Optional[str] = None,
delegates: Optional[Sequence[str]] = None,
) -> None:
super().__init__()
- if key_path and keyfile_dict:
- raise AirflowException(
- "The `keyfile_dict` and `key_path` fields are mutually exclusive. "
- "Please provide only one value."
+ key_options = [key_path, key_secret_name, keyfile_dict]
+ if len([x for x in key_options if x]) > 1:
+ raise AirflowException(f"The `keyfile_dict`, `key_path`, and `key_secret_name` fields"
+ "are all mutually exclusive. Please provide only one value."
)
Review comment:
Use `bool()` instead of `is not None` if empty string is a concern (the previous `is None` was a typo; sorry about that)
--
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] keze commented on a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
keze commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r735899996
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -202,20 +204,22 @@ def __init__(
self,
key_path: Optional[str] = None,
keyfile_dict: Optional[Dict[str, str]] = None,
+ key_secret_name: Optional[str] = None,
scopes: Optional[Collection[str]] = None,
delegate_to: Optional[str] = None,
disable_logging: bool = False,
target_principal: Optional[str] = None,
delegates: Optional[Sequence[str]] = None,
) -> None:
super().__init__()
- if key_path and keyfile_dict:
- raise AirflowException(
- "The `keyfile_dict` and `key_path` fields are mutually exclusive. "
- "Please provide only one value."
+ key_options = [key_path, key_secret_name, keyfile_dict]
+ if len([x for x in key_options if x]) > 1:
+ raise AirflowException(f"The `keyfile_dict`, `key_path`, and `key_secret_name` fields"
+ "are all mutually exclusive. Please provide only one value."
)
Review comment:
No I didn't mean that. If two vars have some value it wouldn't throw exception and it should, because they are mutually exclusive. (e.g there can not be key_path and key_secret_name set, but in your case with XORs would be false and wouldn't throw exception)
--
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] mik-laj commented on a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r735418664
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -283,6 +289,31 @@ def _get_credentials_using_key_path(self):
project_id = credentials.project_id
return credentials, project_id
+ def _get_credentials_using_key_secret_name(self):
+ self._log_debug('Getting connection using JSON key data from GCP secret: %s', self.key_secret_name)
+
+ # Use ADC to access GCP Secret Manager.
+ adc_credentials, adc_project_id = google.auth.default(scopes=self.scopes)
+ secret_manager_client = _SecretManagerClient(credentials=adc_credentials)
Review comment:
I'm not sure if it's a good idea to store a service account in Secret Manager. Have you considered using impersonation to change service accounts?
http://airflow.apache.org/docs/apache-airflow/stable/production-deployment.html#impersonate-service-accounts
--
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] mik-laj commented on a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r735418664
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -283,6 +289,31 @@ def _get_credentials_using_key_path(self):
project_id = credentials.project_id
return credentials, project_id
+ def _get_credentials_using_key_secret_name(self):
+ self._log_debug('Getting connection using JSON key data from GCP secret: %s', self.key_secret_name)
+
+ # Use ADC to access GCP Secret Manager.
+ adc_credentials, adc_project_id = google.auth.default(scopes=self.scopes)
+ secret_manager_client = _SecretManagerClient(credentials=adc_credentials)
Review comment:
I'm not sure if it's a good idea to store a service account file in Secret Manager. Have you considered using impersonation to change service accounts? Thanks to this, you will have a long-lived secret that can leak, but instead, you will only use a short-lived token, which will expire very quickly in the event of a leak.
http://airflow.apache.org/docs/apache-airflow/stable/production-deployment.html#impersonate-service-accounts
--
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] keze commented on a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
keze commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r735706319
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -283,6 +289,31 @@ def _get_credentials_using_key_path(self):
project_id = credentials.project_id
return credentials, project_id
+ def _get_credentials_using_key_secret_name(self):
+ self._log_debug('Getting connection using JSON key data from GCP secret: %s', self.key_secret_name)
+
+ # Use ADC to access GCP Secret Manager.
+ adc_credentials, adc_project_id = google.auth.default(scopes=self.scopes)
+ secret_manager_client = _SecretManagerClient(credentials=adc_credentials)
Review comment:
Hi Kamil, I agree with you that using impersonation is better, but we already have two less secure options, key file path to GCS and enconded key data.
This is an important feature request from the Composer customer which wanted a more secure version next to the existing two. Also, currently impersonation doesn't work for all GCP operators
--
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] keze commented on a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
keze commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r736304119
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -283,6 +289,31 @@ def _get_credentials_using_key_path(self):
project_id = credentials.project_id
return credentials, project_id
+ def _get_credentials_using_key_secret_name(self):
+ self._log_debug('Getting connection using JSON key data from GCP secret: %s', self.key_secret_name)
+
+ # Use ADC to access GCP Secret Manager.
+ adc_credentials, adc_project_id = google.auth.default(scopes=self.scopes)
+ secret_manager_client = _SecretManagerClient(credentials=adc_credentials)
Review comment:
> These configuration options are mainly intended for non-GCP environments where you do not have access to the GCP metaserver.
I understand this point, but that is interface we allowed our users to use. One huge customer has its own access control management service (general, not just for airflow) and they want to plug it in and don't want to expose keys, which makes sense.
--
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] github-actions[bot] commented on pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#issuecomment-968369927
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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 pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#issuecomment-950638486
Please fix linter errors. To prevent lenghy wait on CI, please also consider setting up pre-commit locally to make sure the checks pass before pushing.
--
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] mik-laj commented on a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r736061339
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -283,6 +289,31 @@ def _get_credentials_using_key_path(self):
project_id = credentials.project_id
return credentials, project_id
+ def _get_credentials_using_key_secret_name(self):
+ self._log_debug('Getting connection using JSON key data from GCP secret: %s', self.key_secret_name)
+
+ # Use ADC to access GCP Secret Manager.
+ adc_credentials, adc_project_id = google.auth.default(scopes=self.scopes)
+ secret_manager_client = _SecretManagerClient(credentials=adc_credentials)
Review comment:
> key file path to GCS and enconded key data.
These configuration options are mainly intended for non-GCP environments where you do not have access to the GCP metaserver.
> Also, currently impersonation doesn't work for all GCP operators
Only 5 operators are not supported. And `GKEStartPodOperator` is relatively easy to fix. Just add [`--impersonate-service-account`](https://cloud.google.com/sdk/gcloud/reference#--impersonate-service-account) option to the gcloud call.
https://github.com/apache/airflow/blob/87769db98f963338855f59cfc440aacf68e008c9/airflow/providers/google/cloud/operators/kubernetes_engine.py#L342-L352
In the case of Google Dataflow, Apache Beam is a limitation, so we should at least submit a feature request before we start implementing workarounds.
I am also wondering, might it not be a better idea to use [Google Cloud Secret Manager Backend](http://airflow.apache.org/docs/apache-airflow-providers-google/stable/secrets-backends/google-cloud-secret-manager-backend.html)? It allows you to store all the connection in the backend secret, and in case it is not found, it checks the database also.
--
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] keze commented on a change in pull request #19164: Add support in GCP connection for reading key from Secret Manager
Posted by GitBox <gi...@apache.org>.
keze commented on a change in pull request #19164:
URL: https://github.com/apache/airflow/pull/19164#discussion_r736314685
##########
File path: airflow/providers/google/cloud/utils/credentials_provider.py
##########
@@ -283,6 +289,31 @@ def _get_credentials_using_key_path(self):
project_id = credentials.project_id
return credentials, project_id
+ def _get_credentials_using_key_secret_name(self):
+ self._log_debug('Getting connection using JSON key data from GCP secret: %s', self.key_secret_name)
+
+ # Use ADC to access GCP Secret Manager.
+ adc_credentials, adc_project_id = google.auth.default(scopes=self.scopes)
+ secret_manager_client = _SecretManagerClient(credentials=adc_credentials)
Review comment:
> These configuration options are mainly intended for non-GCP environments where you do not have access to the GCP metaserver.
I understand this point, but that is interface we allowed our users to use. One huge customer has its own access control management service (general, not just for airflow) and they want to plug it in and don't want to expose insecurely keys, which makes sense.
As for impersonation, that is completely other topic, we have plan to fix it, it is in our backlog.
Using [Google Cloud Secret Manager Backend](http://airflow.apache.org/docs/apache-airflow-providers-google/stable/secrets-backends/google-cloud-secret-manager-backend.html) instead of `SecretManagerClient` is not possible because it would create infinite recursion of calls of `get_credentials_and_project_id`
https://github.com/apache/airflow/blob/cdbb481338f8a93290e9ca308bf111c3a30fa6ce/airflow/providers/google/cloud/secrets/secret_manager.py#L105
Actually, Google Cloud Secret Manager Backend under the hood uses `SecretManagerClient` in the same way. so this implementation reduces unnecessary coupling
--
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