Move webhook registration behind feature gate flag #5099
Conversation
Move webhook registration behind feature gate flags similar to controller registration. Signed-off-by: Bryan Cox <[email protected]>
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @bryan-cox. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
| setupLog.Error(err, "unable to create webhook", "webhook", "AzureManagedMachinePool") | ||
| os.Exit(1) | ||
| } | ||
| // NOTE: AzureManagedCluster is behind AKS feature gate flag; the webhook |
There was a problem hiding this comment.
Is this comment still valid or can it be removed? Looks like its from a few years back.
There was a problem hiding this comment.
Pretty sure this can be removed. 👍🏻
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
We use the webhooks to forbid creating resources disabled by feature flags. That's also what CAPI does so I think we should align with that: https://github.com/kubernetes-sigs/cluster-api/blob/be86b82e7e30a844bca141ff8bcdc450b0499549/exp/internal/webhooks/machinepool.go#L168. Does a user still get some kind of error here when they try to create an AzureMachinePool when the MachinePool flag is disabled? This seems fine as long as users do some extra work to ensure those CRDs are not installed at all when the feature flags are disabled, but that would force users to adapt to keep the existing behavior and clusterctl doesn't make that easy. Are you seeing any adverse behavior besides the error message? |
We aren't using AzureMachinePool. Yeah, we are seeing more than just the log message; the CAPZ pod restarts constantly. Here are some additional logs before the pod restarts: We have the MachinePool feature turned off in our pod deployment: |
|
FWIW the machines do get provisioned and join our cluster. The CAPZ pod just consistently restarts. |
|
CAPA seems to follow this same pattern as well - https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/4507c0bc7371dd44e7b7b719c393f86452be60dd/main.go#L323. |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 11c709afba588489dedad6cf904e145c3451676a
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5099 +/- ##
==========================================
- Coverage 51.17% 51.16% -0.01%
==========================================
Files 273 273
Lines 24616 24618 +2
==========================================
Hits 12597 12597
- Misses 11234 11236 +2
Partials 785 785 ☔ View full report in Codecov by Sentry. |
|
If they're no longer reachable, can we remove the checks in the webhooks that the corresponding feature gates for resources are enabled? e.g. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Move webhook registration behind feature gate flags similar to controller registration.
Without this PR, from a self-managed / externally managed infrastructure perspective, if you want to exclude the CRDs behind the MachinePool and ASOAPI feature flags, you'll get an error because the webhook for them is still registered.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
Release note: