Skip to content
New issue

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

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

Already on GitHub? # to your account

Add managed identity client id parameter to data explorer scaler #2822

Conversation

yardenoff
Copy link
Contributor

@yardenoff yardenoff commented Mar 25, 2022

Signed-off-by: Yarden Siboni yardensib@gmail.com

Add support for new parameter in trigger metadata for Azure Data Explorer scaler - msiClientId.
This parameter is mandatory when user choose to authenticate through pod identity.

keda-docs PR: kedacore/keda-docs#731

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated and is aligned with our changelog requirements

Fixes #2770

@yardenoff yardenoff requested a review from a team as a code owner March 25, 2022 12:33
@yardenoff yardenoff force-pushed the yasiboni/fixKustoManagedIdentityE2eBug branch from 576daaa to 37bebc1 Compare March 25, 2022 12:36
@yardenoff
Copy link
Contributor Author

Can you please add env var AZURE_MSI_CLIENT_ID to e2e environment?

@@ -5,6 +5,7 @@ import test from 'ava'

const dataExplorerDb = process.env['AZURE_DATA_EXPLORER_DB']
const dataExplorerEndpoint = process.env['AZURE_DATA_EXPLORER_ENDPOINT']
const msiClientId = process.env['AZURE_MSI_CLIENT_ID']
Copy link
Member

Choose a reason for hiding this comment

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

We should add this env variable to worklflows under .github/workflows, eg.

Copy link
Member

Choose a reason for hiding this comment

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

@JorTurFer @tomkerkhove we need to add this variable to GH secrets

Copy link
Member

Choose a reason for hiding this comment

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

As far as I can see, we don't have AAD-Pod-Identity deployed in the clusters (nor in nightly nor pr) and I cannot do it.
If we need it, @tomkerkhove or @ahmelsayed have to do it and create the proper identities

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can see, we don't have AAD-Pod-Identity deployed in the clusters (nor in nightly nor pr) and I cannot do it. If we need it, @tomkerkhove or @ahmelsayed have to do it and create the proper identities

@JorTurFer - Are You sure you don't have aad-pod-identity in your e2e cluster? because one of the e2e scenarios of azure-data-explorer scaler is relying on that and it completed successfully until the bug this pr is going to fix

Signed-off-by: Yarden Siboni <yasiboni@microsoft.com>
Signed-off-by: Yarden Siboni <yasiboni@microsoft.com>
Signed-off-by: Yarden Siboni <yasiboni@microsoft.com>
Signed-off-by: Yarden Siboni <yasiboni@microsoft.com>
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

While the intention here is good, it's only scoped to Azure Data Explorer and think we should abandon it in favor of #2741 which has a nicer fix.

@yardenoff
Copy link
Contributor Author

While the intention here is good, it's only scoped to Azure Data Explorer and think we should abandon it in favor of #2741 which has a nicer fix.

Agree, closing this pr.

@yardenoff yardenoff closed this Mar 31, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure Data Explorer e2e tests are failing
5 participants