Decouple webhooks from APIs

[camilamacedo86]

What do you want to happen?

This issue aims to centralize information regarding the requests to change the kubebuilder project layouts to decouple the APIs from webhooks. What we need to do here is:

• Understand what would be the best approach and recommendations from Controller-Runtime maintainer to where/how scaffolds the projects after:
  Closes Changes to webhook.Validator pattern cause version update problems controller-runtime#2596

For more context, see the discussion and concerns raised in the following issues:

• Controller-runtime issue #2596: Highlights the need to decouple API packages from controller-runtime versions.
• Kubebuilder issue #3215: The original request where users expressed their concern about the inclusion of controller-runtime code in public API packages. Key comment: Issue #3215 Comment

NOTE

We need avoid breaking changes and we cannot impact our end users. Therefore, we would need to consider creating a go/v5-alpha plugin. Unless we decide to move forward based in some rationality exception case, as a follow up of #4060 and with easy options for users, OR a solution that only applied for new projects and does not impact pre-existent ones. (we need deeply consider the impact for the existent projects)

Extra Labels

No response

[camilamacedo86]

c/c @troy0820 @nathanperkins @malt3 @joelanford @varshaprasad96

[camilamacedo86]

Hi @troy0820, @nathanperkins, @malt3,

If you'd like to contribute to this effort (since it seems you were the ones who initially requested this change), I suggest starting by creating a PR that updates the go/v4 plugin as a follow-up to #4060. We need to get that PR merged first to streamline this process.

Once those changes are made and pass the CI/tests, it will be easier to move the updates to a new plugin version if needed. This approach will also allow us to discuss the proposed changes more effectively.

Thank you for your attention and collaboration!

[camilamacedo86]

Just to share, if we need to change paths and scaffolds we might able to do that and ensure backwards compatibility by doing something like:

if f.Path == "" {
    if r.Resource.Webhooks != nil && (hasWebhooksWith("api") || hasWebhooksWith(filepath.Join("api", f.Resource.Group))) {
        // Scaffold in the current path since the webhook scaffold exists under `api` or `api/group`
    } else {
        // Use the new path `webhook` or webhook/group to avoid breaking changes
    }
}

We already have functions to perform these checks, as seen in the links below:

• Check existing functions
• Resource model

In the boilerplate, we can utilize these checks where we already import machinery.ResourceMixin. Specifically, we can use r.Resource.Webhooks != nil to determine if webhooks are present.

Here's an example of the utility functions:

// hasWebhooksWith checks if there are any files with a name that contains "webhook" in the given directory.
func hasWebhooksWith(dir string) bool {
    return hasFileWhichContainsNameWithPath(dir, "webhook")
}

// hasFileWhichContainsNameWith checks if there are any files in the given directory that contain the specified substring in their name.
func hasFileWhichContainsNameWith(dir string, nameSubstring string) bool {
    found = false
    
    filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
        if err != nil {
            return err
        }

        if strings.Contains(info.Name(), nameSubstring) {
            found = true
            return filepath.SkipDir
        }

        return nil
    })

    return found
}

[camilamacedo86]

Hi @troy0820, @nathanperkins, @malt3,

Could you please provide here the suggestion about how you think should be the kubebuilder scaffold by default regards webhook we stop to use the deprecated methods: #4060?

[camilamacedo86]

Hi @alvaroaleman, @sbueringer, @vincepri,

We are now moving forward with using the new interfaces introduced in this PR, with the following description:

admission.Validator/Defaulter require importing controller-runtime into API packages. This is not recommended, as API packages should themselves not have any dependencies except other API packages in order to make importing them easy without dependency issues and possible incompatibilities. This change marks the two as deprecated. We are not going to remove them yet to give folks some time to migrate off them.

Given this change, how do you suggest webhooks and APIs should be scaffolded in projects? Could you please clarify the best practices for implementing webhooks and APIs with controller-runtime now? Specifically, how do you think developers should structure their projects to align with these updates?

Should the webhooks not be scaffold under the api directory? If so, how we do:

func (r *Kind) SetupWebhookWithManager(mgr ctrl.Manager) error {
	return ctrl.NewWebhookManagedBy(mgr).
		For(r).
                 WithValidator(&KindCustomValidator{}).
		WithDefaulter(&KindJobCustomDefaulter{}).
		Complete()
}

Should ONLY the custom interfaces be scaffold in a directory that is not the api?

Could you please help us to understand how people ideally should develop their projects based on the changes so that we can try to address this in the kubebuider layouts and docs?

Currently, for projects that do not scaffold APIs for more than one group, we have layouts like this:

├── api
│   └── v1
│       ├── *_types.go
│       ├── *_webhook.go <- ALL CODE IMPL FOR WEBHOOKS IS IN THIS FILE
│       ├── *_webhook_test.go
│       ├── groupversion_info.go
│       ├── webhook_suite_test.go
│       └── zz_generated.deepcopy.go

And for a multi-group layout, an example could be:

├── api
│   ├── group1
│   │   └── v1
│   │       ├── *_types.go
│   │       ├── *_webhook.go
│   │       ├── groupversion_info.go
│   │       └── zz_generated.deepcopy.go
│   ├── group2
│   │   └── v1beta1
│   │       ├── *_types.go
│   │       ├── groupversion_info.go
│   │       └── zz_generated.deepcopy.go
│   └── group3
│       └── v1
│           ├── *_types.go
│           ├── *_webhook.go
│           ├── groupversion_info.go
│           └── zz_generated.deepcopy.go

Example of webhook implementation is:

package v1

import (
	"context"
	"fmt"
	"github.com/robfig/cron"
	apierrors "k8s.io/apimachinery/pkg/api/errors"
	"k8s.io/apimachinery/pkg/runtime"
	"k8s.io/apimachinery/pkg/runtime/schema"
	"k8s.io/apimachinery/pkg/util/validation"
	"k8s.io/apimachinery/pkg/util/validation/field"
	ctrl "sigs.k8s.io/controller-runtime"
	"sigs.k8s.io/controller-runtime/pkg/log"
	"sigs.k8s.io/controller-runtime/pkg/webhook"
	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
)

var cronjoblog = log.Log.WithName("cronjob-resource")

func (r *CronJob) SetupWebhookWithManager(mgr ctrl.Manager) error {
	return ctrl.NewWebhookManagedBy(mgr).
		For(r).
		WithValidator(&CronJobCustomValidator{}).
		WithDefaulter(&CronJobCustomDefaulter{}).
		Complete()
}

// +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

type CronJobCustomDefaulter struct{}

var _ webhook.CustomDefaulter = &CronJobCustomDefaulter{}

func (d *CronJobCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error {
	cronjoblog.Info("CustomDefaulter for CronJob")
	req, err := admission.RequestFromContext(ctx)
	if err != nil {
		return fmt.Errorf("expected admission.Request in ctx: %w", err)
	}
	if req.Kind.Kind != "CronJob" {
		return fmt.Errorf("expected Kind CronJob got %q", req.Kind.Kind)
	}
	castedObj, ok := obj.(*CronJob)
	if !ok {
		return fmt.Errorf("expected a CronJob object but got %T", obj)
	}
	cronjoblog.Info("default", "name", castedObj.GetName())

	if castedObj.Spec.ConcurrencyPolicy == "" {
		castedObj.Spec.ConcurrencyPolicy = AllowConcurrent
	}
	if castedObj.Spec.Suspend == nil {
		castedObj.Spec.Suspend = new(bool)
	}
	if castedObj.Spec.SuccessfulJobsHistoryLimit == nil {
		castedObj.Spec.SuccessfulJobsHistoryLimit = new(int32)
		*castedObj.Spec.SuccessfulJobsHistoryLimit = 3
	}
	if castedObj.Spec.FailedJobsHistoryLimit == nil {
		castedObj.Spec.FailedJobsHistoryLimit = new(int32)
		*castedObj.Spec.FailedJobsHistoryLimit = 1
	}

	return nil
}

// +kubebuilder:webhook:verbs=create;update;delete,path=/validate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,versions=v1,name=vcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1

type CronJobCustomValidator struct{}

var _ webhook.CustomValidator = &CronJobCustomValidator{}

func (v *CronJobCustomValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
	cronjoblog.Info("Creation Validation for CronJob")

	req, err := admission.RequestFromContext(ctx)
	if err != nil {
		return nil, fmt.Errorf("expected admission.Request in ctx: %w", err)
	}
	if req.Kind.Kind != "CronJob" {
		return nil, fmt.Errorf("expected Kind CronJob got %q", req.Kind.Kind)
	}
	castedObj, ok := obj.(*CronJob)
	if !ok {
		return nil, fmt.Errorf("expected a CronJob object but got %T", obj)
	}
	cronjoblog.Info("default", "name", castedObj.GetName())

	return nil, v.validateCronJob(castedObj)
}

func (v *CronJobCustomValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
	cronjoblog.Info("Update Validation for CronJob")
	req, err := admission.RequestFromContext(ctx)
	if err != nil {
		return nil, fmt.Errorf("expected admission.Request in ctx: %w", err)
	}
	if req.Kind.Kind != "CronJob" {
		return nil, fmt.Errorf("expected Kind CronJob got %q", req.Kind.Kind)
	}
	castedObj, ok := newObj.(*CronJob)
	if !ok {
		return nil, fmt.Errorf("expected a CronJob object but got %T", newObj)
	}
	cronjoblog.Info("default", "name", castedObj.GetName())

	return nil, v.validateCronJob(castedObj)
}

func (v *CronJobCustomValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
	cronjoblog.Info("Deletion Validation for CronJob")
	req, err := admission.RequestFromContext(ctx)
	if err != nil {
		return nil, fmt.Errorf("expected admission.Request in ctx: %w", err)
	}
	if req.Kind.Kind != "CronJob" {
		return nil, fmt.Errorf("expected Kind CronJob got %q", req.Kind.Kind)
	}
	castedObj, ok := obj.(*CronJob)
	if !ok {
		return nil, fmt.Errorf("expected a CronJob object but got %T", obj)
	}
	cronjoblog.Info("default", "name", castedObj.GetName())

	return nil, nil
}

func (v *CronJobCustomValidator) validateCronJob(castedObj *CronJob) error {
	var allErrs field.ErrorList
	if err := v.validateCronJobName(castedObj); err != nil {
		allErrs = append(allErrs, err)
	}
	if err := v.validateCronJobSpec(castedObj); err != nil {
		allErrs = append(allErrs, err)
	}
	if len(allErrs) == 0 {
		return nil
	}

	return apierrors.NewInvalid(
		schema.GroupKind{Group: "batch.tutorial.kubebuilder.io", Kind: "CronJob"},
		castedObj.Name, allErrs)
}

func (v *CronJobCustomValidator) validateCronJobSpec(castedObj *CronJob) *field.Error {
	return validateScheduleFormat(
		castedObj.Spec.Schedule,
		field.NewPath("spec").Child("schedule"))
}

func validateScheduleFormat(schedule string, fldPath *field.Path) *field.Error {
	if _, err := cron.ParseStandard(schedule); err != nil {
		return field.Invalid(fldPath, schedule, err.Error())
	}
	return nil
}

func (v *CronJobCustomValidator) validateCronJobName(castedObj *CronJob) *field.Error {
	if len(castedObj.ObjectMeta.Name) > validation.DNS1035LabelMaxLength-11 {
		return field.Invalid(field.NewPath("metadata").Child("name"), castedObj.Name, "must be no more than 52 characters")
	}
	return nil
}

So, how people should implement for example, the above sample? Where should be placed the CustomValidators for CronJob ?

Thank you a lot for all your help and support.

[sbueringer]

My answer from Slack:

It sounds like the question is mostly where to put the CustomValidator / CustomDefaulter implementations?
I would recommend internal/webhooks
I would not add subpackages below that for apiVersions, as the webhooks are only implemented for one version.
Looking at the multigroup example you probably want to have sub-packages for apiGroups in the multi-group case
https://kubernetes.slack.com/archives/C02MRBMN00Z/p1723470424366189?thread_ts=1723373582.440229&cid=C02MRBMN00Z