fix: use GW API v1 in webhook config only when KIC >= 3.0.0

Kong / charts

Helm chart for Kong

Apache License 2.0

243 stars 475 forks source link

fix: use GW API v1 in webhook config only when KIC >= 3.0.0 #954

Closed czeslavo closed 9 months ago

czeslavo commented 9 months ago

What this PR does / why we need it:

Because of this configuration, KIC 2.12's admission webhook receives requests to validate v1 resources which it doesn't know about:

msg="failed to run validation" error="unknown resource type to validate: gateway.networking.k8s.io/v1 gateways"
msg="failed to run validation" error="unknown resource type to validate: gateway.networking.k8s.io/v1 httproutes

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

[x] PR is based off the current tip of the main branch.
[x] Changes are documented under the "Unreleased" header in CHANGELOG.md
[x] New or modified sections of values.yaml are documented in the README.md
[x] Commits follow the Kong commit message guidelines

rainest commented 9 months ago

Could we remove the GWAPI configuration for our webhook altogether? Between @pmalek's findings that our validations were mostly (all?) checks we added early on but probably shouldn't have, the expectation that most users will be on v1 (with built-in CEL validation), and the availability of the upstream GWAPI webhook for older versions, I think we can probably remove ours.

https://github.com/Kong/kubernetes-ingress-controller/blob/87196d183f8e0093899de32dc6ab97be852bb471/internal/admission/validator.go#L367-L444 is still in KIC main but I thought the plan was to remove all of it.

czeslavo commented 9 months ago

To be fair I do not have enough context on the removal of validation in KIC. If someone has investigated it and we have an agreement our checks are redundant I'd be more than happy to remove them from here as well as from KIC.

rainest commented 9 months ago

As best I can tell from https://github.com/Kong/kubernetes-ingress-controller/issues/5197 and earlier chat discussion that is the case.

Pinged others in chat, though unfortunately everyone else involved is out today. If Mattia can confirm, we can go with https://github.com/Kong/charts/pull/956 to remove them entirely.

rainest commented 9 months ago

We're apparently unfortunately stuck with it until the feature support stuff is in place.