Path Configuration for Kubebuilder Webhook Markers for Core Types

camilamacedo86 commented 5 days ago

What broke? What's expected?

Kubebuilder currently does not distinguish between Core Types within the core API group and Core Types in other groups (e.g., apps) when generating webhook paths. This affects the accuracy of the path configuration in generated webhook markers. For further information see the doc; https://book.kubebuilder.io/reference/using_an_external_resource

Expected Behavior

For Core Types with Group set to core:
- The path should leave the group empty, formatted as /validate--v1-kind (e.g., /validate--v1-pod).
For Core Types where Group is not core:
- The path should include the group name, formatted as /validate-groupname-v1-kind (e.g., /validate-apps-v1-deployment for apps group types like Deployment).

Current Code Reference

This issue is present in the following line of code, where .Resource.Core is used to determine if the path should exclude the group name:

https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v4/scaffolds/internal/templates/webhooks/webhook.go#L201

Suggested Fix

Update the template logic to ensure that:
- Paths for core group types are formatted without a group name.
- Paths for other Core Types include the specific group name.

Reproducing this issue

mkdir -p deployment-validating-webhook
cd deployment-validating-webhook
kubebuilder init --repo deployment-validating-webhook
kubebuilder create webhook --group apps --version v1 --kind Deployment --programmatic-validation

Source: https://kubernetes.slack.com/archives/CAR30FCJZ/p1730954016456359

KubeBuilder (CLI) Version

4.3.0

PROJECT version

No response

Plugin versions

No response

Other versions

No response

Extra Labels

No response

damsien commented 5 days ago

I have a proposal for this. Can I create a PR?

damsien commented 5 days ago

Moreover, I can modify the scaffold a little bit to make the Core type default path looks like this:

// +kubebuilder:webhook:path=/validate-v1-pod,...

instead of:

// +kubebuilder:webhook:path=/validate--v1-pod,...

I don't know if it is a good idea

camilamacedo86 commented 5 days ago

Hi @damsien,

I am not sure if we can use // +kubebuilder:webhook:path=/validate-v1-pod,.. when it is a CoreType. As far I know that must to be // +kubebuilder:webhook:path=/validate--v1-pod,.. to work. Can you please do a test/POC to validate it and share with us?

However, regards your specific case and this issue specifically we probably just need to change the condition for from {{ if .Resource.Core }} to {{ if and .Resource.Core (eq .Resource.QualifiedGroup "core") }}

Something like

// +kubebuilder:webhook:{{ if ne .Resource.Webhooks.WebhookVersion "v1" }}webhookVersions={{"{"}}{{ .Resource.Webhooks.WebhookVersion }}{{"}"}},{{ end }}path=/validate-{{ if and .Resource.Core (eq .Resource.QualifiedGroup "core") }}{{ .Resource.Version }}-{{ lower .Resource.Kind }}{{ else }}{{ .QualifiedGroupWithDash }}-{{ .Resource.Version }}-{{ lower .Resource.Kind }}{{ end }},mutating=false,failurePolicy=fail,sideEffects=None,groups={{ if .Resource.Core }}""{{ else }}{{ .Resource.QualifiedGroup }}{{ end }},resources={{ .Resource.Plural }},verbs=create;update,versions={{ .Resource.Version }},name=v{{ lower .Resource.Kind }}-{{ .Resource.Version }}.kb.io,admissionReviewVersions={{ .AdmissionReviewVersions }}

damsien commented 5 days ago

That's what I've done! Let me check for the core path and I'll tell you!

damsien commented 5 days ago

/assign

damsien commented 5 days ago

@camilamacedo86 I just tried and yes it does not work because of these lines in the controller-runtime project: https://github.com/kubernetes-sigs/controller-runtime/blob/48ec3b71211f9fe1a313e34a9b44c39ca3adeec2/pkg/builder/webhook.go#L281-L289

Either I make a PR on the controller-runtime project or we keep the /validate--v1-pod path.

My PR for this issue (without including the core path "fix") is ready btw.

camilamacedo86 commented 5 days ago

Hi @damsien

Yes, that is the point. 👍 we should keep if is from the group "core" the --. Thank you for confirm that. But in the future if people want to change that in their own project they will be able to use : https://github.com/kubernetes-sigs/kubebuilder/issues/4295

damsien commented 5 days ago

Yes sure! I've made the PR #4301

kubernetes-sigs / kubebuilder