fix: prevent modification of annotations on completed TaskRuns

[l-qing]

In TaskRun, the annotation pipeline.tekton.dev/release records the version information of the pipeline. If the Pipeline is updated, the annotation in the completed TaskRuns should not be modified.

• pipeline/pkg/reconciler/taskrun/taskrun.go

  Lines 142 to 155 in e1c7828

  	// If the TaskRun is complete, run some post run fixtures when applicable
  	if tr.IsDone() {
  	logger.Infof("taskrun done : %s \n", tr.Name)
  	// We may be reading a version of the object that was stored at an older version
  	// and may not have had all of the assumed default specified.
  	tr.SetDefaults(ctx)
  	if err := c.stopSidecars(ctx, tr); err != nil {
  	return err
  	}
  	return c.finishReconcileUpdateEmitEvents(ctx, tr, before, nil)
  	}

• pipeline/pkg/reconciler/taskrun/taskrun.go

  Lines 295 to 319 in e1c7828

  	func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, tr *v1.TaskRun, beforeCondition *apis.Condition, previousError error) error {
  	ctx, span := c.tracerProvider.Tracer(TracerName).Start(ctx, "finishReconcileUpdateEmitEvents")
  	defer span.End()
  	logger := logging.FromContext(ctx)
  	afterCondition := tr.Status.GetCondition(apis.ConditionSucceeded)
  	if afterCondition.IsFalse() && !tr.IsCancelled() && tr.IsRetriable() {
  	retryTaskRun(tr, afterCondition.Message)
  	afterCondition = tr.Status.GetCondition(apis.ConditionSucceeded)
  	}
  	// Send k8s events and cloud events (when configured)
  	events.Emit(ctx, beforeCondition, afterCondition, tr)
  	_, err := c.updateLabelsAndAnnotations(ctx, tr)
  	if err != nil {
  	logger.Warn("Failed to update TaskRun labels/annotations", zap.Error(err))
  	events.EmitError(controller.GetEventRecorder(ctx), err, tr)
  	}
  	merr := multierror.Append(previousError, err).ErrorOrNil()
  	if controller.IsPermanentError(previousError) {
  	return controller.NewPermanentError(merr)
  	}
  	return merr
  	}

• pipeline/pkg/reconciler/taskrun/taskrun.go

  Lines 650 to 657 in e1c7828

  	func (c *Reconciler) updateLabelsAndAnnotations(ctx context.Context, tr *v1.TaskRun) (*v1.TaskRun, error) {
  	ctx, span := c.tracerProvider.Tracer(TracerName).Start(ctx, "updateLabelsAndAnnotations")
  	defer span.End()
  	// Ensure the TaskRun is properly decorated with the version of the Tekton controller processing it.
  	if tr.Annotations == nil {
  	tr.Annotations = make(map[string]string, 1)
  	}
  	tr.Annotations[podconvert.ReleaseAnnotation] = changeset.Get()

Changes

Submitter Checklist

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

• Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
• 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). See some examples of good release notes.
• Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

fix: the pipeline controller will no longer modify any annotation it has set on completed pipelineruns

/kind bug

[tekton-robot]

Hi @l-qing. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.1

[khrm]

/ok-to-test

[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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.1

[vdemeester]

This is most likely going to be a problem as some things need to update annotations once the TaskRun or PipelineRun is done. An example is the chains component that signs outputs once the run is done and adds annotations to the object once signed.

cc @tektoncd/core-maintainers @tektoncd/chains-maintainers

[lcarva]

This is most likely going to be a problem as some things need to update annotations once the TaskRun or PipelineRun is done. An example is the chains component that signs outputs once the run is done and adds annotations to the object once signed.

Yup! I believe this is also true for the Tekton Results project. cc @tektoncd/results-maintainers

@l-qing, could provide more context on why the suggested changes are desired?

[l-qing]

could provide more context on why the suggested changes are desired?

As described above: After upgrading the Pipeline, I found that the Annotations pipeline.tekton.dev/release of the historical TaskRun have all been modified. I believe this operation is not expected.
There may be several thousand TaskRuns in my environment.
The Pipeline I'm using is a custom-built version, such annotations changes happen quite frequently, unlike the community ones.

I might not be very familiar with tekton result.
But looking at the code logic here, it seems like it won't affect other components.
Because this change, only affects those taskruns that were already in a done state before entering reconcile. Originally they weren't doing much anyway.

pipeline/pkg/reconciler/taskrun/taskrun.go

Lines 107 to 155 in e1c7828

	// ReconcileKind compares the actual state with the desired, and attempts to
	// converge the two. It then updates the Status block of the Task Run
	// resource with the current status of the resource.
	func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1.TaskRun) pkgreconciler.Event {
	logger := logging.FromContext(ctx)
	ctx = cloudevent.ToContext(ctx, c.cloudEventClient)
	ctx = initTracing(ctx, c.tracerProvider, tr)
	ctx, span := c.tracerProvider.Tracer(TracerName).Start(ctx, "TaskRun:ReconcileKind")
	defer span.End()
	span.SetAttributes(attribute.String("taskrun", tr.Name), attribute.String("namespace", tr.Namespace))
	// Read the initial condition
	before := tr.Status.GetCondition(apis.ConditionSucceeded)
	// Record the duration and count after the reconcile cycle.
	defer c.durationAndCountMetrics(ctx, tr, before)
	// If the TaskRun is just starting, this will also set the starttime,
	// from which the timeout will immediately begin counting down.
	if !tr.HasStarted() {
	tr.Status.InitializeConditions()
	// In case node time was not synchronized, when controller has been scheduled to other nodes.
	if tr.Status.StartTime.Sub(tr.CreationTimestamp.Time) < 0 {
	logger.Warnf("TaskRun %s createTimestamp %s is after the taskRun started %s", tr.GetNamespacedName().String(), tr.CreationTimestamp, tr.Status.StartTime)
	tr.Status.StartTime = &tr.CreationTimestamp
	}
	// Emit events. During the first reconcile the status of the TaskRun may change twice
	// from not Started to Started and then to Running, so we need to sent the event here
	// and at the end of 'Reconcile' again.
	// We also want to send the "Started" event as soon as possible for anyone who may be waiting
	// on the event to perform user facing initialisations, such has reset a CI check status
	afterCondition := tr.Status.GetCondition(apis.ConditionSucceeded)
	events.Emit(ctx, nil, afterCondition, tr)
	}
	// If the TaskRun is complete, run some post run fixtures when applicable
	if tr.IsDone() {
	logger.Infof("taskrun done : %s \n", tr.Name)
	// We may be reading a version of the object that was stored at an older version
	// and may not have had all of the assumed default specified.
	tr.SetDefaults(ctx)
	if err := c.stopSidecars(ctx, tr); err != nil {
	return err
	}
	return c.finishReconcileUpdateEmitEvents(ctx, tr, before, nil)
	}

I have updated the description to elucidate the impact.

pipeline/pkg/reconciler/taskrun/taskrun.go

Lines 310 to 313 in 469906f

	// If the Run has been completed before and remains so at present,
	// no need to update the labels and annotations
	skipUpdateLabelsAndAnnotations := !afterCondition.IsUnknown() && !beforeCondition.IsUnknown()
	if !skipUpdateLabelsAndAnnotations {

[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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.1

[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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.1

[vdemeester]

@l-qing chains doesn't act "until" the taskrun is done (see here), and will put an annotation on it (see here). I didn't see it as obvious as this in the results codebase, but, essentially, it might block possible other "usage" (outside of chains and results).

That said, given your usecase, we probably want to disallow mutating some of the annotations (or ignore these mutation), and essentially, pipeline.tekton.dev/release.

[khrm]

@vdemeester
I don't think this PR stops tekton-chains or tekton-results from adding annotations. It just blocks this function

pipeline/pkg/reconciler/taskrun/taskrun.go

Line 650 in 39cdda0

func (c *Reconciler) updateLabelsAndAnnotations(ctx context.Context, tr *v1.TaskRun) (*v1.TaskRun, error) {

Changes aren't in the validator code but reconciler. It's okay if the annotation for a completed pipelinerun/taskrun is not updated by the pipeline controllers.
Is my understanding correct, @l-qing ?

[khrm]

/test pull-tekton-pipeline-unit-tests

[l-qing]

@vdemeester I don't think this PR stops tekton-chains or tekton-results from adding annotations. It just blocks this function

pipeline/pkg/reconciler/taskrun/taskrun.go

Line 650 in 39cdda0

func (c *Reconciler) updateLabelsAndAnnotations(ctx context.Context, tr *v1.TaskRun) (*v1.TaskRun, error) {

Changes aren't in the validator code but reconciler. It's okay if the annotation for a completed pipelinerun/taskrun is not updated by the pipeline controllers. Is my understanding correct, @l-qing ?

Yes, that's exactly what I mean.

In the reconcile of the PipelineRun, I noticed that there is no location where the pipeline.tekton.dev/release annotations are set, so I reverted the code related to PipelineRun. 😆

[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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.1

[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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.1

[vdemeester]

Ah I mis-read the PR then. It's preventing the pipeline controller itself to updated labels and annotations once the run is done 👍🏼. Then yes, it shouldn't be a problem !

/approve

[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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.1

[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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.1

[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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.1

[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/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.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/reconciler/taskrun/taskrun.go	85.4%	85.5%	0.1

[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/reconciler/taskrun/taskrun.go	85.4%	85.5%	0.1

[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/reconciler/taskrun/taskrun.go	86.5%	86.6%	0.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/reconciler/taskrun/taskrun.go	86.5%	86.6%	0.1

[l-qing]

/retest-required

[dibyom]

Thanks @l-qing - I updated the release note to make it a bit clearer that this is only preventing modification of the annotations that are set by the pipeline controller.

/lgtm

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, 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:

• OWNERS [dibyom,vdemeester]

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

[l-qing]

Thanks @l-qing - I updated the release note to make it a bit clearer that this is only preventing modification of the annotations that are set by the pipeline controller.

Thanks!