From 09e2c7def6d779afbffb92fbd4d25b590a4123e1 Mon Sep 17 00:00:00 2001 From: qingliu Date: Tue, 23 Jan 2024 18:32:26 +0800 Subject: [PATCH] fix: prevent modification of annotations on completed TaskRuns 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. --- pkg/reconciler/taskrun/taskrun.go | 17 ++-- pkg/reconciler/taskrun/taskrun_test.go | 103 ++++++++++++++++++++++--- 2 files changed, 105 insertions(+), 15 deletions(-) diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 09ca1b7a151..25c5596e5b2 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -305,13 +305,20 @@ func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, tr *v1 // 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).ErrorOrNil() + + // 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 { + _, 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(merr, err).ErrorOrNil() } - merr := multierror.Append(previousError, err).ErrorOrNil() if controller.IsPermanentError(previousError) { return controller.NewPermanentError(merr) } diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index c681f8ae04b..b597912a592 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -423,7 +423,7 @@ metadata: namespace: foo spec: taskRef: - apiVersion: a1 + apiVersion: v1 name: test-task `) taskRunWithSaSuccess := parse.MustParseV1TaskRun(t, ` @@ -433,7 +433,7 @@ metadata: spec: serviceAccountName: test-sa taskRef: - apiVersion: a1 + apiVersion: v1 name: test-with-sa `) taskruns := []*v1.TaskRun{taskRunSuccess, taskRunWithSaSuccess} @@ -635,7 +635,7 @@ metadata: namespace: foo spec: taskRef: - apiVersion: a1 + apiVersion: v1 name: test-task `) taskRunWithSaSuccess := parse.MustParseV1TaskRun(t, ` @@ -645,7 +645,7 @@ metadata: spec: serviceAccountName: test-sa taskRef: - apiVersion: a1 + apiVersion: v1 name: test-with-sa `) taskRunSubstitution := parse.MustParseV1TaskRun(t, ` @@ -661,7 +661,7 @@ spec: - name: configmapname value: configbar taskRef: - apiVersion: a1 + apiVersion: v1 name: test-task-with-substitution `) taskRunWithTaskSpec := parse.MustParseV1TaskRun(t, ` @@ -4120,7 +4120,7 @@ metadata: namespace: foo spec: taskRef: - apiVersion: a1 + apiVersion: v1 name: test-task-with-workspace `) d := test.Data{ @@ -4180,7 +4180,7 @@ metadata: namespace: foo spec: taskRef: - apiVersion: a1 + apiVersion: v1 name: test-task-with-workspace `) d := test.Data{ @@ -4244,7 +4244,7 @@ metadata: namespace: foo spec: taskRef: - apiVersion: a1 + apiVersion: v1 name: test-task-with-workspace `) d := test.Data{ @@ -4383,7 +4383,7 @@ metadata: namespace: foo spec: taskRef: - apiVersion: a1 + apiVersion: v1 name: test-task-two-workspaces workspaces: - name: ws1 @@ -4480,7 +4480,7 @@ metadata: namespace: foo spec: taskRef: - apiVersion: a1 + apiVersion: v1 name: test-task-with-workspace workspaces: - name: ws1 @@ -6765,3 +6765,86 @@ func signInterface(signer signature.Signer, i interface{}) ([]byte, error) { return sig, nil } + +func TestReconcile_TaskRun_UpdateLabelsAndAnnotations(t *testing.T) { + taskRunCompleted := parse.MustParseV1TaskRun(t, ` +metadata: + name: test-taskrun-completed + namespace: foo + annotations: + pipeline.tekton.dev/release: release-sha +spec: + taskRef: + name: test-task +status: + completionTime: "2024-01-23T09:55:17Z" + conditions: + - lastTransitionTime: "2024-01-23T09:55:17Z" + message: All Steps have completed executing + reason: Succeeded + status: "True" + type: Succeeded +`) + taskRunCancelled := parse.MustParseV1TaskRun(t, ` +metadata: + name: test-taskrun-cancelled + namespace: foo + annotations: + pipeline.tekton.dev/release: release-sha +spec: + status: TaskRunCancelled + taskRef: + name: test-task +status: + conditions: + - lastTransitionTime: "2024-01-23T11:19:24Z" + reason: Pending + status: Unknown + type: Succeeded +`) + taskruns := []*v1.TaskRun{taskRunCompleted, taskRunCancelled} + d := test.Data{ + TaskRuns: taskruns, + Tasks: []*v1.Task{simpleTask}, + } + for _, tc := range []struct { + name string + taskRun *v1.TaskRun + wantAnnotations map[string]string + }{{ + name: "completed", + taskRun: taskRunCompleted, + wantAnnotations: map[string]string{ + // annotation not updated + "pipeline.tekton.dev/release": "release-sha", + }, + }, { + name: "cancelled", + taskRun: taskRunCancelled, + wantAnnotations: map[string]string{ + // annotation updated + "pipeline.tekton.dev/release": "unknown", + }, + }} { + t.Run(tc.name, func(t *testing.T) { + testAssets, cancel := getTaskRunController(t, d) + defer cancel() + c := testAssets.Controller + clients := testAssets.Clients + + if err := c.Reconciler.Reconcile(testAssets.Ctx, getRunName(tc.taskRun)); err != nil { + t.Errorf("expected no error. Got error %v", err) + } + + tr, err := clients.Pipeline.TektonV1().TaskRuns(tc.taskRun.Namespace).Get(testAssets.Ctx, tc.taskRun.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("getting updated taskrun: %v", err) + } + + annotations := tr.GetAnnotations() + if d := cmp.Diff(tc.wantAnnotations, annotations); d != "" { + t.Errorf("TaskRun annotations doesn't match %s", diff.PrintWantGot(d)) + } + }) + } +}