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

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

Merged

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Sep 9, 2024

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

Closes: #4062

@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot 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 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 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 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 camilamacedo86 force-pushed the webhooks-path-change branch 3 times, most recently from 4b5368a to 86dce22 Compare September 10, 2024 18:37
@camilamacedo86 camilamacedo86 changed the title IGNORE - WIP - TEST WIP ⚠️ decouple webhooks from APIs Sep 10, 2024
@camilamacedo86 camilamacedo86 marked this pull request as ready for review September 10, 2024 18:38
@k8s-ci-robot 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 camilamacedo86 force-pushed the webhooks-path-change branch 4 times, most recently from 8c0d1f8 to dbde977 Compare September 14, 2024 09:15
@camilamacedo86 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 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 camilamacedo86 changed the title ⚠️ (go/v4) decouple webhooks from APIs - Move Webhooks from api/<version> or api/<group>/<version> to internal/webhook/<version> or internal/webhook/<group>/<version> ⚠️ (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
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

"context"
"fmt"
"fmt"`,
`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? (

)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 force-pushed the webhooks-path-change branch 3 times, most recently from 11684fe to 56c00e3 Compare September 21, 2024 19:24
@camilamacedo86
Copy link
Member Author

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

(only reviewed the projects in testdata/*)

Looks good from my side

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Sep 23, 2024

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.

Copy link
Contributor

@mogsie mogsie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using (g Gomega)

Suggested change
Eventually(func() error {
Eventually(func(g gomega) {

and g.Expect() for error checking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2024
…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.
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

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

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

[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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-blocker size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple webhooks from APIs
6 participants