From 469906fc47089a091bd3c946dc62577d25762c52 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/pipelinerun/pipelinerun.go | 18 ++- .../pipelinerun/pipelinerun_test.go | 103 ++++++++++++++++++ pkg/reconciler/taskrun/taskrun.go | 17 ++- pkg/reconciler/taskrun/taskrun_test.go | 103 ++++++++++++++++-- 4 files changed, 221 insertions(+), 20 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index b9a01f558e5..fb0fb8487c1 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -306,13 +306,21 @@ func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, pr *v1 afterCondition := pr.Status.GetCondition(apis.ConditionSucceeded) events.Emit(ctx, beforeCondition, afterCondition, pr) - _, err := c.updateLabelsAndAnnotations(ctx, pr) - if err != nil { - logger.Warn("Failed to update PipelineRun labels/annotations", zap.Error(err)) - events.EmitError(controller.GetEventRecorder(ctx), err, pr) + + 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, pr) + if err != nil { + logger.Warn("Failed to update PipelineRun labels/annotations", zap.Error(err)) + events.EmitError(controller.GetEventRecorder(ctx), err, pr) + } + merr = multierror.Append(previousError, err).ErrorOrNil() } - merr := multierror.Append(previousError, err).ErrorOrNil() if controller.IsPermanentError(previousError) { return controller.NewPermanentError(merr) } diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 1b100cd1b71..6f856bf647b 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -25,6 +25,7 @@ import ( "errors" "fmt" "strconv" + "strings" "testing" "time" @@ -17418,3 +17419,105 @@ func signInterface(signer signature.Signer, i interface{}) ([]byte, error) { return sig, nil } + +func getPipelineRunName(pr *v1.PipelineRun) string { + return strings.Join([]string{pr.Namespace, pr.Name}, "/") +} + +func TestReconcile_PipelineRun_UpdateLabelsAndAnnotations(t *testing.T) { + ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, ` +metadata: + name: test-pipeline + namespace: foo +spec: + tasks: + - name: inline + taskSpec: + steps: + - name: echo + image: ubuntu + script: | + #!/bin/bash + echo "hello world!" +`)} + pipelineRunCompleted := parse.MustParseV1PipelineRun(t, ` +metadata: + name: test-pipelinerun-completed + namespace: foo + annotations: + pipeline.tekton.dev/release: release-sha +spec: + pipelineRef: + name: test-pipeline +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 +`) + pipelineRunCancelled := parse.MustParseV1PipelineRun(t, ` +metadata: + name: test-pipelinerun-cancelled + namespace: foo + annotations: + pipeline.tekton.dev/release: release-sha +spec: + status: Cancelled + pipelineRef: + name: test-pipeline +status: + conditions: + - lastTransitionTime: "2024-01-23T11:19:24Z" + reason: Pending + status: Unknown + type: Succeeded +`) + pipelineruns := []*v1.PipelineRun{pipelineRunCompleted, pipelineRunCancelled} + d := test.Data{ + PipelineRuns: pipelineruns, + Pipelines: ps, + } + for _, tc := range []struct { + name string + pipelineRun *v1.PipelineRun + wantAnnotations map[string]string + }{{ + name: "completed", + pipelineRun: pipelineRunCompleted, + wantAnnotations: map[string]string{ + // annotation not updated + "pipeline.tekton.dev/release": "release-sha", + }, + }, { + name: "cancelled", + pipelineRun: pipelineRunCancelled, + wantAnnotations: map[string]string{ + // annotation updated + "pipeline.tekton.dev/release": "unknown", + }, + }} { + t.Run(tc.name, func(t *testing.T) { + testAssets, cancel := getPipelineRunController(t, d) + defer cancel() + c := testAssets.Controller + clients := testAssets.Clients + + if err := c.Reconciler.Reconcile(testAssets.Ctx, getPipelineRunName(tc.pipelineRun)); err != nil { + t.Errorf("expected no error. Got error %v", err) + } + + pr, err := clients.Pipeline.TektonV1().PipelineRun(tc.pipelineRun.Namespace).Get(testAssets.Ctx, tc.pipelineRun.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("getting updated pipelinerun: %v", err) + } + + annotations := pr.GetAnnotations() + if d := cmp.Diff(tc.wantAnnotations, annotations); d != "" { + t.Errorf("PipelineRun annotations doesn't match %s", diff.PrintWantGot(d)) + } + }) + } +} 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)) + } + }) + } +}