feat(config): add webhook as kustomize component#122
feat(config): add webhook as kustomize component#122AvineshTripathi wants to merge 3 commits intokubernetes-sigs:mainfrom
Conversation
Signed-off-by: AvineshTripathi <avineshtripathi1@gmail.com>
✅ Deploy Preview for node-readiness-controller canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AvineshTripathi The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @AvineshTripathi. 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. DetailsInstructions 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. |
| name: validating-webhook-configuration | ||
| annotations: | ||
| cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME) | ||
| cert-manager.io/inject-ca-from: nrr-system/nrr-serving-cert |
There was a problem hiding this comment.
There is a reason these values are hardcoded here: they were previously populated using vars. However, vars is now deprecated (kubernetes-sigs/kustomize#5046
), and we should move to using replacements. When running Kustomize, it also throws the following warning indicating this deprecation.
# Warning: 'vars' is deprecated. Please use 'replacements' instead. [EXPERIMENTAL] Run 'kustomize edit fix' to update your Kustomization automatically.
If we switch to replacements, we run into a dependency issue. Both the webhook and metrics services need these replacements, but they are individual components and may or may not be deployed together. Because of this, we cannot keep the replacements in config/default. Placing them in individual components also does not work, as it fails to populate the nrr- prefix in the DNS names and annotations.
So I thought a better solution would be to hardcode it. Open for suggestions
Other places:
|
/ok-to-test |
|
@Priyankasaggu11929 had interests in testing the validation webhook, could you find time for this review? |
yes, let me test it over the coming week and get back. |
|
One thing (and maybe not for the scope of this PR and can be handled in follow ups) - How would we manage scheduling cert-manager deployments on a tainted worker node (infact all other componets too?) The other PR #117 only handle injecting matching tolerations for daemonsets. And I don't think we can upfront manually insert matching tolerations in our provided kustomization components yaml? |
Signed-off-by: AvineshTripathi <avineshtripathi1@gmail.com>
|
LGTM from my side. Thanks! |
Signed-off-by: AvineshTripathi <avineshtripathi1@gmail.com>
Description
This PR converts the webhook config to a component like metrics and cert-manager and enables it. It also removes dependency of service monitor from the controller.
NOTE: webhooks require TLS, so cert manager crds installation is mandatory and
ENABLE_TLSneeds to betrue.Related Issue
Testing
Checklist
make testpassesmake lintpasses