Only create per-object webhooks for configured types

[erikgb]

Fixes #152

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[erikgb]

/test all

[erikgb]

/test pull-hnc-test

[erikgb]

/test pull-hnc-test

[erikgb]

/test pull-hnc-test

[erikgb]

/test pull-hnc-test

[erikgb]

@adrianludwin Existing tests are running fine now. Before writing some tests for this new feature, I would like to get your opinion on the approach. Because of the cert-rotator (as always 😉), I cannot move the now dynamic objects-webhook to a dedicated validating webhook configuration resource. IMO we should not mess with resources provisioned as part of HNC. That will never work with modern GitOps-tools like FluxCD/ArgoCD - as their reconcilers will compete with our internal reconciler. It "might" work if it was possible to create a webhook without rules (for the initial configuration), but it seems like this is not allowed by the API server. Update: This was not possible when defining the webhook with the kubebuilder markers, but I worked around this by extending the patch to create the complete webhook (without rules).

I would love to get your thoughts anyway, but I will continue writing tests now.

[adrianludwin]

@adrianludwin Existing tests are running fine now. Before writing some tests for this new feature, I would like to get your opinion on the approach. Because of the cert-rotator (as always 😉), I cannot move the now dynamic objects-webhook to a dedicated validating webhook configuration resource. IMO we should not mess with resources provisioned as part of HNC. That will never work with modern GitOps-tools like FluxCD/ArgoCD - as their reconcilers will compete with our internal reconciler.

Is this only a problem with cert-rotator? Or would cert-manager have the same problem? I would have thought that any controller that tried to inject certs into the webhook would have the same problem?

How does cert-manager work with gitops tools today? Is there no way to say "please ignore these fields?"

This was not possible when defining the webhook with the kubebuilder markers, but I worked around this by extending the patch to create the complete webhook (without rules).

Does this have something to do with the cert-rotator problem? I thought the caBundle needs to be injected to every new webhook (e.g. there isn't one caBundle per config) so why does moving these to a new config help? Just so Flux/Argo don't try to reconcile it away?

Instead of hand-writing a patch, why not create the entire webhook config in code? There's not a lot in a webhook config other than the list of webhooks.

[erikgb]

@adrianludwin Existing tests are running fine now. Before writing some tests for this new feature, I would like to get your opinion on the approach. Because of the cert-rotator (as always 😉), I cannot move the now dynamic objects-webhook to a dedicated validating webhook configuration resource. IMO we should not mess with resources provisioned as part of HNC. That will never work with modern GitOps-tools like FluxCD/ArgoCD - as their reconcilers will compete with our internal reconciler.

Is this only a problem with cert-rotator? Or would cert-manager have the same problem? I would have thought that any controller that tried to inject certs into the webhook would have the same problem?

I think my formulation was bad, sorry. The real problem is not with cert-rotator (or cert-manager). But with cert-manager, we could create a dedicated dynamic ValidatingWebhookConfiguration from code, and not mess with the VWC statically provisioned as part of the HNC deployment. Having multiple controllers working on the same resource is error-prone, but as long as the controllers do not touch the same fields, it can work. So without specifying the object-webhook rules in the static HNC configuration, this seems to work ok.

This was not possible when defining the webhook with the kubebuilder markers, but I worked around this by extending the patch to create the complete webhook (without rules).

Does this have something to do with the cert-rotator problem? I thought the caBundle needs to be injected to every new webhook (e.g. there isn't one caBundle per config) so why does moving these to a new config help? Just so Flux/Argo don't try to reconcile it away?

Instead of hand-writing a patch, why not create the entire webhook config in code? There's not a lot in a webhook config other than the list of webhooks.

WDYM? The kubebuilder markers does not allow a user to configure a webhook without rules. That is why I suggest to add the complete object-webhook using a patch. All other webhooks are sourced from kubebuilder-markers, so the webhook/manifests.yaml file is generated.

[adrianludwin]

I think my formulation was bad, sorry. The real problem is not with cert-rotator (or cert-manager). But with cert-manager, we could create a dedicated dynamic ValidatingWebhookConfiguration from code, and not mess with the VWC statically provisioned as part of the HNC deployment.

Why doesn't this work with cert-rotator as well?

Having multiple controllers working on the same resource is error-prone, but as long as the controllers do not touch the same fields, it can work. So without specifying the object-webhook rules in the static HNC configuration, this seems to work ok.

Yeah, this is what I'd expect as well. As long as the new controller doesn't respond to changes to caBundle, and cert-rotator doesn't respond to anything except changes to caBundle, we should be fine.

WDYM? The kubebuilder markers does not allow a user to configure a webhook without rules. That is why I suggest to add the complete object-webhook using a patch. All other webhooks are sourced from kubebuilder-markers, so the webhook/manifests.yaml file is generated.

Yup that sgtm. My only suggestion was that instead of hand-creating the object webhook in a patch, the HNC controller just has the entire object built-in. That is, if (say) hnc-object-controller-vwc is missing, HNC just creates it - it doesn't rely on anything being deployed by kubectl apply, Flux, or anyone else. This is what we do with HNCConfiguration - there's no HNC Config in the manifests, we just create one when HNC starts up if it doesn't already exist.

[adrianludwin]

... I guess the only problem with creating it automatically is that if users want to disable it somehow, they can't. Maybe that's a good reason to have HNC rely on the VWC config existing:

• Users can delete all object webhooks simply by deleting the config, and HNC won't recreate it
• We could let users add annotations to the VWC (e.g. "don't update" for temporary investigations). We don't need this on day 1, but it's a nice future option.

Given this, a hand-written, empty VWC sgtm!

[erikgb]

/test pull-hnc-test

[erikgb]

@adrianludwin I think the code is almost done and ready for review. All existing tests are passing, and I think this indirectly verifies that this feature works. The e2e-tests would never succeed if the objects-webhook was not reconciled correctly.
But I wonder if we should test some more corner cases in the integration tests? Writing unit tests for the new code seems almost impossible if I am supposed to follow the existing code style - which I think you prefer. That would eventually require more types to be exported as tests of reconcilers are currently located in a dedicated test-package. WDYT?

[mzain]

Hey @adrianludwin @erikgb when do we think this will be merged and available for use?
it is an important feature for our usage of HNC.

[erikgb]

@adrianludwin I have now reverted all suggested changes to the integration test setup in ac97676 - as you had a lot of comments/questions to the proposed changes. I am pretty sure CI should fail now. Feel free to look into it. Since we are no longer using HNC I am not prepared to work more on this PR.

[erikgb]

As expected, it fails now: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_hierarchical-namespaces/285/pull-hnc-test/1766783682422509568. If I remember this correctly, a webhook is required in the reconciler integration tests because the reconciler is now managing a webhook. And since the envTest represents a real control plane - we also need to mock/fake the webhook endpoint. I am going to revert the revert now, and hope you can look at this PR again - with the understanding that the new webhook setup IS required in the reconciler integration test.

[rjbez17]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erikgb, rjbez17

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rjbez17]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment