YAML Parser tests accept arbitrary fields

[lbernick]

Expected Behavior

Test inputs representing TaskRuns, PipelineRuns, etc, should not contain fields that aren't part of the spec of those objects.

Actual Behavior

The following test passes:

func TestParseTaskRun(t *testing.T) {
	MustParseTaskRun(t,
		`
spec:
  taskRef:
    name: mytask
  foo: bar
`)
}

We use the yaml parser extensively in pipelinerun_test.go and taskrun_test.go

[06kellyjac]

Tekton Pipelines resources have no schema so they just accept arbitrary data happily

$ kubectl get crd taskruns.tekton.dev -o json | jq '.spec.versions[].schema'
{
  "openAPIV3Schema": {
    "type": "object",
    "x-kubernetes-preserve-unknown-fields": true
  }
}

If you make a task with random nonsense it'd successfully be created but then every time you make a pod it will be rejected for the invalid nonsense field

[jerop]

related discussion in #297

there's a downside that @jonjohnsonjr pointed out:

If you don't allow unknown fields and you ever plan to add a new field, your users won't be able to downgrade build-pipeline, because the new field will be unrecognized by the old build-pipeline.

this was before I joined tekton, but I believe build-pipeline that's mentioned there was the old name for tekton

[dibyom]

If you make a task with random nonsense it'd successfully be created but then every time you make a pod it will be rejected for the invalid nonsense field

I don't know if this is true - we have a validation webhook that should fail this validation
edit: this is what I am talking about https://github.com/tektoncd/pipeline/blob/main/cmd/webhook/main.go#L105-L106

though I think that's a different issue from our test yaml parser allowing arbitrary fields

[06kellyjac]

Tekton already has upgrade schema migrations, is it not possible to have downgrade schema migrations?

Also if there is a breaking difference across schemas I dont think it should be as easy as change the version and apply it because they're different.
You could apply something that doesnt work at all and then youve gone and broken all the pods it churns out.

And if its at the cost of not getting any validation feedback for mistakes thats pretty painful

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[afrittoli]

@lbernick is it ok to close this issue now?

[lbernick]

Thanks for checking @afrittoli, I think my main concern is that it's possible to define invalid yaml in reconciler tests without being alerted by a test failure. We don't have to change the allow-unknown-fields in our CRD schemas since we have the validation webhook (that's probably a separate discussion, and better tracked by #1461) but I think this issue has value in ensuring our tests aren't providing false positive signals.

/remove-lifecycle rotten

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[tekton-robot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

[tekton-robot]

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

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.