feat: Always enforce {Validating,Mutating}WebhookConfiguration state
#566
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viccuad
force-pushed
the
feat-webhooks-reconciled
branch
3 times, most recently
from
bd0efb6
to
65a7712
Compare
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Mark `{Validating,Mutating}WebhookConfigurations` that are created ore
updated when reconciling a {validating,mutating} policy as owned by the
policy.
Note that this would mean they also get garbage-collected by Kubernetes,
which is ok.
The WebhookConfigurations get their OwnerReference set with
`controllerutil.SetControllerReference()` (don't confuse with
`controllerutil.SetOwnerReference`).
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
viccuad
force-pushed
the
feat-webhooks-reconciled
branch
from
65a7712
to
6bc440c
Compare
|
rebased on top of main. |
The {Validating,Mutating}WebhookConfigurations have an OwnerReference
that points to their policy.
Make the controller reconcile the WebhookConfigurations if they have any
change, by running the policy reconciler.
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
viccuad
force-pushed
the
feat-webhooks-reconciled
branch
from
6bc440c
to
1815f2b
Compare
jvanz
requested changes
Oct 30, 2023
jvanz
approved these changes
Oct 30, 2023
flavio
added a commit
that referenced
this pull request
Nov 2, 2023
Revert "Merge pull request #566 from viccuad/feat-webhooks-reconciled"
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fix #224
{Validating,Mutating}WebhookConfigurationswhen they are changed by any other means than our controller. This is done by triggering the policy reconciler, which ends up reconciling the WebhookConfigurations.{Validating,Mutating}WebhookConfigurationshave anOwnerReferencethat points to their policy.ownerReferences.blockOwnerDeletionfield is automatically set to true, which means that WebhookConfigurations block the deletion of policy CRDs (see here). This doesn't create any problem, and it's beneficial for us.Test
Webhooks.ClientConfig.Service.Pathfor example, to simulate a MITM. After the edit getting successfully applied (bumping theGeneration), our controller successfully updates the Webhook and restores its configuration.Additional Information
Tradeoff
Given that now the WebhookConfigurations have an
OwnerReference, we could defer to the K8s garbage collector instead of deleting the webhooks on our own via the policy reconciler. Still, I prefer to let our reconciler do it.Potential improvement
Note that the reconciliation of older WebhookConfigurations that don't have the
OwnerReferenceyet doesn't happen until they or the policies get an update. Once that happens, they will have theOwnerReferenceand be watched.This is the case for already-running Kubewarden deployments. We could try to treat this case (by adding controller code, refreshing all the policies resources, etc).