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.
tektoncd · Jan 24, 2024 · 09e2c7d · 09e2c7d
1 parent 39cdda0
commit 09e2c7d
Show file tree

Hide file tree

Showing 2 changed files with 105 additions and 15 deletions.
diff --git 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
@@ -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))
+			}
+		})
+	}
+}