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.

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

Provide validation for identityId with Azure pod identity & Azure AD Workload Identity in TriggerAuthentication #4696

Merged
merged 41 commits into from
Aug 29, 2023
Merged

Provide validation for identityId with Azure pod identity & Azure AD Workload Identity in TriggerAuthentication #4696

merged 41 commits into from
Aug 29, 2023

Conversation

SpiritZhou
Copy link
Contributor

@SpiritZhou SpiritZhou commented Jun 16, 2023 •
edited by tomkerkhove
Loading

add validation for identityId with Azure pod identity in TriggerAuthentication in case that user pass an empty application ID
and causes error.

  1. Change IdentityID from string to *string pointer
  2. Add nil or empty checking in ResolveAuthRefAndPodIdentity when resoveing scaler
  3. Update other azure related scalers

Checklist

Relates to #4528

@SpiritZhou
Update
9c9af60 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou SpiritZhou requested a review from a team as a code owner June 16, 2023 09:13
@github-actions
Copy link

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

While you are waiting, make sure to:

Learn more about:

tomkerkhove
tomkerkhove suggested changes Jun 16, 2023
View reviewed changes
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.

Thanks! Would you mind adding some unit & e2e tests please?

CHANGELOG.md Outdated Show resolved Hide resolved
@SpiritZhou @tomkerkhove
Update CHANGELOG.md
4857d65 
Co-authored-by: Tom Kerkhove <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
@tomkerkhove tomkerkhove requested a review from JorTurFer July 4, 2023 11:58
@SpiritZhou
Add webhooks
70c5a14 
Signed-off-by: SpiritZhou <[email protected]>
JorTurFer
JorTurFer reviewed Jul 5, 2023
View reviewed changes
pkg/scalers/rabbitmq_scaler.go Outdated Show resolved Hide resolved
pkg/scaling/resolver/scale_resolvers.go Outdated Show resolved Hide resolved
SpiritZhou added 7 commits July 6, 2023 11:22
@SpiritZhou
Update
c74f164 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Update
31178f2 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Merge branch 'main' into spiritzhou/checkemptyidentityid
ab3d9d0 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Update
966fb35 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
fix
e43e3f0 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Update
1898b7b 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Update
294a0b8 
Signed-off-by: SpiritZhou <[email protected]>
@JorTurFer
Copy link
Member

JorTurFer commented Jul 6, 2023 •
edited
Loading

Could you also add some e2e tests for this new admission webhook?

@SpiritZhou
Update
8f13085 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Copy link
Contributor Author

Should I also add some e2e test with real identity id? If so, where can I get the real identity id info? @JorTurFer @tomkerkhove

@tomkerkhove
Copy link
Member

Should I also add some e2e test with real identity id? If so, where can I get the real identity id info? @JorTurFer @tomkerkhove

What do you think @JorTurFer? I think it's OK to extend our existing e2e tests with just the negative scenario (invalid specified/empty string) as the rest should already be covered by other tests implicitly

@SpiritZhou SpiritZhou requested a review from JorTurFer August 18, 2023 03:26
@SpiritZhou
Update
58d32c8 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Update
82ee9aa 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Update
f0bd980 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Fix order
12f1fb4 
Signed-off-by: SpiritZhou <[email protected]>
tomkerkhove
tomkerkhove approved these changes Aug 21, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@SpiritZhou @tomkerkhove
Update CHANGELOG.md
00a359f 
Co-authored-by: Tom Kerkhove <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou SpiritZhou requested a review from tomkerkhove August 21, 2023 08:00
@SpiritZhou
Update CHANGELOG.md
1d2344c 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Update CHANGELOG.md
dfa0e7f 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Update CHANGELOG.md
c00289d 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Update CHANGELOG.md
0f93a62 
Signed-off-by: SpiritZhou <[email protected]>
JorTurFer
JorTurFer reviewed Aug 24, 2023
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Only one small nit in azure-monitor scaler 💪

pkg/scalers/azure/azure_monitor.go Outdated Show resolved Hide resolved
pkg/scaling/resolver/scale_resolvers.go Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented Aug 24, 2023 •
edited by github-actions bot
Loading

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

@tomkerkhove
Copy link
Member

tomkerkhove commented Aug 25, 2023 •
edited by github-actions bot
Loading

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

JorTurFer
JorTurFer approved these changes Aug 25, 2023
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

@tomkerkhove
Copy link
Member

Thank you @SpiritZhou! @JorTurFer any idea why codeql is stuck? Do I need to queue e2e again?

@JorTurFer
Copy link
Member

JorTurFer commented Aug 29, 2023 •
edited
Loading

That check is the e2e test and not CodeQL... Although the e2e check is created on E2E section, IDK why the GH API adds it randomly to any other section.
As it work for requiring the e2e tests I haven't gone deeper, but that's why you see stuck CodeQL check, if you read the second part (the right side of the /), you will see that it's the e2e test. It could be randomly attached to any section.

Why it is required again if e2e tests already passed? Because a new commit has been added 😄 if the commit require testigg you can trigger them, if not you can skip them

@tomkerkhove
Copy link
Member

Ok let's run them again!

@tomkerkhove
Copy link
Member

tomkerkhove commented Aug 29, 2023 •
edited by github-actions bot
Loading

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

@tomkerkhove tomkerkhove enabled auto-merge (squash) August 29, 2023 09:09
@tomkerkhove tomkerkhove merged commit 046d2bb into kedacore:main Aug 29, 2023
Adarsh-verma-14 pushed a commit to Adarsh-verma-14/keda that referenced this pull request Sep 4, 2023
@SpiritZhou @tomkerkhove
@JorTurFer @Adarsh-verma-14
Provide validation for identityId with Azure pod identity & Azure AD …
c7d9b8c 
…Workload Identity in TriggerAuthentication (kedacore#4696)

Co-authored-by: Tom Kerkhove <[email protected]>
Co-authored-by: Jorge Turrado Ferrero <[email protected]>
@radekfojtik radekfojtik mentioned this pull request Oct 25, 2023
Merged
4 tasks
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
@SpiritZhou @tomkerkhove
@JorTurFer @toniiiik
Provide validation for identityId with Azure pod identity & Azure AD …
ce4dda6 
…Workload Identity in TriggerAuthentication (kedacore#4696)

Co-authored-by: Tom Kerkhove <[email protected]>
Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: anton.lysina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JorTurFer JorTurFer JorTurFer approved these changes

@tomkerkhove tomkerkhove tomkerkhove 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.
3 participants
@SpiritZhou @JorTurFer @tomkerkhove