Skip to content

Commit c1731a2

Browse files
waveywaveswaveywaves
committed
fix: allow finalizer updates on completed PipelineRuns
fixes issue #8824 where webhook denied finalizer updates on completed PipelineRuns. The validation webhook was blocking all updates when PipelineRun.IsDone() returned true, preventing from clearing finalizers to make sure that 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 Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>
1 parent 0cb253c commit c1731a2

File tree

4 files changed

+265
-16
lines changed

4 files changed

+265
-16
lines changed

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

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -138,17 +138,32 @@ func (ps *PipelineRunSpec) ValidateUpdate(ctx context.Context) (errs *apis.Field
138138
if !ok || oldObj == nil {
139139
return
140140
}
141-
old := &oldObj.Spec
141+
if oldObj.IsDone() {
142+
// try comparing without any copying first
143+
// this handles the common case where only finalizers changed
144+
if equality.Semantic.DeepEqual(&oldObj.Spec, ps) {
145+
return nil // Specs identical, allow update
146+
}
147+
148+
// Specs differ, this could be due to different defaults after upgrade
149+
// Apply current defaults to old spec to normalize
150+
oldCopy := oldObj.Spec.DeepCopy()
151+
oldCopy.SetDefaults(ctx)
152+
153+
if equality.Semantic.DeepEqual(oldCopy, ps) {
154+
return nil // Difference was only defaults, allow update
155+
}
142156

143-
// If already in the done state, the spec cannot be modified. Otherwise, only the status field can be modified.
144-
tips := "Once the PipelineRun is complete, no updates are allowed"
145-
if !oldObj.IsDone() {
146-
old = old.DeepCopy()
147-
old.Status = ps.Status
148-
tips = "Once the PipelineRun has started, only status updates are allowed"
157+
// Real spec changes detected, reject update
158+
errs = errs.Also(apis.ErrInvalidValue("Once the PipelineRun is complete, no updates are allowed", ""))
159+
return errs
149160
}
161+
162+
// Handle started but not done case
163+
old := oldObj.Spec.DeepCopy()
164+
old.Status = ps.Status
150165
if !equality.Semantic.DeepEqual(old, ps) {
151-
errs = errs.Also(apis.ErrInvalidValue(tips, ""))
166+
errs = errs.Also(apis.ErrInvalidValue("Once the PipelineRun has started, only status updates are allowed", ""))
152167
}
153168

154169
return

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

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1775,6 +1775,119 @@ func TestPipelineRunSpec_ValidateUpdate(t *testing.T) {
17751775
}
17761776
}
17771777

1778+
func TestPipelineRunSpec_ValidateUpdate_FinalizerChanges(t *testing.T) {
1779+
tests := []struct {
1780+
name string
1781+
baselinePipelineRun *v1.PipelineRun
1782+
pipelineRun *v1.PipelineRun
1783+
expectedError string
1784+
}{
1785+
{
1786+
name: "allow finalizer update when specs are identical",
1787+
baselinePipelineRun: &v1.PipelineRun{
1788+
ObjectMeta: metav1.ObjectMeta{
1789+
Name: "test-pr",
1790+
},
1791+
Spec: v1.PipelineRunSpec{
1792+
PipelineRef: &v1.PipelineRef{
1793+
Name: "test-pipeline",
1794+
ResolverRef: v1.ResolverRef{
1795+
Resolver: "bundles",
1796+
},
1797+
},
1798+
Timeouts: &v1.TimeoutFields{
1799+
Pipeline: &metav1.Duration{Duration: 60 * time.Minute},
1800+
},
1801+
},
1802+
Status: v1.PipelineRunStatus{
1803+
Status: duckv1.Status{
1804+
Conditions: duckv1.Conditions{
1805+
{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
1806+
},
1807+
},
1808+
},
1809+
},
1810+
pipelineRun: &v1.PipelineRun{
1811+
ObjectMeta: metav1.ObjectMeta{
1812+
Name: "test-pr",
1813+
Finalizers: []string{"chains.tekton.dev/finalizer"},
1814+
},
1815+
Spec: v1.PipelineRunSpec{
1816+
PipelineRef: &v1.PipelineRef{
1817+
Name: "test-pipeline",
1818+
ResolverRef: v1.ResolverRef{
1819+
Resolver: "bundles",
1820+
},
1821+
},
1822+
Timeouts: &v1.TimeoutFields{
1823+
Pipeline: &metav1.Duration{Duration: 60 * time.Minute},
1824+
},
1825+
},
1826+
},
1827+
expectedError: "",
1828+
},
1829+
{
1830+
name: "block actual spec changes on completed PipelineRun",
1831+
baselinePipelineRun: &v1.PipelineRun{
1832+
ObjectMeta: metav1.ObjectMeta{
1833+
Name: "test-pr",
1834+
},
1835+
Spec: v1.PipelineRunSpec{
1836+
PipelineRef: &v1.PipelineRef{
1837+
Name: "test-pipeline",
1838+
},
1839+
},
1840+
Status: v1.PipelineRunStatus{
1841+
Status: duckv1.Status{
1842+
Conditions: duckv1.Conditions{
1843+
{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
1844+
},
1845+
},
1846+
},
1847+
},
1848+
pipelineRun: &v1.PipelineRun{
1849+
ObjectMeta: metav1.ObjectMeta{
1850+
Name: "test-pr",
1851+
Finalizers: []string{"chains.tekton.dev/finalizer"},
1852+
},
1853+
Spec: v1.PipelineRunSpec{
1854+
PipelineRef: &v1.PipelineRef{
1855+
Name: "different-pipeline",
1856+
},
1857+
},
1858+
},
1859+
expectedError: "invalid value: Once the PipelineRun is complete, no updates are allowed",
1860+
},
1861+
}
1862+
1863+
for _, tt := range tests {
1864+
t.Run(tt.name, func(t *testing.T) {
1865+
ctx := config.ToContext(t.Context(), &config.Config{
1866+
Defaults: &config.Defaults{
1867+
DefaultResolverType: "bundles",
1868+
DefaultTimeoutMinutes: 60,
1869+
DefaultServiceAccount: "default",
1870+
},
1871+
})
1872+
ctx = apis.WithinUpdate(ctx, tt.baselinePipelineRun)
1873+
1874+
err := tt.pipelineRun.Spec.ValidateUpdate(ctx)
1875+
1876+
if tt.expectedError == "" {
1877+
if err != nil {
1878+
t.Errorf("Expected no error, but got: %v", err)
1879+
}
1880+
} else {
1881+
if err == nil {
1882+
t.Errorf("Expected error containing %q, but got none", tt.expectedError)
1883+
} else if !strings.Contains(err.Error(), tt.expectedError) {
1884+
t.Errorf("Expected error containing %q, but got: %v", tt.expectedError, err)
1885+
}
1886+
}
1887+
})
1888+
}
1889+
}
1890+
17781891
func TestPipelineRunTaskRunSpecTimeout_Validate(t *testing.T) {
17791892
tests := []struct {
17801893
name string

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

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -158,17 +158,32 @@ func (ps *PipelineRunSpec) ValidateUpdate(ctx context.Context) (errs *apis.Field
158158
if !ok || oldObj == nil {
159159
return
160160
}
161-
old := &oldObj.Spec
161+
if oldObj.IsDone() {
162+
// try comparing without any copying first
163+
// this handles the common case where only finalizers changed
164+
if equality.Semantic.DeepEqual(&oldObj.Spec, ps) {
165+
return nil // Specs identical, allow update
166+
}
167+
168+
// Specs differ, this could be due to different defaults after upgrade
169+
// Apply current defaults to old spec to normalize
170+
oldCopy := oldObj.Spec.DeepCopy()
171+
oldCopy.SetDefaults(ctx)
172+
173+
if equality.Semantic.DeepEqual(oldCopy, ps) {
174+
return nil // Difference was only defaults, allow update
175+
}
162176

163-
// If already in the done state, the spec cannot be modified. Otherwise, only the status field can be modified.
164-
tips := "Once the PipelineRun is complete, no updates are allowed"
165-
if !oldObj.IsDone() {
166-
old = old.DeepCopy()
167-
old.Status = ps.Status
168-
tips = "Once the PipelineRun has started, only status updates are allowed"
177+
// Real spec changes detected, reject update
178+
errs = errs.Also(apis.ErrInvalidValue("Once the PipelineRun is complete, no updates are allowed", ""))
179+
return errs
169180
}
181+
182+
// Handle started but not done case
183+
old := oldObj.Spec.DeepCopy()
184+
old.Status = ps.Status
170185
if !equality.Semantic.DeepEqual(old, ps) {
171-
errs = errs.Also(apis.ErrInvalidValue(tips, ""))
186+
errs = errs.Also(apis.ErrInvalidValue("Once the PipelineRun has started, only status updates are allowed", ""))
172187
}
173188

174189
return

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

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1922,6 +1922,112 @@ func TestPipelineRunSpec_ValidateUpdate(t *testing.T) {
19221922
}
19231923
}
19241924

1925+
func TestPipelineRunSpec_ValidateUpdate_FinalizerChanges(t *testing.T) {
1926+
tests := []struct {
1927+
name string
1928+
baselinePipelineRun *v1beta1.PipelineRun
1929+
pipelineRun *v1beta1.PipelineRun
1930+
expectedError string
1931+
}{
1932+
{
1933+
name: "allow finalizer update when specs are identical",
1934+
baselinePipelineRun: &v1beta1.PipelineRun{
1935+
ObjectMeta: metav1.ObjectMeta{
1936+
Name: "test-pr",
1937+
},
1938+
Spec: v1beta1.PipelineRunSpec{
1939+
PipelineRef: &v1beta1.PipelineRef{
1940+
Name: "test-pipeline",
1941+
},
1942+
Timeouts: &v1beta1.TimeoutFields{
1943+
Pipeline: &metav1.Duration{Duration: 60 * time.Minute},
1944+
},
1945+
},
1946+
Status: v1beta1.PipelineRunStatus{
1947+
Status: duckv1.Status{
1948+
Conditions: duckv1.Conditions{
1949+
{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
1950+
},
1951+
},
1952+
},
1953+
},
1954+
pipelineRun: &v1beta1.PipelineRun{
1955+
ObjectMeta: metav1.ObjectMeta{
1956+
Name: "test-pr",
1957+
Finalizers: []string{"chains.tekton.dev/finalizer"},
1958+
},
1959+
Spec: v1beta1.PipelineRunSpec{
1960+
PipelineRef: &v1beta1.PipelineRef{
1961+
Name: "test-pipeline",
1962+
},
1963+
Timeouts: &v1beta1.TimeoutFields{
1964+
Pipeline: &metav1.Duration{Duration: 60 * time.Minute},
1965+
},
1966+
},
1967+
},
1968+
expectedError: "",
1969+
},
1970+
{
1971+
name: "block actual spec changes on completed PipelineRun",
1972+
baselinePipelineRun: &v1beta1.PipelineRun{
1973+
ObjectMeta: metav1.ObjectMeta{
1974+
Name: "test-pr",
1975+
},
1976+
Spec: v1beta1.PipelineRunSpec{
1977+
PipelineRef: &v1beta1.PipelineRef{
1978+
Name: "test-pipeline",
1979+
},
1980+
},
1981+
Status: v1beta1.PipelineRunStatus{
1982+
Status: duckv1.Status{
1983+
Conditions: duckv1.Conditions{
1984+
{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
1985+
},
1986+
},
1987+
},
1988+
},
1989+
pipelineRun: &v1beta1.PipelineRun{
1990+
ObjectMeta: metav1.ObjectMeta{
1991+
Name: "test-pr",
1992+
Finalizers: []string{"chains.tekton.dev/finalizer"},
1993+
},
1994+
Spec: v1beta1.PipelineRunSpec{
1995+
PipelineRef: &v1beta1.PipelineRef{
1996+
Name: "different-pipeline",
1997+
},
1998+
},
1999+
},
2000+
expectedError: "invalid value: Once the PipelineRun is complete, no updates are allowed",
2001+
},
2002+
}
2003+
2004+
for _, tt := range tests {
2005+
t.Run(tt.name, func(t *testing.T) {
2006+
ctx := config.ToContext(t.Context(), &config.Config{
2007+
Defaults: &config.Defaults{
2008+
DefaultResolverType: "bundles",
2009+
DefaultTimeoutMinutes: 60,
2010+
},
2011+
})
2012+
ctx = apis.WithinUpdate(ctx, tt.baselinePipelineRun)
2013+
2014+
err := tt.pipelineRun.Spec.ValidateUpdate(ctx)
2015+
2016+
if tt.expectedError == "" {
2017+
if err != nil {
2018+
t.Errorf("Expected no error, but got: %v", err)
2019+
}
2020+
} else {
2021+
if err == nil {
2022+
t.Errorf("Expected error containing %q, but got none", tt.expectedError)
2023+
} else if !strings.Contains(err.Error(), tt.expectedError) {
2024+
t.Errorf("Expected error containing %q, but got: %v", tt.expectedError, err)
2025+
}
2026+
}
2027+
})
2028+
}
2029+
}
2030+
19252031
func TestPipelineRunTaskRunSpecTimeout_Validate(t *testing.T) {
19262032
tests := []struct {
19272033
name string

0 commit comments

Comments
 (0)