feat: Provide support for specifying identity to use for Azure managed identity auth #2741

stephaneey · 2022-03-12T17:22:10Z

This PR aims at improving the way pod managed identities are used with Azure Services and Azure Storage in particular. Workloads can specify the identity to be used by the operator when peeking the Azure service

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
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)
Changelog has been updated and is aligned with our changelog requirements

Relates to #2656.

CHANGELOG.md

tomkerkhove · 2022-03-14T08:13:54Z

Would you mind sending a PR for our docs please?

stephaneey · 2022-03-14T08:26:48Z

Would you mind sending a PR for our docs please?

No problem. I was waiting for this PR to move on :)

JorTurFer

First, thanks a ton for this huge effort, not only the code, also the research that you have done.
❤️ ❤️ ❤️

From technological pov I can't say anything because I'm not an expert in Azure Identities, the idea that you proposed in the issue IMO is achieved.

From code and KEDA pov, I left some comments inline and in general I prefer to avoid using magic strings if there is a constant for it.
Things like kedav1alpha1.AuthPodIdentity{Provider: "azure"}} could be kedav1alpha1.AuthPodIdentity{Provider: kedav1alpha1.PodIdentityProviderAzure}}.
It's more verbose, but we avoid magic strings and give us the option to change it in the future if it's needed

CHANGELOG.md

pkg/scalers/azure_eventhub_scaler_test.go

apis/keda/v1alpha1/triggerauthentication_types.go

tomkerkhove · 2022-03-30T08:44:37Z

Let us know if you need any other feedback @stephaneey!

Signed-off-by: Stephane Eyskens <[email protected]>

stephaneey · 2022-03-31T08:01:25Z

@tomkerkhove I applied all the changes on my own branch. Should I make a new PR?

tomkerkhove · 2022-03-31T12:18:51Z

Nope, you can just push them here indeed, thanks!

Would you mind rebasing/merging latest from main to fix the conflicts please?

stephaneey · 2022-03-31T15:00:44Z

Nope, you can just push them here indeed, thanks!

Would you mind rebasing/merging latest from main to fix the conflicts please?

Hey @tomkerkhove

Sorry to bother but I'm not sure what I'm supposed to do here. I applied all the changes to this branch on my own repo: https://github.com/stephaneey/keda/tree/stephaneey/podidentity
I thought of closing this PR and making a new one so that all the latest changes would be part of the new PR. Merging is greyed out for me...I also don't want to do something that violates the DCO (I lost quite some time with this already). So, what am I supposed to to exactly?

Thanks

tomkerkhove · 2022-04-01T08:13:54Z

No need to create a new PR or we will lose the context from this one.

What you should do is:

Add upstream remote to kedacore/keda
Fetch main branch and pull it
Merge changes to your fork's branch

This should help you : https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/merging-an-upstream-repository-into-your-fork

zroubalik · 2022-04-01T12:53:29Z

It's alwasy better to do git pull --rebase instead of merging new changes from main. DCO is fixable (Details on DCO provides a customized command that fixes DCO).
But if you think that it is too complicated, there's no need to open a new PR, you can just force push to your branch :)

Signed-off-by: Stephane Eyskens <[email protected]>

stephaneey · 2022-04-02T10:43:19Z

Hey @tomkerkhove @zroubalik

Thanks for your review. I resolved the conflicts and the PR is being validated.

Best regards

tomkerkhove · 2022-04-02T11:28:06Z

Thanks a ton!

zroubalik

Looking good, only HTTP client usage

pkg/scalers/azure/azure_aad_podidentity.go

Signed-off-by: Stephane Eyskens <[email protected]>

zroubalik · 2022-04-07T19:24:10Z

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

zroubalik

Looking good, but I think we should document this, could you please open docs PR?

CHANGELOG.md

Signed-off-by: Stephane Eyskens <[email protected]>

stephaneey · 2022-04-09T09:51:39Z

Looking good, but I think we should document this, could you please open docs PR?
@tomkerkhove @zroubalik

Where should it be documented? I don't see a "general" category in the docs. Can you elaborate on what is expected to avoid back & forth conversations? I already "documented" the changes in the initial issue #2656. I have forked the docs repo..

Thanks

tomkerkhove · 2022-04-09T10:31:21Z

All docs live in kedacore/keda-docs (see PR description) where we need to document this new feature in how people can configure to use a different identity (not perse how it works under the covers)

tomkerkhove · 2022-04-09T10:32:36Z

Feel free to have a look at our contribution guide.

stephaneey · 2022-04-09T10:38:29Z

Feel free to have a look at our contribution guide.

That's exactly what I did (as mentioned I already cloned the doc repo & created my own branch) and why I come back to ask what is expected. The contribution guide just says that you must open a PR for the doc...On Keda Docs, there are only instructions for:

Adding a new blob post
Adding a new scaler
Adding a new FAQ
Adding a new troubleshooting guidance

So my question is: where should I document my changes, given none of the above category corresponds. The closest one could be a blog post. Do you confirm?

Thanks

tomkerkhove · 2022-04-09T10:52:21Z

No, we already have a documentation page for trigger authentication where you can just extend the pod Identity option for Azure.

https://keda.sh/docs/latest/concepts/authentication/

Just make sure this is only in v2.7.

Every documentation page also has a "suggest a change" button that takes you to the exact file.

tomkerkhove · 2022-04-09T10:53:06Z

The reason why it's not in that section is because there is no dedicated page per auth provider.

But I'll update the guidance to look for the suggest a change button as well if need be.

stephaneey · 2022-04-09T11:13:31Z

Ok thanks, I'll have a look at it

JorTurFer

Some inline comments.
There still are some magic strings, but looks good in general!
Thanks for this

JorTurFer · 2022-04-09T11:32:24Z

CHANGELOG.md

@@ -39,7 +39,7 @@
 - **General**: Support for `ValueMetricType` in `ScaledObject` for all scalers except CPU/Memory ([#2030](https://github.com/kedacore/keda/issues/2030))

 ### Improvements
-
+- **TriggerAuthentication:** Provide support for specifying identity to use for Azure managed identity auth ([#2656](https://github.com/kedacore/keda/issues/2656))


Please, rename this to General,

General: Provide support for specifying identity to use for Azure managed identity auth (#2656)

JorTurFer · 2022-04-09T11:36:08Z

pkg/scalers/azure_eventhub_scaler.go

@@ -149,7 +149,7 @@ func parseAzureEventHubMetadata(config *ScalerConfig) (*eventHubMetadata, error)
 	}
 	meta.eventHubInfo.ActiveDirectoryEndpoint = activeDirectoryEndpoint

-	if config.PodIdentity == "" || config.PodIdentity == v1alpha1.PodIdentityProviderNone {
+	if (config.PodIdentity == kedav1alpha1.AuthPodIdentity{}) || (config.PodIdentity == kedav1alpha1.AuthPodIdentity{Provider: "none"}) {


Replace this magic string (kedav1alpha1.PodIdentityProviderNone)

JorTurFer · 2022-04-09T11:36:59Z

pkg/scalers/azure/azure_eventhub_checkpoint.go

@@ -214,7 +214,7 @@ func (checkpointer *defaultCheckpointer) extractCheckpoint(get *azblob.DownloadR
 }

 func getCheckpoint(ctx context.Context, httpClient util.HTTPDoer, info EventHubInfo, checkpointer checkpointer) (Checkpoint, error) {
-	blobCreds, storageEndpoint, err := ParseAzureStorageBlobConnection(ctx, httpClient, kedav1alpha1.PodIdentityProviderNone, info.StorageConnection, "", "")
+	blobCreds, storageEndpoint, err := ParseAzureStorageBlobConnection(ctx, httpClient, kedav1alpha1.AuthPodIdentity{Provider: "none"}, info.StorageConnection, "", "")


Replace this magic string (kedav1alpha1.PodIdentityProviderNone)

JorTurFer · 2022-04-09T11:37:08Z

pkg/scalers/azure/azure_eventhub_test.go

@@ -339,7 +340,7 @@ func TestShouldParseCheckpointForGoSdk(t *testing.T) {
 func createNewCheckpointInStorage(urlPath string, containerName string, partitionID string, checkpoint string, metadata map[string]string) (context.Context, error) {
 	ctx := context.Background()

-	credential, endpoint, _ := ParseAzureStorageBlobConnection(ctx, http.DefaultClient, "none", StorageConnectionString, "", "")
+	credential, endpoint, _ := ParseAzureStorageBlobConnection(ctx, http.DefaultClient, kedav1alpha1.AuthPodIdentity{Provider: "none"}, StorageConnectionString, "", "")


Replace this magic string (kedav1alpha1.PodIdentityProviderNone)

JorTurFer · 2022-04-09T11:37:29Z

pkg/scalers/azure_app_insights_scaler_test.go

@@ -216,7 +216,7 @@ func TestAzureAppInsightsGetMetricSpecForScaling(t *testing.T) {
 			}
 			mockAzureAppInsightsScaler := azureAppInsightsScaler{
 				metadata:    meta,
-				podIdentity: kedav1alpha1.PodIdentityProviderAzure,
+				podIdentity: kedav1alpha1.AuthPodIdentity{Provider: "azure"},


Replace this magic string)

JorTurFer · 2022-04-09T11:38:02Z

pkg/scalers/azure_log_analytics_scaler.go

@@ -165,8 +165,8 @@ func parseAzureLogAnalyticsMetadata(config *ScalerConfig) (*azureLogAnalyticsMet
 		meta.clientSecret = clientSecret

 		meta.podIdentity = ""
-	case kedav1alpha1.PodIdentityProviderAzure:
-		meta.podIdentity = string(config.PodIdentity)
+	case kedav1alpha1.AuthPodIdentity{Provider: "azure"}:


Replace this magic string

JorTurFer · 2022-04-09T11:38:20Z

pkg/scalers/azure_monitor_scaler.go

@@ -176,7 +176,7 @@ func parseAzureMonitorMetadata(config *ScalerConfig) (*azureMonitorMetadata, err

 // parseAzurePodIdentityParams gets the activeDirectory clientID and password
 func parseAzurePodIdentityParams(config *ScalerConfig) (clientID string, clientPassword string, err error) {
-	if config.PodIdentity == "" || config.PodIdentity == kedav1alpha1.PodIdentityProviderNone {
+	if (config.PodIdentity == kedav1alpha1.AuthPodIdentity{}) || (config.PodIdentity == kedav1alpha1.AuthPodIdentity{Provider: "none"}) {


Replace this magic string

JorTurFer · 2022-04-09T11:38:42Z

pkg/scalers/azure_queue_scaler.go

 		if val == "true" {
-			config.PodIdentity = kedav1alpha1.PodIdentityProviderAzure
+			config.PodIdentity = kedav1alpha1.AuthPodIdentity{Provider: "azure"}


Replace this magic string

JorTurFer · 2022-04-09T11:39:15Z

pkg/scaling/resolver/scale_resolvers_test.go

@@ -241,7 +241,7 @@ func TestResolveAuthRef(t *testing.T) {
 			},
 			soar:                &kedav1alpha1.ScaledObjectAuthRef{Name: triggerAuthenticationName},
 			expected:            map[string]string{"host": secretData},
-			expectedPodIdentity: kedav1alpha1.PodIdentityProviderNone,
+			expectedPodIdentity: kedav1alpha1.AuthPodIdentity{Provider: "none"},


Replace this magic string

JorTurFer · 2022-04-09T11:39:38Z

pkg/scaling/resolver/scale_resolvers.go

 			authParams["awsRoleArn"] = podTemplateSpec.ObjectMeta.Annotations[kedav1alpha1.PodIdentityAnnotationKiam]
 		}
 		return authParams, podIdentity, nil
 	}

 	authParams, _ := resolveAuthRef(ctx, client, logger, triggerAuthRef, nil, namespace)
-	return authParams, kedav1alpha1.PodIdentityProviderNone, nil
+	return authParams, kedav1alpha1.AuthPodIdentity{Provider: "none"}, nil


Replace this magic string

tomkerkhove · 2022-04-25T11:15:38Z

Feel free to let us know if you need any help @stephaneey

tomkerkhove · 2022-05-04T07:12:14Z

Bump @stephaneey, anything we can help you with?

Do note that our workload identity support is close as well - #2907

stephaneey · 2022-05-05T05:17:38Z

Bump @stephaneey, anything we can help you with?

Do note that our workload identity support is close as well - #2907

Hi Tom,

Indeed, sorry for the late reply. I guess we'd better discard the PR with the arrival of workload identities and the low value added to the product.

Best Regards

tomkerkhove · 2022-05-05T05:27:17Z

Oh I don't think this is low value add as it allows for identity segregation but maybe you don't have time to work on it anymore?

stephaneey · 2022-05-05T05:47:25Z

Oh I don't think this is low value add as it allows for identity segregation but maybe you don't have time to work on it anymore?

Well, indeed, it's a bit tricky these days and I spent more time in fixing conflicts than writing the actual code. The review process takes time (it's normal and even a very good practice to ensure high quality), but this leads to new conflicts each time I adjust the changes to the remarks, so it's a never ending story and I don't have much time to allocate (my bad). I don't know if someone can take this over, else, it was a nice attempt :)

tomkerkhove · 2022-05-05T12:18:48Z

No problem at all - I will merge this in to azure-identity-seperation branch and hopefully somebody else can pick this up.

Thanks for the initial start.

Oh I don't think this is low value add as it allows for identity segregation but maybe you don't have time to work on it anymore?

Well, indeed, it's a bit tricky these days and I spent more time in fixing conflicts than writing the actual code. The review process takes time (it's normal and even a very good practice to ensure high quality), but this leads to new conflicts each time I adjust the changes to the remarks, so it's a never ending story and I don't have much time to allocate (my bad). I don't know if someone can take this over, else, it was a nice attempt :)

We are always happy to hear feedback on how we can improve though

stephaneey requested a review from a team as a code owner March 12, 2022 17:22

stephaneey force-pushed the stephaneey/podidentity branch from 09e3b24 to 15d5205 Compare March 13, 2022 06:54

tomkerkhove changed the title ~~Stephaneey/podidentity~~ feat: Provide support for specifying identity to use for Azure managed identity auth Mar 14, 2022

tomkerkhove reviewed Mar 14, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

zroubalik requested a review from JorTurFer March 22, 2022 20:17

JorTurFer requested changes Mar 22, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/scalers/azure_eventhub_scaler_test.go Outdated Show resolved Hide resolved

pkg/scalers/azure_eventhub_scaler_test.go Outdated Show resolved Hide resolved

apis/keda/v1alpha1/triggerauthentication_types.go Outdated Show resolved Hide resolved

tomkerkhove mentioned this pull request Mar 30, 2022

Add Azure Data Explorer e2e tests with active directory credentials. #2841

Closed

PR 2741 review. Signed-off-by: Stephane Eyskens <[email protected]>

53f17e0

Signed-off-by: Stephane Eyskens <[email protected]>

stephaneey force-pushed the stephaneey/podidentity branch from f29b10d to 53f17e0 Compare March 31, 2022 07:56

Adjusted changedlog PR 2741.

16da7f2

Signed-off-by: Stephane Eyskens <[email protected]>

yardenoff mentioned this pull request Mar 31, 2022

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

Closed

4 tasks

stephaneey added 2 commits April 2, 2022 10:57

Synced main branch and resolved conflicts

d6cbd90

Signed-off-by: Stephane Eyskens <[email protected]>

Adjusted data explorer scaler to fit with managed identities

438df9c

Signed-off-by: Stephane Eyskens <[email protected]>

tomkerkhove requested a review from a team April 2, 2022 11:27

zroubalik reviewed Apr 6, 2022

View reviewed changes

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

Used kedautil.httpClient

cc6bcc7

Signed-off-by: Stephane Eyskens <[email protected]>

zroubalik reviewed Apr 8, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Moved changed to general in changelog

2f34c33

Signed-off-by: Stephane Eyskens <[email protected]>

JorTurFer reviewed Apr 9, 2022

View reviewed changes

tomkerkhove changed the base branch from main to azure-identity-seperation May 5, 2022 12:17

Merge branch 'azure-identity-seperation' into stephaneey/podidentity

3aa5556

tomkerkhove merged commit abc1991 into kedacore:azure-identity-seperation May 5, 2022

feat: Provide support for specifying identity to use for Azure managed identity auth #2741

feat: Provide support for specifying identity to use for Azure managed identity auth #2741

Conversation

stephaneey commented Mar 12, 2022 • edited by zroubalik Loading

Checklist

tomkerkhove commented Mar 14, 2022

stephaneey commented Mar 14, 2022

JorTurFer left a comment

Choose a reason for hiding this comment

tomkerkhove commented Mar 30, 2022

stephaneey commented Mar 31, 2022

tomkerkhove commented Mar 31, 2022

stephaneey commented Mar 31, 2022

tomkerkhove commented Apr 1, 2022

zroubalik commented Apr 1, 2022

stephaneey commented Apr 2, 2022

tomkerkhove commented Apr 2, 2022

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik commented Apr 7, 2022 • edited by github-actions bot Loading

zroubalik left a comment

Choose a reason for hiding this comment

stephaneey commented Apr 9, 2022

tomkerkhove commented Apr 9, 2022

tomkerkhove commented Apr 9, 2022

stephaneey commented Apr 9, 2022

tomkerkhove commented Apr 9, 2022

tomkerkhove commented Apr 9, 2022

stephaneey commented Apr 9, 2022

JorTurFer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomkerkhove commented Apr 25, 2022

tomkerkhove commented May 4, 2022 • edited Loading

stephaneey commented May 5, 2022

tomkerkhove commented May 5, 2022

stephaneey commented May 5, 2022

tomkerkhove commented May 5, 2022

stephaneey commented Mar 12, 2022 •

edited by zroubalik

Loading

zroubalik commented Apr 7, 2022 •

edited by github-actions bot

Loading

tomkerkhove commented May 4, 2022 •

edited

Loading