[release-v1.0.x] fix: allow finalizer updates on completed TaskRun and PipelineRuns

pkg/apis/pipeline/v1/pipelinerun_validation.go

@@ -131,26 +131,41 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError)
 // ValidateUpdate validates the update of a PipelineRunSpec
 func (ps *PipelineRunSpec) ValidateUpdate(ctx context.Context) (errs *apis.FieldError) {
 	if !apis.IsInUpdate(ctx) {
-		return
+		return errs
 	}
 	oldObj, ok := apis.GetBaseline(ctx).(*PipelineRun)
 	if !ok || oldObj == nil {
-		return
+		return errs
 	}
-	old := &oldObj.Spec
-
-	// If already in the done state, the spec cannot be modified. Otherwise, only the status field can be modified.
-	tips := "Once the PipelineRun is complete, no updates are allowed"
-	if !oldObj.IsDone() {
-		old = old.DeepCopy()
-		old.Status = ps.Status
-		tips = "Once the PipelineRun has started, only status updates are allowed"
+	if oldObj.IsDone() {
+		// try comparing without any copying first
+		// this handles the common case where only finalizers changed
+		if equality.Semantic.DeepEqual(&oldObj.Spec, ps) {
+			return nil // Specs identical, allow update
+		}
+
+		// Specs differ, this could be due to different defaults after upgrade
+		// Apply current defaults to old spec to normalize
+		oldCopy := oldObj.Spec.DeepCopy()
+		oldCopy.SetDefaults(ctx)
+
+		if equality.Semantic.DeepEqual(oldCopy, ps) {
+			return nil // Difference was only defaults, allow update
+		}
+
+		// Real spec changes detected, reject update
+		errs = errs.Also(apis.ErrInvalidValue("Once the PipelineRun is complete, no updates are allowed", ""))
+		return errs
 	}
+
+	// Handle started but not done case
+	old := oldObj.Spec.DeepCopy()
+	old.Status = ps.Status
 	if !equality.Semantic.DeepEqual(old, ps) {
-		errs = errs.Also(apis.ErrInvalidValue(tips, ""))
+		errs = errs.Also(apis.ErrInvalidValue("Once the PipelineRun has started, only status updates are allowed", ""))
 	}
 
-	return
+	return errs
 }
 
 func (ps *PipelineRunSpec) validatePipelineRunParameters(ctx context.Context) (errs *apis.FieldError) {

pkg/apis/pipeline/v1/pipelinerun_validation_test.go

@@ -18,6 +18,7 @@ package v1_test
 
 import (
 	"context"
+	"strings"
 	"testing"
 	"time"
 
@@ -1690,3 +1691,116 @@ func TestPipelineRunSpec_ValidateUpdate(t *testing.T) {
 		})
 	}
 }
+
+func TestPipelineRunSpec_ValidateUpdate_FinalizerChanges(t *testing.T) {
+	tests := []struct {
+		name                string
+		baselinePipelineRun *v1.PipelineRun
+		pipelineRun         *v1.PipelineRun
+		expectedError       string
+	}{
+		{
+			name: "allow finalizer update when specs are identical",
+			baselinePipelineRun: &v1.PipelineRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-pr",
+				},
+				Spec: v1.PipelineRunSpec{
+					PipelineRef: &v1.PipelineRef{
+						Name: "test-pipeline",
+						ResolverRef: v1.ResolverRef{
+							Resolver: "bundles",
+						},
+					},
+					Timeouts: &v1.TimeoutFields{
+						Pipeline: &metav1.Duration{Duration: 60 * time.Minute},
+					},
+				},
+				Status: v1.PipelineRunStatus{
+					Status: duckv1.Status{
+						Conditions: duckv1.Conditions{
+							{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
+						},
+					},
+				},
+			},
+			pipelineRun: &v1.PipelineRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-pr",
+					Finalizers: []string{"chains.tekton.dev/finalizer"},
+				},
+				Spec: v1.PipelineRunSpec{
+					PipelineRef: &v1.PipelineRef{
+						Name: "test-pipeline",
+						ResolverRef: v1.ResolverRef{
+							Resolver: "bundles",
+						},
+					},
+					Timeouts: &v1.TimeoutFields{
+						Pipeline: &metav1.Duration{Duration: 60 * time.Minute},
+					},
+				},
+			},
+			expectedError: "",
+		},
+		{
+			name: "block actual spec changes on completed PipelineRun",
+			baselinePipelineRun: &v1.PipelineRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-pr",
+				},
+				Spec: v1.PipelineRunSpec{
+					PipelineRef: &v1.PipelineRef{
+						Name: "test-pipeline",
+					},
+				},
+				Status: v1.PipelineRunStatus{
+					Status: duckv1.Status{
+						Conditions: duckv1.Conditions{
+							{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
+						},
+					},
+				},
+			},
+			pipelineRun: &v1.PipelineRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-pr",
+					Finalizers: []string{"chains.tekton.dev/finalizer"},
+				},
+				Spec: v1.PipelineRunSpec{
+					PipelineRef: &v1.PipelineRef{
+						Name: "different-pipeline",
+					},
+				},
+			},
+			expectedError: "invalid value: Once the PipelineRun is complete, no updates are allowed",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := config.ToContext(context.Background(), &config.Config{
+				Defaults: &config.Defaults{
+					DefaultResolverType:   "bundles",
+					DefaultTimeoutMinutes: 60,
+					DefaultServiceAccount: "default",
+				},
+			})
+			ctx = apis.WithinUpdate(ctx, tt.baselinePipelineRun)
+
+			err := tt.pipelineRun.Spec.ValidateUpdate(ctx)
+
+			if tt.expectedError == "" {
+				if err != nil {
+					t.Errorf("Expected no error, but got: %v", err)
+				}
+			} else {
+				if err == nil {
+					t.Errorf("Expected error containing %q, but got none", tt.expectedError)
+				} else if !strings.Contains(err.Error(), tt.expectedError) {
+					t.Errorf("Expected error containing %q, but got: %v", tt.expectedError, err)
+				}
+			}
+		})
+	}
+}

pkg/apis/pipeline/v1/taskrun_validation.go

@@ -125,29 +125,42 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
 // ValidateUpdate validates the update of a TaskRunSpec
 func (ts *TaskRunSpec) ValidateUpdate(ctx context.Context) (errs *apis.FieldError) {
 	if !apis.IsInUpdate(ctx) {
-		return
+		return errs
 	}
 	oldObj, ok := apis.GetBaseline(ctx).(*TaskRun)
 	if !ok || oldObj == nil {
-		return
+		return errs
 	}
-	old := &oldObj.Spec
+	if oldObj.IsDone() {
+		// try comparing without any copying first
+		// this handles the common case where only finalizers changed
+		if equality.Semantic.DeepEqual(&oldObj.Spec, ts) {
+			return nil // Specs identical, allow update
+		}
+
+		// Specs differ, this could be due to different defaults after upgrade
+		// Apply current defaults to old spec to normalize
+		oldCopy := oldObj.Spec.DeepCopy()
+		oldCopy.SetDefaults(ctx)
 
-	// If already in the done state, the spec cannot be modified.
-	// Otherwise, only the status, statusMessage field can be modified.
-	tips := "Once the TaskRun is complete, no updates are allowed"
-	if !oldObj.IsDone() {
-		old = old.DeepCopy()
-		old.Status = ts.Status
-		old.StatusMessage = ts.StatusMessage
-		tips = "Once the TaskRun has started, only status and statusMessage updates are allowed"
+		if equality.Semantic.DeepEqual(oldCopy, ts) {
+			return nil // Difference was only defaults, allow update
+		}
+
+		// Real spec changes detected, reject update
+		errs = errs.Also(apis.ErrInvalidValue("Once the TaskRun is complete, no updates are allowed", ""))
+		return errs
 	}
 
+	// Handle started but not done case
+	old := oldObj.Spec.DeepCopy()
+	old.Status = ts.Status
+	old.StatusMessage = ts.StatusMessage
 	if !equality.Semantic.DeepEqual(old, ts) {
-		errs = errs.Also(apis.ErrInvalidValue(tips, ""))
+		errs = errs.Also(apis.ErrInvalidValue("Once the TaskRun has started, only status and statusMessage updates are allowed", ""))
 	}
 
-	return
+	return errs
 }
 
 // validateInlineParameters validates that any parameters called in the

pkg/apis/pipeline/v1/taskrun_validation_test.go

@@ -18,6 +18,7 @@ package v1_test
 
 import (
 	"context"
+	"strings"
 	"testing"
 	"time"
 
@@ -1130,3 +1131,112 @@ func TestTaskRunSpec_ValidateUpdate(t *testing.T) {
 		})
 	}
 }
+
+func TestTaskRunSpec_ValidateUpdate_FinalizerChanges(t *testing.T) {
+	tests := []struct {
+		name            string
+		baselineTaskRun *v1.TaskRun
+		taskRun         *v1.TaskRun
+		expectedError   string
+	}{
+		{
+			name: "allow finalizer update when specs are identical",
+			baselineTaskRun: &v1.TaskRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-tr",
+				},
+				Spec: v1.TaskRunSpec{
+					TaskRef: &v1.TaskRef{
+						Name: "test-task",
+						ResolverRef: v1.ResolverRef{
+							Resolver: "bundles",
+						},
+					},
+					Timeout: &metav1.Duration{Duration: 60 * time.Minute},
+				},
+				Status: v1.TaskRunStatus{
+					Status: duckv1.Status{
+						Conditions: duckv1.Conditions{
+							{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
+						},
+					},
+				},
+			},
+			taskRun: &v1.TaskRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-tr",
+					Finalizers: []string{"chains.tekton.dev/finalizer"},
+				},
+				Spec: v1.TaskRunSpec{
+					TaskRef: &v1.TaskRef{
+						Name: "test-task",
+						ResolverRef: v1.ResolverRef{
+							Resolver: "bundles",
+						},
+					},
+					Timeout: &metav1.Duration{Duration: 60 * time.Minute},
+				},
+			},
+			expectedError: "",
+		},
+		{
+			name: "block actual spec changes on completed TaskRun",
+			baselineTaskRun: &v1.TaskRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-tr",
+				},
+				Spec: v1.TaskRunSpec{
+					TaskRef: &v1.TaskRef{
+						Name: "test-task",
+					},
+				},
+				Status: v1.TaskRunStatus{
+					Status: duckv1.Status{
+						Conditions: duckv1.Conditions{
+							{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
+						},
+					},
+				},
+			},
+			taskRun: &v1.TaskRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-tr",
+					Finalizers: []string{"chains.tekton.dev/finalizer"},
+				},
+				Spec: v1.TaskRunSpec{
+					TaskRef: &v1.TaskRef{
+						Name: "different-task",
+					},
+				},
+			},
+			expectedError: "invalid value: Once the TaskRun is complete, no updates are allowed",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := config.ToContext(context.Background(), &config.Config{
+				Defaults: &config.Defaults{
+					DefaultResolverType:   "bundles",
+					DefaultTimeoutMinutes: 60,
+					DefaultServiceAccount: "default",
+				},
+			})
+			ctx = apis.WithinUpdate(ctx, tt.baselineTaskRun)
+
+			err := tt.taskRun.Spec.ValidateUpdate(ctx)
+
+			if tt.expectedError == "" {
+				if err != nil {
+					t.Errorf("Expected no error, but got: %v", err)
+				}
+			} else {
+				if err == nil {
+					t.Errorf("Expected error containing %q, but got none", tt.expectedError)
+				} else if !strings.Contains(err.Error(), tt.expectedError) {
+					t.Errorf("Expected error containing %q, but got: %v", tt.expectedError, err)
+				}
+			}
+		})
+	}
+}