Add CRD OpenAPI validation for Pipeline, PipelineResource, Task

[sbose78]

Changes

Generated the OpenAPI validation schema for the

• Pipeline CRD
• PipelineResource CRD
• Task CRD

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

• If you are adding a new binary/image to the cmd dir, please update
  the release Task and TaskRun to build and release this image

Reviewer Notes

If API changes
are included, additive changes
must be approved by at least two OWNERS
and backwards incompatible changes
must be approved by more than 50% of the OWNERS,
and they must first be added
in a backwards compatible way.

Release Notes

OpenAPI Validation schema added for the Pipeline CRD.

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sbose78
To complete the pull request process, please assign dlorenc
You can assign the PR to them by writing /assign @dlorenc 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

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[tekton-robot]

Hi @sbose78. 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.

[sbose78]

@googlebot I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[tekton-robot]

@sbose78: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

[abayer]

/ok-to-test

[vdemeester]

/test pull-tekton-pipeline-integration-tests

[bobcatfish]

Thanks for this @sbose78 !

My main question for this change: did you generate these with a tool, or did you create this by hand? If it's with a tool, we should add something to https://github.com/tektoncd/pipeline/blob/master/DEVELOPMENT.md#adding-new-types about how to keep this up to date (and maybe add an issue to the backlog to add some automation to make sure it stays up to date!).

If it's done by hand then I'm a bit hesitant to include this since we'll have to keep updating it as we make changes - and it's not 100% clear to me what we get from this validation that we don't get from our existing validation? (Or maybe we can remove some of it?)

(Also note that in #98 we actually removed open API validation, but maybe we won't run into those same problems anymore?)

Looks like there is a failure in the integration tests:

I0812 08:43:53.705] The Pipeline "list-files-pipeline" is invalid: []: Invalid value: map[string]interface {}{"apiVersion":"tekton.dev/v1alpha1", "kind":"Pipeline", "metadata":map[string]interface {}{"name":"list-files-pipeline", "namespace":"default", "generation":1, "uid":"508aa80b-bcdd-11e9-8fe2-42010a80008d", "annotations":map[string]interface {}{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"tekton.dev/v1alpha1\",\"kind\":\"Pipeline\",\"metadata\":{\"annotations\":{},\"name\":\"list-files-pipeline\",\"namespace\":\"default\"},\"spec\":{\"params\":[{\"default\":\"abc\",\"name\":\"x\"},{\"default\":\"abc\",\"name\":\"y\"}],\"resources\":[{\"name\":\"source-repo\",\"type\":\"git\"}],\"tasks\":[{\"conditions\":[{\"conditionRef\":\"strings-equal\",\"params\":[{\"name\":\"x\",\"value\":\"${params.x}\"},{\"name\":\"y\",\"value\":\"${params.y}\"}]}],\"name\":\"list-files-1\",\"resources\":{\"inputs\":[{\"name\":\"workspace\",\"resource\":\"source-repo\"}]},\"taskRef\":{\"name\":\"list-files\"}}]}}\n"}, "creationTimestamp":"2019-08-12T08:43:53Z"}, "spec":map[string]interface {}{"params":[]interface {}{map[string]interface {}{"name":"x", "type":"string", "default":"abc"}, map[string]interface {}{"default":"abc", "name":"y", "type":"string"}}, "resources":[]interface {}{map[string]interface {}{"type":"git", "name":"source-repo"}}, "tasks":[]interface {}{map[string]interface {}{"conditions":[]interface {}{map[string]interface {}{"params":[]interface {}{map[string]interface {}{"name":"x", "value":"${params.x}"}, map[string]interface {}{"name":"y", "value":"${params.y}"}}, "conditionRef":"strings-equal"}}, "name":"list-files-1", "resources":map[string]interface {}{"inputs":[]interface {}{map[string]interface {}{"name":"workspace", "resource":"source-repo"}}}, "taskRef":map[string]interface {}{"name":"list-files"}}}}}: validation failure list:
I0812 08:43:53.705] spec.tasks.conditions.params.value in body must be of type object: "string"

Since params can be either array type or string type, maybe OpenAPI validation can't handle that kind of syntax?

[sbose78]

Thank you @bobcatfish for your thoughts on this proposal:

did you generate these with a tool,

yes, using the kubegen openapi generator via operator-sdk .

Since params can be either array type or string type, maybe OpenAPI validation can't handle that kind of syntax?

Yes, that scenario did need some manual fixing.

I feel irrespective of whether generators can do a 100% good job - we should use some openapi validation in the CRD because

• It helps the user look at the API and understand what it needs
• It helps clients/servers validate the contract before creating the object in etcd. Today we could potentially create an object with any structure?
• Given that we have a pretty neat test suite now, we needn't have a full openapi validation spec for corner cases. We could leave out specific nested areas as object - need to think a bit how to achieve that.

[sbose78]

For attributes of ArrayOrString I removed the validation information from the spec from those fields because there's no good way to do so with the openapi validation spec today.

[bobcatfish]

Thanks for the detailed explanation @sbose78 !

I'm on board with this if you could also include in this PR a doc that explains how to update these (basically the description you already gave above) - probably https://github.com/tektoncd/pipeline/blob/master/DEVELOPMENT.md#adding-new-types would be a good place for it.

Also I think we should:

• Get some other OWNERS opinions + @sbwsg @dibyom @abayer @dlorenc @vdemeester @imjasonh @afrittoli
• Make a follow up issue to add some linting around this (so we can catch cases where it should be updated and isnt)

[vdemeester]

I am also onboard with this but I really wish we could tool a bit of it 👼

[sbose78]

Sounds good, will do!

[bobcatfish]

Quick ping: @sbose78 is this something you want to keep working on? (no worries if not!)

[sbose78]

I will clean this up later this week, sorry this had gone off my radar. If anyone wishes to take a look at it in the meantime, I'm good as well.

[vdemeester]

Closing in favor of #1776

/close

[tekton-robot]

@vdemeester: Closed this PR.

In response to this:

Closing in favor of #1776

/close

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.