Add structural OpenAPI schema to Tekton CRDs

[burigolucas]

Changes

Address Issue 1461 by implementing a script to update the OpenAPI schema to Tekton CRDs using controller-gen and adding the updated CRDs with OpenAPI schema.

Revise unit tests and example YAML files to address issues caused by the now enabled API server schema validation.

Note that the current script hack/update-schemas.sh return several errors/warnings and status code 1 from execution of the controller-gen CLI due to issues when parsing the schema from the source code. Besides, processing multiple files at once is causing segmentation fault. Therefore, in the script, each file is processed separately. In addition, FIXME notes are added to be addressed in the future once new features become available in controller-gen.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
• Has Tests included if any functionality added or changed
• pre-commit Passed
• Follows the commit message standard
• Meets the Tekton contributor standards (including functionality, content, code)
• Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
• Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
• Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

action required: The structural OpenAPI schema to Tekton CRDs are added enabling API server schema validation and supporting `kubectl explain` to describe fields and structure of Tekton CRDs. Due to the API server schema validation, users should make sure Tekton CRs have a valid schema when creating or updating CRs.

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign afrittoli after the PR has been reviewed.
You can assign the PR to them by writing /assign @afrittoli in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: burigolucas / name: Lucas Burigo (9a8c0b4, ff2f380, e4bbd38, fc5726e)

[tekton-robot]

Hi @burigolucas. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[burigolucas]

/kind documentation

[vdemeester]

/ok-to-test

[burigolucas]

Hi @vdemeester, I have identified issues with the CRDs. A few of them were invalid due to recursive fields. Following the fix proposed in this issue Generating CRD manifest for struct with recursive field, I have added the markers

	// +kubebuilder:pruning:PreserveUnknownFields
	// +kubebuilder:validation:Schemaless

in the affected fields and regenerated the CRDs. I have also disabled the embedded object meta as it does not provide useful schema in my opinion and only makes the files more bloated.

AFAIK, there is still one issue to be solved, namely, the CRD for the PipelineRun is too large. We reach the file limit with kubectl create. Any ideas how best to address that?

[vdemeester]

AFAIK, there is still one issue to be solved, namely, the CRD for the PipelineRun is too large. We reach the file limit with kubectl create. Any ideas how best to address that?

Yeah, that's was the inital problem 😅

[vdemeester]

Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "pipelines.tekton.dev" is invalid: metadata.annotations: Too long: must have at most 262144 bytes
Error from server (RequestEntityTooLarge): error when creating "STDIN": Request entity too large: limit is 3145728
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "tasks.tekton.dev" is invalid: metadata.annotations: Too long: must have at most 262144 bytes
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "taskruns.tekton.dev" is invalid: metadata.annotations: Too long: must have at most 262144 bytes

[vdemeester]

@burigolucas could we refer to another schema ? PipelineRun gets too big because of pipelineSpec that is, well the Pipeline.Spec field (and that one also contains Task.Spec with taskSpec). Maybe we should "skip" those "embedded" type in the schema and instead "document" or "refer" to the other schema ?

[burigolucas]

Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "pipelines.tekton.dev" is invalid: metadata.annotations: Too long: must have at most 262144 bytes
Error from server (RequestEntityTooLarge): error when creating "STDIN": Request entity too large: limit is 3145728
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "tasks.tekton.dev" is invalid: metadata.annotations: Too long: must have at most 262144 bytes
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "taskruns.tekton.dev" is invalid: metadata.annotations: Too long: must have at most 262144 bytes

Are you trying kubectl apply? In that case it will fail as it cannot store the changes in the annotations. It should work with kubectl create except for the PipelineRun due to the size. If you remove the older version of the PipelineRun in the CRD, then you can also verify that the CRD is valid. Perhaps we could omit parts of the schema of the PipelineRun for the older CRD version?

[vdemeester]

Are you trying kubectl apply? In that case it will fail as it cannot store the changes in the annotations. It should work with kubectl create except for the PipelineRun due to the size.

Yeah it's what (kubectl apply) the CI (github workflow) does (and most our users as well 😅 )

If you remove the older version of the PipelineRun in the CRD, then you can also verify that the CRD is valid. Perhaps we could omit parts of the schema of the PipelineRun for the older CRD version?

You mean the v1beta1 part right ?

[vdemeester]

I would be fine only having the shema for the v1 (or the highest) version available in the CRD to be honest.
@afrittoli @tektoncd/core-maintainers what do you think ?

[burigolucas]

I have just tried to remove the pipelineSpec from the schema of the pipelineRun and it is now small enough, even with both versions of the CRD. But we might need to reduce other CRDs further if we need to support kubectl apply due to the limits of the annotation field.

[burigolucas]

Removed also the schema of TaskSpec in TaskRun and added a comment for the user to refer to the schema of Task. This also reduced size but currently Task, TaskRun, Pipeline and PipelineRun are all too large to be stored in the annotations field when using kubectl apply. Any further suggestions what else to remove from the schema to address the length of the files?

[vdemeester]

Removed also the schema of TaskSpec in TaskRun and added a comment for the user to refer to the schema of Task. This also reduced size but currently Task, TaskRun, Pipeline and PipelineRun are all too large to be stored in the annotations field when using kubectl apply. Any further suggestions what else to remove from the schema to address the length of the files?

I am "fine" with requiring things to be done using kubectl create and document this I guess ?
cc @afrittoli

[afrittoli]

Removed also the schema of TaskSpec in TaskRun and added a comment for the user to refer to the schema of Task. This also reduced size but currently Task, TaskRun, Pipeline and PipelineRun are all too large to be stored in the annotations field when using kubectl apply. Any further suggestions what else to remove from the schema to address the length of the files?

I am "fine" with requiring things to be done using kubectl create and document this I guess ? cc @afrittoli

This would be for the CRDs only, right?
Existing installation scripts and tools may break if we make this change. To provide for a smoother transition, we could add an extra release file to our releases:

• release.yaml: includes sha and tag for images, includes CRDs with schema
• release.notag.yaml: includes CRDs with schema
• release.oldcrds.yaml: includes sha and tag for images, includes CRDs without schema

This way users could point to release.oldcrds.yaml until they migrate to installing via kubectl create.

[vdemeester]

@afrittoli should it be the other way around for a while. Like next release we have a release-with-schemas.yaml and we announce a given timeline for changing the default and then we do the other way around ?

[afrittoli]

@afrittoli should it be the other way around for a while. Like next release we have a release-with-schemas.yaml and we announce a given timeline for changing the default and then we do the other way around ?

Sure, we can do that, I was trying to avoid too many changes :D but it's safer the way you proposed

[burigolucas]

Should we then keep the current CRDs untouched and store the new generated CRDs with schemas in a different path?

[afrittoli]

Should we then keep the current CRDs untouched and store the new generated CRDs with schemas in a different path?

If possible, I would prefer to have one version of the CRDs in the repos and, at release time:

• copy release.yaml to release.schemas-in-crds.yaml
• strip the schemas from release.yaml

@vdemeester @burigolucas WDYT?

[vdemeester]

Should we then keep the current CRDs untouched and store the new generated CRDs with schemas in a different path?

If possible, I would prefer to have one version of the CRDs in the repos and, at release time:

* copy `release.yaml` to `release.schemas-in-crds.yaml`

* strip the schemas from `release.yaml`

@vdemeester @burigolucas WDYT?

I agree, but it means we need to change our CI script to use create instead of apply 👼🏼 .

[burigolucas]

Thanks for your feedbacks @vdemeester and @afrittoli. I will soon review the scripts accordingly. Besides, I will have a second look on the schemas of Task, TaskRun, Pipeline and PipelineRun to check whether there is a chance to remove schema from well-known types to reduce the size in order to support kubectl apply.

[burigolucas]

@vdemeester, @afrittoli I have analyzed the schema more carefully and identified good candidates to omit from the schema (e.g., Pod.spec.volumes, Pod.spec.affinity, PersistentVolumeClaim). While omitting these schema, I have added a note in the description for the user where to locate the schema in the K8s API. With these changes, the CRDs are now significantly smaller and can be kubectl applyed. In my opinion, this is a reasonable solution and does not require different release files. However, the e2e tests are broken. I will need help to address that.

[afrittoli]

@vdemeester, @afrittoli I have analyzed the schema more carefully and identified good candidates to omit from the schema (e.g., Pod.spec.volumes, Pod.spec.affinity, PersistentVolumeClaim). While omitting these schema, I have added a note in the description for the user where to locate the schema in the K8s API. With these changes, the CRDs are now significantly smaller and can be kubectl applyed. In my opinion, this is a reasonable solution and does not require different release files. However, the e2e tests are broken. I will need help to address that.

Thank you for this work @burigolucas - this sounds like a great solution to me

[burigolucas]

One of the issues with the e2e is related to schema for fields which can have different types. E.g., the schema for Pipeline.spec.params.default is set by controller-gen as object, throwing a validation error when attempting to create a pipeline with a string value as the default param value. I will revise the CRDs schema again to try resolving this and other related issues.

[burigolucas]

I have added the schema for the feature flags as well and had to update unit tests and a few YAML examples due to the API server schema validation. This validation will cause breaking changes to users who were not creating CRs with a valid schema. I have updated the release notes accordingly. Besides, the schema for feature flags has changed with the fields starting with lower case instead of upper case. I am not able to run the e2e tests locally to fully test though.