Skip to content

feat(config): add webhook as kustomize component#122

Merged
k8s-ci-robot merged 4 commits intokubernetes-sigs:mainfrom
AvineshTripathi:feat/webhook-config
Feb 19, 2026
Merged

feat(config): add webhook as kustomize component#122
k8s-ci-robot merged 4 commits intokubernetes-sigs:mainfrom
AvineshTripathi:feat/webhook-config

Conversation

@AvineshTripathi
Copy link
Contributor

@AvineshTripathi AvineshTripathi commented Feb 7, 2026 •
edited
Loading

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_TLS needs to be true.

Related Issue

Testing

Checklist

  • make test passes
  • make lint passes

@AvineshTripathi
feat(config): add webhook as kustomize component
4aec454 
Signed-off-by: AvineshTripathi <avineshtripathi1@gmail.com>
@netlify
Copy link

netlify bot commented Feb 7, 2026 •
edited
Loading

✅ Deploy Preview for node-readiness-controller canceled.

Name Link
🔨 Latest commit cacffab
🔍 Latest deploy log https://app.netlify.com/projects/node-readiness-controller/deploys/69970cea3f43e80008c8318c

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 7, 2026
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 7, 2026
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 7, 2026

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 7, 2026
AvineshTripathi
AvineshTripathi commented Feb 7, 2026
View reviewed changes
config/webhook/webhook_patch.yaml
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
Copy link
Contributor Author

@AvineshTripathi AvineshTripathi Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Contributor

@ajaysundark ajaysundark Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non blocking comment-

I need to make myself familiarize with the replacements field. but from the description seems it could work? -

Substitute field(s) in N target(s) with a field from a source.

we are looking to invest in helm for long-term so it's not urgent to handle this right away. maybe create a follow up backlog?

@ajaysundark
Copy link
Contributor

ajaysundark commented Feb 7, 2026

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 7, 2026
@ajaysundark ajaysundark requested review from Priyankasaggu11929 and ajaysundark and removed request for dchen1107 and haircommander February 7, 2026 23:59
@ajaysundark
Copy link
Contributor

ajaysundark commented Feb 8, 2026

@Priyankasaggu11929 had interests in testing the validation webhook, could you find time for this review?

@Priyankasaggu11929
Copy link
Member

Priyankasaggu11929 commented Feb 8, 2026

@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.

Priyankasaggu11929
Priyankasaggu11929 reviewed Feb 10, 2026
View reviewed changes
Priyankasaggu11929
Priyankasaggu11929 reviewed Feb 10, 2026
View reviewed changes
Priyankasaggu11929
Priyankasaggu11929 reviewed Feb 10, 2026
View reviewed changes
Priyankasaggu11929
Priyankasaggu11929 reviewed Feb 10, 2026
View reviewed changes
Priyankasaggu11929
Priyankasaggu11929 reviewed Feb 10, 2026
View reviewed changes
Priyankasaggu11929
Priyankasaggu11929 reviewed Feb 10, 2026
View reviewed changes
Priyankasaggu11929
Priyankasaggu11929 reviewed Feb 10, 2026
View reviewed changes
Priyankasaggu11929
Priyankasaggu11929 reviewed Feb 10, 2026
View reviewed changes
@Priyankasaggu11929
Copy link
Member

Priyankasaggu11929 commented Feb 10, 2026

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?
(Maybe we later create MAP Policy/Policy-binding scoped to just these components?)

@AvineshTripathi
feat(config): remove service monitor config
6cdc0a9 
Signed-off-by: AvineshTripathi <avineshtripathi1@gmail.com>
@ajaysundark ajaysundark mentioned this pull request Feb 16, 2026
Merged
2 tasks
@Priyankasaggu11929
Copy link
Member

Priyankasaggu11929 commented Feb 16, 2026

LGTM from my side. Thanks!

@AvineshTripathi
nit(config): trim metrics svc name
3630fda 
Signed-off-by: AvineshTripathi <avineshtripathi1@gmail.com>
ajaysundark
ajaysundark reviewed Feb 18, 2026
View reviewed changes
Copy link
Contributor

@ajaysundark ajaysundark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Makefile
deploy-full: deploy ## Deploy with all features: metrics, TLS, webhook.

.PHONY: undeploy-full
undeploy-full: ENABLE_METRICS=true
Copy link
Contributor

@ajaysundark ajaysundark Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like undeploy target already ignores if resources are not found? would it then suffice to just keep one undeploy?

config/webhook/webhook_patch.yaml
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
Copy link
Contributor

@ajaysundark ajaysundark Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non blocking comment-

I need to make myself familiarize with the replacements field. but from the description seems it could work? -

Substitute field(s) in N target(s) with a field from a source.

we are looking to invest in helm for long-term so it's not urgent to handle this right away. maybe create a follow up backlog?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2026
@AvineshTripathi AvineshTripathi mentioned this pull request Feb 18, 2026
Closed
11 tasks
ajaysundark
ajaysundark reviewed Feb 19, 2026
View reviewed changes
@ajaysundark
Copy link
Contributor

ajaysundark commented Feb 19, 2026

/approve
/hold

to address the comments at Makefile. but dont think it needs to be blocking, feel free to unhold to handle merge

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2026
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 19, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ajaysundark, AvineshTripathi

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2026
@AvineshTripathi @ajaysundark
doc: update makefile comment
cacffab 
Co-authored-by: ajaysundar.k <ajaysundar.k@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2026
@ajaysundark
Copy link
Contributor

ajaysundark commented Feb 19, 2026

/lgtm
/unhold

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 19, 2026
@k8s-ci-robot k8s-ci-robot merged commit 996937a into kubernetes-sigs:main Feb 19, 2026
7 checks passed
@AvineshTripathi AvineshTripathi mentioned this pull request Feb 25, 2026
Open
7 tasks
@Priyankasaggu11929 Priyankasaggu11929 mentioned this pull request Feb 25, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ajaysundark ajaysundark ajaysundark left review comments

@Priyankasaggu11929 Priyankasaggu11929 Priyankasaggu11929 left review comments

Assignees

@ajaysundark ajaysundark

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AvineshTripathi @k8s-ci-robot @ajaysundark @Priyankasaggu11929