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

feat(event-hubs-scaler): Add pod/workload identity support to checkpoint in Azure Blob Storage #3573

Merged
merged 7 commits into from
Oct 31, 2022

Conversation

andyatwork
Copy link
Contributor

@andyatwork andyatwork commented Aug 20, 2022

Add support for pod identity based authentication to blob storage accounts in Event Hub scaler

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

Relates to #3569

Relates to kedacore/keda-docs#905

@andyatwork andyatwork requested a review from a team as a code owner August 20, 2022 02:25
@andyatwork andyatwork changed the title Add pod identity support to Blob Storage Event Hub Scaler: Add pod identity support to Blob Storage Aug 20, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
@tomkerkhove tomkerkhove changed the title Event Hub Scaler: Add pod identity support to Blob Storage feat(event-hubs-scaler): Add pod identity support to checkpoint in Azure Blob Storage Aug 22, 2022
@tomkerkhove tomkerkhove changed the title feat(event-hubs-scaler): Add pod identity support to checkpoint in Azure Blob Storage feat(event-hubs-scaler): Add pod/workload identity support to checkpoint in Azure Blob Storage Aug 22, 2022
@andyatwork andyatwork force-pushed the eventhub-podidentity-storage branch 2 times, most recently from acc7995 to e3d2a52 Compare August 23, 2022 19:36
@v-shenoy
Copy link
Contributor

v-shenoy commented Aug 24, 2022

I checked this PR out and tested it. Confirmed that it works with Workload Identity as well @tomkerkhove @andyatwork

apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx-deployment
  labels:
    app: nginx
spec:
  replicas: 0
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - name: nginx
        image: nginx:1.14.2
        ports:
        - containerPort: 80
---
apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: eventhub-ta
spec:
  podIdentity:
    provider: azure-workload
---
apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  name: eventhub-so
spec:
  scaleTargetRef:
    name: nginx-deployment
  minReplicaCount: 0
  maxReplicaCount: 5
  triggers:
  - type: azure-eventhub
    metadata:
      storageAccountName: testhubstorage
      checkpointStrategy: azureFunction
      eventHubNamespace: test-hub-ns
      eventHubName: hub-1
    authenticationRef:
      name: eventhub-ta

@andyatwork
Copy link
Contributor Author

I checked this PR out and tested it. Confirmed that it works with Workload Identity as well @tomkerkhove @andyatwork

apiVersion: apps/v1
kind: Deployment
metadata:

...

Excellent, thanks for testing this!

@andyatwork
Copy link
Contributor Author

@tomkerkhove , can you follow up on the change request above?

@tomkerkhove
Copy link
Member

Not from my side

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Looking good, some comments inline.
Should we add an e2e test checking this?
Thanks for this contribution! ❤️

pkg/scalers/azure/azure_eventhub_checkpoint.go Outdated Show resolved Hide resolved
pkg/scalers/azure_eventhub_scaler.go Show resolved Hide resolved
@andyatwork
Copy link
Contributor Author

Not from my side

Sorry, I had responded to your comment inline above and realized I hadn't published my comments yet. They remained "pending", so I guess you hadn't actually seen my response. Hope the replies addresses your concerns.

@andyatwork andyatwork force-pushed the eventhub-podidentity-storage branch from 58e4ca2 to 5ec4e02 Compare September 6, 2022 21:12
@andyatwork andyatwork requested review from JorTurFer and tomkerkhove and removed request for tomkerkhove and JorTurFer September 7, 2022 22:28
@v-shenoy
Copy link
Contributor

@JorTurFer Any changes here after upgrading the event hubs SDK?

@JorTurFer
Copy link
Member

I didn't touch the eventhub sdk, only the service-bus sdk which I'd say is different

@JorTurFer
Copy link
Member

JorTurFer commented Sep 14, 2022

I have checked the SDK for this and, oh surprise, uses other authentication mechanism different from the service-bus SDK...
Maybe in the future they unify the SDK in a single SDK for azure or at least reuse the authentication part, but atm I think it's a mess

@v-shenoy
Copy link
Contributor

I didn't touch the eventhub sdk, only the service-bus sdk which I'd say is different

I thought you upgraded it to v3, which is why I was wondering if anything broke over there that would require changes here.

@JorTurFer
Copy link
Member

I didn't touch the eventhub sdk, only the service-bus sdk which I'd say is different

I thought you upgraded it to v3, which is why I was wondering if anything broke over there that would require changes here.

Lol, let me check because I didn't notice that

@JorTurFer
Copy link
Member

did you mean with this PR?
I only changed the service-bus sdk
image
others are minor versions

@v-shenoy
Copy link
Contributor

did you mean with this PR? I only changed the service-bus sdk image others are minor versions

Oh, I just saw the title for this issue, #2986 and I assumed it was upgrading TO v3. If it's minor versions, then shouldn't cause any issues.

@JorTurFer
Copy link
Member

No no, it's a minor version, but that minor version created conflicts I think because it uses generics somewhere. That's why I created the issue to remember bumping it after update golang version

@JorTurFer
Copy link
Member

JorTurFer commented Sep 14, 2022

BTW; I have checked and there is a beta version for event-hub in the azure-sdk-for-go so eventually we could migrate to it once it's stable and simplify the pod identities reusing the approach of service-bus 🤞

@andyatwork andyatwork requested review from JorTurFer and removed request for tomkerkhove October 4, 2022 20:08
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

I have to apologize @andyatwork because I missed this PR...
Let's check if @tomkerkhove has anything to say

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.

LGTM, @v-shenoy any concerns from your side?

CHANGELOG.md Show resolved Hide resolved
@JorTurFer
Copy link
Member

@andyatwork , could you rebase your branch? 🙏 We added e2e test for event hub some weeks ago and rebasing the branch we can execute them before merging the PR

@andyatwork
Copy link
Contributor Author

Yes, will rebase and update the PR.

@andyatwork andyatwork force-pushed the eventhub-podidentity-storage branch 2 times, most recently from ff8eafd to 8b3bfa8 Compare October 31, 2022 21:04
andyatwork and others added 7 commits October 31, 2022 14:05
Signed-off-by: Andy Petralli <andres.petralli@microsoft.com>
Signed-off-by: Andy Petralli <andres.petralli@microsoft.com>
Co-authored-by: Vighnesh Shenoy <vighneshq@gmail.com>
Signed-off-by: Andy Petralli <andres.petralli@microsoft.com>
Signed-off-by: Andres Petralli <andres.petralli@microsoft.com>
Signed-off-by: Andres Petralli <andres.petralli@microsoft.com>
Signed-off-by: Andres Petralli <andres.petralli@microsoft.com>
Signed-off-by: Andres Petralli <andres.petralli@microsoft.com>
@andyatwork andyatwork force-pushed the eventhub-podidentity-storage branch from 8b3bfa8 to 6a5c91b Compare October 31, 2022 21:07
@JorTurFer
Copy link
Member

JorTurFer commented Oct 31, 2022

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

@JorTurFer JorTurFer merged commit 92efd86 into kedacore:main Oct 31, 2022
26tanishabanik pushed a commit to 26tanishabanik/keda that referenced this pull request Nov 1, 2022
…int in Azure Blob Storage (kedacore#3573)

Co-authored-by: Vighnesh Shenoy <vighneshq@gmail.com>
Signed-off-by: 26tanishabanik <26tanishabanik@gmail.com>
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.

4 participants