You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ying-w (via GitHub)" <gi...@apache.org> on 2024/03/15 05:31:54 UTC
[PR] add impersonation_scopes [airflow]
ying-w opened a new pull request, #38169:
URL: https://github.com/apache/airflow/pull/38169
closes: https://github.com/apache/airflow/issues/33400
Currently `scopes` for google libraries are taken from connection parameters, this PR creates an additional parameter `impersonation_scopes` that will be used in situations when `impersonation_chain` is being used.
The flow to get credentials for gcp providers goes something like this
1. A library like bigquery will call get_credentials() https://github.com/apache/airflow/blob/00cd44e4ccb4a7d6406e75d3cf8f80277545d5d2/airflow/providers/google/cloud/hooks/bigquery.py#L163
2. This is provided by GoogleBaseHook https://github.com/apache/airflow/blob/00cd44e4ccb4a7d6406e75d3cf8f80277545d5d2/airflow/providers/google/common/hooks/base_google.py#L307
3. This then makes a call to `self.scopes()` https://github.com/apache/airflow/blob/00cd44e4ccb4a7d6406e75d3cf8f80277545d5d2/airflow/providers/google/common/hooks/base_google.py#L290
4. scopes will then return scopes from connection extras or default (in `_get_scopes()`) https://github.com/apache/airflow/blob/00cd44e4ccb4a7d6406e75d3cf8f80277545d5d2/airflow/providers/google/common/hooks/base_google.py#L414
Possible alternative solutions
1. Override `scopes()` definition in custom hook - as detailed in above issue
2. Introduce a parameter called `scopes` - it would be confusing what takes precedence if there is scopes in parameter and connection
I would need some help testing and bringing this over the finish line
Should be able to do something like
```py
bq_hook = BigQueryHook(
gcp_conn_id="my_con",
impersonation_chain="service-account@myproject.iam.gserviceaccount.com",
use_legacy_sql=False,
location="us",
api_resource_configs={"query": {"useQueryCache": False, "priority": "BATCH"}},
scopes = (
"https://www.googleapis.com/auth/cloud-platform",
"https://www.googleapis.com/auth/drive",
),
)
bq_hook.scopes # check that it got set
```
--
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
Re: [PR] Add `impersonation_scopes` to BigQuery [airflow]
Posted by "ying-w (via GitHub)" <gi...@apache.org>.
ying-w commented on code in PR #38169:
URL: https://github.com/apache/airflow/pull/38169#discussion_r1527202747
##########
airflow/providers/google/common/hooks/base_google.py:
##########
@@ -190,6 +190,7 @@ class GoogleBaseHook(BaseHook):
If set as a sequence, the identities from the list must grant
Service Account Token Creator IAM role to the directly preceding identity, with first
account from the list granting this role to the originating account.
+ :param impersonation_scopes: Optional list of scopes for impersonated account. Will override scopes from connection.
Review Comment:
i'm glad that splitting providers is still be considered. just pushed the changes
--
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
Re: [PR] Add `impersonation_scopes` to BigQuery [airflow]
Posted by "ying-w (via GitHub)" <gi...@apache.org>.
ying-w commented on PR #38169:
URL: https://github.com/apache/airflow/pull/38169#issuecomment-2011174045
I need some help figuring out how to write a test. I see that there is a cloud/operators/test_bigquery.py file with a `impersonation_chain` call but no `impersonation_chain` call in cloud/hooks/test_bigquery.py
The other example I looked at https://github.com/apache/airflow/blob/952d04a3c10a7b511e4c39528d6f7aca7d729df6/tests/providers/google/cloud/operators/test_cloud_storage_transfer_service.py#L265
but I'm not quite sure what that line is doing
--
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
Re: [PR] Add `impersonation_scopes` to BigQuery [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #38169:
URL: https://github.com/apache/airflow/pull/38169#issuecomment-2013463439
> cloud/hooks/test_bigquery.py
> I need some help figuring out how to write a test. I see that there is a cloud/operators/test_bigquery.py file with a `impersonation_chain` call but no `impersonation_chain` call in cloud/hooks/test_bigquery.py
>
> The other example I looked at
>
> https://github.com/apache/airflow/blob/952d04a3c10a7b511e4c39528d6f7aca7d729df6/tests/providers/google/cloud/operators/test_cloud_storage_transfer_service.py#L265
>
>
> but I'm not quite sure what that line is doing
You could mock the impersonation chain as done in other tests of GCP hooks (`hooks/test_compute`, for example; there are more).
As for the other example, it is called `google_impersonation_chain` to differentiate it from a similar parameter in the target/source cloud platform of the transfer operator (which is not necessarily GCP).
--
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
Re: [PR] Add `impersonation_scopes` to BigQuery [airflow]
Posted by "ying-w (via GitHub)" <gi...@apache.org>.
ying-w commented on code in PR #38169:
URL: https://github.com/apache/airflow/pull/38169#discussion_r1527197740
##########
airflow/providers/google/common/hooks/base_google.py:
##########
@@ -190,6 +190,7 @@ class GoogleBaseHook(BaseHook):
If set as a sequence, the identities from the list must grant
Service Account Token Creator IAM role to the directly preceding identity, with first
account from the list granting this role to the originating account.
+ :param impersonation_scopes: Optional list of scopes for impersonated account. Will override scopes from connection.
Review Comment:
`impersonation` is a feature of gcp, it is theoretically possible that another gcp service would also need this (searching for 'gcp missing scope' gives examples of other services needing scope but not sure how many relevant for airflow use case).
What I was trying to solve here was a narrowly scoped service account would be able to get impersonate another account and have bigger scope. I would agree though that the most common use case for this is bigquery accessing google sheets. I can switch it over to bigquery library if you think that 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
Re: [PR] Add `impersonation_scopes` to BigQuery [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #38169:
URL: https://github.com/apache/airflow/pull/38169#issuecomment-2002057323
Can you add unit test to cover the change?
--
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
Re: [PR] Add `impersonation_scopes` to BigQuery [airflow]
Posted by "ying-w (via GitHub)" <gi...@apache.org>.
ying-w commented on PR #38169:
URL: https://github.com/apache/airflow/pull/38169#issuecomment-2002927103
i'll do another pass at it sometime this week
--
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
Re: [PR] Add `impersonation_scopes` to BigQuery [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #38169:
URL: https://github.com/apache/airflow/pull/38169
--
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
Re: [PR] add impersonation_scopes [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #38169:
URL: https://github.com/apache/airflow/pull/38169#discussion_r1527139123
##########
airflow/providers/google/common/hooks/base_google.py:
##########
@@ -190,6 +190,7 @@ class GoogleBaseHook(BaseHook):
If set as a sequence, the identities from the list must grant
Service Account Token Creator IAM role to the directly preceding identity, with first
account from the list granting this role to the originating account.
+ :param impersonation_scopes: Optional list of scopes for impersonated account. Will override scopes from connection.
Review Comment:
https://github.com/apache/airflow/issues/33400 reported the problem on BigQuery, then why are you adding it to base google class?
--
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
Re: [PR] Add `impersonation_scopes` to BigQuery [airflow]
Posted by "ying-w (via GitHub)" <gi...@apache.org>.
ying-w commented on PR #38169:
URL: https://github.com/apache/airflow/pull/38169#issuecomment-2027931420
i think i fixed the test
--
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
Re: [PR] Add `impersonation_scopes` to BigQuery [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #38169:
URL: https://github.com/apache/airflow/pull/38169#discussion_r1527205822
##########
airflow/providers/google/common/hooks/base_google.py:
##########
@@ -190,6 +190,7 @@ class GoogleBaseHook(BaseHook):
If set as a sequence, the identities from the list must grant
Service Account Token Creator IAM role to the directly preceding identity, with first
account from the list granting this role to the originating account.
+ :param impersonation_scopes: Optional list of scopes for impersonated account. Will override scopes from connection.
Review Comment:
Composer team is on it https://github.com/apache/airflow/pull/37223#issuecomment-1964943387
--
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
Re: [PR] Add `impersonation_scopes` to BigQuery [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #38169:
URL: https://github.com/apache/airflow/pull/38169#discussion_r1527200404
##########
airflow/providers/google/common/hooks/base_google.py:
##########
@@ -190,6 +190,7 @@ class GoogleBaseHook(BaseHook):
If set as a sequence, the identities from the list must grant
Service Account Token Creator IAM role to the directly preceding identity, with first
account from the list granting this role to the originating account.
+ :param impersonation_scopes: Optional list of scopes for impersonated account. Will override scopes from connection.
Review Comment:
Lets please limit to BigQuery/GCP otherwise it will make things harder for https://github.com/apache/airflow/issues/15933
--
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
Re: [PR] Add `impersonation_scopes` to BigQuery [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #38169:
URL: https://github.com/apache/airflow/pull/38169#discussion_r1527200404
##########
airflow/providers/google/common/hooks/base_google.py:
##########
@@ -190,6 +190,7 @@ class GoogleBaseHook(BaseHook):
If set as a sequence, the identities from the list must grant
Service Account Token Creator IAM role to the directly preceding identity, with first
account from the list granting this role to the originating account.
+ :param impersonation_scopes: Optional list of scopes for impersonated account. Will override scopes from connection.
Review Comment:
Lets please limit to BigQuery otherwise it will make things harder for https://github.com/apache/airflow/issues/15933
--
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