Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add structural OpenAPI schema to Tekton CRDs #8490

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

burigolucas
Copy link

Changes

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

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.

Furthermore, the controller-gen does not preserve the field "x-kubernetes-preserve-unknown-fields: true" in the root of the schema. This field is added by the hack/update-schemas.sh script after the CRD has been updated to include the schema.

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

The structural OpenAPI schema to Tekton CRDs are added allowing the user to use `kubectl explain` to review the CRD definition.

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 15, 2025
@tekton-robot
Copy link
Collaborator

[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:

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

Copy link

linux-foundation-easycla bot commented Jan 15, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: burigolucas / name: Lucas Burigo (f1f45fb)

@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 15, 2025
@tekton-robot
Copy link
Collaborator

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.

@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 15, 2025
@burigolucas
Copy link
Author

/kind documentation

@tekton-robot tekton-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Jan 15, 2025
@vdemeester
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 15, 2025
@burigolucas
Copy link
Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

burigolucas commented Jan 16, 2025

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?

- Implement POSIX script to update the OpenAPI schema to Tekton CRDs using controller-gen
- Add updated CRDs with OpenAPI schema

Note1: 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.

Note2: the controller-gen does not preserve the field "x-kubernetes-preserve-unknown-fields: true" in the root of the schema. This field is added by the 'hack/update-schemas.sh' script

Note3: Markes were addded to avoid creating the schema for recursive types that results in invalid schemata. Related issue: kubernetes-sigs/controller-tools#585

Note4: Schema for PipelineSpec was omitted from PipelineRun to reduce size. A comment was added in the description of the field for the user to refer to the schema of Pipeline.

Note5: Schema of TaskSpec was omitted from TaskRun to reduce size. A comment was added in the description of the field for the user to refer to the schema of Task.
@tekton-robot
Copy link
Collaborator

@burigolucas: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-tekton-pipeline-build-tests f1f45fb link true /test pull-tekton-pipeline-build-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@vdemeester
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants