Losslessly roundtrip Pipelines with Finally from beta to alpha and back

[ghost]

Changes

Fixes #3206

Prior to this commit pipelines returned an error when converting from
a v1beta1 pipeline to a v1alpha1 pipeline if that pipeline had a Finally
section in it. As a result of this error the kubeapi would repeatedly
request v1alpha1 pipelines every second, filling up our webhook logs
with these errors.

This commit allows Finally to be serialized into alpha Pipelines, keeping
the Finally Tasks as an annotation on the resource. If the alpha Pipeline
is then re-applied to the cluster the Finally annotation is rehydrated into
the Finally section of the stored v1beta1 resource.

This is a follow-up to #3757. In that PR we simply squashed the error message, but in this PR we squash the error while also retaining the Finally information during conversion roundtrips.

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)
• Commit messages follow commit message best practices
• Release notes block has been filled in or deleted (only if no user facing changes)

Release Notes

v1beta1 Pipelines can now be requested with v1alpha1 version without losing Finally tasks. Applying the returned v1alpha1 version will store the resource as v1beta1 with the Finally section restored to its original state.

[ghost]

/hold

This is a proof-of-concept that we can pass Finally back to v1alpha1 without losing information when storing it back again to v1beta1. Open to feedback and questions here, still tidying up and editing tests.

[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/v1alpha1/pipeline_conversion.go	96.6%	94.6%	-2.0

[ghost]

/kind bug

[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/v1alpha1/pipeline_conversion.go	96.6%	94.6%	-2.0

[ghost]

Also, some more context, Kubernetes' guidance for CRD versioning links out to their API changes guide wherein they state the following rule:

It must be possible to round-trip your change (convert to different API versions and back) with no loss of information.

[pritidesai]

Thanks @sbwsg for initiating this PR.

I tried creating a sample pipeline in v1beta1 with finally in it but it failed with this error:

kubectl apply -f 01-pipeline-with-finally.yaml
Error from server (BadRequest): error when creating "01-pipeline-with-finally.yaml": admission webhook "webhook.pipeline.tekton.dev" denied the request: mutation failed: cannot decode incoming new object: json: unknown field "finally"

Webhook logs:

{"level":"info","ts":"2021-02-22T19:03:10.335Z","logger":"tekton-pipelines-webhook","caller":"webhook/admission.go:131","msg":"remote admission controller audit annotations=map[string]string(nil)","commit":"9731b96","knative.dev/kind":"tekton.dev/v1alpha1, Kind=Pipeline","knative.dev/namespace":"default","knative.dev/name":"01-pipeline-with-final-task","knative.dev/operation":"CREATE","knative.dev/resource":"tekton.dev/v1alpha1, Resource=pipelines","knative.dev/subresource":"","knative.dev/userinfo":"{minikube-user  [system:masters system:authenticated] map[]}","admissionreview/uid":"614e9634-79ce-4e22-84aa-7f7ee90a7316","admissionreview/allowed":false,"admissionreview/result":"&Status{ListMeta:ListMeta{SelfLink:,ResourceVersion:,Continue:,RemainingItemCount:nil,},Status:Failure,Message:mutation failed: cannot decode incoming new object: json: unknown field \"finally\",Reason:BadRequest,Details:nil,Code:400,}"}

I am looking into your changes next.

[ghost]

Here's an example manual test to try with this PR's branch:

# Apply a v1beta1 pipeline with finally section
$ k apply -f ./examples/v1beta1/pipelineruns/pipelinerun-with-final-tasks.yaml

# Get the v1alpha1 rendering of that pipeline
$ kubectl get pipelines.v1alpha1.tekton.dev clone-cleanup-workspace -o yaml  > test.yaml

# Open test.yaml in your text editor of choice.
# Edit to have a new name.
# Edit to remove the UUID from the metadata.

# Re-apply the v1alpha1 yaml
k apply -f ./test.yaml

Now when you kubectl get the pipeline stored from your v1alpha1 yaml you'll see the finally list is still available. The v1beta1 resource has successfully round-tripped to v1alpha1 and back.

[pritidesai]

I tried creating a sample pipeline in v1beta1 with finally in it but it failed with this error:

Sorry, it was my bad 😞 , the sample pipeline I was using was invalid (finally with v1alpha1 😜 )

This works great with annotations having the entire finally section in it:

metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"tekton.dev/v1beta1","kind":"Pipeline","metadata":{"annotations":{},"name":"clone-cleanup-workspace","namespace":"default"},"spec":{"finally":[{"name":"cleanup","taskRef":{"name":"cleanup-workspace"},"workspaces":[{"name":"source","workspace":"git-source"}]},{"name":"check-git-commit","params":[{"name":"commit","value":"$(tasks.clone-app-repo.results.commit)"}],"taskSpec":{"params":[{"name":"commit"}],"steps":[{"image":"alpine","name":"check-commit-initialized","script":"if [[ ! $(params.commit) ]]; then\n  exit 1\nfi\n"}]}}],"tasks":[{"name":"clone-app-repo","params":[{"name":"url","value":"https://github.com/tektoncd/community.git"},{"name":"subdirectory","value":"application"}],"taskRef":{"name":"git-clone-from-catalog"},"workspaces":[{"name":"output","workspace":"git-source"}]}],"workspaces":[{"name":"git-source"}]}}
    tekton.dev/v1beta1Finally: '[{"name":"cleanup","taskRef":{"name":"cleanup-workspace","kind":"Task"},"workspaces":[{"name":"source","workspace":"git-source"}]},{"name":"check-git-commit","taskSpec":{"metadata":{},"params":[{"name":"commit","type":"string"}],"steps":[{"name":"check-commit-initialized","image":"alpine","resources":{},"script":"if
      [[ ! $(params.commit) ]]; then\n  exit 1\nfi\n"}]},"params":[{"name":"commit","value":"$(tasks.clone-app-repo.results.commit)"}]}]'

And verified its possible to round-trip.

But I am not sure if this is the right solution though (I can approve these changes since at least the log spamming is stopped with this PR 😄 ).

My concerns:

1. We are getting into this kind of annotations with every new field we add in beta(at least until we have v1).
2. I just tried creating beta pipeline with when expressions and listing it in pipelines.v1alpha1.tekton.dev. The conversion silently (without any error) ignores when expressions and the converted pipeline does not include any of those expressions.

beta:

kind: Pipeline
metadata:
  name: 01-pipeline-with-when
spec:
  tasks:
    - name: dag-task
      when:
        - input: "foo"
          operator: in
          values: [ "foo" ]
      taskSpec:
        steps:
          - name: success
            image: ubuntu
            script: |
              exit 0

converted to alpha:

spec:
  tasks:
  - name: dag-task
    taskSpec:
      steps:
      - image: ubuntu
        name: success
        resources: {}
        script: |
          exit 0

And, we haven't heard any complains on these missing when expressions which kind of tells us that the users are not dependent on this conversion.

We will have to fix when expressions if we go this route.

3. How are we going to handle conversions from v1 to v1beta1? Will there be expectation of having such annotations in v1 resources?

[afrittoli]

I tried creating a sample pipeline in v1beta1 with finally in it but it failed with this error:

Sorry, it was my bad 😞 , the sample pipeline I was using was invalid (finally with v1alpha1 😜 )

This works great with annotations having the entire finally section in it:

metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"tekton.dev/v1beta1","kind":"Pipeline","metadata":{"annotations":{},"name":"clone-cleanup-workspace","namespace":"default"},"spec":{"finally":[{"name":"cleanup","taskRef":{"name":"cleanup-workspace"},"workspaces":[{"name":"source","workspace":"git-source"}]},{"name":"check-git-commit","params":[{"name":"commit","value":"$(tasks.clone-app-repo.results.commit)"}],"taskSpec":{"params":[{"name":"commit"}],"steps":[{"image":"alpine","name":"check-commit-initialized","script":"if [[ ! $(params.commit) ]]; then\n  exit 1\nfi\n"}]}}],"tasks":[{"name":"clone-app-repo","params":[{"name":"url","value":"https://github.com/tektoncd/community.git"},{"name":"subdirectory","value":"application"}],"taskRef":{"name":"git-clone-from-catalog"},"workspaces":[{"name":"output","workspace":"git-source"}]}],"workspaces":[{"name":"git-source"}]}}
    tekton.dev/v1beta1Finally: '[{"name":"cleanup","taskRef":{"name":"cleanup-workspace","kind":"Task"},"workspaces":[{"name":"source","workspace":"git-source"}]},{"name":"check-git-commit","taskSpec":{"metadata":{},"params":[{"name":"commit","type":"string"}],"steps":[{"name":"check-commit-initialized","image":"alpine","resources":{},"script":"if
      [[ ! $(params.commit) ]]; then\n  exit 1\nfi\n"}]},"params":[{"name":"commit","value":"$(tasks.clone-app-repo.results.commit)"}]}]'

And verified its possible to round-trip.

But I am not sure if this is the right solution though (I can approve these changes since at least the log spamming is stopped with this PR 😄 ).

My concerns:

1. We are getting into this kind of annotations with every new field we add in beta(at least until we have v1).
2. I just tried creating beta pipeline with when expressions and listing it in pipelines.v1alpha1.tekton.dev. The conversion silently (without any error) ignores when expressions and the converted pipeline does not include any of those expressions.

beta:

kind: Pipeline
metadata:
  name: 01-pipeline-with-when
spec:
  tasks:
    - name: dag-task
      when:
        - input: "foo"
          operator: in
          values: [ "foo" ]
      taskSpec:
        steps:
          - name: success
            image: ubuntu
            script: |
              exit 0

converted to alpha:

spec:
  tasks:
  - name: dag-task
    taskSpec:
      steps:
      - image: ubuntu
        name: success
        resources: {}
        script: |
          exit 0

And, we haven't heard any complains on these missing when expressions which kind of tells us that the users are not dependent on this conversion.

We will have to fix when expressions if we go this route.

Yes, will have to add annotations for every new field we add (or that we added in the past).

To be honest the alternatives we've considered until now do not seem better:

• stop serving v1alpha1: we could perhaps do that, but that won't fix the issue for feature API versions, so we'd still need to find a solution there
• back-port new fields to old API versions: in my opinion this goes against the whole idea of API versions, but I'd be happy to be proven wrong

I'm not entirely sure what the format of these annotation should be, we could discuss whether we want to have one per field, one per api version, or perhaps event store the complete resource as it's stored in an annotation. Things might get complicated if we add a new field with a new field for instance...

1. How are we going to handle conversions from v1 to v1beta1? Will there be expectation of having such annotations in v1 resources?

I think so, unless we find a better solution.

[afrittoli]

Question: would one be able to create resources with these annotations? So effectively create v1beta1 resources through the v1alpha1 API? I think the answer is yes, so we would need to run validation on the part of the resource which is converted from the annotation. That might be already the case with this PR, I'm not entirely sure.

[afrittoli]

Another idea on how to adding fields to API versions.
Let's say we serve v1beta1 and v1. To add a new field to v1 we would:

• add the new field to v1
• create v1beta2 with the new field back-ported
• stop serving v1beta1

Issues with this approach:

• Creating new versions is probably more work
• If we create a new version for every new field, we may end up creating too much api version churn
• We probably cannot just stop serving v1beta1
• Using these revamped old versions (v1beta2) would require users to change their resources... if they do so, they might as well change to the most recent API version instead.

Possible solutions:

• we could use annotations, and once we reach a critical mass of new fields make a new version
• instead of stopping to serve old versions right away, we could keep them around for a while with annotations
• we could provide a v1beta1 -> v1beta2 migration script: migrating resources from v1beta1 to v1beta2 would be very easy to automate, simply replace the api version on the top

[afrittoli]

Another idea on how to adding fields to API versions.
Let's say we serve v1beta1 and v1. To add a new field to v1 we would:

• add the new field to v1
• create v1beta2 with the new field back-ported
• stop serving v1beta1

Issues with this approach:

• Creating new versions is probably more work
• If we create a new version for every new field, we may end up creating too much api version churn
• We probably cannot just stop serving v1beta1
• Using these revamped old versions (v1beta2) would require users to change their resources... if they do so, they might as well change to the most recent API version instead.

Possible solutions:

• we could use annotations, and once we reach a critical mass of new fields make a new version
• instead of stopping to serve old versions right away, we could keep them around for a while with annotations
• we could provide a v1beta1 -> v1beta2 migration script: migrating resources from v1beta1 to v1beta2 would be very easy to automate, simply replace the api version on the top

@vdemeester I think these processes we should define and document as part of the feature gates TEP

[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/v1alpha1/pipeline_conversion.go	96.6%	89.6%	-7.0

[ghost]

Question: would one be able to create resources with these annotations? So effectively create v1beta1 resources through the v1alpha1 API? I think the answer is yes, so we would need to run validation on the part of the resource which is converted from the annotation. That might be already the case with this PR, I'm not entirely sure.

Good point, I was surprised by this - validation is not performed on the converted resource. So upgrading from v1alpha1 w/ invalid finally annotation did not result in error.

I've pushed a change so that the Finally is validated after unmarshalling from json annotation.

[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/v1alpha1/pipeline_conversion.go	96.6%	93.5%	-3.1

[ghost]

For reference I went looking for any other examples where an annotation is used to store upgrade/downgrade information. The kubernetes Deployment type stores deprecated.deployment.rollback.to in an annotation going from apps to v1: https://github.com/kubernetes/kubernetes/blob/6cacfe9e3a54ca9d532f9f085adfb6455d3b8c82/pkg/apis/apps/v1/conversion.go#L69-L78
* The deprecated value type is an int64
* The upgraded value type is a struct with the int64 in a nested field.

Edit: and similarly with a DaemonSet field https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/apps/v1/conversion.go#L89

[dibyom]

@sbwsg what do you think about storing the entire v1beta1 spec instead of just just the finally field in a special annotation? That way we don't have to keep adding new annotations as we keep adding new fields (e.g. when expressions).

[ghost]

@sbwsg what do you think about storing the entire v1beta1 spec instead of just just the finally field in a special annotation? That way we don't have to keep adding new annotations as we keep adding new fields (e.g. when expressions).

I think that makes a lot of sense!

If a user makes changes to the v1alpha1 yaml, do we accept those changes or ignore them when re-hydrating the v1beta1 version from the annotation?

Edit: I'd probably lean toward copying the v1alpha1 fields directly over the top of the rehydrated v1beta1 fields, so keeping the changes.

[souleb]

A question concerning the Finally section issue.

From my understanding, if new feature is introduced it is in the following order v1alpha -> v1beta -> V1. If correct then new features would always be introduced first in the alpha version of v1 until we freeze it for v2?

[dibyom]

Edit: I'd probably lean toward copying the v1alpha1 fields directly over the top of the rehydrated v1beta1 fields, so keeping the changes.

Yeah, that sounds good to me!

[pritidesai]

With every new API version we introduce, the lower API versions are required to implement conversion routines i.e. ConvertTo the stored version/s and ConvertFrom the served higher versions. While introducing v1beta1, v1alpha1 implemented a function converting the alpha resources to v1beta1 and a function converting from beta resources to v1alpha1.

This is what we have today (serving v1alpha1 and v1beta1 while storing v1beta1):

API version	ConvertTo	ConvertFrom	served	stored
v1alpha1	v1beta1	v1beta1	true	false
v1beta1	None	None	true	true

Just to get an overall idea with conversion process if we add v1beta2 and v1 (stored by default) (while also serving v1alpha1 and v1beta1):

API version	ConvertTo	ConvertFrom	served	stored
v1alpha1	v1	v1beta1, v1beta2, v1	true	false
v1beta1	v1	v1beta2, v1	true	false
v1beta2	v1	v1	true	false
v1	None	None	true	true

Edit: a couple of format changes after adding comment

[ghost]

Just to get an overall idea with conversion process if we add v1beta2 and v1 (stored by default) (while also serving v1alpha1 and v1beta1):

I had no idea the lower versions had compounding ConvertFrom responsibilities like that. I assumed k8s (or knative?) would call ConvertFrom for each step in the chain down from v1 -> v1beta2 -> v1beta1 -> v1alpha1. Interesting!

[ghost]

Update from yesterday's API WG call: it sounded like most folks agreed that putting the whole v1beta1 object into an annotation on the v1alpha1 yaml is likely the best route to go down. We're going to look at this again in next week's API WG as the discussion spilled over time this week.

In the meantime I'm going to update this PR to implement that approach and see if there are any immediate gotchas. I'll add a test for this with a very large document to see if anything explodes along the way.

Edit: Discussion in Slack lead to us wanting to get this in ASAP without this change after all.

[pritidesai]

sorry, couldn't resist sharing, this tutorial has a great example of round tripping two separate API versions and explanation on conversion: https://youtu.be/AAxuEPIzHUQ?t=2994

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

• pkg/apis/OWNERS [vdemeester]

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

[pritidesai]

/lgtm

Thanks a bunch @sbwsg for adding this fix. As per the discussion on slack, we are limiting the fix to a pipeline with finally for now. We will revisit this in future to address all the new fields added in beta and API conversions in general.

(there are conflicts and will need to resolve them 😉 :)

[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/v1alpha1/pipeline_conversion.go	96.6%	93.5%	-3.1

[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/v1alpha1/pipeline_conversion.go	96.6%	93.5%	-3.1

[vdemeester]

/auto-cc

[vdemeester]

/uncc @imjasonh
/uncc @dlorenc

[pritidesai]

/lgtm

[ghost]

/hold cancel

[munnerz]

I'm very late to the party on this, but happy to see it fixed :)

back-port new fields to old API versions: in my opinion this goes against the whole idea of API versions, but I'd be happy to be proven wrong

I see this was mentioned as an option - this is in fact the correct solution to this problem, and what is recommended upstream.

The purpose of API versioning/creating a new API version is to permit authors of APIs to make breaking changes (i.e., renaming a field) to the schema of an API version. It is not to denote what functionality is supported in a given API.

When adding a net-new field, it is perfectly valid to add this within the same API version (without creating anything new). This is the approach upstream APIs taken regularly, and cert-manager has been taking as well.

Doing it this way will also avoid huge amounts of annotations being added in future for other fields as you add them to the latest API version (and also stop you needing to cut v2, v3, v4 etc whenever you add new functionality 😄)