Skip to content

Commit eaf7dc1

Browse files
waveywavestekton-robot
waveywaves
authored and
tekton-robot
committed
fix: allow finalizer updates on completed TaskRuns
Similar to issue #8824 for PipelineRuns, the webhook was denying finalizer updates on completed TaskRuns. The validation webhook was blocking all updates when TaskRun.IsDone() returned true, preventing tools like Tekton Chains from clearing finalizers. To ensure we don't have old obj default drift, we are adding a fast path check for spec equality and are normalizing the old spec with current defaults before comparison. Then we allow updates when only finalizers differ and reject spec changes. This mirrors the fix applied to PipelineRun validation.
1 parent 1fd63cd commit eaf7dc1

File tree

4 files changed

+266
-20
lines changed

4 files changed

+266
-20
lines changed

‎pkg/apis/pipeline/v1/taskrun_validation.go‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -130,20 +130,33 @@ func (ts *TaskRunSpec) ValidateUpdate(ctx context.Context) (errs *apis.FieldErro
130130
if !ok || oldObj == nil {
131131
return
132132
}
133-
old := &oldObj.Spec
133+
if oldObj.IsDone() {
134+
// try comparing without any copying first
135+
// this handles the common case where only finalizers changed
136+
if equality.Semantic.DeepEqual(&oldObj.Spec, ts) {
137+
return nil // Specs identical, allow update
138+
}
139+
140+
// Specs differ, this could be due to different defaults after upgrade
141+
// Apply current defaults to old spec to normalize
142+
oldCopy := oldObj.Spec.DeepCopy()
143+
oldCopy.SetDefaults(ctx)
134144

135-
// If already in the done state, the spec cannot be modified.
136-
// Otherwise, only the status, statusMessage field can be modified.
137-
tips := "Once the TaskRun is complete, no updates are allowed"
138-
if !oldObj.IsDone() {
139-
old = old.DeepCopy()
140-
old.Status = ts.Status
141-
old.StatusMessage = ts.StatusMessage
142-
tips = "Once the TaskRun has started, only status and statusMessage updates are allowed"
145+
if equality.Semantic.DeepEqual(oldCopy, ts) {
146+
return nil // Difference was only defaults, allow update
147+
}
148+
149+
// Real spec changes detected, reject update
150+
errs = errs.Also(apis.ErrInvalidValue("Once the TaskRun is complete, no updates are allowed", ""))
151+
return errs
143152
}
144153

154+
// Handle started but not done case
155+
old := oldObj.Spec.DeepCopy()
156+
old.Status = ts.Status
157+
old.StatusMessage = ts.StatusMessage
145158
if !equality.Semantic.DeepEqual(old, ts) {
146-
errs = errs.Also(apis.ErrInvalidValue(tips, ""))
159+
errs = errs.Also(apis.ErrInvalidValue("Once the TaskRun has started, only status and statusMessage updates are allowed", ""))
147160
}
148161

149162
return

‎pkg/apis/pipeline/v1/taskrun_validation_test.go‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package v1_test
1818

1919
import (
2020
"context"
21+
"strings"
2122
"testing"
2223
"time"
2324

@@ -1130,3 +1131,112 @@ func TestTaskRunSpec_ValidateUpdate(t *testing.T) {
11301131
})
11311132
}
11321133
}
1134+
1135+
func TestTaskRunSpec_ValidateUpdate_FinalizerChanges(t *testing.T) {
1136+
tests := []struct {
1137+
name string
1138+
baselineTaskRun *v1.TaskRun
1139+
taskRun *v1.TaskRun
1140+
expectedError string
1141+
}{
1142+
{
1143+
name: "allow finalizer update when specs are identical",
1144+
baselineTaskRun: &v1.TaskRun{
1145+
ObjectMeta: metav1.ObjectMeta{
1146+
Name: "test-tr",
1147+
},
1148+
Spec: v1.TaskRunSpec{
1149+
TaskRef: &v1.TaskRef{
1150+
Name: "test-task",
1151+
ResolverRef: v1.ResolverRef{
1152+
Resolver: "bundles",
1153+
},
1154+
},
1155+
Timeout: &metav1.Duration{Duration: 60 * time.Minute},
1156+
},
1157+
Status: v1.TaskRunStatus{
1158+
Status: duckv1.Status{
1159+
Conditions: duckv1.Conditions{
1160+
{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
1161+
},
1162+
},
1163+
},
1164+
},
1165+
taskRun: &v1.TaskRun{
1166+
ObjectMeta: metav1.ObjectMeta{
1167+
Name: "test-tr",
1168+
Finalizers: []string{"chains.tekton.dev/finalizer"},
1169+
},
1170+
Spec: v1.TaskRunSpec{
1171+
TaskRef: &v1.TaskRef{
1172+
Name: "test-task",
1173+
ResolverRef: v1.ResolverRef{
1174+
Resolver: "bundles",
1175+
},
1176+
},
1177+
Timeout: &metav1.Duration{Duration: 60 * time.Minute},
1178+
},
1179+
},
1180+
expectedError: "",
1181+
},
1182+
{
1183+
name: "block actual spec changes on completed TaskRun",
1184+
baselineTaskRun: &v1.TaskRun{
1185+
ObjectMeta: metav1.ObjectMeta{
1186+
Name: "test-tr",
1187+
},
1188+
Spec: v1.TaskRunSpec{
1189+
TaskRef: &v1.TaskRef{
1190+
Name: "test-task",
1191+
},
1192+
},
1193+
Status: v1.TaskRunStatus{
1194+
Status: duckv1.Status{
1195+
Conditions: duckv1.Conditions{
1196+
{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
1197+
},
1198+
},
1199+
},
1200+
},
1201+
taskRun: &v1.TaskRun{
1202+
ObjectMeta: metav1.ObjectMeta{
1203+
Name: "test-tr",
1204+
Finalizers: []string{"chains.tekton.dev/finalizer"},
1205+
},
1206+
Spec: v1.TaskRunSpec{
1207+
TaskRef: &v1.TaskRef{
1208+
Name: "different-task",
1209+
},
1210+
},
1211+
},
1212+
expectedError: "invalid value: Once the TaskRun is complete, no updates are allowed",
1213+
},
1214+
}
1215+
1216+
for _, tt := range tests {
1217+
t.Run(tt.name, func(t *testing.T) {
1218+
ctx := config.ToContext(t.Context(), &config.Config{
1219+
Defaults: &config.Defaults{
1220+
DefaultResolverType: "bundles",
1221+
DefaultTimeoutMinutes: 60,
1222+
DefaultServiceAccount: "default",
1223+
},
1224+
})
1225+
ctx = apis.WithinUpdate(ctx, tt.baselineTaskRun)
1226+
1227+
err := tt.taskRun.Spec.ValidateUpdate(ctx)
1228+
1229+
if tt.expectedError == "" {
1230+
if err != nil {
1231+
t.Errorf("Expected no error, but got: %v", err)
1232+
}
1233+
} else {
1234+
if err == nil {
1235+
t.Errorf("Expected error containing %q, but got none", tt.expectedError)
1236+
} else if !strings.Contains(err.Error(), tt.expectedError) {
1237+
t.Errorf("Expected error containing %q, but got: %v", tt.expectedError, err)
1238+
}
1239+
}
1240+
})
1241+
}
1242+
}

‎pkg/apis/pipeline/v1beta1/taskrun_validation.go‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -130,20 +130,33 @@ func (ts *TaskRunSpec) ValidateUpdate(ctx context.Context) (errs *apis.FieldErro
130130
if !ok || oldObj == nil {
131131
return
132132
}
133-
old := &oldObj.Spec
133+
if oldObj.IsDone() {
134+
// try comparing without any copying first
135+
// this handles the common case where only finalizers changed
136+
if equality.Semantic.DeepEqual(&oldObj.Spec, ts) {
137+
return nil // Specs identical, allow update
138+
}
139+
140+
// Specs differ, this could be due to different defaults after upgrade
141+
// Apply current defaults to old spec to normalize
142+
oldCopy := oldObj.Spec.DeepCopy()
143+
oldCopy.SetDefaults(ctx)
134144

135-
// If already in the done state, the spec cannot be modified.
136-
// Otherwise, only the status, statusMessage field can be modified.
137-
tips := "Once the TaskRun is complete, no updates are allowed"
138-
if !oldObj.IsDone() {
139-
old = old.DeepCopy()
140-
old.Status = ts.Status
141-
old.StatusMessage = ts.StatusMessage
142-
tips = "Once the TaskRun has started, only status and statusMessage updates are allowed"
145+
if equality.Semantic.DeepEqual(oldCopy, ts) {
146+
return nil // Difference was only defaults, allow update
147+
}
148+
149+
// Real spec changes detected, reject update
150+
errs = errs.Also(apis.ErrInvalidValue("Once the TaskRun is complete, no updates are allowed", ""))
151+
return errs
143152
}
144153

154+
// Handle started but not done case
155+
old := oldObj.Spec.DeepCopy()
156+
old.Status = ts.Status
157+
old.StatusMessage = ts.StatusMessage
145158
if !equality.Semantic.DeepEqual(old, ts) {
146-
errs = errs.Also(apis.ErrInvalidValue(tips, ""))
159+
errs = errs.Also(apis.ErrInvalidValue("Once the TaskRun has started, only status and statusMessage updates are allowed", ""))
147160
}
148161

149162
return

‎pkg/apis/pipeline/v1beta1/taskrun_validation_test.go‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package v1beta1_test
1818

1919
import (
2020
"context"
21+
"strings"
2122
"testing"
2223
"time"
2324

@@ -1101,3 +1102,112 @@ func TestTaskRunSpec_ValidateUpdate(t *testing.T) {
11011102
})
11021103
}
11031104
}
1105+
1106+
func TestTaskRunSpec_ValidateUpdate_FinalizerChanges(t *testing.T) {
1107+
tests := []struct {
1108+
name string
1109+
baselineTaskRun *v1beta1.TaskRun
1110+
taskRun *v1beta1.TaskRun
1111+
expectedError string
1112+
}{
1113+
{
1114+
name: "allow finalizer update when specs are identical",
1115+
baselineTaskRun: &v1beta1.TaskRun{
1116+
ObjectMeta: metav1.ObjectMeta{
1117+
Name: "test-tr",
1118+
},
1119+
Spec: v1beta1.TaskRunSpec{
1120+
TaskRef: &v1beta1.TaskRef{
1121+
Name: "test-task",
1122+
ResolverRef: v1beta1.ResolverRef{
1123+
Resolver: "bundles",
1124+
},
1125+
},
1126+
Timeout: &metav1.Duration{Duration: 60 * time.Minute},
1127+
},
1128+
Status: v1beta1.TaskRunStatus{
1129+
Status: duckv1.Status{
1130+
Conditions: duckv1.Conditions{
1131+
{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
1132+
},
1133+
},
1134+
},
1135+
},
1136+
taskRun: &v1beta1.TaskRun{
1137+
ObjectMeta: metav1.ObjectMeta{
1138+
Name: "test-tr",
1139+
Finalizers: []string{"chains.tekton.dev/finalizer"},
1140+
},
1141+
Spec: v1beta1.TaskRunSpec{
1142+
TaskRef: &v1beta1.TaskRef{
1143+
Name: "test-task",
1144+
ResolverRef: v1beta1.ResolverRef{
1145+
Resolver: "bundles",
1146+
},
1147+
},
1148+
Timeout: &metav1.Duration{Duration: 60 * time.Minute},
1149+
},
1150+
},
1151+
expectedError: "",
1152+
},
1153+
{
1154+
name: "block actual spec changes on completed TaskRun",
1155+
baselineTaskRun: &v1beta1.TaskRun{
1156+
ObjectMeta: metav1.ObjectMeta{
1157+
Name: "test-tr",
1158+
},
1159+
Spec: v1beta1.TaskRunSpec{
1160+
TaskRef: &v1beta1.TaskRef{
1161+
Name: "test-task",
1162+
},
1163+
},
1164+
Status: v1beta1.TaskRunStatus{
1165+
Status: duckv1.Status{
1166+
Conditions: duckv1.Conditions{
1167+
{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
1168+
},
1169+
},
1170+
},
1171+
},
1172+
taskRun: &v1beta1.TaskRun{
1173+
ObjectMeta: metav1.ObjectMeta{
1174+
Name: "test-tr",
1175+
Finalizers: []string{"chains.tekton.dev/finalizer"},
1176+
},
1177+
Spec: v1beta1.TaskRunSpec{
1178+
TaskRef: &v1beta1.TaskRef{
1179+
Name: "different-task",
1180+
},
1181+
},
1182+
},
1183+
expectedError: "invalid value: Once the TaskRun is complete, no updates are allowed",
1184+
},
1185+
}
1186+
1187+
for _, tt := range tests {
1188+
t.Run(tt.name, func(t *testing.T) {
1189+
ctx := config.ToContext(t.Context(), &config.Config{
1190+
Defaults: &config.Defaults{
1191+
DefaultResolverType: "bundles",
1192+
DefaultTimeoutMinutes: 60,
1193+
DefaultServiceAccount: "default",
1194+
},
1195+
})
1196+
ctx = apis.WithinUpdate(ctx, tt.baselineTaskRun)
1197+
1198+
err := tt.taskRun.Spec.ValidateUpdate(ctx)
1199+
1200+
if tt.expectedError == "" {
1201+
if err != nil {
1202+
t.Errorf("Expected no error, but got: %v", err)
1203+
}
1204+
} else {
1205+
if err == nil {
1206+
t.Errorf("Expected error containing %q, but got none", tt.expectedError)
1207+
} else if !strings.Contains(err.Error(), tt.expectedError) {
1208+
t.Errorf("Expected error containing %q, but got: %v", tt.expectedError, err)
1209+
}
1210+
}
1211+
})
1212+
}
1213+
}

0 commit comments

Comments
 (0)