⚠️ (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

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.

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 :) ✨ 👏

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

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

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?

If you see areas for improvement, please feel free to raise a PR. We can definitely make further enhancements in a follow-up.

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

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.

I think we can update it in a follow-up once all the checks and adjustments for this test are made according to your suggestions. We will need to revise the entire content and tests related to this matter. What do you think? Could this be included as part of the issue you’re currently working on?

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

varshaprasad96

/lgtm

I think it's ready to be merged too! Thank you!

k8s-ci-robot · 2024-09-29T17:20:38Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,varshaprasad96]

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 force-pushed the webhooks-path-change branch 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

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

camilamacedo86 force-pushed the webhooks-path-change branch from 982d23d to 755429a Compare September 25, 2024 14:49

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

camilamacedo86 force-pushed the webhooks-path-change branch from 755429a to 0e472c3 Compare September 25, 2024 15:01

camilamacedo86 force-pushed the webhooks-path-change branch from 0e472c3 to 0abd1a8 Compare September 25, 2024 15:06

varshaprasad96 approved these changes Sep 29, 2024

View reviewed changes

k8s-ci-robot assigned varshaprasad96 Sep 29, 2024

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

camilamacedo86 merged commit 6afb09d into kubernetes-sigs:master Oct 1, 2024
23 of 25 checks passed

camilamacedo86 deleted the webhooks-path-change branch October 1, 2024 15:46

EronWright mentioned this pull request Oct 4, 2024

Set some default resource requests on the workspace pod pulumi/pulumi-kubernetes-operator#707

Merged

This was referenced Oct 10, 2024

Proposed Changes for 4.3.0 Release #4214

Closed

📖 Update roadmap_2024.md - Complete goal to align webhook scaffold with the latest changes in controller-runtime #4216

Merged

SaphMB mentioned this pull request Oct 11, 2024

chore: migrate from using webhook.Defaulter and webhook.Validator syntasso/kratix#252

Open

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

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

camilamacedo86 Sep 25, 2024

mogsie Sep 23, 2024

camilamacedo86 Sep 25, 2024

varshaprasad96 left a comment

k8s-ci-robot commented Sep 29, 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

Conversation

camilamacedo86 commented Sep 9, 2024 • edited Loading

k8s-ci-robot commented Sep 9, 2024

varshaprasad96 left a comment

Choose a reason for hiding this comment

varshaprasad96 Sep 21, 2024

Choose a reason for hiding this comment

camilamacedo86 Sep 21, 2024

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

mogsie Sep 23, 2024

Choose a reason for hiding this comment

camilamacedo86 Sep 25, 2024

Choose a reason for hiding this comment

mogsie Sep 23, 2024

Choose a reason for hiding this comment

camilamacedo86 Sep 25, 2024

Choose a reason for hiding this comment

varshaprasad96 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 29, 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 23, 2024 •

edited

Loading