Provide support for Azure Key Vault in TriggerAuthentication. #2727

v-shenoy · 2022-03-09T11:37:11Z

Authentication via Azure Key Vault is now supported.
Sample for the new TriggerAuthenticationSpec -

apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: triggerAuthName
  namespace: default
spec:
  azureKeyVault:
    vaultUri: <vault-address>
    credentials:  
      clientId: <azureAD-clientID>
      clientSecret:
        valueFrom:
          secretKeyRef:
            name: <secret-name>
            key: <key-within-secret>
      tenantId: <azureAD-tenantID>
    secrets: 
    - parameter: <param-name-for-authenticating>
      name: <secret-name-in-key-vault>
      version: <secret-version> # Optional

Edit 1 - Updated spec and checklist.
Edit 2- Raised documentation PR.
Edit 3 - Update checklist with tests added.

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

Fixes #900

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

tomkerkhove · 2022-03-09T12:27:01Z

Can we also support managed identity or not?

tomkerkhove · 2022-03-09T12:28:04Z

apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: triggerAuthName
  namespace: default
spec:
  azureKeyVault:
    vaultUri: <vault-address>
    credentials:  
      clientId: <azureAD-clientID>
      clientSecret:
        kubeSecret: <kubernetes-secret-containing-azureAD-secret>
        key: <key-within-the-kubernetes-secret>
      tenantId: <azureAD-tenantID>
    secrets: 
    - parameter: <param-name-for-authenticating>
      name: <secret-name-in-key-vault>
      version: <secret-version> # Optional

Some quick feedback on azureKeyVault.credentials.clientSecret. Can we align with the existing secret ref approach in Kubernetes?

v-shenoy · 2022-03-09T15:18:44Z

Can we also support managed identity or not?

I am not really sure how to go about this. Need some guidance.

v-shenoy · 2022-03-09T15:20:09Z

apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: triggerAuthName
  namespace: default
spec:
  azureKeyVault:
    vaultUri: <vault-address>
    credentials:  
      clientId: <azureAD-clientID>
      clientSecret:
        kubeSecret: <kubernetes-secret-containing-azureAD-secret>
        key: <key-within-the-kubernetes-secret>
      tenantId: <azureAD-tenantID>
    secrets: 
    - parameter: <param-name-for-authenticating>
      name: <secret-name-in-key-vault>
      version: <secret-version> # Optional

Some quick feedback on azureKeyVault.credentials.clientSecret. Can we align with the existing secret ref approach in Kubernetes?

Which secret ref approach are you referring to?

tomkerkhove · 2022-03-09T15:59:18Z

The native Kubernetes approach - https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-environment-variables:

valueFrom:
  secretKeyRef:
    name: mysecret
    key: username

v-shenoy · 2022-03-09T16:08:40Z

The native Kubernetes approach - https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-environment-variables:
valueFrom:
  secretKeyRef:
    name: mysecret
    key: username

So, when you say align with it, you mean just renaming the object to use valueFrom, secretKeyRef instead of clientSecret? Because (and this might be a gap in my knowledge), I am not aware of a way where you can directly just reference secrets from a CRD.

tomkerkhove · 2022-03-09T16:36:45Z

I would go with this:

apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: triggerAuthName
  namespace: default
spec:
  azureKeyVault:
    vaultUri: <vault-address>
    credentials:  
      clientId: <azureAD-clientID>
      clientSecret:
        valueFrom:
          secretKeyRef:
            name: mysecret
            key: username
      tenantId: <azureAD-tenantID>
    secrets: 
    - parameter: <param-name-for-authenticating>
      name: <secret-name-in-key-vault>
      version: <secret-version> # Optional

v-shenoy · 2022-03-09T16:40:02Z

I would go with this:

apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: triggerAuthName
  namespace: default
spec:
  azureKeyVault:
    vaultUri: <vault-address>
    credentials:  
      clientId: <azureAD-clientID>
      clientSecret:
        valueFrom:
          secretKeyRef:
            name: mysecret
            key: username
      tenantId: <azureAD-tenantID>
    secrets: 
    - parameter: <param-name-for-authenticating>
      name: <secret-name-in-key-vault>
      version: <secret-version> # Optional

I'll make the changes.

v-shenoy · 2022-03-09T16:40:53Z

Also, could you point me to where all do I need to make changes for the helm charts (besides the YAMLs for the CRDs)? I am not too familiar with those.

tomkerkhove · 2022-03-09T16:50:37Z

Since this is our data plane I don't think we need any

v-shenoy · 2022-03-09T16:59:59Z

What about these, 02-crd-clustertriggerauthentications.yaml, 05-crd-triggerauthentications.keda.sh.yaml?

tomkerkhove · 2022-03-09T21:18:13Z

CRDs is a different story but I'll need @zroubalik for this.

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

tomkerkhove · 2022-03-10T08:39:50Z

Also, would you mind opening a documentation PR for this nice addition please?

pkg/scaling/resolver/azure_keyvault_handler.go

v-shenoy · 2022-03-10T08:48:06Z

Also, would you mind opening a documentation PR for this nice addition please?

Yeah, I am in the middle of doing it.

v-shenoy · 2022-03-10T08:54:36Z

As the helm charts need to be updated due to the CRD changes, what version, appVersion do I use here?

tomkerkhove · 2022-03-10T09:06:00Z

As the helm charts need to be updated due to the CRD changes, what version, appVersion do I use here?

It's very kind but you don't have to do anything, CRD updates are part of our release process: https://github.com/kedacore/keda/blob/main/RELEASE-PROCESS.MD#6-prepare-our-helm-chart

v-shenoy · 2022-03-10T09:20:01Z

Raised a PR for the documentation - kedacore/keda-docs#704

JorTurFer · 2022-03-10T13:23:24Z

I'm not an expert in Azure Key Vault, but the code looks good.
Could you add an e2e test that uses the Key Vault?
Can we provide it @tomkerkhove?

tomkerkhove · 2022-03-11T07:19:29Z

Certainly, we can store a Service Bus connection string in the vault to ensure it can authenticate to it. That way, we can re-use existing SB tests - What do you think?

JorTurFer · 2022-03-11T07:27:02Z

I'd do another test only for checking this. AFAIR, if the connection string fails, KEDA doesn't create the HPA, we could store one connection string on KeyVault and try to use it. Or maybe doing the query manually just for checking that response is not an error.

But I'd go with another test

tomkerkhove · 2022-03-11T12:27:26Z

Let's do this:

add unit tests in this PR
add e2e tests for key vault in this PR
add e2e test using key vault with Azure storage scaler
I will open issue for service bus e2e tests as follow-up by somebody having time

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

v-shenoy · 2022-03-12T10:14:13Z

I don't think it makes sense to have unit test considering the AzureKeyVaultHandler is only initialized and just fetches secrets. Have added the e2e test using queue trigger. Please take a look. @tomkerkhove @JorTurFer @zroubalik

tomkerkhove · 2022-03-14T07:16:23Z

I don't think it makes sense to have unit test considering the AzureKeyVaultHandler is only initialized and just fetches secrets.

I don't think this can hurt, no?

v-shenoy · 2022-03-14T07:23:03Z

The struct exposes only two methods, Initialize, Read. Not really sure how to test those without providing actual uri, creds and try to fetch a secret. But that would not be a unit test.

tomkerkhove · 2022-03-14T07:27:24Z

Fine by me if it's fine for the others

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

zroubalik

Looking good, just some minor nits

tests/.env

pkg/scaling/resolver/azure_keyvault_handler.go

CHANGELOG.md

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

tests/scalers/azure-keyvault-queue.test.ts

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

zroubalik · 2022-03-16T09:27:17Z

/run-e2e azure-keyvault-queue.test.ts
Update: You can check the progres here

zroubalik

LGTM

SebastianSchuetze · 2022-03-16T10:17:44Z

Last question for me. Does this support also managed identity as requested or does this come later?

v-shenoy · 2022-03-16T10:22:29Z

Last question for me. Does this support also managed identity as requested or does this come later?

@SebastianSchuetze It doesn't currently, it will have to be taken up later.

…re#2727) Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com> Signed-off-by: Ram Cohen <ram.cohen@gmail.com>

mingue · 2022-04-26T22:50:55Z

Looking into using this once released, but will this also be supported for ClusterTriggerAuthentication? The updated documentation doesn't make any exception to exclude this feature. If so where will it get the credentials for the clientSecret:

clientSecret:
  valueFrom:
    secretKeyRef: # Where is this secret coming from in a ClusterTriggerAuthentication?
      name: <secret-name>
      key: <key-within-secret>

tomkerkhove · 2022-04-27T06:43:01Z

That's something we should improve in our docs indeed, @v-shenoy I presume this would be from a secret in the same namespace as the ClusterTriggerAuthentication itself, right?

v-shenoy · 2022-04-27T06:47:43Z

Yes, it would be. Do we add this to the documentation, or should we have cluster trigger authentication be able to fetch secrets from other namespaces as well by specifying a namespace key in the object? (Only for Cluster, not normal trigger auth)

tomkerkhove · 2022-04-27T07:28:16Z

No, I think the current approach is perfect but let's improve the docs a bit.

v-shenoy · 2022-04-27T07:52:16Z

Last question for me. Does this support also managed identity as requested or does this come later?

While this thread is active, @SebastianSchuetze Pod Identity support is being added to key vault, along with workload identity.

v-shenoy · 2022-04-27T17:39:45Z

Added clarification - kedacore/keda-docs#753

tomkerkhove · 2022-04-28T06:58:44Z

Merged, thanks!

v-shenoy added 2 commits March 9, 2022 14:13

Update trigger authentication CRDs.

1f93123

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Implement handler for Azure key vault.

e2f4e49

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

v-shenoy requested a review from a team as a code owner March 9, 2022 11:37

v-shenoy marked this pull request as draft March 9, 2022 11:38

v-shenoy and others added 3 commits March 10, 2022 11:09

Modify CRDs to align with Kubernetes secretKeyRef approach.

6aea332

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Update CHANGELOG.

458dfe1

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Merge branch 'main' into azure-key-vault-support

f9ae4be

tomkerkhove reviewed Mar 10, 2022

View reviewed changes

pkg/scaling/resolver/azure_keyvault_handler.go Show resolved Hide resolved

v-shenoy mentioned this pull request Mar 10, 2022

Documentation changes for Azure Key Vault support. kedacore/keda-docs#704

Merged

1 task

v-shenoy marked this pull request as ready for review March 10, 2022 10:01

Add E2E test for key vault using Azure queue trigger.

9213c7f

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Fix merge conflicts.

7786127

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

zroubalik reviewed Mar 14, 2022

View reviewed changes

tests/.env Outdated Show resolved Hide resolved

pkg/scaling/resolver/azure_keyvault_handler.go Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

v-shenoy added 2 commits March 14, 2022 14:08

Fix nitpicks.

93ba5b9

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Merge with main.

7158c93

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

tomkerkhove mentioned this pull request Mar 14, 2022

feat: Provide e2e variables for Azure Key Vault #2756

Merged

5 tasks

tomkerkhove reviewed Mar 14, 2022

View reviewed changes

tests/scalers/azure-keyvault-queue.test.ts Outdated Show resolved Hide resolved

Rename env variables, and secret.

e6af320

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

v-shenoy requested review from tomkerkhove and zroubalik March 14, 2022 16:10

zroubalik approved these changes Mar 16, 2022

View reviewed changes

zroubalik merged commit 4e1b63b into kedacore:main Mar 16, 2022

v-shenoy deleted the azure-key-vault-support branch March 16, 2022 09:56

RamCohen pushed a commit to RamCohen/keda that referenced this pull request Mar 23, 2022

Provide support for Azure Key Vault in TriggerAuthentication. (kedaco…

118880e

…re#2727) Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com> Signed-off-by: Ram Cohen <ram.cohen@gmail.com>

v-shenoy mentioned this pull request Apr 27, 2022

docs: Clarify namespace from which keyvault credential secret is fetc… kedacore/keda-docs#753

Merged

1 task

Provide support for Azure Key Vault in TriggerAuthentication. #2727

Provide support for Azure Key Vault in TriggerAuthentication. #2727

Conversation

v-shenoy commented Mar 9, 2022 • edited Loading

Checklist

tomkerkhove commented Mar 9, 2022

tomkerkhove commented Mar 9, 2022

v-shenoy commented Mar 9, 2022

v-shenoy commented Mar 9, 2022

tomkerkhove commented Mar 9, 2022

v-shenoy commented Mar 9, 2022 • edited Loading

tomkerkhove commented Mar 9, 2022

v-shenoy commented Mar 9, 2022

v-shenoy commented Mar 9, 2022 • edited Loading

tomkerkhove commented Mar 9, 2022

v-shenoy commented Mar 9, 2022

tomkerkhove commented Mar 9, 2022

tomkerkhove commented Mar 10, 2022

v-shenoy commented Mar 10, 2022

v-shenoy commented Mar 10, 2022

tomkerkhove commented Mar 10, 2022

v-shenoy commented Mar 10, 2022

JorTurFer commented Mar 10, 2022

tomkerkhove commented Mar 11, 2022

JorTurFer commented Mar 11, 2022 • edited Loading

tomkerkhove commented Mar 11, 2022

v-shenoy commented Mar 12, 2022 • edited Loading

tomkerkhove commented Mar 14, 2022

v-shenoy commented Mar 14, 2022 • edited Loading

tomkerkhove commented Mar 14, 2022

zroubalik left a comment • edited Loading

Choose a reason for hiding this comment

zroubalik commented Mar 16, 2022 • edited by github-actions bot Loading

zroubalik left a comment

Choose a reason for hiding this comment

SebastianSchuetze commented Mar 16, 2022

v-shenoy commented Mar 16, 2022 • edited Loading

mingue commented Apr 26, 2022 • edited Loading

tomkerkhove commented Apr 27, 2022

v-shenoy commented Apr 27, 2022 • edited Loading

tomkerkhove commented Apr 27, 2022

v-shenoy commented Apr 27, 2022

v-shenoy commented Apr 27, 2022

tomkerkhove commented Apr 28, 2022

v-shenoy commented Mar 9, 2022 •

edited

Loading

v-shenoy commented Mar 9, 2022 •

edited

Loading

v-shenoy commented Mar 9, 2022 •

edited

Loading

JorTurFer commented Mar 11, 2022 •

edited

Loading

v-shenoy commented Mar 12, 2022 •

edited

Loading

v-shenoy commented Mar 14, 2022 •

edited

Loading

zroubalik left a comment •

edited

Loading

zroubalik commented Mar 16, 2022 •

edited by github-actions bot

Loading

v-shenoy commented Mar 16, 2022 •

edited

Loading

mingue commented Apr 26, 2022 •

edited

Loading

v-shenoy commented Apr 27, 2022 •

edited

Loading