Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for service account impersonation with computeEngineSSHHook (google provider) and IAP tunnel #35136

Merged
merged 7 commits into from
Nov 25, 2023

Conversation

ginolegigot
Copy link
Contributor

@ginolegigot ginolegigot commented Oct 23, 2023

Hello !
This PR aims to solve the issue where we can't use service account impersonation when we want to connect with ssh between 2 Google Compute Engine instances.
It is a new version of an old PR i submitted but i did not have the proper specs/money/time to test the code. Now I have them, am willing to learn on the testing side given how newbie i am on this part.
I'll check the airflow developers guideline again meanwhile, i submit this piece of code which add service account impersonation support for ssh connections between 2 GCE instances using IAP tunneling

@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Oct 23, 2023
@potiuk
Copy link
Member

potiuk commented Oct 23, 2023

It will need unit test. We rarely accept changes without corresponding unit tests - mostly when they are difficult to write. This one seems quite reasonable.

@ginolegigot
Copy link
Contributor Author

Roger!
I'll try to understand how to write some unit tests in my particular context using airflow tests libs

@Taragolis
Copy link
Contributor

The simplest way have a look how it done in other Google Provider test, for example

class TestGoogleAnalyticsRetrieveAdsLinksListOperator:
@mock.patch("airflow.providers.google.marketing_platform.operators.analytics.GoogleAnalyticsHook")
def test_execute(self, hook_mock):
op = GoogleAnalyticsRetrieveAdsLinksListOperator(
account_id=ACCOUNT_ID,
web_property_id=WEB_PROPERTY_ID,
api_version=API_VERSION,
gcp_conn_id=GCP_CONN_ID,
task_id="test_task",
impersonation_chain=IMPERSONATION_CHAIN,
)
op.execute(context=None)
hook_mock.assert_called_once()
hook_mock.return_value.list_ad_words_links.assert_called_once()
hook_mock.assert_called_once_with(
gcp_conn_id=GCP_CONN_ID,
api_version=API_VERSION,
impersonation_chain=IMPERSONATION_CHAIN,
)
hook_mock.return_value.list_ad_words_links.assert_called_once_with(
account_id=ACCOUNT_ID, web_property_id=WEB_PROPERTY_ID
)

L65: Provide some value to operator
L73: Check that hook (mocked) was call with this value

@ginolegigot ginolegigot changed the title Fix service account impersonation with computeEngineSSHHook (google provider) Fix service account impersonation with computeEngineSSHHook (google provider) and IAP tunnel Nov 20, 2023
@ginolegigot
Copy link
Contributor Author

Hey!
I think i understood how to setup a proper airflow dev environment, launch tests and I think my unit test even though it might not optimal at least is functional (first time i code unit test).
One more point, usually an impersonation chain is chosen in google operators, so either a service acount email as a string might be used or a list of strings of service account emails.
Given the particular nature of the ComputeEngineSSHHook, (at least in my understanding), it creates a shell command based on gcloud. And I don't know how to pass multiple service accounts to the "--impersonate-service-account=" argument. I did not test yet (so maybe it's trivial) but i'm not sure that's the scope of this PR. I'm opened to discuss it.

@ginolegigot ginolegigot changed the title Fix service account impersonation with computeEngineSSHHook (google provider) and IAP tunnel Add support for service account impersonation with computeEngineSSHHook (google provider) and IAP tunnel Nov 22, 2023
@ginolegigot
Copy link
Contributor Author

Hello @potiuk and @Taragolis!
May i have a review ? Or call someone who can do it ?

Copy link
Contributor

@Taragolis Taragolis left a comment

Choose a reason for hiding this comment

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

Not familiar with GCP but look good to me

@eladkal
Copy link
Contributor

eladkal commented Nov 24, 2023

@shahar1 can you take a look?

Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

@eladkal LGTM :)

@ginolegigot
Copy link
Contributor Author

Hello thanks for your review!
What are the next steps ? I'm not familiar with the PR process. Are there additional steps from you ? Or shall i do something in addition ?
I'm afraid the update branch button cancelled the approval sorry for that

@potiuk potiuk merged commit 770f164 into apache:main Nov 25, 2023
Copy link

boring-cyborg bot commented Nov 25, 2023

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

@potiuk
Copy link
Member

potiuk commented Nov 25, 2023

What are the next steps ? I'm not familiar with the PR process. Are there additional steps from you ? Or shall i do something in addition ?

Just wait until PR gets green and merged by a committer.

@ginolegigot
Copy link
Contributor Author

Thanks a lot for your reviews and information.
Great day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants