Update Knative Serving schema

[dprotaso]

Part of #11980

Proposed Changes

• I've pushed x-kubernetes-preserve-unknown-fields markers further down in the schema to the properties behind feature flags
• This should prevent people from supplying a bad known property - ie. misspelling containerConcurrency
• I've omitted the schema for features behind flags - they're essentially the wild west
• After this I'll follow up with a PR to relax our webhooks to not error out on unknown properties

Release Note

CRD schemas have been updated and `x-kubernetes-preserve-unknown-fields` is now only specified for attributes behind feature flags

[dprotaso]

I'm also seeing if we can upstream our controller-tools changes - kubernetes-sigs/controller-tools#705

[dprotaso]

/assign @nader-ziada

[codecov]

Codecov Report

Merging #13095 (ec4b0c8) into main (752c336) will decrease coverage by 0.28%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #13095      +/-   ##
==========================================
- Coverage   87.05%   86.77%   -0.29%     
==========================================
  Files         197      197              
  Lines       14443    14477      +34     
==========================================
- Hits        12573    12562      -11     
- Misses       1576     1619      +43     
- Partials      294      296       +2     

Impacted Files	Coverage Δ
pkg/apis/serving/fieldmask.go	95.55% <ø> (-0.08%)	⬇️
pkg/reconciler/revision/cruds.go	62.50% <0.00%> (-14.43%)	⬇️
pkg/reconciler/revision/reconcile_resources.go	66.88% <0.00%> (-13.92%)	⬇️
pkg/reconciler/revision/revision.go	92.13% <0.00%> (-4.34%)	⬇️
pkg/reconciler/configuration/configuration.go	83.67% <0.00%> (-1.54%)	⬇️
cmd/queue/main.go	0.64% <0.00%> (ø)
cmd/activator/main.go	0.00% <0.00%> (ø)
cmd/controller/main.go	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 752c336...ec4b0c8. Read the comment docs.

[nader-ziada]

looks like the failures in e2e are legit

[nader-ziada]

looks like the failures in e2e are legit

the serving install fails on the kind cluster before running the tests

[dprotaso]

Yeah looks like the generated Schema is invalid - will take a look tomorrow

[dprotaso]

/test build-tests_serving_main

[dprotaso]

/test build-tests_serving_main

[dprotaso]

Just checking which version of the branch is being pulled from the go cache

/test build-tests_serving_main

[dprotaso]

/assign @evankanderson for conformance test changes

[nader-ziada]

lgtm

[evankanderson]

Pruning seems like unexpected behavior -- for example, if I write:

...
  posts:
    - port: 5000
      name: http

Having Knative attempt to start my pod and eventually fail the health check seems less useful than "Hey dummy, what is posts, I don't know about it."

Is there a way to error on fields not in the schema?

[dprotaso]

Is there a way to error on fields not in the schema?

Unfortunately no and to catch these errors would mean you would have to preserve fields and still validate with webhooks. Also to preserve unknown fields while having the entire spec defined means you would need excess x-kubernetes-preserve-unknown-fields: true properties set at every node in the schema.

(with pruning) It does buy contributors some benefits

• we can introduce a new field and use it in the same release
• we have move validation logic out of the webhook and into the schema (so it's enforced by the k8s api server)
• having the flushed out schema could introduce some client side validation (ie. non-go clients)

[dprotaso]

/assign @psschwei since Nader's OOO

[evankanderson]

Testing this, I followed the structural schema validation example, and defined the CRD they mentioned. I then defined a custom resources where I mis-spelled replicas as replica:

apiVersion: "stable.example.com/v1"
kind: CronTab
metadata:
  name: my-new-cron-object
spec:
  cronSpec: "* * * * *"
  image: my-awesome-cron-image
  replica: 15

Running kubectl apply on these resource produces the following error:

error: error validating "/tmp/cr.yaml": error validating data: ValidationError(CronTab.spec): unknown field "replica" in com.example.stable.v1.CronTab.spec; if you choose to ignore these errors, turn validation off with --validate=false

But, it turns out this error is client-side, and if I pass --validate=false, the field is silently cleared from the resource without error and accepted by the apiserver.

Since the Knative spec was an attempt to back-document the Kubernetes apiserver behavior, I think we'd have to give this one a pass for now, but I'd love it if @dprotaso could file an issue upstream asking for a "validation means validation, not silently make my request something different" feature.

[dprotaso]

Created an upstream issue here: kubernetes/kubernetes#111074

[evankanderson]

/approve
/lgtm

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, evankanderson

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:

• config/OWNERS [dprotaso,evankanderson]
• hack/OWNERS [dprotaso,evankanderson]
• pkg/apis/OWNERS [dprotaso,evankanderson]
• test/OWNERS [dprotaso,evankanderson]

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