Incorporate schema into CRDs

[mattmoor]

K8s added the ability to specify schema to CRD definitions in 1.9. We should take advantage of that.

[grantr]

kubebuilder makes this automatic, see https://github.com/kubernetes-sigs/kubebuilder/blob/a81a1ccff079f2509b93f2735a5828bf5fedc586/samples/full/controller/src/samplecontroller/pkg/apis/samplecontroller/v1alpha1/zz_generated.kubebuilder.go#L65-L101.

[mattmoor]

Related: kubernetes/kubernetes#59154

[jonjohnsonjr]

We might need kubernetes-sigs/kubebuilder#406 to be fixed before we can use the crd generate tool from kubebuilder.

[knative-housekeeping-robot]

Issues go stale after 90 days of inactivity.
Mark the issue as fresh by adding the comment /remove-lifecycle stale.
Stale issues rot after an additional 30 days of inactivity and eventually close.
If this issue is safe to close now please do so by adding the comment /close.

Send feedback to Knative Productivity Slack channel or file an issue in knative/test-infra.

/lifecycle stale

[knative-housekeeping-robot]

Stale issues rot after 30 days of inactivity.
Mark the issue as fresh by adding the comment /remove-lifecycle rotten.
Rotten issues close after an additional 30 days of inactivity.
If this issue is safe to close now please do so by adding the comment /close.

Send feedback to Knative Productivity Slack channel or file an issue in knative/test-infra.

/lifecycle rotten

[markusthoemmes]

/remove-lifecycle rotten

I think this is still valuable.

[markusthoemmes]

The bug above seems fixed, is this worth another look?

[evankanderson]

/remove-area build

[knative-prow-robot]

@evankanderson: Those labels are not set on the issue: area/build

In response to this:

/remove-area build

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.

[evankanderson]

/good-first-issue

It looks like #10003 just needs a little help across the finish line

/triage accepted

[knative-prow-robot]

@evankanderson:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

It looks like #10003 just needs a little help across the finish line

/triage accepted

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.

[markusthoemmes]

So, I've taken a whack at schema generation today, just to see where we stand. Opened #11243 so everybody can play with it easily.

To copy my remarks from the PR, here are the blockers we have today.

1. Our own URL type is making issues. Doing the same trick as I did with VolatileTime didn't solve the issue, but I haven't looked immensely deep. This currently runs a patched version of controller-gen to force the type of URL to be string in-code. Might be a bug.
2. Our PodSpec validation differs from upstream's. This is a fairly big one and I don't quite know how to work around that without creating our own PodSpec type hierarchy, that mirrors that of upstream. That'd have the additional benefit of us dropping all the fields we don't actually allow, cleaning the schema up significantly. The only alternative is to drop the podSpec part off the schema entirely and have it be an untyped object but... does that really help?

1 we could patch (as I did) in the vendored version of the generator if need be, but 2. seems to be the elephant in the room for us quite honestly. Maybe it is time to create our own PodSpec type and use that? That would probably also aid in API docs generation etc.

What do others think?

[julz]

I actually think if we could make it so we use the schema to validate the podSpec fields we don't allow we might actually end up with less code than before, because we could drop our custom validation, potentially. If so then that starts to seem like a win, but I worry we'll run in to size/maintenance issues as we support more and more of the podSpec. Do I have a memory that when we tried this before we ran in to maximum size limits for the schema, or am I misremembering?

[markusthoemmes]

The size limits were hit, because we served 3 versions of the APIs. The schema is copied across those versions. The current Service schema clocks in at 340K, which is not small but also not breaking anything yet.

[markusthoemmes]

Made some good progress with adjusting some of the K8s tooling for our needs, which ultimately led me to #11244, which has a full schema for all of our types, filtered down by the surface we allow in PodSpec (+ feature flags, but we could drop those fields from the schema too if that's contentious).

If all agree, we could ship those schemas as a first step and then continue working on the tooling to be able to continuously generate them automatically.

/assign

[evankanderson]

My $0.02 in terms of feature-flagged fields supported in PodSpec would be to be maximal in the OpenAPI spec, and make sure we have documentation that certain fields may not be supported on all clusters. (Which is true anyway, since you could also have OPA policy or other admission webhooks enforcing limits.)

[julz]

+1 to what Evan said. It's already the case that certain pod fields may be in the api but forbidden by policy, so including the maximal set feels reasonable to me too.

(Does make me wonder a little if we should start thinking about moving the remaining validation to an actual policy engine at some point, but that feels like an area that's not quite ready for us to adopt yet so 🤷‍♂️. I guess some of it will happen via the new PSP replacement stuff anyway)

[markusthoemmes]

Another updated from my side:

#11244 is now submittable as-is (modulo getting the pkg dependencies updated, which I'm waiting for the robot to do).

I've excluded the script from update-codegen.sh from now, as there's a bit of trickery to be done with run_go_tool and I'd love to get our own fork of controller-tools going before we fix this all. A separate script might be good enough for now too.

My plan going forward would be:

1. Merge Add complete schemas to all CRDs #11244 to close this issue.
2. Send a PR against knative/networking generating the schemas there as well.
3. Work on creating our own fork of controller-tools in sandbox, so we have a definitive edition of the custom code.
4. Finalize hooking up our own fork of controller-tools with update-codegen.sh to continuously run it.
5. (optional but desired): Use this tooling in eventing as well.
6. Propose CRD-override settings upstream to potentially obsolete the fork.

[markusthoemmes]

Correction from the above: Switch 1 and 2 and do the networking change first to obsolete undoing stuff in vendor after generation.

[dprotaso]

The size limits were hit, because we served 3 versions of the APIs. The schema is copied across those versions. The current Service schema clocks in at 340K, which is not small but also not breaking anything yet.

When does this break again? ie. if we add a v2 do we reach that limit?

[markusthoemmes]

Good question! Actually, since we're now able to filter the PodSpec down to only the things we support, the Service CRD clocks in at a mere 104K. Assuming a 1M size limit in K8s (which is what a quick search yielded) we should be quite a way away from hitting any YAML size limits here.