feat: Azure AD Workload Identity support for Azure Scalers and Key Vault #2907

v-shenoy · 2022-04-13T14:35:45Z

Signed-off-by: Vighnesh Shenoy [email protected]

Support for Workload Identity as a pod identity provider.

Related PRs -
Helm Changes - kedacore/charts#263, kedacore/charts#264
Doc changes - kedacore/keda-docs#752

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
Changelog has been updated and is aligned with our changelog requirements
A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
A PR is opened to update the documentation on (repo) (if applicable)

Relates to #2487

tomkerkhove

LGTM, added a few remarks.

Would you mind opening a PR for the docs please?

config/crd/bases/keda.sh_triggerauthentications.yaml

pkg/scalers/azure/azure_eventhub.go

pkg/scalers/azure_monitor_scaler.go

v-shenoy · 2022-04-22T11:19:46Z

LGTM, added a few remarks.

Would you mind opening a PR for the docs please?

Yeah, I have some minor changes left to do. Will get onto the docs after I am done with those.

v-shenoy · 2022-04-22T13:50:02Z

@tomkerkhove @JorTurFer @zroubalik Would be helpful if you can run all the azure* e2e tests.

tomkerkhove · 2022-04-25T05:34:26Z

/run-e2e azure*
Update: You can check the progres here

v-shenoy · 2022-04-25T10:33:06Z

@tomkerkhove I am not sure what changed exactly, but the Application Insight tests started failing recently (even on my dev cluster). Looking at the error, it seemed that the library was not able to send the metrics correctly with just the instrumentation key, and using the connection string resolves the issue.

Could you add AZURE_APP_INSIGHTS_CONNECTION_STRING GitHub secret, and rerun the azure* e2e tests?
(Note - I have already updated the workflow yamls with the env variable.)

tomkerkhove · 2022-04-25T13:17:01Z

PR is open: #2934

JorTurFer · 2022-04-25T13:56:11Z

/run-e2e azure*
Update: You can check the progres here

JorTurFer · 2022-04-25T13:57:14Z

(Note - I have already updated the workflow yamls with the env variable.)

It's not enough, the yaml needs to be in main to be applied. Please undo the changes in the workflow yamls 🙏

v-shenoy · 2022-04-25T14:50:06Z

@tomkerkhove @JorTurFer I want to raise a PR for the documentation here, but before that we would need to finalize upon and complete the helm changes.

I can do the helm changes if someone guides me on how to go about them.

tomkerkhove · 2022-04-26T06:23:30Z

Why does the docs have a pre-requisite on Helm? Normally that should not be the case.

Our Helm charts are available in https://github.com/kedacore/charts where you can just open a PR to change the YAML specs (no need to bump versions) Let me know if you need help.

v-shenoy · 2022-04-26T06:39:43Z

Why does the docs have a pre-requisite on Helm? Normally that should not be the case.

Our Helm charts are available in https://github.com/kedacore/charts where you can just open a PR to change the YAML specs (no need to bump versions) Let me know if you need help.

Similar to pod identity, we need to mention the flags the user can use, right?

v-shenoy · 2022-04-26T06:53:38Z

I will go ahead with the helm changes as per the flags mentioned in this comment. Had a question regarding this. There are other changes in the main KEDA code, that also require helm changes (ex - TriggerAuth / ClusterTriggerAuth CRD changes done for Key Vault). Do I need to copy those changes over as well, right now, or do I just add my changes and the rest will be copied over during the release process?

For context, I only need to change the service account template and values used for helm.

@zroubalik

tomkerkhove · 2022-04-26T06:57:31Z

Why does the docs have a pre-requisite on Helm? Normally that should not be the case.
Our Helm charts are available in kedacore/charts where you can just open a PR to change the YAML specs (no need to bump versions) Let me know if you need help.

Similar to pod identity, we need to mention the flags the user can use, right?

You can already change v2.7 docs; there's no problem there as they only go live when we ship v2.7.

Our new Helm chart version will only be available then as well.

v-shenoy · 2022-04-26T07:05:34Z

You can already change v2.7 docs; there's no problem there as they only go live when we ship v2.7.

No, I was confused about the specific flags to add in docs as I was not sure if we had finalized them :)

v-shenoy · 2022-04-26T07:58:39Z

Helm Changes - kedacore/charts#263, kedacore/charts#264

v-shenoy · 2022-04-26T12:28:47Z

Doc changes - kedacore/keda-docs#752

v-shenoy · 2022-04-26T12:46:09Z

@tomkerkhove Could you once again run the e2e tests? Thanks.

tomkerkhove · 2022-04-26T12:49:09Z

/run-e2e azure*

Update: You can check the progres here

v-shenoy · 2022-04-27T06:48:09Z

No tests seem to be running here, weirdly.

…ccount. Signed-off-by: Vighnesh Shenoy <[email protected]>

Signed-off-by: Vighnesh Shenoy <[email protected]>

JorTurFer · 2022-05-09T19:17:24Z

~~@v-shenoy , rebase main again please, there is a missing line~~
It's there, running

JorTurFer · 2022-05-09T19:18:43Z

/run-e2e azure*
Update: You can check the progres here

JorTurFer · 2022-05-09T19:54:08Z

Checking the cluster, there are resources related with aad-wi even do the e2e tests have finished

Is this expected?

Signed-off-by: Vighnesh Shenoy <[email protected]>

v-shenoy · 2022-05-09T20:27:09Z

Checking the cluster, there are resources related with aad-wi even do the e2e tests have finished Is this expected?

@JorTurFer
I was initially installing these components in the setup file, and removing them during the clean up. I ran into some issues where the tests would randomly fail in this scenario. I wasn't able to figure out what exactly was causing these random failures. So, I removed the clean up, considering we would need the components on the test cluster anyway.

My guess is that the failures were due to the webhook components not being ready by the time keda is deployed. I have added some sleep in the setup section to try and resolve this issue.

You can try running the test again. I would want for the test to at least pass twice before we can begin trusting them.

JorTurFer · 2022-05-09T20:33:09Z

/run-e2e azure*
Update: You can check the progres here

JorTurFer · 2022-05-09T21:30:40Z

/run-e2e azure*
Update: You can check the progres here

JorTurFer · 2022-05-10T05:44:51Z

/run-e2e azure*
Update: You can check the progres here

JorTurFer · 2022-05-10T06:31:49Z

Personally, I don't have problems with keeping aad-wi there because there are e2e clusters and they are for that, but one question. Are both clusters ready (from Azure pov) or only this PR-e2e clusters is ready?
I mean, all needed resources/grants in Azure and their manage identities

JorTurFer

LGTM!
Thanks for this huge effort

v-shenoy · 2022-05-10T06:36:51Z

@JorTurFer I am fine with either, keeping the components in the cluster or cleaning them up. Right now, the tests seem to be working with the clean-up and sleep included, so I guess we can keep them. The reason this test run failed was that both the normal service bus queue and workload identity tests ran together at the same time.

If you want to be sure, you can run just the workload identity tests, regex - *workload-identity*.

As for the permissions / grants, they have been done for both the pr-e2e and main-build clusters. But obviously they have not been run on the other cluster yet, so after merging we will need to check that they work over there as well.

JorTurFer · 2022-05-10T06:51:59Z

The reason this test run failed was that both the normal service bus queue and workload identity tests ran together at the same time.

wow, that's true and I didn't notice! Please use another queue for this test to avoid fails in case of running both at the same time (like it has happened). I mean, changing the queue name should be enough for avoiding the collisions during tests, right?

JorTurFer · 2022-05-10T06:52:23Z

Right now, the tests seem to be working with the clean-up and sleep included, so I guess we can keep them

yes, if it's already the behavior, lets keep it

Signed-off-by: Vighnesh Shenoy <[email protected]>

v-shenoy · 2022-05-10T06:58:46Z

The reason this test run failed was that both the normal service bus queue and workload identity tests ran together at the same time.

wow, that's true and I didn't notice! Please use another queue for this test to avoid fails in case of running both at the same time (like it has happened). I mean, changing the queue name should be enough for avoiding the collisions during tests, right?

Yup, it just kind of slipped my mind that this would be a problem when the tests ran parallelly. Changed.

Could you run the tests? (Hopefully last time, fingers crossed).

JorTurFer · 2022-05-10T11:02:08Z

/run-e2e azure-service-bus*
Update: You can check the progres here

tomkerkhove · 2022-05-10T11:23:46Z

/run-e2e azure*

Update: You can check the progres here

v-shenoy · 2022-05-10T15:16:21Z

@JorTurFer @tomkerkhove Any more work to be done here?

JorTurFer · 2022-05-10T16:02:03Z

hey @v-shenoy
Sorry for the slow answer, I have been busy with the hotfix release. It's all, thanks for the huge effort you have done

v-shenoy mentioned this pull request Apr 13, 2022

Support Azure AD Workload Identity as an identity pod #2487

Closed

v-shenoy force-pushed the azwi branch from eff0e2d to 82e9434 Compare April 22, 2022 09:03

tomkerkhove reviewed Apr 22, 2022

View reviewed changes

config/crd/bases/keda.sh_triggerauthentications.yaml Show resolved Hide resolved

pkg/scalers/azure/azure_eventhub.go Show resolved Hide resolved

pkg/scalers/azure_monitor_scaler.go Show resolved Hide resolved

v-shenoy marked this pull request as ready for review April 22, 2022 13:29

v-shenoy requested a review from a team as a code owner April 22, 2022 13:29

v-shenoy changed the title ~~Azure AD Workload Identity support for Azure Scalers.~~ Azure AD Workload Identity support for Azure Scalers and Key Vault. Apr 22, 2022

v-shenoy force-pushed the azwi branch from 8523c7d to 149abb0 Compare April 25, 2022 10:17

v-shenoy mentioned this pull request Apr 26, 2022

Fix errors in service account template. kedacore/charts#264

Merged

3 tasks

v-shenoy force-pushed the azwi branch from acceedc to e418172 Compare April 26, 2022 12:28

tomkerkhove assigned zroubalik and JorTurFer Apr 26, 2022

tomkerkhove changed the title ~~Azure AD Workload Identity support for Azure Scalers and Key Vault.~~ feat: Azure AD Workload Identity support for Azure Scalers and Key Vault Apr 26, 2022

v-shenoy added 4 commits May 10, 2022 00:43

Use kustomize for setting require labels and annotations on service a…

54b7722

…ccount. Signed-off-by: Vighnesh Shenoy <[email protected]>

Minor changes.

92e0fdb

Signed-off-by: Vighnesh Shenoy <[email protected]>

Add more logs for debugging.

7eb63b7

Signed-off-by: Vighnesh Shenoy <[email protected]>

Add sleep for webhook, remove cleanup code.

da1d3f5

Signed-off-by: Vighnesh Shenoy <[email protected]>

v-shenoy force-pushed the azwi branch from 68eec59 to da1d3f5 Compare May 9, 2022 19:13

Remove debug logs, TODOs, fix changelog.

d0268e5

Signed-off-by: Vighnesh Shenoy <[email protected]>

JorTurFer approved these changes May 10, 2022

View reviewed changes

Change queue name in e2e test to avoid conflict.

1edd2ed

Signed-off-by: Vighnesh Shenoy <[email protected]>

v-shenoy mentioned this pull request May 10, 2022

Better segregation of permissions when using managed identities #2656

Closed

JorTurFer merged commit 98509a8 into kedacore:main May 10, 2022

pinkfloydx33 mentioned this pull request Jul 3, 2022

[FEATURE] Azure Workload Identity Support SparebankenVest/azure-key-vault-to-kubernetes#378

Open

pinkfloydx33 mentioned this pull request Jul 15, 2022

Support Azure Workload Identities kubernetes-sigs/external-dns#2724

Closed

v-shenoy mentioned this pull request Nov 4, 2022

feat: Add support to use pod identities for authentication in Azure Key Vault #3814

Merged

4 tasks

pinkfloydx33 mentioned this pull request Dec 13, 2022

TriggerAuthentication with Azure Key Vault and azure-workload causes Panic/crash after upgrade to 2.9.0 #4010

Closed

feat: Azure AD Workload Identity support for Azure Scalers and Key Vault #2907

feat: Azure AD Workload Identity support for Azure Scalers and Key Vault #2907

Conversation

v-shenoy commented Apr 13, 2022 • edited Loading

Checklist

tomkerkhove left a comment

Choose a reason for hiding this comment

v-shenoy commented Apr 22, 2022

v-shenoy commented Apr 22, 2022

tomkerkhove commented Apr 25, 2022 • edited by github-actions bot Loading

v-shenoy commented Apr 25, 2022 • edited Loading

tomkerkhove commented Apr 25, 2022

JorTurFer commented Apr 25, 2022 • edited by github-actions bot Loading

JorTurFer commented Apr 25, 2022

v-shenoy commented Apr 25, 2022 • edited Loading

tomkerkhove commented Apr 26, 2022

v-shenoy commented Apr 26, 2022

v-shenoy commented Apr 26, 2022 • edited Loading

tomkerkhove commented Apr 26, 2022

v-shenoy commented Apr 26, 2022

v-shenoy commented Apr 26, 2022 • edited Loading

v-shenoy commented Apr 26, 2022

v-shenoy commented Apr 26, 2022

tomkerkhove commented Apr 26, 2022 • edited by github-actions bot Loading

v-shenoy commented Apr 27, 2022

JorTurFer commented May 9, 2022 • edited Loading

JorTurFer commented May 9, 2022 • edited by github-actions bot Loading

JorTurFer commented May 9, 2022 • edited Loading

v-shenoy commented May 9, 2022

JorTurFer commented May 9, 2022 • edited by github-actions bot Loading

JorTurFer commented May 9, 2022 • edited by github-actions bot Loading

JorTurFer commented May 10, 2022 • edited by github-actions bot Loading

JorTurFer commented May 10, 2022 • edited Loading

JorTurFer left a comment

Choose a reason for hiding this comment

v-shenoy commented May 10, 2022 • edited Loading

JorTurFer commented May 10, 2022

JorTurFer commented May 10, 2022

v-shenoy commented May 10, 2022 • edited Loading

JorTurFer commented May 10, 2022 • edited by github-actions bot Loading

tomkerkhove commented May 10, 2022 • edited by github-actions bot Loading

v-shenoy commented May 10, 2022

JorTurFer commented May 10, 2022

v-shenoy commented Apr 13, 2022 •

edited

Loading

tomkerkhove commented Apr 25, 2022 •

edited by github-actions bot

Loading

v-shenoy commented Apr 25, 2022 •

edited

Loading

JorTurFer commented Apr 25, 2022 •

edited by github-actions bot

Loading

v-shenoy commented Apr 25, 2022 •

edited

Loading

v-shenoy commented Apr 26, 2022 •

edited

Loading

v-shenoy commented Apr 26, 2022 •

edited

Loading

tomkerkhove commented Apr 26, 2022 •

edited by github-actions bot

Loading

JorTurFer commented May 9, 2022 •

edited

Loading

JorTurFer commented May 9, 2022 •

edited by github-actions bot

Loading

JorTurFer commented May 9, 2022 •

edited

Loading

JorTurFer commented May 9, 2022 •

edited by github-actions bot

Loading

JorTurFer commented May 9, 2022 •

edited by github-actions bot

Loading

JorTurFer commented May 10, 2022 •

edited by github-actions bot

Loading

JorTurFer commented May 10, 2022 •

edited

Loading

v-shenoy commented May 10, 2022 •

edited

Loading

v-shenoy commented May 10, 2022 •

edited

Loading

JorTurFer commented May 10, 2022 •

edited by github-actions bot

Loading

tomkerkhove commented May 10, 2022 •

edited by github-actions bot

Loading