✨ Allow customizing generated webhook configuration's name #1002

davidxia · 2024-07-10T18:00:07Z

example usage

❯ GOBIN=(pwd)/bin go install ./cmd/*

❯ ./bin/controller-gen webhook -w

Webhook

+kubebuilder:webhook:admissionReviewVersions=<[]string>,failurePolicy=<string>,groups=<[]string>[,matchPolicy=<string>],mutating=<bool>,name=<string>[,path=<string>][,reinvocationPolicy=<string>],resources=<[]string>[,sideEffects=<string>][,timeoutSeconds=<int>][,url=<string>],verbs=<[]string>,versions=<[]string>[,webhookVersions=<[]string>]  package  specifies how a webhook should be served.  
+kubebuilder:webhookconfiguration:mutating=<bool>[,name=<string>]                                                                                                                                                                                                                                                                                        package  specifies how a webhook should be served.

contributes to #865

k8s-ci-robot · 2024-07-10T18:00:16Z

Welcome @davidxia!

It looks like this is your first PR to kubernetes-sigs/controller-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-07-10T18:00:16Z

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

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.

davidxia · 2024-07-10T18:02:30Z

@sbueringer thanks for your tip. How does this look? If this looks good overall, I can write a test for it. Should the test be similar to this?

davidxia

tested manually by

GOBIN=(pwd)/bin go install ./cmd/*
copy the ./bin/controller-gen executable into a kubebuilder-scaffolded project's bin/controller-gen-vX.Y.Z path
remove the kubectl apply from the kubebuilder project's Makefile's deploy target

case 1: backwards compatibility

run make deploy and check that kind: MutatingWebhookConfiguration and kind: ValidatingWebhookConfiguration have the same names as before.

apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  name: project-mutating-webhook-configuration

apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  name: project-validating-webhook-configuration

case 2: specify marker only for MutatingWebhookConfiguration

add // +kubebuilder:webhookconfiguration:name=DEADBEEF,mutating=true to api/v1/cronjob_webhook.go

run make deploy and check that kind: MutatingWebhookConfiguration has the following.

apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  name: project-DEADBEEF

instead of

apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  name: project-mutating-webhook-configuration

check that kind: ValidatingWebhookConfiguration has the same name as before: project-validating-webhook-configuration

case 2: specify markers for both MutatingWebhookConfiguration and ValidatingWebhookConfiguration

add // +kubebuilder:webhookconfiguration:name=DEADBEEF,mutating=true and // +kubebuilder:webhookconfiguration:name=FOOBAR,mutating=false to api/v1/cronjob_webhook.go

run make deploy and check for the following.

apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  name: project-DEADBEEF

...

apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  name: project-FOOBAR

case 3: add multiple `kubebuilder:webhookconfiguration:mutating=true` markers

add both // +kubebuilder:webhookconfiguration:name=A,mutating=true and // +kubebuilder:webhookconfiguration:name=a,mutating=true (in that order) to api/v1/cronjob_webhook.go
run make deploy and check for the following. controller-gen sorts and uses the last one. a sorts to a later position than A.
```
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  name: project-a
```

pkg/webhook/parser.go

sbueringer · 2024-07-11T10:32:09Z

@sbueringer thanks for your tip. How does this look? If this looks good overall, I can write a test for it. Should the test be similar to this?

Yup. I think something similar to the test coverage in this PR would be good

Oh I think I missed something before. The existing kubebuilder:webhook marker is used to define individual webhooks in the object. While the name is global for the entire MutatingWebhookConfiguration / ValidatingWebhookConfiguration configuration. Might be a bit awkward to define the name of the overall obect on the marker that defines a single element in the webhook array.

WDYT about introducing a separate marker called kubebuilder:webhookconfiguration with a name field instead?

Looking at the issue, the service namespace (& maybe also name?) can be added to the Config struct though (just like Path)

pkg/webhook/parser.go

davidxia · 2024-07-11T14:53:10Z

pkg/webhook/parser.go

+			} else {
+				// The only possible version in supportedWebhookVersions is v1,
+				// so use it for all versioned types in this context.
+				objRaw = &admissionregv1.MutatingWebhookConfiguration{}
+				objRaw.SetName("mutating-webhook-configuration")
+			}


for backwards-compat

davidxia · 2024-07-11T14:53:19Z

pkg/webhook/parser.go

+			} else {
+				// The only possible version in supportedWebhookVersions is v1,
+				// so use it for all versioned types in this context.
+				objRaw = &admissionregv1.ValidatingWebhookConfiguration{}
+				objRaw.SetName("validating-webhook-configuration")
+			}


for backwards-compat

davidxia · 2024-07-11T14:54:53Z

WDYT about introducing a separate marker called kubebuilder:webhookconfiguration with a name field instead?

done, PTAL and let me know if that looks good

Looking at the issue, the service namespace (& maybe also name?) can be added to the Config struct though (just like Path)

Good idea. I can make another PR for that once we get something merged for this.

davidxia · 2024-07-17T13:09:25Z

@joelanford @vincepri do you have time to give this a quick initial review?

sbueringer · 2024-08-09T06:40:00Z

/ok-to-test

sbueringer

Sorry for the delay, looks good so far!

pkg/webhook/parser.go

davidxia · 2024-09-02T22:57:16Z

@sbueringer updated. Lmk if I'm missing anything!

sbueringer · 2024-09-10T18:00:38Z

@davidxia Looks good. PTAL at #1002 (comment)

Otherwise happy to merge

davidxia · 2024-09-10T20:42:43Z

@sbueringer thanks, but the bot message doesn't say why the commit message is invalid or how to fix. Are there instructions?

sbueringer · 2024-09-11T06:05:54Z

Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages.

I would just drop everything that contains "closes" or an issue number in the entire commit message

## example usage ``` ❯ GOBIN=(pwd)/bin go install ./cmd/* ❯ ./bin/controller-gen webhook -w Webhook +kubebuilder:webhook:admissionReviewVersions=<[]string>,failurePolicy=<string>,groups=<[]string>[,matchPolicy=<string>],mutating=<bool>,name=<string>[,path=<string>][,reinvocationPolicy=<string>],resources=<[]string>[,sideEffects=<string>][,timeoutSeconds=<int>][,url=<string>],verbs=<[]string>,versions=<[]string>[,webhookVersions=<[]string>] package specifies how a webhook should be served. +kubebuilder:webhookconfiguration:mutating=<bool>[,name=<string>] package specifies how a webhook should be served. ``` Signed-off-by: David Xia <[email protected]>

davidxia · 2024-09-11T21:51:36Z

@sbueringer that did the trick. Ready for review and/or merge now.

sbueringer · 2024-09-12T08:44:37Z

Thank you!

/lgtm
/approve

k8s-ci-robot · 2024-09-12T08:44:43Z

LGTM label has been added.

Git tree hash: 44d35f3ef0371f7cee392d80cd6259f75063bf7f

k8s-ci-robot · 2024-09-12T08:44:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidxia, sbueringer

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~~ [sbueringer]

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 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 10, 2024

k8s-ci-robot requested review from joelanford and vincepri July 10, 2024 18:00

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 10, 2024

davidxia commented Jul 10, 2024

View reviewed changes

pkg/webhook/parser.go Outdated Show resolved Hide resolved

davidxia force-pushed the webhook-name branch from a577c5c to 1006456 Compare July 11, 2024 14:39

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 11, 2024

davidxia commented Jul 11, 2024

View reviewed changes

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 Aug 9, 2024

sbueringer reviewed Aug 9, 2024

View reviewed changes

pkg/webhook/parser.go Show resolved Hide resolved

pkg/webhook/parser.go Outdated Show resolved Hide resolved

pkg/webhook/parser.go Outdated Show resolved Hide resolved

sbueringer reviewed Aug 9, 2024

View reviewed changes

pkg/webhook/parser.go Show resolved Hide resolved

davidxia force-pushed the webhook-name branch from 1006456 to 6d789cf Compare September 2, 2024 22:37

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 2, 2024

davidxia force-pushed the webhook-name branch from 6d789cf to 1d9d9cf Compare September 2, 2024 22:40

davidxia marked this pull request as ready for review September 2, 2024 22:57

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 2, 2024

sbueringer changed the title ~~✨ feat: allow customizing generated webhook's name~~ ✨ Allow customizing generated webhook configuration's name Sep 10, 2024

davidxia force-pushed the webhook-name branch 2 times, most recently from 521a807 to 8b7192e Compare September 10, 2024 20:42

davidxia force-pushed the webhook-name branch from 8b7192e to 287a105 Compare September 11, 2024 21:50

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 11, 2024

k8s-ci-robot assigned sbueringer Sep 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2024

k8s-ci-robot merged commit c69110a into kubernetes-sigs:main Sep 12, 2024
13 checks passed

davidxia deleted the webhook-name branch September 12, 2024 16:42

davidxia mentioned this pull request Sep 12, 2024

allow customizing generated webhook's name and its service's namespace and name #865

Open

This was referenced Oct 10, 2024

chore(deps): update all non-major dependencies cert-manager/aws-privateca-issuer#325

Open

chore(deps): update all non-major dependencies bmsiegel/aws-privateca-issuer#3

Open

renovate-bot mentioned this pull request Oct 28, 2024

chore(deps): update kubernetes runtime dependencies - autoclosed GoogleCloudPlatform/cloud-sql-proxy-operator#596

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Allow customizing generated webhook configuration's name #1002

✨ Allow customizing generated webhook configuration's name #1002

davidxia commented Jul 10, 2024 •

edited

Loading

k8s-ci-robot commented Jul 10, 2024

k8s-ci-robot commented Jul 10, 2024

davidxia commented Jul 10, 2024

davidxia left a comment •

edited

Loading

sbueringer commented Jul 11, 2024

davidxia Jul 11, 2024

davidxia Jul 11, 2024

davidxia commented Jul 11, 2024

davidxia commented Jul 17, 2024

sbueringer commented Aug 9, 2024

sbueringer left a comment

davidxia commented Sep 2, 2024

sbueringer commented Sep 10, 2024

davidxia commented Sep 10, 2024

sbueringer commented Sep 11, 2024 •

edited

Loading

davidxia commented Sep 11, 2024

sbueringer commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

✨ Allow customizing generated webhook configuration's name #1002

✨ Allow customizing generated webhook configuration's name #1002

Conversation

davidxia commented Jul 10, 2024 • edited Loading

example usage

k8s-ci-robot commented Jul 10, 2024

k8s-ci-robot commented Jul 10, 2024

davidxia commented Jul 10, 2024

davidxia left a comment • edited Loading

Choose a reason for hiding this comment

case 1: backwards compatibility

case 2: specify marker only for MutatingWebhookConfiguration

case 2: specify markers for both MutatingWebhookConfiguration and ValidatingWebhookConfiguration

case 3: add multiple kubebuilder:webhookconfiguration:mutating=true markers

sbueringer commented Jul 11, 2024

davidxia Jul 11, 2024

Choose a reason for hiding this comment

davidxia Jul 11, 2024

Choose a reason for hiding this comment

davidxia commented Jul 11, 2024

davidxia commented Jul 17, 2024

sbueringer commented Aug 9, 2024

sbueringer left a comment

Choose a reason for hiding this comment

davidxia commented Sep 2, 2024

sbueringer commented Sep 10, 2024

davidxia commented Sep 10, 2024

sbueringer commented Sep 11, 2024 • edited Loading

davidxia commented Sep 11, 2024

sbueringer commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

davidxia commented Jul 10, 2024 •

edited

Loading

davidxia left a comment •

edited

Loading

case 3: add multiple `kubebuilder:webhookconfiguration:mutating=true` markers

sbueringer commented Sep 11, 2024 •

edited

Loading