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

[experimental] Move admission webhook into skipper for better validation #2478

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MustafaSaber
Copy link
Member

Efforts to solve this #1618

@@ -273,6 +274,12 @@ type Config struct {

LuaModules *listFlag `yaml:"lua-modules"`
LuaSources *listFlag `yaml:"lua-sources"`

// admission webhook
Copy link
Member

Choose a reason for hiding this comment

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

I guess you want to rename it to validation instead of admission.
That's at least what we intend to do even if it runs at admission. https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#validatingadmissionwebhook

@MustafaSaber MustafaSaber self-assigned this Nov 29, 2023
@MustafaSaber MustafaSaber added the architectural all changes in the hot path, big changes in the control plane, control flow changes in filters label Jan 29, 2024
@@ -1959,6 +1976,16 @@ func run(o Options, sig chan os.Signal, idleConnsCH chan struct{}) error {
routing := routing.New(ro)
defer routing.Close()

if o.EnableValidationWebhook {
Copy link
Member

Choose a reason for hiding this comment

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

Here I see the code starting some alternative mode of skipper binary. So I would expect something like routesrv.Run() call nearby. Could you please kindly show me where it is? Is it possible to put it near this line of code?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the next line you can see 'webhook.Run()'

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I see webhook.Run(), but I would expect to see routesrv.Run() too. Both of them, basically.
From my side it makes some sense, because both "webhook" and "routesrv" are some "custom mode" of running skipper binary.

Copy link
Member Author

Choose a reason for hiding this comment

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

why should they run together? Every mode is a separate one and everyone run it's own required place and own required options

@szuecs
Copy link
Member

szuecs commented Feb 6, 2024

I think it's better to have a separate component as we have today.
It serves a different aspect of the control plane and I don't see why we need to merge it like this.

@szuecs szuecs added do-not-merge breaking change Breaking major change should be planned by a coordinationissue labels Feb 6, 2024
Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architectural all changes in the hot path, big changes in the control plane, control flow changes in filters breaking change Breaking major change should be planned by a coordinationissue do-not-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants