search

kubernetes-sigs / kubebuilder

Kubebuilder - SDK for building Kubernetes APIs using CRDs
http://book.kubebuilder.io
Apache License 2.0
7.89k stars 1.45k forks source link

Decouple webhooks from APIs #4062

Closed camilamacedo86 closed 1 month ago

camilamacedo86 commented 3 months ago

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:

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

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 https://github.com/kubernetes-sigs/kubebuilder/pull/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 commented 3 months ago

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

camilamacedo86 commented 3 months ago

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 https://github.com/kubernetes-sigs/kubebuilder/pull/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 commented 3 months ago

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:

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 commented 3 months ago

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: https://github.com/kubernetes-sigs/kubebuilder/pull/4060?

camilamacedo86 commented 3 months ago

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 commented 3 months ago

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

camilamacedo86 commented 3 months ago

Maybe we can:

camilamacedo86 commented 2 months ago

We will need to decouple the webhooks from the API to comply with the changes introduced in controller-runtime prior the next release. We need find a way to do and help users moving forward ideally we need a solution that does not requires a new plugin version so I will be working on that.