[rollout-operator] Add support for webhooks #2785
Conversation
m1rp
commented
Nov 14, 2023
m1rp
commented
- updated deployment and service to expose correct ports
- added role, clusterroles and associated bindings (needed for CA creation)
- added webhooks needed for downscale/no-downscale
- updated readme
|
|
m1rp
force-pushed
the
serverTLS-ports
branch
2 times, most recently
from
20347ac to
3680c2e
Compare
| - apiGroups: [apps] | ||
| apiVersions: [v1] | ||
| operations: [UPDATE] | ||
| resources: |
There was a problem hiding this comment.
I think this should only need statefulsets and statefulsets/scale (that's how we have it configured internally).
There was a problem hiding this comment.
🤔 this part was mostly a copy/paste job from the docs
if this PR needs updating then the docs are potentially slightly off.
| namespace: {{.Release.Namespace}} | ||
| path: /admission/prepare-downscale | ||
| port: 443 | ||
| failurePolicy: Fail |
There was a problem hiding this comment.
This may create a deadlock when applying it. If this webhook is created but rollout-operator isn't updated yet, then the rollout operator won't have the https endpoint and will always fail, rejecting any operation like trying to update the rollout operator.
It should be deployed with Ignore strategy first, and then with Fail. Note that Ignore only ignores failed calls, it doesn't ignore explicit rejections.
There was a problem hiding this comment.
is this only true for the mutatingwebhookconfiguration.yaml or does this also apply to the validatingwebhookconfiguration.yaml?
is something like this:
{{- if eq((lookup "v1" "MutatingWebhookConfiguration" "{{.Release.Namespace}}" ) "" ) }}
failurePolicy: Ignore
{{- else }}
failurePolicy: Fail
{{- end }}
what you had in mind?
There was a problem hiding this comment.
This applies to both webhooks.
I don't understand the condition of your diff (not saying it's wrong, I'm just not familiar with Helm), but I could imagine this as an extra setting, something to be disabled by default, and to be enabled after rolling out for more robustness (so it wouldn't allow everything if rollout-operator is just crashing).
Also, your diff says Ignore in both branches :D
There was a problem hiding this comment.
thanks for the update/clarification, i think i get what you're saying.
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
Signed-off-by: sam clulow <sam.clulow@springernature.com>
| {{- $failpol := (lookup "v1" "ValidatingWebhookConfiguration" "{{.Release.Namespace}}" "no-downscale-{{.Release.Namespace}}") -}} | ||
| {{- if empty $failpol }} |
There was a problem hiding this comment.
I think it would be nice to have an explicit switch that would disable the failurePolicy, just in case something goes wrong here (like: the webhook is created, but the rollout-operator isn't updated so it's not updating its TLS certs)
There was a problem hiding this comment.
by disable, do you mean set failurePolicy: Ignore?
sorry i'm a bit out of the loop honestly this PR has been going on for a while 😄
There was a problem hiding this comment.
also should the same switch apply to validatingwebhookconfiguration.yaml and mutatingwebhookconfiguration.yaml ?
Co-authored-by: Cem Deniz Kabakci <cem.kabakci@springer.com> Signed-off-by: sam clulow <sam.clulow@springernature.com>
|
could i get an update on this? |
Signed-off-by: sam clulow <m1rp@users.noreply.github.com>
