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.

Sign up for GitHub

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): Replace usage of deprecated webhook.Validator and webhook.Defaulter interfaces #3723

Closed
wants to merge 1 commit into from
Closed

⚠️ : (go/v4): Replace usage of deprecated webhook.Validator and webhook.Defaulter interfaces #3723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ limitations under the License.
package v1

import (
"context"
"github.com/robfig/cron"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -46,6 +47,8 @@ Then, we set up the webhook with the manager.
func (r *CronJob) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
For(r).
WithValidator(r).
WithDefaulter(r).
Copy link
Member

Choose a reason for hiding this comment

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

Did you test it out without the WithValidator?
Because looking the controlle-runtime code it seems that the structure that should be passed is one for the Validator? Am I right ? Do we need to use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, before this change, with the deprecated webhook interface For was enough to enable both the validation and defaulting logic, if r implemented either (there are runtime checks here, if I am not mistaken).
For custom validator and custom defaulter, you need to specifically WithValidator or WithDefaulter to enable them.

A test for just the CustomDefaulter is here and for CustomValidator here.

Copy link
Member

@camilamacedo86 camilamacedo86 Feb 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But seems that we are saying here.
The implementation is For the struct r
and with the validator using the same struct.
Should we not have a new type, struct for the validation? (i.e. CronJobCustomValidator)

Copy link
Member

Choose a reason for hiding this comment

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

See: https://github.com/kubernetes-sigs/controller-runtime/blob/4000e996a202917ad7d40f02ed8a2079a9ce25e9/pkg/builder/webhook.go#L70-L74

I understand that you pass an struct representing the Validator and not the object that will be validated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that's what's been done here, though. This also hasn't changed. It's just, that, instead of doing this check at runtime, using a runtime type assertion, we need to explicitly tell the controller runtime to use this implementation to validate/provide defaults.

I sort of get what you mean, though. However, changing that would be quite a breaking change, and the implications of that change would go over my head.

Copy link
Member

Choose a reason for hiding this comment

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

We need the validator struct
We should not be passing :

WithValidator(r).
WithDefaulter(r).

Copy link
Member

@sbueringer sbueringer Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agree with Camila. CustomValidator and CustomDefaulter should be implemented on a separate struct.

Otherwise there would have been no point to introducing these if we still implement them on the API type.

(the goal is to have CustomValidator/CustomDefaulter outside of the api package to reduce the dependencies to controller-runtime in the API package)

Copy link
Member

Choose a reason for hiding this comment

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

Thank you to confirm my understanding about the requirements for the changes

Complete()
}

Expand All @@ -59,16 +62,16 @@ The meaning of each marker can be found [here](/reference/markers/webhook.md).
//+kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1

/*
We use the `webhook.Defaulter` interface to set defaults to our CRD.
We use the `webhook.CustomDefaulter` interface to set defaults to our CRD.
A webhook will automatically be served that calls this defaulting.

The `Default` method is expected to mutate the receiver, setting the defaults.
*/

var _ webhook.Defaulter = &CronJob{}
var _ webhook.CustomDefaulter = &CronJob{}

// Default implements webhook.Defaulter so a webhook will be registered for the type
func (r *CronJob) Default() {
// Default implements webhook.CustomDefaulter so a webhook will be registered for the type
func (r *CronJob) Default(ctx context.Context, obj runtime.Object) error {
cronjoblog.Info("default", "name", r.Name)

if r.Spec.ConcurrencyPolicy == "" {
Expand All @@ -85,6 +88,8 @@ func (r *CronJob) Default() {
r.Spec.FailedJobsHistoryLimit = new(int32)
*r.Spec.FailedJobsHistoryLimit = 1
}

return nil
}

/*
Expand Down Expand Up @@ -115,24 +120,24 @@ Here, however, we just use the same shared validation for `ValidateCreate` and
validate anything on deletion.
*/

var _ webhook.Validator = &CronJob{}
var _ webhook.CustomValidator = &CronJob{}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
func (r *CronJob) ValidateCreate() (admission.Warnings, error) {
// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type
func (r *CronJob) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
cronjoblog.Info("validate create", "name", r.Name)

return nil, r.validateCronJob()
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
func (r *CronJob) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type
func (r *CronJob) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
cronjoblog.Info("validate update", "name", r.Name)

return nil, r.validateCronJob()
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
func (r *CronJob) ValidateDelete() (admission.Warnings, error) {
// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type
func (r *CronJob) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
cronjoblog.Info("validate delete", "name", r.Name)

// TODO(user): fill in your validation logic upon object deletion.
Expand Down
1 change: 0 additions & 1 deletion docs/book/src/cronjob-tutorial/testdata/project/internal/controller/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ Kubebuilder scaffolded a `internal/controller/suite_test.go` file that does the
First, it will contain the necessary imports.
*/


package controller

import (
Expand Down
4 changes: 2 additions & 2 deletions docs/book/src/cronjob-tutorial/webhook-implementation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# Implementing defaulting/validating webhooks

If you want to implement [admission webhooks](../reference/admission-webhook.md)
for your CRD, the only thing you need to do is to implement the `Defaulter`
and (or) the `Validator` interface.
for your CRD, the only thing you need to do is to implement the `CustomDefaulter`
and (or) the `CustomValidator` interface.
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved

Kubebuilder takes care of the rest for you, such as

Expand Down
9 changes: 6 additions & 3 deletions hack/docs/internal/cronjob-tutorial/generate_cronjob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,8 @@ func updateWebhook(sp *Sample) {
err = pluginutil.ReplaceInFile(
filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"),
`import (
"context"

"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
logf "sigs.k8s.io/controller-runtime/pkg/log"
Expand Down Expand Up @@ -427,6 +429,7 @@ Then, we set up the webhook with the manager.
`cronjoblog.Info("default", "name", r.Name)

// TODO(user): fill in your defaulting logic.
return nil
`, WebhookValidate)
CheckError("fixing cronjob_webhook.go by adding logic", err)

Expand All @@ -448,7 +451,7 @@ Then, we set up the webhook with the manager.

err = pluginutil.InsertCode(
filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"),
`func (r *CronJob) ValidateDelete() (admission.Warnings, error) {
`func (r *CronJob) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
cronjoblog.Info("validate delete", "name", r.Name)

// TODO(user): fill in your validation logic upon object deletion.
Expand Down Expand Up @@ -596,8 +599,8 @@ func updateExample(sp *Sample) {
}

func addControllerTest(sp *Sample) {
var fs = afero.NewOsFs()
err := afero.WriteFile(fs, filepath.Join(sp.ctx.Dir, "internal/controller/cronjob_controller_test.go"), []byte(ControllerTest), 0600)
fs := afero.NewOsFs()
err := afero.WriteFile(fs, filepath.Join(sp.ctx.Dir, "internal/controller/cronjob_controller_test.go"), []byte(ControllerTest), 0o600)
CheckError("adding cronjob_controller_test", err)
}

Expand Down
5 changes: 4 additions & 1 deletion hack/docs/internal/cronjob-tutorial/webhook_implementation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package cronjob

const WebhookIntro = `import (
"context"
"github.com/robfig/cron"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -47,7 +48,7 @@ The meaning of each marker can be found [here](/reference/markers/webhook.md).
//+kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1

/*
We use the` + " `" + `webhook.Defaulter` + "`" + ` interface to set defaults to our CRD.
We use the` + " `" + `webhook.CustomDefaulter` + "`" + ` interface to set defaults to our CRD.
A webhook will automatically be served that calls this defaulting.

The` + " `" + `Default` + "`" + ` method is expected to mutate the receiver, setting the defaults.
Expand All @@ -70,6 +71,8 @@ const WebhookValidate = ` cronjoblog.Info("default", "name", r.Name)
r.Spec.FailedJobsHistoryLimit = new(int32)
*r.Spec.FailedJobsHistoryLimit = 1
}

return nil
}

/*
Expand Down
8 changes: 4 additions & 4 deletions pkg/plugin/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ func ImplementWebhooks(filename string) error {
str, err = EnsureExistAndReplace(
str,
"// TODO(user): fill in your defaulting logic.",
`if r.Spec.Count == 0 {
r.Spec.Count = 5
`if res.Spec.Count == 0 {
res.Spec.Count = 5
}`)
if err != nil {
return err
Expand All @@ -163,7 +163,7 @@ func ImplementWebhooks(filename string) error {
str, err = EnsureExistAndReplace(
str,
"// TODO(user): fill in your validation logic upon object creation.",
`if r.Spec.Count < 0 {
`if res.Spec.Count < 0 {
return nil, errors.New(".spec.count must >= 0")
}`)
if err != nil {
Expand All @@ -172,7 +172,7 @@ func ImplementWebhooks(filename string) error {
str, err = EnsureExistAndReplace(
str,
"// TODO(user): fill in your validation logic upon object update.",
`if r.Spec.Count < 0 {
`if newRes.Spec.Count < 0 {
return nil, errors.New(".spec.count must >= 0")
}`)
if err != nil {
Expand Down
36 changes: 22 additions & 14 deletions pkg/plugins/golang/v4/scaffolds/internal/templates/api/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,14 @@ package {{ .Resource.Version }}
import (
ctrl "sigs.k8s.io/controller-runtime"
logf "sigs.k8s.io/controller-runtime/pkg/log"
{{- if .Resource.HasValidationWebhook }}
"k8s.io/apimachinery/pkg/runtime"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
{{- end }}
{{- if or .Resource.HasValidationWebhook .Resource.HasDefaultingWebhook }}
"context"
"k8s.io/apimachinery/pkg/runtime"
"sigs.k8s.io/controller-runtime/pkg/webhook"
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
{{- end }}
{{- if .Resource.HasValidationWebhook }}
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
{{- end }}

)

Expand All @@ -102,6 +103,12 @@ var {{ lower .Resource.Kind }}log = logf.Log.WithName("{{ lower .Resource.Kind }
func (r *{{ .Resource.Kind }}) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
For(r).
{{- if .Resource.HasValidationWebhook }}
WithValidator(r).
{{- end }}
{{- if .Resource.HasDefaultingWebhook }}
WithDefaulter(r).
{{- end }}
Complete()
}

Expand All @@ -112,13 +119,14 @@ func (r *{{ .Resource.Kind }}) SetupWebhookWithManager(mgr ctrl.Manager) error {
defaultingWebhookTemplate = `
//+kubebuilder:webhook:{{ if ne .Resource.Webhooks.WebhookVersion "v1" }}webhookVersions={{"{"}}{{ .Resource.Webhooks.WebhookVersion }}{{"}"}},{{ end }}path=/mutate-{{ .QualifiedGroupWithDash }}-{{ .Resource.Version }}-{{ lower .Resource.Kind }},mutating=true,failurePolicy=fail,sideEffects=None,groups={{ .Resource.QualifiedGroup }},resources={{ .Resource.Plural }},verbs=create;update,versions={{ .Resource.Version }},name=m{{ lower .Resource.Kind }}.kb.io,admissionReviewVersions={{ .AdmissionReviewVersions }}

var _ webhook.Defaulter = &{{ .Resource.Kind }}{}
var _ webhook.CustomDefaulter = &{{ .Resource.Kind }}{}

// Default implements webhook.Defaulter so a webhook will be registered for the type
func (r *{{ .Resource.Kind }}) Default() {
// Default implements webhook.CustomDefaulter so a webhook will be registered for the type
func (r *{{ .Resource.Kind }}) Default(ctx context.Context, obj runtime.Object) error {
{{ lower .Resource.Kind }}log.Info("default", "name", r.Name)

// TODO(user): fill in your defaulting logic.
return nil
}
`

Expand All @@ -127,26 +135,26 @@ func (r *{{ .Resource.Kind }}) Default() {
// TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation.
//+kubebuilder:webhook:{{ if ne .Resource.Webhooks.WebhookVersion "v1" }}webhookVersions={{"{"}}{{ .Resource.Webhooks.WebhookVersion }}{{"}"}},{{ end }}path=/validate-{{ .QualifiedGroupWithDash }}-{{ .Resource.Version }}-{{ lower .Resource.Kind }},mutating=false,failurePolicy=fail,sideEffects=None,groups={{ .Resource.QualifiedGroup }},resources={{ .Resource.Plural }},verbs=create;update,versions={{ .Resource.Version }},name=v{{ lower .Resource.Kind }}.kb.io,admissionReviewVersions={{ .AdmissionReviewVersions }}

var _ webhook.Validator = &{{ .Resource.Kind }}{}
var _ webhook.CustomValidator = &{{ .Resource.Kind }}{}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
func (r *{{ .Resource.Kind }}) ValidateCreate() (admission.Warnings, error) {
// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type
func (r *{{ .Resource.Kind }}) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
{{ lower .Resource.Kind }}log.Info("validate create", "name", r.Name)

// TODO(user): fill in your validation logic upon object creation.
return nil, nil
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
func (r *{{ .Resource.Kind }}) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type
func (r *{{ .Resource.Kind }}) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
{{ lower .Resource.Kind }}log.Info("validate update", "name", r.Name)

// TODO(user): fill in your validation logic upon object update.
return nil, nil
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
func (r *{{ .Resource.Kind }}) ValidateDelete() (admission.Warnings, error) {
// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type
func (r *{{ .Resource.Kind }}) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
{{ lower .Resource.Kind }}log.Info("validate delete", "name", r.Name)

// TODO(user): fill in your validation logic upon object deletion.
Expand Down
25 changes: 15 additions & 10 deletions testdata/project-v4-multigroup-with-deploy-image/api/crew/v1/captain_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package v1

import (
"context"

"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
logf "sigs.k8s.io/controller-runtime/pkg/log"
Expand All @@ -31,45 +33,48 @@ var captainlog = logf.Log.WithName("captain-resource")
func (r *Captain) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
For(r).
WithValidator(r).
WithDefaulter(r).
Complete()
}

// TODO(user): EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!

//+kubebuilder:webhook:path=/mutate-crew-testproject-org-v1-captain,mutating=true,failurePolicy=fail,sideEffects=None,groups=crew.testproject.org,resources=captains,verbs=create;update,versions=v1,name=mcaptain.kb.io,admissionReviewVersions=v1

var _ webhook.Defaulter = &Captain{}
var _ webhook.CustomDefaulter = &Captain{}

// Default implements webhook.Defaulter so a webhook will be registered for the type
func (r *Captain) Default() {
// Default implements webhook.CustomDefaulter so a webhook will be registered for the type
func (r *Captain) Default(ctx context.Context, obj runtime.Object) error {
captainlog.Info("default", "name", r.Name)

// TODO(user): fill in your defaulting logic.
return nil
}

// TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation.
//+kubebuilder:webhook:path=/validate-crew-testproject-org-v1-captain,mutating=false,failurePolicy=fail,sideEffects=None,groups=crew.testproject.org,resources=captains,verbs=create;update,versions=v1,name=vcaptain.kb.io,admissionReviewVersions=v1

var _ webhook.Validator = &Captain{}
var _ webhook.CustomValidator = &Captain{}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
func (r *Captain) ValidateCreate() (admission.Warnings, error) {
// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type
func (r *Captain) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
captainlog.Info("validate create", "name", r.Name)

// TODO(user): fill in your validation logic upon object creation.
return nil, nil
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
func (r *Captain) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type
func (r *Captain) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
captainlog.Info("validate update", "name", r.Name)

// TODO(user): fill in your validation logic upon object update.
return nil, nil
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
func (r *Captain) ValidateDelete() (admission.Warnings, error) {
// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type
func (r *Captain) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
captainlog.Info("validate delete", "name", r.Name)

// TODO(user): fill in your validation logic upon object deletion.
Expand Down
11 changes: 8 additions & 3 deletions testdata/project-v4-multigroup-with-deploy-image/api/ship/v1/destroyer_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ limitations under the License.
package v1

import (
"context"

"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/webhook"
Expand All @@ -29,18 +32,20 @@ var destroyerlog = logf.Log.WithName("destroyer-resource")
func (r *Destroyer) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
For(r).
WithDefaulter(r).
Complete()
}

// TODO(user): EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!

//+kubebuilder:webhook:path=/mutate-ship-testproject-org-v1-destroyer,mutating=true,failurePolicy=fail,sideEffects=None,groups=ship.testproject.org,resources=destroyers,verbs=create;update,versions=v1,name=mdestroyer.kb.io,admissionReviewVersions=v1

var _ webhook.Defaulter = &Destroyer{}
var _ webhook.CustomDefaulter = &Destroyer{}

// Default implements webhook.Defaulter so a webhook will be registered for the type
func (r *Destroyer) Default() {
// Default implements webhook.CustomDefaulter so a webhook will be registered for the type
func (r *Destroyer) Default(ctx context.Context, obj runtime.Object) error {
destroyerlog.Info("default", "name", r.Name)

// TODO(user): fill in your defaulting logic.
return nil
}
2 changes: 1 addition & 1 deletion testdata/project-v4-multigroup-with-deploy-image/api/ship/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading