Skip to content

Fix pod identity ignored when scaled target is CRD #5351

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 5, 2024
Merged

Fix pod identity ignored when scaled target is CRD #5351

merged 3 commits into from
Jan 5, 2024

Conversation

wozniakjan
Copy link
Member

@wozniakjan wozniakjan commented Jan 5, 2024 •
edited by JorTurFer
Loading

Checklist

Fixes #5011
Fixes #5255
Fixes #4344

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

@wozniakjan wozniakjan requested a review from a team as a code owner January 5, 2024 14:02
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 5, 2024

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

While you are waiting, make sure to:

Learn more about:

@wozniakjan wozniakjan force-pushed the fix/crd_pod_identity_ignored branch from c5a5974 to d9e8356 Compare January 5, 2024 14:04
@wozniakjan wozniakjan marked this pull request as draft January 5, 2024 14:05
@wozniakjan 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 wozniakjan marked this pull request as ready for review January 5, 2024 14:34
@wozniakjan wozniakjan force-pushed the fix/crd_pod_identity_ignored branch from 0aec6f7 to 09f9db2 Compare January 5, 2024 14:43
@juldrixx @wozniakjan
Fix CRD PodIdentity not considered
aa25090 
Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
@juldrixx @wozniakjan
Update CHANGELOG
df74300 
Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
@wozniakjan wozniakjan force-pushed the fix/crd_pod_identity_ignored branch from 09f9db2 to 4f9cdc9 Compare January 5, 2024 14:48
JorTurFer
JorTurFer approved these changes Jan 5, 2024
View reviewed changes
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.

LGTM! just a small nit inline

@sammaxwellxyz @wozniakjan
Add expectedPodIndity to ResolveAuthRef tests
b4d862c 
Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
@wozniakjan wozniakjan force-pushed the fix/crd_pod_identity_ignored branch from 4f9cdc9 to b4d862c Compare January 5, 2024 15:30
@wozniakjan wozniakjan requested a review from JorTurFer January 5, 2024 16:05
@wozniakjan
Copy link
Member Author

wozniakjan commented Jan 5, 2024 •
edited
Loading

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
Copy link
Member

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

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

@JorTurFer JorTurFer enabled auto-merge (squash) January 5, 2024 16:19
@JorTurFer
Copy link
Member

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

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

@zroubalik
Copy link
Member

zroubalik commented Jan 5, 2024 •
edited
Loading

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 🤷‍♂️

@JorTurFer JorTurFer merged commit 3118fbc into kedacore:main Jan 5, 2024
@wozniakjan wozniakjan mentioned this pull request Jan 6, 2024
Closed
@wozniakjan
Copy link
Member Author

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.

toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
@wozniakjan @juldrixx
@sammaxwellxyz @toniiiik
Fix pod identity ignored when scaled target is CRD (kedacore#5351)
a2c9e42 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik approved these changes

@JorTurFer JorTurFer JorTurFer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GCP PubSub Trigger with Workload Identity not working ScaledObject with argo Rollout scaleTargetRef fails to reconcile
5 participants
@wozniakjan @JorTurFer @zroubalik @juldrixx @sammaxwellxyz