Pipelines is incorrectly removing Tekton Chains annotations

[wlynch]

Expected Behavior

If I set chains.tekton.dev/transparency-upload=true on a Pipeline, this should propagate down to child Tasks during a run.

Actual Behavior

Pipelines controller filters out all chains.tekton.dev annotations. This breaks the behavior of the transparency-enabled: manual in Chains.

See https://tektoncd.slack.com/archives/CJ4ERJWAU/p1697627188085879, #6441

Steps to Reproduce the Problem

1. Create pipeline w/ chains.tekton.dev/transparency-upload=true annotation (e.g. https://github.com/tektoncd/chains/blob/3fe5c46e9a259f3a562f85c115418cb4a1106e00/release/publish.yaml#L21)
2. Run pipeline
3. TaskRun doesn't get annotated.

Additional Info

• Kubernetes version:

Client Version: version.Info{Major:"1", Minor:"25", GitVersion:"v1.25.4", GitCommit:"872a965c6c6526caa949f0c6ac028ef7aff3fb78", GitTreeState:"clean", BuildDate:"2022-11-09T13:28:30Z", GoVersion:"go1.19.3", Compiler:"gc", Platform:"darwin/arm64"}
Kustomize Version: v4.5.7
Server Version: version.Info{Major:"1", Minor:"24", GitVersion:"v1.24.14-gke.2700", GitCommit:"20f1946282011a3f0cec885eaafe3decc9c367c9", GitTreeState:"clean", BuildDate:"2023-06-22T09:23:35Z", GoVersion:"go1.19.9 X:boringcrypto", Compiler:"gc", Platform:"linux/amd64"}

• Tekton Pipeline version:

Client version: 0.30.1
Chains version: v0.16.0
Pipeline version: v0.49.0
Triggers version: v0.24.1
Dashboard version: v0.39.0

/cc @vdemeester @khrm

[khrm]

Yes. We need to remove filter for chains. We removed for results also. If chains required some annotation, then it should either write k8s CEL admission policy or webhook.

[vdemeester]

@wlynch question, should it be on Pipeline or on PipelineRun ?

[wlynch]

I believe either? My expectation would be the annotations trickle down PipelineRun > Pipeline > TaskRun > Task.

[vdemeester]

Kind of, but ideally, Pipeline annotations shouldn't affect PipelineRun, only lower (TaskRun), … which is not the case today. As today, if you set chains.tekton.dev/transparency-upload=true on a Pipeline, the PipelineRun would inherit it (and in the future, it might not anymore #6127)

[chitrangpatel]

I wanted to raise this discussion again since at this point, Chains e2e tests only run with very old versions of Tekton Pipelines. It is challenging to test against newer features like StepActions etc.
I don't have a lot of context so I wanted to ask what should the solution be?

cc @vdemeester @wlynch @khrm

[vdemeester]

@chitrangpatel do you agree with the above sentence:

ideally, Pipeline annotations shouldn't affect PipelineRun, only lower (TaskRun), … which is not the case today.

Today, this is messed up, #6127 is trying to remove this behavior, but it might be a very breaking change (and I didn't really got time or will to keep rebasing at some point). We could make this behavior optional (or behind a feature flag) and switch it later on (giving users relying on it time to adapt).

[chitrangpatel]

Yes, I agree with that sentence. So the idea is that we don't want pipelineRun to inherit anything. It should be explicitly declared there and passed on to the layers below.
i.e. we want annotations to trickle down, not up.

[chitrangpatel]

Today, if you look at tektoncd/chains#1117, I think that the issue is that even within a standalone TaskRun, we zap the annotation completely. Isn't that wrong?

I think the issue is that Pipelines is removing the chains annotations completely regardless of whether it is being propagated or not. Please keep me honest here @renzodavid9, @wlynch . I think that's wrong 🤔?