More robust channel validation admission hook

[akashrv]

Fixes #779

Summary of issue: If we create a Channel with a non-installed channel
provisioner that then the channel gets created but events never flow through.
There is no way for user to know what is wrong in the eventing pipeline

Proposed Changes

Proposed changes:

1. Updated the Validation Admission Webhook for Channel. Channel creation will fail in case the specified ChannelProvisioner is not installed. It doesn't call the API server, and instead uses a cache from a shared ChannelProvisioner Informer
2. Added check to ensure that Channel provisioner has to be of the kind ClusterChannelProvisioner.

Release Note

NONE

[Harwayne]

/assign @Harwayne

[n3wscott]

I am not sold on a global function to do the validation or blocking channel creation when a provisioner does not exist yet. The k8s way is to set a status and keep trying until the provisioner exists.

/hold

[n3wscott]

This breaks late binding. This really should be a status on the channel, not a failure to create the channel.

[akashrv]

I explored that option and discarded it. The problem with Status is that now we will need a common Controller that controls some of the Channel Status while there already exists Channel-type-specific controller that controls its Status.
SO we either make it mandatory for each Channel Controller to take care of this which is difficult to maintain and guarantee, or have two controller control the status of a single object which is also a no-no.

So finally after discussing with Adam we decided to stick with a Webhook which will help improve user experience but as you mentioned will break late binding and not catch issues later on if say post Channel provisioning the ChannelProvisioner is deleted.

Thoughts?

[n3wscott]

I do not understand the value of setting a GlobalChannelValidator. Please explain.

[akashrv]

This Webhook is based off of knative/pkg/webhook. It is structured in a way that you specify handlers like:
Handlers: map[schema.GroupVersionKind]webhook.GenericCRD{
// For group eventing.knative.dev,
eventingv1alpha1.SchemeGroupVersion.WithKind("Channel"): &eventingv1alpha1.Channel{},
},

Here it is expected that Channel{} implements api.Validatable which has one method "Validate() *FieldError" - See pkg/apis/eventing/v1alpha1/channel_validation.go

So to do this special Validation where we either make use an informer to keep track on channelprovisioners and then use the in-memory store to validate existence of the channelprovisioner, I created this global instance of Validator which implements the actual validation rather than polluting pkg/apis/eventing/v1alpha1/channel_validation.go which is part of apis. This lets me do this special case validation without making it part of "knative/pkg/webhook"

I based this off of how channel_defaulter is designed to keep it consistent with existing design.
If there are other better alternatives then please feel free to share.

[n3wscott]

I think the channel defaulter is solving a different problem. The ChannelValidator code should live in the channel impl area. No need to have this magic pointer to a function that may or may not be set. It is harder to understand and not dynamic.

[akashrv]

What do you mean by channel impl area? If you mean same folder or close by then I am fine with that for better readability. However if you mean to move it inside Channel type, then I am not sure if that will be a good design. Channel is defined as API type with fields that are specific to that custom resource. Adding extra fields that hold the Store or Logger to it may not be a good idea. Hence I moved the validation responsibility to another type which is not part of exposed types.

Moreover, passing the Store to the Validate() on Channel type through the Knative webhook doesn't look right. Hence injecting it felt like the optimal way to go.

Please let me know if I didn't understand the concern correctly. I'll anyways chat with you in-person tomorrow and then note the summary here for reference :)

[akashrv]

/cc vaikas-google

[knative-metrics-robot]

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/webhook/defaulters/channel_defaulter.go	Do not exist	100.0%
pkg/webhook/validators/channel_validator.go	Do not exist	89.5%

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akashrv

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment