Implement a Validating Webhook

[robscott]

Although we've discussed this in a variety of previous community meetings, we didn't have an issue to track work on this. We've labeled issues that need to be covered with this webhook with area/webhook. Those include:

• Support for setting a default GatewayClass
• Enforce GatewayClass controller immutability
• Validate HTTPRequestMirrorFilter

This is likely not an exhaustive list, but should provide a good starting point. For simplicity, implementation of this webhook should likely live within this repo.

[cmluciano]

I would would like to take this on if no one has started yet

/assign @cmluciano

[cmluciano]

I have the webhook ready to be reviewed at #506 .

@bowei @robscott Do we currently have a process for docker image management ?

[robscott]

Thanks for the work on this @cmluciano! We don't currently have anything set up for docker image management, but I believe a PR to https://github.com/kubernetes/k8s.io/tree/master/k8s.gcr.io could fix that. I'm not very familiar with the process but happy to help however I can.

[cmluciano]

Thanks for the work on this @cmluciano! We don't currently have anything set up for docker image management, but I believe a PR to https://github.com/kubernetes/k8s.io/tree/master/k8s.gcr.io could fix that. I'm not very familiar with the process but happy to help however I can.

Thanks! So it sounds like we might need to merge the PR I have open first and then we can add a new job there.

[danehans]

Will this issue resolve the following Gateway validation use cases:

1. Is gateway.spec.listeners[x].hostname valid according to the spec of the field, i.e. not an IP address?
2. If gateway.spec.listeners[x].protocol is “HTTPS” or “TLS” that gateway.spec.listeners[x].tls is specified?
3. If gateway.spec.listeners[x].tls.mode is “Terminate”, that gateway.spec.listeners[x].tls. certificateRef is specified.
4. If gateway.spec.listeners[x].tls. certificateRef is specified, that the referenced object exists?
5. If gateway.spec.listeners[x].routes.namespaces.from is "Selector", that gateway.spec.listeners[x].routes.namespaces.selector.matchExpression or gateway.spec.listeners[x].routes.namespaces.selector.matchLabels is specified?

[jpeach]

Will this issue resolve the following Gateway validation use cases:

1. Is gateway.spec.listeners[x].hostname valid according to the spec of the field, i.e. not an IP address?

Can this be a regex validation check?

2. If gateway.spec.listeners[x].protocol is “HTTPS” or “TLS” that gateway.spec.listeners[x].tls is specified?
3. If gateway.spec.listeners[x].tls.mode is “Terminate”, that gateway.spec.listeners[x].tls. certificateRef is specified.

It should be OK (and be an error on the listener condition) for the ref to not exist when the gateway is posted.

4. If gateway.spec.listeners[x].tls. certificateRef is specified, that the referenced object exists?

This should be OK too (listener condition).

5. If gateway.spec.listeners[x].routes.namespaces.from is "Selector", that gateway.spec.listeners[x].routes.namespaces.selector.matchExpression or gateway.spec.listeners[x].routes.namespaces.selector.matchLabels is specified?

[danehans]

Can this be a regex validation check?

@jpeach gateway.spec.listeners[x].hostname is based on Ingress host, so I was thinking the validating webhook implement the same logic for consistency.

Thanks for the feedback!

[danehans]

@jpeach if multiple listeners reference the same port number, should the ListenerReasonPortUnavailable get triggered or be covered by this issue?

[jpeach]

Can this be a regex validation check?

@jpeach gateway.spec.listeners[x].hostname is based on Ingress host, so I was thinking the validating webhook implement the same logic for consistency.

Ah I see. IsWildcardDNS1123Subdomain is basically a regex, so I think it should be possible to regex this case with a validation annotation.

[jpeach]

@jpeach if multiple listeners reference the same port number, should the ListenerReasonPortUnavailable get triggered or be covered by this issue?

The listener field says:

        // If this field specifies multiple Listeners that have the same
        // Port value but are not compatible, the implementation must raise
        // a "Conflicted" condition in the Listener status.

And the reason is:

        // This reason is used with the "Conflicted" condition when
        // multiple Listeners are specified with the same Listener port
        // number, but have conflicting protocol specifications.
        ListenerReasonProtocolConflict ListenerConditionReason = "ProtocolConflict"

I think that it's OK for a gateway to squash listener specs down to some combined configuration, which means that listeners can have the same port as long as they are "compatible" (e.g. HTTP listeners on port 80 that use different hostnames).

ListenerReasonPortUnavailable is more for cases where the gateway can't give you the requested port. maybe for some administrative or technical reason, like you asked for port 22 on the host network.

[danehans]

@jpeach even though these status conditions exist, I don't think it should preclude us from implementing these use cases and others in the validating webhook. Thoughts?

[jpeach]

If an error can be expressed by status then it should be. That gives better feedback that validation IMHO.

For other constraints, perhaps the best approach is to build a validation package that can be consumed by an optional webhook, or bu controllers directly.

[danehans]

@robscott @bowei @hbagdi thoughts on the comments above? The Gateway API implementation for Contour is blocked until this issue is resolved. I prefer not building a downstream Gateway API validation pkg and the validating webhook is not ready.

[robscott]

@danehans Thanks for following up on this! I completely agree that we should try to include as much core validation as possible here. I think we should prioritize validation in the following way:

1. CRD validation with kubebuilder annotations on the types
2. If that's not possible, use webhook validation
3. If that's not possible, use status conditions that are populated by a controller

Of the examples you listed above, I think all but the 4th one can and should be accomplished by either CRD or webhook validation. As far as the 4th one, there's traditionally been a lot of hesitancy to implement cross resource validation in upstream Kubernetes. Beyond the additional dependencies it creates, it can break unexpectedly if both the referencing and referenced resources are being created at the same time. It would also require the webhook to have read access on all resources that could potentially be referenced.

It sounds like @cmluciano could use a bit of help in getting the initial webhook implementation in place, so if anyone has extra cycles, this would be a great area to contribute.

[danehans]

It sounds like @cmluciano could use a bit of help in getting the initial webhook implementation in place, so if anyone has extra cycles, this would be a great area to contribute.

@robscott thanks for all the great feedback. What are your thoughts on building a validation pkg that can be used by the webhook implementation, xref #487 (comment)?

[robscott]

What are your thoughts on building a validation pkg that can be used by the webhook implementation, xref #487 (comment)?

How would that validation package be used? Would it be used for controllers to populate status if the webhook was not installed or working properly?

[danehans]

How would that validation package be used? Would it be used for controllers to populate status if the webhook was not installed or working properly?

@robscott that's correct. The validating webhook implementation could also use the validation pkg.

[stevesloka]

Some things to think about are optional fields that maybe a Controller might not adopt, for instance the Protocol field. Some may not support UDP for instance. I'll have to think up some more examples, but just wanted to throw that out there when we were discussing this earlier today @danehans.

[hbagdi]

/assign
xref #617

[hbagdi]

xref kubernetes/test-infra#21881
xref kubernetes/k8s.io#1954

[hbagdi]

Update:

The following needs to happen now:

• Merge the code: implement an admission webhook server #617
• Merge the cloudbuild config: automate webhook server image build and publishing #621
• Merge rename service-apis to gateway-api kubernetes/k8s.io#1954
• Do steps in https://github.com/kubernetes/k8s.io/tree/main/k8s.gcr.io#creating-staging-repos with help from a community member

Once the above is completed, we need to get kubernetes/test-infra#21881 merged in. That should be it if we didn't make any mistake.

[robscott]

@hbagdi I think everything listed above is complete, do you think we can close this out? I'm also happy to just pull this from the milestone if that's easier.

[hbagdi]

The images publishing part is broken for an unknown reason. Let's pull this out because of external dependencies.

[hbagdi]

Update:
I was writing up an issue in kubernetes/k8s.io when I realized that there is a typo which probably caused the error. The PR to fix it is up: kubernetes/test-infra#22061. Once that PR is merged in, we will see if the builds on master branch are fixed or not.