Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Path Configuration for Kubebuilder Webhook Markers for Core Types #4300

Closed
camilamacedo86 opened this issue Nov 7, 2024 · 8 comments
Closed
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@camilamacedo86
Copy link
Member

camilamacedo86 commented Nov 7, 2024

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

@camilamacedo86 camilamacedo86 added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 7, 2024
@damsien
Copy link
Contributor

damsien commented Nov 7, 2024

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

@damsien
Copy link
Contributor

damsien commented Nov 7, 2024

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
Copy link
Member Author

camilamacedo86 commented Nov 7, 2024

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
Copy link
Contributor

damsien commented Nov 7, 2024

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

@damsien
Copy link
Contributor

damsien commented Nov 7, 2024

/assign

@damsien
Copy link
Contributor

damsien commented Nov 7, 2024

@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
Copy link
Member Author

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 : #4295

@damsien
Copy link
Contributor

damsien commented Nov 7, 2024

Yes sure! I've made the PR #4301

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

No branches or pull requests

2 participants