Skip to content

Commit

Permalink
separate webhooks from the api types package
Browse files Browse the repository at this point in the history 
coupling api/ type packages with webhook by implementing
webhook.Validator interface is, in hindsight, a bad design since api
types are frequently imported in other projects and should therefore be
as minimal as possible. The issue is discussed in kubernetes-sigs/controller-runtime#2596.

B/c of that, webhook.Validator and webhook.Defaulter interfaces are
deprecated from controller-runtime and removed in v0.20

Moving webhooks to a separate package allows repos that import
api/v1beta1 to upgrade controller-runtime to v0.20, and also prepares
capdo for controller-runtime v0.20 upgrade

Signed-off-by: dkomsa <[email protected]>
  • Loading branch information
@d-honeybadger
d-honeybadger committed Jan 23, 2025
1 parent 59eae28 commit d1d8918
Show file tree
Hide file tree
Showing 7 changed files with 205 additions and 142 deletions.
82 changes: 0 additions & 82 deletions api/v1beta1/domachinetemplate_webhook.go
View file

This file was deleted.

2 changes: 1 addition & 1 deletion api/v1beta1/zz_generated.deepcopy.go
View file

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

49 changes: 31 additions & 18 deletions api/v1beta1/docluster_webhook.go → api/webhooks/docluster_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,17 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package v1beta1
package webhooks

Check failure on line 17 in api/webhooks/docluster_webhook.go

View workflow job for this annotation

GitHub Actions / lint 

package-comments: should have a package comment (revive)

import (
"context"
"fmt"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"

"sigs.k8s.io/cluster-api-provider-digitalocean/api/v1beta1"
ctrl "sigs.k8s.io/controller-runtime"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/webhook"
Expand All @@ -35,46 +37,57 @@ var _ = logf.Log.WithName("docluster-resource")
// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-docluster,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=doclusters,versions=v1beta1,name=validation.docluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1
// +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-docluster,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=doclusters,versions=v1beta1,name=default.docluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1

type DOClusterWebhook struct{}

Check failure on line 40 in api/webhooks/docluster_webhook.go

View workflow job for this annotation

GitHub Actions / lint 

exported: exported type DOClusterWebhook should have comment or be unexported (revive)

var (
_ webhook.Defaulter = &DOCluster{}
_ webhook.Validator = &DOCluster{}
_ webhook.CustomDefaulter = &DOClusterWebhook{}
_ webhook.CustomValidator = &DOClusterWebhook{}
)

func (r *DOCluster) SetupWebhookWithManager(mgr ctrl.Manager) error {
func (w *DOClusterWebhook) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
For(r).
For(&v1beta1.DOCluster{}).
WithDefaulter(&DOClusterWebhook{}).
WithValidator(&DOClusterWebhook{}).
Complete()
}

// Default implements webhook.Defaulter so a webhook will be registered for the type.
func (r *DOCluster) Default() {}
// Default implements webhook.CustomDefaulter so a webhook will be registered for the type.
func (w *DOClusterWebhook) Default(context.Context, runtime.Object) error {
return nil
}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
func (r *DOCluster) ValidateCreate() (admission.Warnings, error) {
// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type.
func (w *DOClusterWebhook) ValidateCreate(context.Context, runtime.Object) (admission.Warnings, error) {
return nil, nil
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
func (r *DOCluster) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type.
func (w *DOClusterWebhook) ValidateUpdate(_ context.Context, objOld, objNew runtime.Object) (admission.Warnings, error) {
var allErrs field.ErrorList

oldDOCluster, ok := old.(*DOCluster)
oldDOCluster, ok := objOld.(*v1beta1.DOCluster)
if !ok {
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an DOCluster old object but got a %T", objOld))
}

newDOCluster, ok := objNew.(*v1beta1.DOCluster)
if !ok {
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an DOCluster but got a %T", old))
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an DOCluster new object but got a %T", objNew))
}

if r.Spec.Region != oldDOCluster.Spec.Region {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "region"), r.Spec.Region, "field is immutable"))
if newDOCluster.Spec.Region != oldDOCluster.Spec.Region {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "region"), newDOCluster.Spec.Region, "field is immutable"))
}

if len(allErrs) == 0 {
return nil, nil
}

return nil, apierrors.NewInvalid(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
return nil, apierrors.NewInvalid(newDOCluster.GroupVersionKind().GroupKind(), newDOCluster.Name, allErrs)
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
func (r *DOCluster) ValidateDelete() (admission.Warnings, error) {
// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type.
func (w *DOClusterWebhook) ValidateDelete(context.Context, runtime.Object) (admission.Warnings, error) {
return nil, nil
}
51 changes: 30 additions & 21 deletions api/v1beta1/doclustertemplate_webhook.go → api/webhooks/doclustertemplate_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,16 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package v1beta1
package webhooks

import (
"context"
"fmt"
"reflect"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"sigs.k8s.io/cluster-api-provider-digitalocean/api/v1beta1"
ctrl "sigs.k8s.io/controller-runtime"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/webhook"
Expand All @@ -31,46 +33,53 @@ import (
// log is for logging in this package.
var doclustertemplatelog = logf.Log.WithName("doclustertemplate-resource")

Check failure on line 34 in api/webhooks/doclustertemplate_webhook.go

View workflow job for this annotation

GitHub Actions / lint 

var `doclustertemplatelog` is unused (unused)

func (r *DOClusterTemplate) SetupWebhookWithManager(mgr ctrl.Manager) error {
func (w *DOClusterTemplateWebhook) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
For(r).
For(&v1beta1.DOClusterTemplate{}).
WithDefaulter(&DOClusterTemplateWebhook{}).
WithValidator(&DOClusterTemplateWebhook{}).
Complete()
}

//+kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-doclustertemplate,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=doclustertemplates,versions=v1beta1,name=default.doclustertemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1
//+kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-doclustertemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=doclustertemplates,versions=v1beta1,name=validation.doclustertemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1

var _ webhook.Defaulter = &DOClusterTemplate{}
type DOClusterTemplateWebhook struct{}

Check failure on line 47 in api/webhooks/doclustertemplate_webhook.go

View workflow job for this annotation

GitHub Actions / lint 

exported: exported type DOClusterTemplateWebhook should have comment or be unexported (revive)

// Default implements webhook.Defaulter so a webhook will be registered for the type.
func (r *DOClusterTemplate) Default() {
doclustertemplatelog.Info("default", "name", r.Name)
}

var _ webhook.Validator = &DOClusterTemplate{}
var (
_ webhook.CustomDefaulter = &DOClusterTemplateWebhook{}
_ webhook.CustomValidator = &DOClusterTemplateWebhook{}
)

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
func (r *DOClusterTemplate) ValidateCreate() (admission.Warnings, error) {
doclustertemplatelog.Info("validate create", "name", r.Name)
// Default implements webhook.CustomDefaulter so a webhook will be registered for the type.
func (w *DOClusterTemplateWebhook) Default(context.Context, runtime.Object) error {
return nil
}

// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type.
func (w *DOClusterTemplateWebhook) ValidateCreate(context.Context, runtime.Object) (admission.Warnings, error) {
return nil, nil
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
func (r *DOClusterTemplate) ValidateUpdate(oldRaw runtime.Object) (admission.Warnings, error) {
old, ok := oldRaw.(*DOClusterTemplate)
// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type.
func (w *DOClusterTemplateWebhook) ValidateUpdate(_ context.Context, objOld, objNew runtime.Object) (admission.Warnings, error) {
old, ok := objOld.(*v1beta1.DOClusterTemplate)
if !ok {
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an DOClusterTemplate old object but got a %T", objOld))
}

new, ok := objNew.(*v1beta1.DOClusterTemplate)

Check failure on line 71 in api/webhooks/doclustertemplate_webhook.go

View workflow job for this annotation

GitHub Actions / lint 

redefines-builtin-id: redefinition of the built-in function new (revive)
if !ok {
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an DOClusterTemplate but got a %T", oldRaw))
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an DOClusterTemplate new object but got a %T", objNew))
}

if !reflect.DeepEqual(r.Spec, old.Spec) {
if !reflect.DeepEqual(new.Spec, old.Spec) {
return nil, apierrors.NewBadRequest("DOClusterTemplate.Spec is immutable")
}
return nil, nil
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
func (r *DOClusterTemplate) ValidateDelete() (admission.Warnings, error) {
doclustertemplatelog.Info("validate delete", "name", r.Name)
// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type.
func (w *DOClusterTemplateWebhook) ValidateDelete(context.Context, runtime.Object) (admission.Warnings, error) {
return nil, nil
}
46 changes: 30 additions & 16 deletions api/v1beta1/domachine_webhook.go → api/webhooks/domachine_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package v1beta1
package webhooks

import (
"context"
"fmt"
"reflect"

"github.com/pkg/errors"
Expand All @@ -25,6 +27,7 @@ import (
runtime "k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"

"sigs.k8s.io/cluster-api-provider-digitalocean/api/v1beta1"
ctrl "sigs.k8s.io/controller-runtime"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/webhook"
Expand All @@ -37,35 +40,46 @@ var _ = logf.Log.WithName("domachine-resource")
// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-domachine,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=domachines,versions=v1beta1,name=validation.domachine.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1
// +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-domachine,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=domachines,versions=v1beta1,name=default.domachine.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1

type DOMachineWebhook struct{}

Check failure on line 43 in api/webhooks/domachine_webhook.go

View workflow job for this annotation

GitHub Actions / lint 

exported: exported type DOMachineWebhook should have comment or be unexported (revive)

var (
_ webhook.Defaulter = &DOMachine{}
_ webhook.Validator = &DOMachine{}
_ webhook.CustomDefaulter = &DOMachineWebhook{}
_ webhook.CustomValidator = &DOMachineWebhook{}
)

func (r *DOMachine) SetupWebhookWithManager(mgr ctrl.Manager) error {
func (w *DOMachineWebhook) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
For(r).
For(&v1beta1.DOMachine{}).
WithDefaulter(&DOMachineWebhook{}).
WithValidator(&DOMachineWebhook{}).
Complete()
}

// Default implements webhook.Defaulter so a webhook will be registered for the type.
func (r *DOMachine) Default() {}
// Default implements webhook.CustomDefaulter so a webhook will be registered for the type.
func (w *DOMachineWebhook) Default(context.Context, runtime.Object) error {
return nil
}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
func (r *DOMachine) ValidateCreate() (admission.Warnings, error) {
// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type.
func (w *DOMachineWebhook) ValidateCreate(context.Context, runtime.Object) (admission.Warnings, error) {
return nil, nil
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
func (r *DOMachine) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type.
func (w *DOMachineWebhook) ValidateUpdate(_ context.Context, objOld, objNew runtime.Object) (admission.Warnings, error) {
var allErrs field.ErrorList

newDOMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(r)
new, ok := objNew.(*v1beta1.DOMachine)

Check failure on line 72 in api/webhooks/domachine_webhook.go

View workflow job for this annotation

GitHub Actions / lint 

redefines-builtin-id: redefinition of the built-in function new (revive)
if !ok {
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an DOMachine new object but got a %T", objNew))
}

newDOMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(objNew)
if err != nil {
return nil, apierrors.NewInternalError(errors.Wrap(err, "failed to convert new DOMachine to unstructured object"))
}

oldDOMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(old)
oldDOMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(objOld)
if err != nil {
return nil, apierrors.NewInternalError(errors.Wrap(err, "failed to convert old DOMachine to unstructured object"))
}
Expand All @@ -89,10 +103,10 @@ func (r *DOMachine) ValidateUpdate(old runtime.Object) (admission.Warnings, erro
return nil, nil
}

return nil, apierrors.NewInvalid(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
return nil, apierrors.NewInvalid(new.GroupVersionKind().GroupKind(), new.Name, allErrs)
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
func (r *DOMachine) ValidateDelete() (admission.Warnings, error) {
// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type.
func (w *DOMachineWebhook) ValidateDelete(context.Context, runtime.Object) (admission.Warnings, error) {
return nil, nil
}
Loading

0 comments on commit d1d8918

Please sign in to comment.