Breaking API change: v0.46 omits data for existing API objects (PipelineResources)

[wlynch]

Expected Behavior

For already existing API objects, the Pipelines API client should return me the same response, regardless of API client version.

Actual Behavior

Because v0.46 removes the API types of pipeline resources along with the functionality, different responses are returned.

This is problematic for downstream users of pipelines (chains, tkn, dashboard, etc.) because we're potentially losing data about what ran if the client version doesn't match the server version.

Steps to Reproduce the Problem

https://github.com/wlynch/test/tree/master/tekton/pipeline-resources-test

1. Take a sample Task/TaskRun w/ PipelineResources created with pipelines <= v0.45.0

2. Use a pipelines v0.46.0 client to fetch the data

3. diff:

   --- v0.45/spec.json	2023-03-24 10:29:21.915291695 -0400
   +++ v0.46/spec.json	2023-03-24 10:30:22.343472020 -0400
   @@ -1,30 +1,6 @@
    {
   -  "resources": {
   -    "outputs": [
   -      {
   -        "name": "image",
   -        "resourceSpec": {
   -          "type": "image",
   -          "params": [
   -            {
   -              "name": "url",
   -              "value": "gcr.io/foo/bar"
   -            }
   -          ]
   -        }
   -      }
   -    ]
   -  },
      "serviceAccountName": "default",
      "taskSpec": {
   -    "resources": {
   -      "outputs": [
   -        {
   -          "name": "image",
   -          "type": "image"
   -        }
   -      ]
   -    },
        "steps": [
          {
            "name": "",

Additional Info

I noticed this with #6150 - I suspect this applies to all deprecated PipelineResource types. /cc @JeromeJu

While PipelineResources have been deprecated for a while, we were still allowing these resources to be created up until v0.45.0.
In general we shouldn't be removing fields from being read alongside feature deprecations - we should block creation of new resources with these fields and wait until we're reasonably confident they are no longer in use. I'd consider putting removal of fields through another deprecation cycle so there's a period where they are effectively in a read-only state.

• Kubernetes version:

  Output of kubectl version:

  $ kubectl version --short
  Client Version: v1.25.4
  Kustomize Version: v4.5.7
  Server Version: v1.25.2
  

• Tekton Pipeline version:

  Output of tkn version or kubectl get pods -n tekton-pipelines -l app=tekton-pipelines-controller -o=jsonpath='{.items[0].metadata.labels.version}'

  $ tkn version
  Client version: 0.29.1
  Chains version: devel
  Pipeline version: v0.45.0
  

[dibyom]

I agree with this in general but PipelineResources are 1. an alpha feature and 2. been deprecated for well over a year. The last version we officially supported was v0.44.0.

As I understand it, we'd have to remove the feature with the breaking change you described eventually. What do we get from postponing this removal to a future release? IMO, we should not guarantee no loss of information for alpha APIs or features.

[wlynch]

I think there needs to be a distinction between dropping support for a feature, and removing the field from API / client libraries.

I'm all for removing support for a feature quickly, especially if it's alpha.

For downstream clients though, because pipelines allowed creation of these resources up through v0.45.0, clients need to be able to handle the field since it could be set by users. By removing the field it forces downstream clients to require their minimum supported version to match their client version. i.e. if chains wanted to upgrade its client version to v0.46.0, we'd also have to accept that v0.46.0 is now the new minimum pipelines server version that is supported, since we can no longer guarantee support for versions <= v0.45 where the field may have been set.

Even within pipelines itself there's a window for potentially problematic behavior - if a resource was accepted but not completed before an upgrade happened, the post upgrade state would ignore the resource fields on re-reconcile.

This deprecation was particularly tricky, since it was a long-standing feature and had made it's way into the beta API surface, even though it was considered an Alpha feature. I think my preference would be to follow alpha deprecation guidelines for removing the feature, but follow beta deprecation guidelines for removal from the API surface (starting from when the field can no longer be set for new resources), and treat these as 2 separate deprecation events. This way we leave clients with a window where they can continue to support older versions, even after the feature is dropped.