Skip to content

Commit

Permalink
fix: prevent modification of annotations on completed TaskRuns
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@l-qing
l-qing committed Jan 24, 2024
1 parent cf4bf79 commit 469906f
Show file tree
Hide file tree
Showing 4 changed files with 221 additions and 20 deletions.
18 changes: 13 additions & 5 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
103 changes: 103 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"errors"
"fmt"
"strconv"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -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))
}
})
}
}
17 changes: 12 additions & 5 deletions pkg/reconciler/taskrun/taskrun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
103 changes: 93 additions & 10 deletions pkg/reconciler/taskrun/taskrun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ metadata:
namespace: foo
spec:
taskRef:
apiVersion: a1
apiVersion: v1
name: test-task
`)
taskRunWithSaSuccess := parse.MustParseV1TaskRun(t, `
Expand All @@ -433,7 +433,7 @@ metadata:
spec:
serviceAccountName: test-sa
taskRef:
apiVersion: a1
apiVersion: v1
name: test-with-sa
`)
taskruns := []*v1.TaskRun{taskRunSuccess, taskRunWithSaSuccess}
Expand Down Expand Up @@ -635,7 +635,7 @@ metadata:
namespace: foo
spec:
taskRef:
apiVersion: a1
apiVersion: v1
name: test-task
`)
taskRunWithSaSuccess := parse.MustParseV1TaskRun(t, `
Expand All @@ -645,7 +645,7 @@ metadata:
spec:
serviceAccountName: test-sa
taskRef:
apiVersion: a1
apiVersion: v1
name: test-with-sa
`)
taskRunSubstitution := parse.MustParseV1TaskRun(t, `
Expand All @@ -661,7 +661,7 @@ spec:
- name: configmapname
value: configbar
taskRef:
apiVersion: a1
apiVersion: v1
name: test-task-with-substitution
`)
taskRunWithTaskSpec := parse.MustParseV1TaskRun(t, `
Expand Down Expand Up @@ -4120,7 +4120,7 @@ metadata:
namespace: foo
spec:
taskRef:
apiVersion: a1
apiVersion: v1
name: test-task-with-workspace
`)
d := test.Data{
Expand Down Expand Up @@ -4180,7 +4180,7 @@ metadata:
namespace: foo
spec:
taskRef:
apiVersion: a1
apiVersion: v1
name: test-task-with-workspace
`)
d := test.Data{
Expand Down Expand Up @@ -4244,7 +4244,7 @@ metadata:
namespace: foo
spec:
taskRef:
apiVersion: a1
apiVersion: v1
name: test-task-with-workspace
`)
d := test.Data{
Expand Down Expand Up @@ -4383,7 +4383,7 @@ metadata:
namespace: foo
spec:
taskRef:
apiVersion: a1
apiVersion: v1
name: test-task-two-workspaces
workspaces:
- name: ws1
Expand Down Expand Up @@ -4480,7 +4480,7 @@ metadata:
namespace: foo
spec:
taskRef:
apiVersion: a1
apiVersion: v1
name: test-task-with-workspace
workspaces:
- name: ws1
Expand Down Expand Up @@ -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))
}
})
}
}

0 comments on commit 469906f

Please sign in to comment.