[webhook] support distribution authors who want to add their custom defaulting and validation

Expected Behavior

As a distribution author it should be easy to include additional defaulting & validation logic without modifying the core API types

Actual Behavior

It's actually non-trivial to add a vendor specific default & validation. We want to avoid leaking things in the upstream API types.

A potential workaround is to define your own stub type but this is error prone because

When defaulting - if your stub doesn't simply passthrough all fields of the type you'll actually remove fields
Keeping stubs up to date with the upstream will be tedious - ie. when new fields are added

Thus we need something better for this situation

@whaught

cc @whaught @trshafer

Background

To elaborate on the issue our defaulting and validating webhooks are tightly integrated with the resourcesemantics.GenericCRD interface

pkg/webhook/resourcesemantics/interface.go

Lines 26 to 30 in 1893541

    
           type GenericCRD interface { 
        
           	apis.Defaultable 
        
           	apis.Validatable 
        
           	runtime.Object 
        
           }

pkg/apis/interfaces.go

Lines 27 to 35 in 1893541

    
           type Defaultable interface { 
        
           	SetDefaults(context.Context) 
        
           } 
        
           // Validatable indicates that a particular type may have its fields validated. 
        
           type Validatable interface { 
        
           	// Validate checks the validity of this types fields. 
        
           	Validate(context.Context) *FieldError 
        
           }

This works fine for our types which are versioned and strongly typed but there's been a need to apply similar logic across different types and/or versions.

Use Cases

I) Custom defaulting on a type across numerous versions

In knative/serving#7038 we wanted to add some labels to Revisions.

Approaches to the problem

1. Add the defaulting logic to our types in `pkg/apis`

The initial approach wasn't ideal because it was adding networking specific labels to our Revision defaulting.

2. Webhook + Stub Solution

Proposed in knative/serving#7081 was to use a separate webhook controller to apply the defaults. This required the use of a stub as follows

type RevisionStub struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	// TODO - add a general solution that captures
	// top level properties in addition to spec & status
	Spec   runtime.RawExtension `json:"spec"`
	Status runtime.RawExtension `json:"status"`
}
func (*RevisionStub) SetDefault(context.Context){...}
func (*RevisionStub) Validate(context.Context) *apis.FieldError {...}

Like I stated above the stub isn't great because

when defaulting - if your stub doesn't simply passthrough all fields of the type you'll actually remove fields
Keeping stubs up to date with the upstream will be tedious - ie. when new fields are added

3. Webhook + Unstructured Stub (unverified whether this works)

If you're operating on the ObjectMeta or TypeMeta using unstructured.Unstructured would give you more flexibility. This would work across types and versions. Though you are required to conform to the GenericCRD interface

type X struct {
	unstructured.Unstructured
}

func (x *X) SetDefaults(context.Context) {...}
func (x *X) Validate(context.Context) *apis.FieldError {...}

4. Rethink what needs the defaulting/validation

In the origin case it was really the deployment the needed the labels. The updated PR knative-extensions/net-istio#4 has the webhook watch deployments which is filtered using a label selector

They still are using a stub approach but stubbing a K8s object.

II) Additional type validation

The issue knative/serving#3425 suggests using k8s dry-run to provide early feedback if the KService/Config's template PodSpec is valid

Approaches to the problem

1. Add the dryrun call to our type's validation method in `pkg/apis`

The approach in knative/serving#7178 attempts to make the dry-run call as part of the Validate(ctx) invocation. This changes the prior semantics and in my opinion the scope of what Validate should do & not do. More thoughts here: knative/serving#7178 (comment)

2. Add another webhook with a stub/unstructured

This hasn't been explored. It would separate concerns - though you're still shimming things in via the GenericCRD interface

Webhook Framework Extensions

I think the following changes to the webhook packages would better support the use cases above

1. First-class support for `unstructured.Unstructured`

Defaulting/Validating webhooks need to conform to the following generic interface

pkg/webhook/admission.go

Lines 33 to 40 in 2907962

    
           // AdmissionController provides the interface for different admission controllers 
        
           type AdmissionController interface { 
        
           	// Path returns the path that this particular admission controller serves on. 
        
           	Path() string 
        
           	// Admit is the callback which is invoked when an HTTPS request comes in on Path(). 
        
           	Admit(context.Context, *admissionv1beta1.AdmissionRequest) *admissionv1beta1.AdmissionResponse 
        
           }

It's actually the resourcesemantics packages that's enforcing the use of GenericCRD

We could support callbacks that operate on unstructured.Unstructured types

type Callback func(unstructured.Unstructured) unstructured.Unstructured

dryRunner := dryrun.NewPodSpecVerifier(k8sClient)

handlers := map[schema.GroupVersionKind]Callback {
   v1.SchemeGroupVersion.WithKind("Revision"): func(unstructured.Unstructured) unstructured.Unstructured {
    ... add network labels ...
  },
  v1.SchemeGroupVersion.WithKind("Service"): func(unstructured.Unstructured) unstructured.Unstructured {
   ... use dryRunner ...
  },
}

I'm mixed whether this should be done with the existing packages or as a separate one. See the next section.

2. Split the webhook and the controller logic

The resource semantics packages actually contain two concerns - once is to reconcile the webhook configurations and the other is act as a stateless defaulting/mutating webhook.

If we copy these packages to support unstructured callbacks it still means we're running separate webhooks. This carries the burden of having additional webhook configurations and multiple webhook invocations from the k8s server.

From a code perspective we could create a webhook controller that can host different types of webhook handlers for a specific type. These handlers could invoked along a chain thus allowing us to mix resource semantics & unstructured callbacks.

This is analogous to having http servers and using http middleware.

This split would require changes to the injection packages (ie. sharedmain) to help wire everything together. Currently it expects a webhook to be a controller.Impl

@mattmoor

cc @mattmoor for some thoughts

Couple thoughts:

I'd be cool with trying to split up GenericCRD so that what's required is closer (or equal) to apis.Defaultable and apis.Validatable, in the respective resourcesemantic controllers. It would be kinda nice to not require both, especially if a type can rely on the new OpenAPI stuff.

It might be useful to decorate the context with whether we're in a DryRun or not. e.g. webhook.IsInDryRun(ctx), so that things can know and act accordingly.

First-class support for unstructured.Unstructured

With the broken out webhooks, it should be relatively easy to introduce a new webhook abstraction that implements AdmissionController, but simply takes a callback on unstructured. This would for instance let you register webhooks on apps/v1/Deployment and provide a callback in terms of unstructured. No type voodoo required, which is nice.

/assign

/unassign

We've created a hook for unstructured, but there are some more good ideas about how to go further here.

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

/lifecycle frozen

One workaround I did is here: knative-extensions/eventing-kafka-broker#2562, with lots of explanation

Navigation Menu

Repository navigation

[webhook] support distribution authors who want to add their custom defaulting and validation #1140

Expected Behavior

Actual Behavior

Background

Use Cases

I) Custom defaulting on a type across numerous versions

Approaches to the problem

1. Add the defaulting logic to our types in `pkg/apis`

2. Webhook + Stub Solution

3. Webhook + Unstructured Stub (unverified whether this works)

4. Rethink what needs the defaulting/validation

II) Additional type validation

Approaches to the problem

1. Add the dryrun call to our type's validation method in `pkg/apis`

2. Add another webhook with a stub/unstructured

Webhook Framework Extensions

1. First-class support for `unstructured.Unstructured`

2. Split the webhook and the controller logic

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	type GenericCRD interface {
	apis.Defaultable
	apis.Validatable
	runtime.Object
	}

	type Defaultable interface {
	SetDefaults(context.Context)
	}

	// Validatable indicates that a particular type may have its fields validated.
	type Validatable interface {
	// Validate checks the validity of this types fields.
	Validate(context.Context) *FieldError
	}

	// AdmissionController provides the interface for different admission controllers
	type AdmissionController interface {
	// Path returns the path that this particular admission controller serves on.
	Path() string

	// Admit is the callback which is invoked when an HTTPS request comes in on Path().
	Admit(context.Context, admissionv1beta1.AdmissionRequest) admissionv1beta1.AdmissionResponse
	}

[webhook] support distribution authors who want to add their custom defaulting and validation #1140

Description

Expected Behavior

Actual Behavior

Activity

dprotaso commented on Mar 4, 2020

dprotaso commented on Mar 13, 2020

Background

Use Cases

I) Custom defaulting on a type across numerous versions

Approaches to the problem

1. Add the defaulting logic to our types in pkg/apis

2. Webhook + Stub Solution

3. Webhook + Unstructured Stub (unverified whether this works)

4. Rethink what needs the defaulting/validation

II) Additional type validation

Approaches to the problem

1. Add the dryrun call to our type's validation method in pkg/apis

2. Add another webhook with a stub/unstructured

Webhook Framework Extensions

1. First-class support for unstructured.Unstructured

2. Split the webhook and the controller logic

dprotaso commented on Mar 13, 2020

mattmoor commented on Mar 13, 2020

whaught commented on Mar 18, 2020

whaught commented on Jun 3, 2020

github-actions commented on Sep 1, 2020

dprotaso commented on Sep 21, 2020

aliok commented on Aug 25, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

1. Add the defaulting logic to our types in `pkg/apis`

1. Add the dryrun call to our type's validation method in `pkg/apis`

1. First-class support for `unstructured.Unstructured`