Revert removal of PipelineResources related fields

[lbernick]

This commit brings back PipelineResources related fields to the API structs, marking them as deprecated and unused. The JSON tags are removed, so CRDs cannot be defined with these fields. Keeping these fields in the Go structs allows clients to maintain backwards compatibility with older servers, and allows downstream projects such as chains to continue to work with older CRDs that may define these fields.

Closes #6430
/kind misc

Submitter Checklist

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

• n/a Has Docs included if any changes are user facing
• n/a Has Tests included if any functionality added or changed
• 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)
• n/a Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Revert removal of client fields for PipelineResources for backwards compatibility. PipelineResources are still no longer functional and will not work with this version of Tekton.
Example command to list tasks that use PipelineResources and must be deleted before upgrading (works for other Tekton CRDs as well):
`kubectl get taskruns --all-namespaces -o json | jq -r '.items[] | select(.metadata.annotations["[tekton.dev/v1beta1Resources](http://tekton.dev/v1beta1Resources)"] | (. != "{}") and (. != "") and (. != null)) | .metadata.namespace + "/" + .metadata.name + " " + .metadata.creationTimestamp'`

[wlynch]

Thanks! 🙏

We probably want to block creation of objects with these fields set in the validating webhook. Otherwise we'll accept and store objects with the fields set, even if we don't act on them.

[lbernick]

Thanks! 🙏

We probably want to block creation of objects with these fields set in the validating webhook. Otherwise we'll accept and store objects with the fields set, even if we don't act on them.

Setting the JSON tag to "-" should take care of this (https://pkg.go.dev/encoding/json#Marshal). If I create the following task definition:

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: foo
spec:
  resources:
    outputs:
      name: storage-gcs
      type: gcs
  steps:
    - image: objectuser/run-java-jar #https://hub.docker.com/r/objectuser/run-java-jar/
      command: [jar]
      args: ["-cvf", "-o", "/workspace/output/storage-gcs/", "projectname.war", "*"]
      env:
        - name: "FOO"
          value: "world"

I get the error

Error from server (BadRequest): error when creating "pipelineresources.yaml": admission webhook "webhook.pipeline.tekton.dev" denied the request: mutation failed: cannot decode incoming new object: json: cannot unmarshal object into Go struct field TaskResources.spec.resources.outputs of type []v1beta1.TaskResource

[wlynch]

That would also impact a client's ability to unmarshal json/yaml, so I don't think we can do that. 😞

Something else I thought of - we'll want to be careful of how we reject requests. If we start to reject an in-progress request that has already been previously accepted, we'll end up in a retry loop because we won't be able to update the object (even to mark it as failed). Ideally we'll only want to reject new requests (admission requests have the old state, so we should be able to detect this). 🤔

[lbernick]

That would also impact a client's ability to unmarshal json/yaml, so I don't think we can do that. 😞

Something else I thought of - we'll want to be careful of how we reject requests. If we start to reject an in-progress request that has already been previously accepted, we'll end up in a retry loop because we won't be able to update the object (even to mark it as failed). Ideally we'll only want to reject new requests (admission requests have the old state, so we should be able to detect this). 🤔

I've fixed the serialization and added validation that these fields cannot be used.

I'm a bit confused about the latter comment. Is the concern that a user who upgrades their server version will no longer be able to update old CRD definitions that contain PipelineResources, and the suggestion to only add this validation for Create requests and not update requests? I think rejecting is a reasonable choice to make-- our release notes for #6150 state that this will break pipelineresources.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go	96.5%	95.9%	-0.6
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.0%	99.0%	0.0
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	96.8%	96.8%	0.0
pkg/apis/pipeline/v1beta1/task_validation.go	96.9%	97.0%	0.0
pkg/apis/pipeline/v1beta1/taskrun_validation.go	97.7%	97.8%	0.0

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go	96.5%	95.9%	-0.6
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.0%	99.0%	0.0
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	96.8%	96.8%	0.0
pkg/apis/pipeline/v1beta1/task_validation.go	96.9%	97.0%	0.0
pkg/apis/pipeline/v1beta1/taskrun_validation.go	97.7%	97.8%	0.0

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go	96.5%	95.9%	-0.6
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.0%	99.0%	0.0
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	96.8%	96.8%	0.0
pkg/apis/pipeline/v1beta1/task_validation.go	96.9%	97.0%	0.0
pkg/apis/pipeline/v1beta1/taskrun_validation.go	97.7%	97.8%	0.0

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go	96.5%	95.9%	-0.6
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.0%	99.0%	0.0
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	96.8%	96.8%	0.0
pkg/apis/pipeline/v1beta1/task_validation.go	96.9%	97.0%	0.0
pkg/apis/pipeline/v1beta1/taskrun_validation.go	97.7%	97.8%	0.0

[wlynch]

I'm a bit confused about the latter comment. Is the concern that a user who upgrades their server version will no longer be able to update old CRD definitions that contain PipelineResources, and the suggestion to only add this validation for Create requests and not update requests? I think rejecting is a reasonable choice to make-- our release notes for #6150 state that this will break pipelineresources.

Kinda - it's a niche edge case, but the concern is for pending runs that didn't get to execute before the update and the controller's ability to modify them. When the new controller picks up the unfinished run, will the validating webhook deny status updates to the run because a pipeline resource is set? If the answer is yes then the resource will be stuck as pending in a retry loop because the reconciler can't update the status to mark the run as completed (even for failure). I don't know if this is actually a problem - I'm not sure how k8s handles validation webhooks for status updates (it might be okay if the validating webhook doesn't trigger on updates to the status subresource).

You could probably simulate this by doing something like:

1. Install v0.45
2. Delete the pipeline controller deployment (but leave the admission webhooks) - this should let you create resources but they won't be able to run.
3. Create a run with a pipeline resource
4. Upgrade to v0.46 w/ your latest patch
5. ???

There's also a secondary issue of when the new reconciler picks up an existing pending run that had pipeline resources set, should it fail the run or continue as if the user never specified it? I can see both sides here 🤷

[lbernick]

We had some similar discussion on #5999 about guaranteeing the behavior of running pipelineruns when someone flips a feature flag, and we decided not to. This seems like a similar question; should we guarantee what happens to running pipelineruns when you update pipelines, and I think the same decision probably applies.

It also seems like a lot of the reason to bring these fields back is for clients to be able to handle older servers, not to extend the lifespan of CRDs using pipelineresources, so I'm not sure we should try to address this edge case.

[wlynch]

Talked with @lbernick off-thread.

I'll create another issue for discussion how we want to handle avoiding reconcile loops in general (this also affects feature flags)

For this PR we're going to move ahead with the current implementation with a release note with a short script for how to identify and delete and pending taskruns that might be stuck in a problematic state.

[lbernick]

Thanks @wlynch, release notes updated!

[wlynch]

Release notes script should be taskruns instead of tasks, but otherwise

/lgtm

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wlynch

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [wlynch]

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