⚠️ (go/v4) decouple webhooks from APIs (Move Webhooks from `api/<version>` or `api/<group>/<version>` to `internal/webhook/<version>` or `internal/webhook/<group>/<version>`) #4150

camilamacedo86 · 2024-09-09T19:22:57Z

This PR decouples the webhooks from the API, aligning with the recent breaking changes introduced in controller-runtime to ensure that kubebuilder still compatbile with its next release. Webhooks are now scaffolded under internal/webhook to comply with the latest standards.

Context:

Controller-runtime deprecated and removed the webhook methods in favor of CustomInterfaces (see controller-runtime#2641). The motivation for this change is outlined in controller-runtime#2596.

See that the current master branch already reflects these changes, using the CustomInterfaces: kubebuilder#4060.

Changes:

Webhooks are now scaffolded in internal/webhook/<version> or internal/webhook/<group>/<version>.
However, to ensure backwards compatibility, a new --legacy flag is introduced. Running kubebuilder create webhook [options] --legacy will scaffold webhooks in the legacy location for projects that need to retain the old structure. However, users will still to address the breaking changes in the source code by replacing the old methods by the new CustomInterfaces.

Example:

(single group / default layout)

$ tree
.
...
├── api
│   └── v1
│       ├── admiral_types.go
│       ├── captain_types.go
│       ├── firstmate_types.go
│       ├── groupversion_info.go
│       └── zz_generated.deepcopy.go
...
├── internal
│   ├── controller
...
│   └── webhook
│       └── v1
│           ├── admiral_webhook.go
│           ├── admiral_webhook_test.go
│           ├── firstmate_webhook.go
│           ├── firstmate_webhook_test.go
│           └── webhook_suite_test.go
...

Notes:

If you do not want to change the layout (move your webhooks to internal/webhook then, ) then use the flag --legacy and ensure that you upgrade the source code to use the CustomInterfaces as the following examples. However, be aware that the legacy path is deprecated and will no longer used in future versions of the Golang Plugins (i.e. go/v5):

Defaulting Webhooks: example - changes
Programatically Validation Webhooks; example - changes

Closes: #4062

k8s-ci-robot · 2024-09-09T19:23:00Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

fischor

Reviewed only the changes under pkg/ for now. The rest I can review later.
Looks good and seems to work in general.

pkg/plugins/golang/v4/scaffolds/internal/templates/webhooks/webhook.go

pkg/plugins/golang/v4/scaffolds/internal/templates/webhooks/webhook_suitetest.go

pkg/plugins/golang/v4/scaffolds/internal/templates/webhooks/webhook_test_template.go

camilamacedo86 · 2024-09-20T08:39:23Z

HI @fischor

Thank you a lot for your review.
See that we added a test to ensure that the scaffolds with the the flag --legacy=true are working fine as before and the make target which allow us to troubleshooting the scenarios if we need locally.

docs/book/src/multiversion-tutorial/testdata/project/internal/webhook/v1/cronjob_conversion.go

testdata/project-v4-multigroup/internal/webhook/crew/v1/captain_webhook.go

varshaprasad96

Some nits, rest looks good! (reviewed till legacy-webhook-path.sh, will come back and review the rest tomorrow).

Thanks @camilamacedo86 for taking care of this!

.../book/src/multiversion-tutorial/testdata/project/internal/webhook/v2/cronjob_webhook_test.go

varshaprasad96 · 2024-09-21T18:03:18Z

hack/docs/internal/cronjob-tutorial/generate_cronjob.go

-	"context"
-	"fmt"
+	"fmt"`,
+		`


Nit: I think its taken care by running go fmt in the end, but the file still has fmt and context as imports. So probably we can revert back the first two statements? (

kubebuilder/docs/book/src/cronjob-tutorial/testdata/project/internal/webhook/v1/cronjob_webhook.go

Lines 21 to 22 in d6f6c34

"context"

"fmt"

)

Previously, we were using Replace. Now, we are using Insert to be more precise and avoid breaking the code. This allows us to insert the rest of the text starting from a specific point. So, I think that is right, but let me know if I missing something.

pkg/plugins/golang/v4/scaffolds/internal/templates/main.go

pkg/plugins/golang/v4/scaffolds/internal/templates/webhooks/webhook.go

camilamacedo86 · 2024-09-21T19:25:57Z

Hi @varshaprasad96 and @sbueringer

Thank you a lot for your time and amazing review !!! 🥇
I think is all addressed now. Please, let me know wdyt and if you think that is good enough to get merged.

…sion>` or `api/<group>/<version>` to `internal/webhook/<version>` or `internal/webhook/<group>/<version>` This PR decouples the webhooks from the API, aligning with the recent breaking changes introduced in controller-runtime to ensure that kubebuilder still compatbile with its next release. Webhooks are now scaffolded under `internal/webhook` to comply with the latest standards. **Context:** Controller-runtime deprecated and removed the webhook methods in favor of CustomInterfaces (see [controller-runtime#2641](kubernetes-sigs/controller-runtime#2641)). The motivation for this change is outlined in [controller-runtime#2596](kubernetes-sigs/controller-runtime#2596). See that the current master branch already reflects these changes, using the CustomInterfaces: [kubebuilder#4060](kubernetes-sigs#4060). **Changes:** - Webhooks are now scaffolded in `internal/webhook/<version>` or `internal/webhook/<group>/<version>`. - However, to ensure backwards compatibility, a new `--legacy` flag is introduced. Running `kubebuilder create webhook [options] --legacy` will scaffold webhooks in the legacy location for projects that need to retain the old structure. However, users will still to address the breaking changes in the source code by replacing the old methods by the new CustomInterfaces.

sbueringer · 2024-09-23T08:38:36Z

(only reviewed the projects in testdata/*)

Looks good from my side

camilamacedo86 · 2024-09-23T09:22:22Z

Hi @sbueringer

(only reviewed the projects in testdata/*)

By looking at the testdata and docs samples you can check the full result of the changes
So that seems more than enough !

The legacy-webhook-path.sh, just to do the same scaffolds that we do in the testdata but with the legacy flag.
So that we could add the GitHub action to run it and after run make tests to ensure that the scaffold is not broken with the legacy one. However, I would expected users moving forward and do not use it in 99% of the cases since C+R has breaking changes anyway to be addressed regards webhooks. The flag is only to help out in case they need/wish to.

mogsie

I always thought it odd that the validation code was in the api :) ✨ 👏

mogsie · 2024-09-23T19:17:10Z

docs/book/src/cronjob-tutorial/testdata/project/internal/webhook/v1/cronjob_webhook_test.go

@@ -19,32 +19,35 @@ package v1
 import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+
+	batchv1 "tutorial.kubebuilder.io/project/api/v1"


This is a minor thing, but often, batchv1 is used when importing Job, and the actual batch/v1 package. Wouldn't it be more normal (in the context of this test) just calling this import "v1" or "apiv1"? It's the project's own API right?

Edit: I see batchv1 is already used extensively, so maybe that should be left for another discussion.

mogsie · 2024-09-23T19:18:33Z

docs/book/src/cronjob-tutorial/testdata/project/internal/webhook/v1/cronjob_webhook_test.go

 	// TODO (user): Add any additional imports if needed
 )

 var _ = Describe("CronJob Webhook", func() {
 	var (
-		obj       *CronJob
-		oldObj    *CronJob
+		obj       = &batchv1.CronJob{}


tiny nit, makes no difference.

Suggested change

obj = &batchv1.CronJob{}

obj *batchv1.CronJob

mogsie · 2024-09-23T19:29:33Z

hack/docs/internal/cronjob-tutorial/webhook_implementation.go

@@ -188,8 +190,11 @@ const webhookTestCreateDefaultingFragment = `// TODO (user): Add logic for defau
 		// Example:
 		// It("Should apply defaults when a required field is empty", func() {
 		//     By("simulating a scenario where defaults should be applied")
+		//     By("simulating a scenario where defaults should be applied")


duplicate line?

mogsie · 2024-09-23T19:31:11Z

hack/docs/internal/cronjob-tutorial/webhook_implementation.go

@@ -235,20 +240,20 @@ const webhookTestingValidatingTodoFragment = `// TODO (user): Add logic for vali
 		// It("Should deny creation if a required field is missing", func() {
 		//     By("simulating an invalid creation scenario")
 		//     obj.SomeRequiredField = ""
-		//     Expect(obj.ValidateCreate(ctx)).Error().To(HaveOccurred())
+		//     Expect(validator.ValidateCreate(ctx, obj)).Error().To(HaveOccurred())


Not directly because of this PR, but this is my previous work on simplifying the gomega assertions. Do you want to replace this with the longer form where err and validations are Expected independently?

mogsie · 2024-09-23T19:49:12Z

testdata/project-v4-multigroup/internal/webhook/example.com/v1alpha1/memcached_webhook.go

@@ -25,16 +25,17 @@ import (
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
+	examplecomv1alpha1 "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/api/example.com/v1alpha1"


Again, I'm not sure I have the full context, but I would call it "apiv1alpha1" or just "v1alpha1".

mogsie · 2024-09-23T20:10:16Z

pkg/plugins/golang/v4/scaffolds/internal/templates/webhooks/webhook_suitetest.go

+	// wait for the webhook server to get ready.
+	dialer := &net.Dialer{Timeout: time.Second}
+	addrPort := fmt.Sprintf("%s:%s", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort)
+	Eventually(func() error {


Consider using (g Gomega)

Suggested change

Eventually(func() error {

Eventually(func(g gomega) {

and g.Expect() for error checking.

k8s-ci-robot · 2024-09-23T20:18:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, mogsie

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

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 9, 2024

k8s-ci-robot requested review from everettraven and rashmigottipati September 9, 2024 19:23

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 9, 2024

camilamacedo86 force-pushed the webhooks-path-change branch 2 times, most recently from e607dd5 to bb55494 Compare September 10, 2024 02:04

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 10, 2024

camilamacedo86 force-pushed the webhooks-path-change branch 3 times, most recently from 4b5368a to 86dce22 Compare September 10, 2024 18:37

camilamacedo86 changed the title ~~IGNORE - WIP - TEST~~ WIP ⚠️ decouple webhooks from APIs Sep 10, 2024

camilamacedo86 marked this pull request as ready for review September 10, 2024 18:38

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2024

camilamacedo86 force-pushed the webhooks-path-change branch 4 times, most recently from 8c0d1f8 to dbde977 Compare September 14, 2024 09:15

camilamacedo86 changed the title ~~WIP ⚠️ decouple webhooks from APIs~~ ⚠️ (go/v4) decouple webhooks from APIs - Move Webhooks from api/<version> or api/<group>/<version> to internal/webhook/<version> or internal/webhook/<group>/<version> Sep 14, 2024

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 14, 2024

camilamacedo86 force-pushed the webhooks-path-change branch from dbde977 to e382294 Compare September 14, 2024 10:27

camilamacedo86 requested review from Kavinjsir, varshaprasad96, joelanford, sbueringer and troy0820 September 14, 2024 10:28

camilamacedo86 force-pushed the webhooks-path-change branch from e382294 to fc621e6 Compare September 14, 2024 10:33

camilamacedo86 requested review from erikgb and joelanford September 14, 2024 12:06

camilamacedo86 added the release-blocker label Sep 15, 2024

This was referenced Sep 15, 2024

create webhook: return error when used with a core type #2141

Open

✨ Add support to scaffold controllers for External Types #4171

Open

This was referenced Sep 16, 2024

✨ Webhooks for external APIs #4176

Open

Unable to Add Additional Webhooks Without Forcing Re-scaffold #4146

Open

fischor reviewed Sep 20, 2024

View reviewed changes

camilamacedo86 force-pushed the webhooks-path-change branch 3 times, most recently from 5e42196 to 67221fc Compare September 20, 2024 08:37

camilamacedo86 requested a review from fischor September 20, 2024 08:38

camilamacedo86 force-pushed the webhooks-path-change branch 2 times, most recently from d0f46d2 to 1c7da29 Compare September 20, 2024 08:48

sbueringer reviewed Sep 21, 2024

View reviewed changes

docs/book/src/multiversion-tutorial/testdata/project/internal/webhook/v1/cronjob_conversion.go Outdated Show resolved Hide resolved

testdata/project-v4-multigroup/internal/webhook/crew/v1/captain_webhook.go Show resolved Hide resolved

camilamacedo86 force-pushed the webhooks-path-change branch from 1c7da29 to d6f6c34 Compare September 21, 2024 16:27

varshaprasad96 reviewed Sep 21, 2024

View reviewed changes

camilamacedo86 force-pushed the webhooks-path-change branch 3 times, most recently from 11684fe to 56c00e3 Compare September 21, 2024 19:24

camilamacedo86 requested review from sbueringer and varshaprasad96 September 21, 2024 19:26

camilamacedo86 force-pushed the webhooks-path-change branch from 56c00e3 to 982d23d Compare September 22, 2024 06:36

mogsie approved these changes Sep 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ (go/v4) decouple webhooks from APIs (Move Webhooks from `api/<version>` or `api/<group>/<version>` to `internal/webhook/<version>` or `internal/webhook/<group>/<version>`) #4150

⚠️ (go/v4) decouple webhooks from APIs (Move Webhooks from `api/<version>` or `api/<group>/<version>` to `internal/webhook/<version>` or `internal/webhook/<group>/<version>`) #4150

camilamacedo86 commented Sep 9, 2024 •

edited

Loading

k8s-ci-robot commented Sep 9, 2024

fischor left a comment

camilamacedo86 commented Sep 20, 2024 •

edited

Loading

varshaprasad96 left a comment

varshaprasad96 Sep 21, 2024

camilamacedo86 Sep 21, 2024

camilamacedo86 commented Sep 21, 2024

sbueringer commented Sep 23, 2024

camilamacedo86 commented Sep 23, 2024 •

edited

Loading

mogsie left a comment

mogsie Sep 23, 2024

mogsie Sep 23, 2024

mogsie Sep 23, 2024

mogsie Sep 23, 2024

mogsie Sep 23, 2024

mogsie Sep 23, 2024

k8s-ci-robot commented Sep 23, 2024

⚠️ (go/v4) decouple webhooks from APIs (Move Webhooks from api/<version> or api/<group>/<version> to internal/webhook/<version> or internal/webhook/<group>/<version>) #4150

Are you sure you want to change the base?

⚠️ (go/v4) decouple webhooks from APIs (Move Webhooks from api/<version> or api/<group>/<version> to internal/webhook/<version> or internal/webhook/<group>/<version>) #4150

Conversation

camilamacedo86 commented Sep 9, 2024 • edited Loading

k8s-ci-robot commented Sep 9, 2024

fischor left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Sep 20, 2024 • edited Loading

varshaprasad96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 commented Sep 21, 2024

sbueringer commented Sep 23, 2024

camilamacedo86 commented Sep 23, 2024 • edited Loading

mogsie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 23, 2024

⚠️ (go/v4) decouple webhooks from APIs (Move Webhooks from `api/<version>` or `api/<group>/<version>` to `internal/webhook/<version>` or `internal/webhook/<group>/<version>`) #4150

⚠️ (go/v4) decouple webhooks from APIs (Move Webhooks from `api/<version>` or `api/<group>/<version>` to `internal/webhook/<version>` or `internal/webhook/<group>/<version>`) #4150

camilamacedo86 commented Sep 9, 2024 •

edited

Loading

camilamacedo86 commented Sep 20, 2024 •

edited

Loading

camilamacedo86 commented Sep 23, 2024 •

edited

Loading