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

Enable Azure Workload Identity to authorize against RabbitMQ manageme… #4657

Merged
merged 4 commits into from
Jun 21, 2023

Conversation

kratkyzobak
Copy link
Contributor

@kratkyzobak kratkyzobak commented Jun 7, 2023

This PR provides support for Azure AD Workload Identity authorization against RabbitMQ HTTP API in RabbitMQ scaler. This utilizes generic OIDC support introduced in RabbitMQ 3.11

Checklist

Fixes #4716

Relates to #

@kratkyzobak kratkyzobak requested a review from a team as a code owner June 7, 2023 19:18
@kratkyzobak
Copy link
Contributor Author

I'm currently not sure how to provide e2e test for this. I have checked, that workload identity are not checked even in most of azure scalers. To provide e2e test, there would need to be Azure AD app registration in test environment. I can provide with details about configuring this app registration in test tenant if needed.

@kratkyzobak kratkyzobak force-pushed the rabbitmq-aad-wi branch 4 times, most recently from 8397cdd to 9bbcf85 Compare June 8, 2023 14:17
@zroubalik
Copy link
Member

I'm currently not sure how to provide e2e test for this. I have checked, that workload identity are not checked even in most of azure scalers. To provide e2e test, there would need to be Azure AD app registration in test environment. I can provide with details about configuring this app registration in test tenant if needed.

@JorTurFer can help with this one, most likely 😄

@JorTurFer
Copy link
Member

I have checked, that workload identity are not checked even in most of azure scalers.

What do you mean? I believe that all the Azure scalers are tested without any pod identity, with AAD pod identity and also with workload identity.
During the e2e startup, workload identity (and others) is deployed.

What would you need for adding the e2e test? I guess that you need an Azure App registration which exposes an api, and granting permissions over that api, am I right?

We have a repository for managing e2e test infrastructure using terraform (the repo is testing-infrastructure), if you open a PR there adding whatever you need, it will be available for e2e tests. We can help you too if you explain us what do you need exactly

@kratkyzobak kratkyzobak closed this Jun 9, 2023
@kratkyzobak kratkyzobak reopened this Jun 9, 2023
@kratkyzobak
Copy link
Contributor Author

kratkyzobak commented Jun 9, 2023

What would you need for adding the e2e test? I guess that you need an Azure App registration which exposes an api, and granting permissions over that api, am I right?

We have a repository for managing e2e test infrastructure using terraform (the repo is testing-infrastructure), if you open a PR there adding whatever you need, it will be available for e2e tests. We can help you too if you explain us what do you need exactly

Yeah, I would need application registration in Azure AD to use as OAuth scope. I will prepare PR into testing-infrastructure with proper azuread_application (would be almost same as rabbit-api resource mentoined here).

Since there is no azuread_* resource within whole testing-infrastructure project, but azuread provider is registered - will SP running github action have right to make app registration, create enterprise app (azuread_service_principal) and assign role of this app to some of managed identity (azuread_app_role_assignment)?

@JorTurFer
Copy link
Member

Since there is no azuread_* resource within whole testing-infrastructure project, but azuread provider is registered - will SP running github action have right to make app registration, create enterprise app (azuread_service_principal) and assign role of this app to some of managed identity (azuread_app_role_assignment)?

I think so, but if it couldn't, we can escalate the SP permissions

@kratkyzobak
Copy link
Contributor Author

I have added e2e tests, but it needs to wait until resolve kedacore/testing-infrastructure#114 (comment). With infrastructure locally created using terraform-infrastructure, added E2E tests are ok.

OT: Are there any logs of previous e2e tests runs available publicly? I cannot find any and I cannot understand how would current rabbitmq tests work. There were missing imports (so tests wont compile), wrong .env file was loaded (probably not problem in Actions runner). Or am I missing something?

@JorTurFer
Copy link
Member

JorTurFer commented Jun 16, 2023

/run-e2e rabbit
Update: You can check the progress here

@JorTurFer
Copy link
Member

Hi,
You can see the logs clicking in the comment:
image

kratkyzobak added a commit to kratkyzobak/keda-docs that referenced this pull request Jun 16, 2023
@kratkyzobak kratkyzobak force-pushed the rabbitmq-aad-wi branch 2 times, most recently from 40b1bd6 to 67a295e Compare June 16, 2023 12:53
@JorTurFer
Copy link
Member

Hi @kratkyzobak ,
DCO check is failing and that's because there is some commit not signed. Could you fix it?
https://github.com/kedacore/keda/blob/main/CONTRIBUTING.md#i-didnt-sign-my-commit-now-what

Jakub Adamus and others added 2 commits June 17, 2023 20:31
@kratkyzobak
Copy link
Contributor Author

@JorTurFer fixed. Sorry for that. I used rebase button in Github, but I have different email locally than in Github account.

@zroubalik
Copy link
Member

zroubalik commented Jun 18, 2023

/run-e2e rabbit
Update: You can check the progress here

CHANGELOG.md Outdated Show resolved Hide resolved
Add missing issue link

Signed-off-by: KratkyZobak <[email protected]>
@zroubalik zroubalik enabled auto-merge (squash) June 21, 2023 09:46
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM

Díky!

@zroubalik zroubalik disabled auto-merge June 21, 2023 10:21
@zroubalik zroubalik merged commit 39d6094 into kedacore:main Jun 21, 2023
SpiritZhou pushed a commit to SpiritZhou/keda that referenced this pull request Jun 30, 2023
kedacore#4657)

Signed-off-by: Jakub Adamus <[email protected]>
Signed-off-by: KratkyZobak <[email protected]>
Co-authored-by: Jakub Adamus <[email protected]>
Co-authored-by: Jakub Adamus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Azure Workload Identity in RabbitMQ scaler
3 participants