Run IPPool webhook handler when SecondaryNetwork is enabled #6691
Conversation
luolanzone
added
kind/bug
|
I think this bug fix should be back-ported in case the users are already using IPPools CRs in their environments, the bug will block users to do any CR updates as well. |
pkg/apiserver/apiserver.go
Outdated
| @@ -299,6 +299,8 @@ func installHandlers(c *ExtraConfig, s *genericapiserver.GenericAPIServer) { | |||
| s.Handler.NonGoRestfulMux.HandleFunc("/endpoint", endpoint.HandleFunc(c.endpointQuerier)) | |||
| // Webhook to mutate Namespace labels and add its metadata.name as a label | |||
| s.Handler.NonGoRestfulMux.HandleFunc("/mutate/namespace", webhook.HandleMutationLabels()) | |||
| s.Handler.NonGoRestfulMux.HandleFunc("/convert/ippool", webhook.HandleCRDConversion(ipam.ConvertIPPool)) | |||
There was a problem hiding this comment.
I think validatingwebhook and conversionwebhook should enabled/disabled together if the APIs can be called, otherwise it would just fail with another error.
But I think it's intentional to not install the webhooks when the feature that uses the API are disabled, and this is not the only case where handlers are controlled by feature gates.
That being said, there is indeed a problem in the current code that IPPool is now used by not only FlexibleIPAM but also SecondaryNetwork, so the condition should be updated.
There was a problem hiding this comment.
@tnqn, so you mean to we should still block the webhook with a feature gate but update the condition with checking both FlexibleIPAM and SecondaryNetwork, and register it as long as one of them is enabled? I can update the commit. But I feel the conversion webhook failure leads confusion, not sure what's the best practice in K8s upstream, if we can't not define the webhook section conditionally, maybe we can reply a no-op conversion response when the feature is not enabled?
There was a problem hiding this comment.
Think it a little bit more, no-ops response may also cause issues when the users enable the feature gate later and existing CRs may not be correct. I will update the commit with the right condition first. maybe check later to see if there is any best practice for this scenario.
There was a problem hiding this comment.
Do we also need to back port this with the new condition?
There was a problem hiding this comment.
Controlling the webhook handlers under feature gate is to isolate experimental code specific to a feature, as being alpha means they are not ready for production use yet. Running webhook code unconditionally breaks the principle and may risk the whole program.
Besides, in some cases the webhook handlers are stateful, meaning that they involve more code specific to a feature, like the networkpolicy validator. Almost all code for the feature needs to run to support the validator.
Sometimes it may make sense to install webhook handlers unconditionally, like when the code becomes very common and it's considered low risk. But avoiding webhook failure is not a valid exception, because we do need to validate and convert the requests, and the code does need to be feature gated. On the other hand, why would users use an API when none of the corresponding feature gates are enabled? AFAIK, the List API is the only exception as some discovery controllers may call it.
I think it can be backported but I guess IPPool and SecondaryNetwork hasn't really been used, because users would fail at the first step due to the issue.
Signed-off-by: Lan Luo <[email protected]>
luolanzone
force-pushed
the
fix-ippool-conversion-failure
branch
from
1dadfb8
to
cbca8cf
Compare
luolanzone
changed the title
|
/skip-all |
|
Can one of the admins verify this patch? |
3 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
…io#6691) Signed-off-by: Lan Luo <[email protected]>
…io#6691) Signed-off-by: Lan Luo <[email protected]>
…io#6691) Signed-off-by: Lan Luo <[email protected]>
…6737) Signed-off-by: Lan Luo <[email protected]>
…6738) Signed-off-by: Lan Luo <[email protected]>
…io#6691) Signed-off-by: Lan Luo <[email protected]>
…6750) Signed-off-by: Lan Luo <[email protected]>
…io#6691) Signed-off-by: Lan Luo <[email protected]>
IPPool is also used when the SecondaryNetwork is enabled, so fix the condition to run IPPool webhook handler as well when SecondaryNetwork feature gate is enabled.