Fix pod identity ignored when scaled target is CRD #5351

wozniakjan · 2024-01-05T14:02:23Z

Checklist

I have verified that my change is according to the deprecations & breaking changes policy
Tests have been added
Changelog has been updated and is aligned with our changelog requirements
Commits are signed with Developer Certificate of Origin (DCO - learn more)

This cherry-picks great work from #5255 and #5021, only clarifies a little on the actual fix and rebased on top of latest main to resolve conflicts

github-actions · 2024-01-05T14:02:36Z

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Add an entry in our changelog in alphabetical order and link related issue
Update the documentation, if needed
Add unit & e2e tests for your changes
GitHub checks are passing
Is the DCO check failing? Here is how you can fix DCO issues

Learn more about:

Our contribution guide

Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>

JorTurFer

LGTM! just a small nit inline

pkg/scaling/resolver/scale_resolvers_test.go

Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>

wozniakjan · 2024-01-05T16:09:25Z

for completion, also confirming from manual testing

keda v2.12.1

$ k get so
NAME               SCALETARGETKIND         SCALETARGETNAME   MIN   MAX   TRIGGERS     AUTHENTICATION   READY   ACTIVE    FALLBACK   PAUSED    AGE
crd-scobj          jw.my.domain/v1.Scobj   scobj-sample      0     5     gcp-pubsub   keda             False   Unknown   Unknown    Unknown   10m
nginx-deployment   apps/v1.Deployment      nginx             0     5     gcp-pubsub   keda             True    True      False      Unknown   169m

$ k describe so crd-scobj
...
  Warning  KEDAScalerFailed         5m41s (x17 over 11m)  keda-operator  error parsing PubSub metadata: google application credentials not found
...

with this PR

$ k get scaledobjects.keda.sh
NAME               SCALETARGETKIND         SCALETARGETNAME   MIN   MAX   TRIGGERS     AUTHENTICATION   READY   ACTIVE   FALLBACK   PAUSED    AGE
crd-scobj          jw.my.domain/v1.Scobj   scobj-sample      0     5     gcp-pubsub   keda             True    True     Unknown    Unknown   13m
nginx-deployment   apps/v1.Deployment      nginx             0     5     gcp-pubsub   keda             True    True     False      Unknown   171m

JorTurFer · 2024-01-05T16:18:59Z

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

JorTurFer · 2024-01-05T16:54:37Z

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

zroubalik · 2024-01-05T17:22:09Z

for completion, also confirming from manual testing

keda v2.12.1

$ k get so
NAME               SCALETARGETKIND         SCALETARGETNAME   MIN   MAX   TRIGGERS     AUTHENTICATION   READY   ACTIVE    FALLBACK   PAUSED    AGE
crd-scobj          jw.my.domain/v1.Scobj   scobj-sample      0     5     gcp-pubsub   keda             False   Unknown   Unknown    Unknown   10m
nginx-deployment   apps/v1.Deployment      nginx             0     5     gcp-pubsub   keda             True    True      False      Unknown   169m

$ k describe so crd-scobj
...
  Warning  KEDAScalerFailed         5m41s (x17 over 11m)  keda-operator  error parsing PubSub metadata: google application credentials not found
...

with this PR

$ k get scaledobjects.keda.sh
NAME               SCALETARGETKIND         SCALETARGETNAME   MIN   MAX   TRIGGERS     AUTHENTICATION   READY   ACTIVE   FALLBACK   PAUSED    AGE
crd-scobj          jw.my.domain/v1.Scobj   scobj-sample      0     5     gcp-pubsub   keda             True    True     Unknown    Unknown   13m
nginx-deployment   apps/v1.Deployment      nginx             0     5     gcp-pubsub   keda             True    True     False      Unknown   171m

Great job!

But what worries me, is the Unknown status for FALLBACK and PAUSED. I don't think it is related to this PR, but it is something we should check. Once the ScaledObject is processed, there should be True or False, not Unkonwn 🤷‍♂️

wozniakjan · 2024-01-07T14:45:16Z

Once the ScaledObject is processed, there should be True or False, not Unkonwn 🤷‍♂️

I will re-check this when time permits, it could have been due to some misconfiguration on my side.

* Fix CRD PodIdentity not considered Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com> * Update CHANGELOG Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com> * Add expectedPodIndity to ResolveAuthRef tests Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com> --------- Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com> Co-authored-by: Juldrixx <juldrixx@gmail.com> Co-authored-by: Sam Maxwell <sam@groundtruthlabs.com> Signed-off-by: anton.lysina <alysina@gmail.com>

wozniakjan requested a review from a team as a code owner January 5, 2024 14:02

wozniakjan force-pushed the fix/crd_pod_identity_ignored branch from c5a5974 to d9e8356 Compare January 5, 2024 14:04

wozniakjan marked this pull request as draft January 5, 2024 14:05

wozniakjan force-pushed the fix/crd_pod_identity_ignored branch 2 times, most recently from d3d93bd to 0aec6f7 Compare January 5, 2024 14:33

wozniakjan marked this pull request as ready for review January 5, 2024 14:34

wozniakjan force-pushed the fix/crd_pod_identity_ignored branch from 0aec6f7 to 09f9db2 Compare January 5, 2024 14:43

juldrixx added 2 commits January 5, 2024 15:48

Fix CRD PodIdentity not considered

aa25090

Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>

Update CHANGELOG

df74300

Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>

wozniakjan force-pushed the fix/crd_pod_identity_ignored branch from 09f9db2 to 4f9cdc9 Compare January 5, 2024 14:48

JorTurFer approved these changes Jan 5, 2024

View reviewed changes

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

Add expectedPodIndity to ResolveAuthRef tests

b4d862c

Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>

wozniakjan force-pushed the fix/crd_pod_identity_ignored branch from 4f9cdc9 to b4d862c Compare January 5, 2024 15:30

wozniakjan requested a review from JorTurFer January 5, 2024 16:05

JorTurFer approved these changes Jan 5, 2024

View reviewed changes

JorTurFer enabled auto-merge (squash) January 5, 2024 16:19

zroubalik approved these changes Jan 5, 2024

View reviewed changes

JorTurFer merged commit 3118fbc into kedacore:main Jan 5, 2024
19 checks passed

wozniakjan mentioned this pull request Jan 6, 2024

Fix GCP PodIdentity not considered with WorkloadIdentity - take 2 #5255

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pod identity ignored when scaled target is CRD #5351

Fix pod identity ignored when scaled target is CRD #5351

wozniakjan commented Jan 5, 2024 •

edited by JorTurFer

Loading

github-actions bot commented Jan 5, 2024

JorTurFer left a comment

wozniakjan commented Jan 5, 2024 •

edited

Loading

JorTurFer commented Jan 5, 2024 •

edited by github-actions bot

Loading

JorTurFer commented Jan 5, 2024 •

edited by github-actions bot

Loading

zroubalik commented Jan 5, 2024 •

edited

Loading

wozniakjan commented Jan 7, 2024

Fix pod identity ignored when scaled target is CRD #5351

Fix pod identity ignored when scaled target is CRD #5351

Conversation

wozniakjan commented Jan 5, 2024 • edited by JorTurFer Loading

Checklist

github-actions bot commented Jan 5, 2024

JorTurFer left a comment

Choose a reason for hiding this comment

wozniakjan commented Jan 5, 2024 • edited Loading

JorTurFer commented Jan 5, 2024 • edited by github-actions bot Loading

JorTurFer commented Jan 5, 2024 • edited by github-actions bot Loading

zroubalik commented Jan 5, 2024 • edited Loading

wozniakjan commented Jan 7, 2024

wozniakjan commented Jan 5, 2024 •

edited by JorTurFer

Loading

wozniakjan commented Jan 5, 2024 •

edited

Loading

JorTurFer commented Jan 5, 2024 •

edited by github-actions bot

Loading

JorTurFer commented Jan 5, 2024 •

edited by github-actions bot

Loading

zroubalik commented Jan 5, 2024 •

edited

Loading