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

[dprotaso]

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

1) When defaulting - if your stub doesn't simply passthrough all fields of the type you'll actually remove fields 2) 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

[dprotaso]

cc @whaught @trshafer

[dprotaso]

Background

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

https://github.com/knative/pkg/blob/1893541a0fac5885032c8ec170fd81234de13f4a/webhook/resourcesemantics/interface.go#L26-L30

https://github.com/knative/pkg/blob/1893541a0fac5885032c8ec170fd81234de13f4a/apis/interfaces.go#L27-L35

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 https://github.com/knative/serving/pull/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 https://github.com/knative/serving/pull/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

1. when defaulting - if your stub doesn't simply passthrough all fields of the type you'll actually remove fields
2. 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 https://github.com/knative/net-istio/pull/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 https://github.com/knative/serving/issues/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 https://github.com/knative/serving/pull/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: https://github.com/knative/serving/pull/7178#issuecomment-596606227

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

https://github.com/knative/pkg/blob/2907962d4c5d3edd11b21dae5fd6df8a72687cba/webhook/admission.go#L33-L40

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

[dprotaso]

cc @mattmoor for some thoughts

[mattmoor]

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.

[whaught]

/assign

[whaught]

/unassign

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

[github-actions[bot]]

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.

[dprotaso]

/lifecycle frozen

[aliok]

One workaround I did is here: https://github.com/knative-sandbox/eventing-kafka-broker/pull/2562, with lots of explanation