Allow unknown fields in webhooks #11848
Conversation
Given we now have schema validation and explicit validation on PodSpec, we should stop relying on json decoders to block unknown fields. This prevents API additions in new versions of kubernetes breaking Knative. Unfortunately this does leave us without a great solution for things such as Tekton where schema validation bumps against etcd limits, so we may need another approach there. Still, for serving this is a low-cost fix to get things working without - I think! - obvious downsides.
knative-prow-robot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: julz 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 |
knative-prow-robot
added
approved
Codecov Report
@@ Coverage Diff @@
## main #11848 +/- ##
==========================================
+ Coverage 87.81% 87.83% +0.02%
==========================================
Files 196 196
Lines 9393 9393
==========================================
+ Hits 8248 8250 +2
+ Misses 889 888 -1
+ Partials 256 255 -1
Continue to review full report at Codecov.
|
|
a potential hybrid that would enable cross-version compatibility with different Kubernetes versions would be to tolerate unknown fields under |
|
So we've landed the K8s 1.22 fixes in a separate PR but I think we still want to reject unknown properties because our schema is not fully specified - we have uses of A few problems I though of
A user installs an older version of Knative on a newer version of Kubernetes. There is a drift between the PodSpecs between our versions. If a user uses a newer PodSpec field Knative will truncate that field when reconciling Revisions => Deployments.
Given that we preserve unknown fields we may in fact be preserving fields which will become part of the PodSpec in a future K8s release. The API WG may choose to limit the use of this field for whatever reason. This would break users when upgrading since valid specs are now invalid. |
|
yeah, I think this is the right way to do this.. but not until we've done #11980 :(. I'll repurpose this to add 1.22 to the actions, since we clearly want to do that either way and that's actually most of what this PR does |
|
/close |
|
@julz: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Pulled the k8s 1.22 CI commit out and opened #11981. |
Given we now have schema validation, we should stop relying on json decoders to block unknown fields. This prevents API additions in new versions of kubernetes breaking Knative (as happened in #11448).
Unfortunately this does leave us without a great solution for things such as Tekton where schema validation bumps against etcd limits, so we may need another approach there. Still, for serving this is a low-cost fix to get things working without - I think! - obvious downsides (due to having schema validation now), and which seems to best match (based on conversations with them) how upstream k8s expects us to hold them.
Also adds k8s 1.22 to CI via the GitHub actions.
Related: #11805
Fixes: #11448
/assign @dprotaso @markusthoemmes
cc @vdemeester
Release Note