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: add scaletargetref exists check in webhook and revise some variable name #6350

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

ctccxxd
Copy link

@ctccxxd ctccxxd commented Nov 20, 2024

check scaletargetref exists in webhook and revise some variable name, originally,
1, There is not scaletargetref exists check code in webhook module.
2, Change the inappropriate variable name Gckr to Gvkr.
3, Upgrade crypto to v0.31.0 which is a imported package to fix vulnerabilities Trivy has detected.
4, If still want to remain the function to create scaleobj first without real scaletargetRef obj. Think whether this function will be a way can be configged, to confirm the real scaletargetRef obj already exists when we create/update scaleobj. In this way, it can avoid the user misconfigurations about scaletargetRef in yaml file.

Checklist

Fixes #

Relates to #
#6354

@ctccxxd ctccxxd requested a review from a team as a code owner November 20, 2024 03:10
@ctccxxd ctccxxd changed the title check scaletargetref exists in webhook and revise some variable name add scaletargetref exists check in webhook and revise some variable name Nov 20, 2024
@ctccxxd ctccxxd force-pushed the checkscaletargetrefexists branch from 7d2395d to c76c19f Compare November 20, 2024 05:20
@SpiritZhou
Copy link
Contributor

Could you please open an issue for this improvement and relate this pr to the issue? This will make it clear and easy to tracing.

@ctccxxd
Copy link
Author

ctccxxd commented Nov 21, 2024

Could you please open an issue for this improvement and relate this pr to the issue? This will make it clear and easy to tracing.

done. Thank you.

@ctccxxd ctccxxd reopened this Nov 21, 2024
@ctccxxd
Copy link
Author

ctccxxd commented Nov 21, 2024

Friendly ping @zroubalik @wozniakjan @SpiritZhou @psi @wonko @fivesheep @JorTurFer hope you to review and discuss, much thanks.

@ctccxxd ctccxxd changed the title add scaletargetref exists check in webhook and revise some variable name feat: add scaletargetref exists check in webhook and revise some variable name Nov 21, 2024
Comment on lines 273 to 278
if err := kc.Get(ctx, client.ObjectKey{Namespace: so.Namespace, Name: so.Spec.ScaleTargetRef.Name}, unstruct); err != nil {
// resource doesn't exist
scaledobjectlog.Error(err, message.ScaleTargetNotFoundMsg, "resource", gvkString, "name", so.Spec.ScaleTargetRef.Name)
return err
}
return nil
Copy link
Member

Choose a reason for hiding this comment

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

As this can potentially happen before the targetRef is available through the cached client it should check the item using getFromCacheOrDirect to rely on user configuration to try using the direct client

Copy link
Author

Choose a reason for hiding this comment

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

As this can potentially happen before the targetRef is available through the cached client it should check the item using getFromCacheOrDirect to rely on user configuration to try using the direct client

Thank you for your comment. I have follow your idea and change the code already. Hope for your reply soon. MuchThanks.

@ctccxxd ctccxxd force-pushed the checkscaletargetrefexists branch from 3ca6939 to 22b489d Compare November 24, 2024 09:29
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! Thanks for the improvement :)

@JorTurFer
Copy link
Member

JorTurFer commented Nov 27, 2024

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

@JorTurFer
Copy link
Member

PTAL @kedacore/keda-contributors

@JorTurFer JorTurFer enabled auto-merge (squash) November 27, 2024 21:46
@JorTurFer
Copy link
Member

@ctccxxd , please open another PR to docs adding the new check to the admission webhooks -> https://github.com/kedacore/keda-docs/blob/main/content/docs/2.17/concepts/admission-webhooks.md

@ctccxxd
Copy link
Author

ctccxxd commented Nov 28, 2024

@ctccxxd , please open another PR to docs adding the new check to the admission webhooks -> https://github.com/kedacore/keda-docs/blob/main/content/docs/2.17/concepts/admission-webhooks.md

ok, thank you much

@ctccxxd
Copy link
Author

ctccxxd commented Nov 28, 2024

LGTM! Thanks for the improvement :)

Thank you much

@ctccxxd
Copy link
Author

ctccxxd commented Nov 28, 2024

@ctccxxd , please open another PR to docs adding the new check to the admission webhooks -> https://github.com/kedacore/keda-docs/blob/main/content/docs/2.17/concepts/admission-webhooks.md

done, kedacore/keda-docs#1506, hope to review. Thank you.

Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

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

I saw a couple of internal tests fail with message.ScaleTargetNotFoundMsg, we should probably check if the tests are flaky or the error is valid

e.g.

    events_test.go:***: 
        	Error Trace:	/__w/keda/keda/tests/internals/events/events_test.go:***     	            				/__w/keda/keda/tests/internals/events/events_test.go:***59    	            				/__w/keda/keda/tests/internals/events/events_test.go:***97
        	Error:      	Not equal: 
        	            	expected: ""
        	            	actual  : "ScaledObjectCheckFailed:Target resource doesn't exist"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -*** +*** @@
        	            	-
        	            	+ScaledObjectCheckFailed:Target resource doesn't exist

@ctccxxd
Copy link
Author

ctccxxd commented Nov 28, 2024

/run-e2e internal

I saw a couple of internal tests fail with message.ScaleTargetNotFoundMsg, we should probably check if the tests are flaky or the error is valid

e.g.

    events_test.go:***: 
        	Error Trace:	/__w/keda/keda/tests/internals/events/events_test.go:***     	            				/__w/keda/keda/tests/internals/events/events_test.go:***59    	            				/__w/keda/keda/tests/internals/events/events_test.go:***97
        	Error:      	Not equal: 
        	            	expected: ""
        	            	actual  : "ScaledObjectCheckFailed:Target resource doesn't exist"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -*** +*** @@
        	            	-
        	            	+ScaledObjectCheckFailed:Target resource doesn't exist

ok, Thank you much

@wozniakjan
Copy link
Member

wozniakjan commented Nov 28, 2024

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

@wozniakjan
Copy link
Member

looks like there are now more failing tests with these changes

Failed tests:
	Execution of tests/internals/scaled_object_validation/scaled_object_validation_test.go, has failed after "three" attempts
	Execution of tests/internals/events/events_test.go, has failed after "three" attempts
	Execution of tests/internals/cloudevent_source/cloudevent_source_test.go, has failed after "three" attempts

https://github.com/kedacore/keda/actions/runs/12064855537/job/33642776237#step:9:19642

auto-merge was automatically disabled December 8, 2024 12:21

Head branch was pushed to by a user without write access

@ctccxxd ctccxxd force-pushed the checkscaletargetrefexists branch from e08ead3 to c017a92 Compare December 8, 2024 12:21
@ctccxxd ctccxxd force-pushed the checkscaletargetrefexists branch 2 times, most recently from 24c4a42 to 2614ea9 Compare December 10, 2024 12:32
@ctccxxd
Copy link
Author

ctccxxd commented Dec 12, 2024

@JorTurFer @zroubalik @wozniakjan hello, I have revise the wrong test cases and test myself. Hope for your review, and e2e test, much thanks.

@ctccxxd ctccxxd force-pushed the checkscaletargetrefexists branch from e955282 to 4260f77 Compare December 17, 2024 00:03
@ctccxxd
Copy link
Author

ctccxxd commented Dec 17, 2024

@zroubalik @JorTurFer @wozniakjan hello, I have revise the wrong test cases and test myself. Hope for your review, and e2e test, much thanks.

@ctccxxd ctccxxd requested a review from wozniakjan December 18, 2024 04:03
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