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

feat: New validation hook to check if scale target is already managed #4001

Merged
merged 42 commits into from
Jan 9, 2023
Merged

feat: New validation hook to check if scale target is already managed #4001

merged 42 commits into from
Jan 9, 2023

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Dec 12, 2022 •
edited
Loading

Signed-off-by: Jorge Turrado jorge_turrado@hotmail.es

Note: This PR uses my own fork of opa/cert-controller until the merge/reject my PR, If they don't merge it, I plan to fork their repo in kedacore organization with the changes

Note 2: I have removed duplicated test code for influx scaler because that test fails after adding the webhooks (due to multiple SO for the same workload)

Checklist

Relates to #3755
Relates to #3087
Relates to #3083
Relates to #3088

@JorTurFer JorTurFer force-pushed the validation-hook branch 2 times, most recently from 1bab396 to 7756767 Compare December 21, 2022 22:52
@JorTurFer
Copy link
Member Author

JorTurFer commented Dec 23, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Dec 24, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer changed the title WIP - feat: New validation hook to check if scale target is already managed feat: New validation hook to check if scale target is already managed Dec 30, 2022
@JorTurFer JorTurFer marked this pull request as ready for review December 30, 2022 18:24
@JorTurFer JorTurFer requested a review from a team as a code owner December 30, 2022 18:24
@kedacore kedacore deleted a comment from semgrep-app bot Dec 30, 2022
@JorTurFer
Copy link
Member Author

JorTurFer commented Dec 30, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer changed the title feat: New validation hook to check if scale target is already managed WIP - feat: New validation hook to check if scale target is already managed Dec 30, 2022
@JorTurFer JorTurFer changed the title WIP - feat: New validation hook to check if scale target is already managed feat: New validation hook to check if scale target is already managed Dec 31, 2022
@JorTurFer
Copy link
Member Author

JorTurFer commented Dec 31, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jan 1, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer mentioned this pull request Jan 2, 2023
Merged
4 tasks
@JorTurFer
Copy link
Member Author

JorTurFer commented Jan 2, 2023 •
edited by github-actions bot
Loading

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

zroubalik
zroubalik reviewed Jan 2, 2023
View reviewed changes
PROJECT Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_types.go Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_webhook.go Outdated Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_webhook.go Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_webhook.go Outdated Show resolved Hide resolved
controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/scaling/resolver/scale_resolvers.go Outdated Show resolved Hide resolved
pkg/util/env_resolver.go Show resolved Hide resolved
config/rbac/role.yaml Show resolved Hide resolved
JorTurFer added 13 commits January 3, 2023 02:03
@JorTurFer
feat: New validation hook to check if scale target is already managed
b5bc368 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
update missing changes
659925c 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
add prometheus metrics to webhooks
b3f8312 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix test
d6e5cf7 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix styles
061ddf2 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
remove unused parameter
3989767 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
use k8s 1.26 for smoke test
c4a8359 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix style
bfcd999 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
add webhook logs to e2e output
15f2217 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
apply feedback
caabea1 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
use kedautil.GetPodNamespace() in adapter
462dd90 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
use gvkr parser for webhooks
0b92987 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
remove the empty secret as requirement
7a56a8d 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
Copy link
Member Author

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

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

@JorTurFer
Copy link
Member Author

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

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

@JorTurFer
update arch picture
281cdab 
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member Author

JorTurFer commented Jan 6, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jan 6, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
use my own fork temporally
232e18f 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
move to the operator the cert generation
6f7839d 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix influx test
d557d74 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix influx test
d24c8b8 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix matching errors
f1f3b99 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
undo incorrect change
925ab2c 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
Copy link
Member Author

JorTurFer commented Jan 8, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Jan 8, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

I'm still thinking about this, and I have doubts about using the operator for cert management or if it could be worth to introduce another component just for managing the certificates and the ca injections (like https://github.com/jet/kube-webhook-certgen/ does, but generating a bundle cert with all our requirements, not just 1 cert per thing). The architecture grows from 2 components to 4, but the certificates component is the piece that end users have to replace with cert-manager or other tool. Or maybe I'm overthinking, and we can introduce this new component in the future if it's necessary.

tomkerkhove
tomkerkhove reviewed Jan 9, 2023
View reviewed changes
config/manager/manager.yaml Show resolved Hide resolved
config/rbac/role.yaml Show resolved Hide resolved
config/webhooks/webhooks.yaml Show resolved Hide resolved
@JorTurFer
remove keda-architecture ppt in favour of keda-docs schematics.pptx
2da02f5 
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
zroubalik
zroubalik reviewed Jan 9, 2023
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Looking really great, just a minor nits.

CHANGELOG.md Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
cmd/operator/main.go Show resolved Hide resolved
cmd/operator/main.go Outdated Show resolved Hide resolved
cmd/operator/main.go Outdated Show resolved Hide resolved
cmd/operator/main.go Show resolved Hide resolved
JorTurFer and others added 4 commits January 9, 2023 11:17
@JorTurFer
Update Makefile
f19569c 
Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
@JorTurFer
apply feedback
04a24f4 
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Merge branch 'main' into validation-hook
c7eb052 
Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
@JorTurFer
add a deletion note in release yaml
d3a3efa 
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member Author

Ready for the final review :)

zroubalik
zroubalik reviewed Jan 9, 2023
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

/run-e2e internals*

@zroubalik
Copy link
Member

zroubalik commented Jan 9, 2023 •
edited by github-actions bot
Loading

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

zroubalik
zroubalik approved these changes Jan 9, 2023
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, great job!

@JorTurFer
Copy link
Member Author

Is it okey for you @tomkerkhove ? can I merge the feature?

tomkerkhove
tomkerkhove approved these changes Jan 9, 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.

A small approve for a review, a big step for KEDA

@JorTurFer JorTurFer merged commit 9e86234 into kedacore:main Jan 9, 2023
@JorTurFer JorTurFer deleted the validation-hook branch January 9, 2023 16:04
JorTurFer added a commit to JorTurFer/keda that referenced this pull request Jan 9, 2023
@JorTurFer
feat: New validation hook to check if scale target is already managed (…
412ed54 
…kedacore#4001)

* feat: New validation hook to check if scale target is already managed

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* update missing changes

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* add prometheus metrics to webhooks

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* fix test

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* fix styles

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* remove unused parameter

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* use k8s 1.26 for smoke test

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* fix style

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* add webhook logs to e2e output

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* apply feedback

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* use kedautil.GetPodNamespace() in adapter

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* use gvkr parser for webhooks

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* remove the empty secret as requirement

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* update go.sum

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* update errors

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* update pictures

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* update external name to use admission-webhooks

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* update bin output

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* add a core release

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* split rbac

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* update changelog

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* fix styles

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* fix typo

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* update parameter name

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* update docs

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* update rbac

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* update contributing

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* add cpu/memory validation

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* solve styles

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* fix errors

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* update arch picture

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* use my own fork temporally

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* move to the operator the cert generation

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* fix influx test

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* fix influx test

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* fix matching errors

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* undo incorrect change

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* remove keda-architecture ppt in favour of keda-docs schematics.pptx

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* Update Makefile

Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>

* apply feedback

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* add a deletion note in release yaml

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
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

@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
@JorTurFer @zroubalik @tomkerkhove