fix: allow finalizer updates on completed TaskRun and PipelineRuns #9011

waveywaves · 2025-09-02T19:14:37Z

Changes

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

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
pre-commit Passed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

/kind bug

tekton-robot · 2025-09-02T19:26:14Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipelinerun_validation.go	93.8%	93.4%	-0.4
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	95.7%	95.3%	-0.4

tekton-robot · 2025-09-02T19:43:14Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipelinerun_validation.go	93.8%	93.4%	-0.4
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	95.7%	95.3%	-0.4

vdemeester · 2025-09-02T20:50:51Z

/retest

waveywaves · 2025-09-03T07:19:01Z

/retest

l-qing · 2025-09-03T08:38:29Z

Your implementation is very clever, avoiding unnecessary copying. 👍
Actually, TaskRun has a similar issue and needs similar handling.

I've encountered a similar problem recently, so I can close my PR #9004 and wait for yours to be merged. 😆

khrm

Isn't there a similar webhook validation on taskrun?

waveywaves · 2025-09-04T07:48:26Z

/retest

waveywaves · 2025-09-04T08:44:01Z

@l-qing looks like we opened for this issue around the same time :) also thanks for pointing out this is the same issue in taskruns, I am pusshing a commit for it now

@khrm yes looks like this issue is happening in taskruns as well, sending an update now

fixes issue tektoncd#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>

Similar to issue tektoncd#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.

tekton-robot · 2025-09-04T09:03:02Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipelinerun_validation.go	93.8%	93.4%	-0.4
pkg/apis/pipeline/v1/taskrun_validation.go	97.4%	96.9%	-0.5
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	95.7%	95.3%	-0.4
pkg/apis/pipeline/v1beta1/taskrun_validation.go	98.1%	97.5%	-0.6

tekton-robot · 2025-09-04T09:07:24Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipelinerun_validation.go	93.8%	93.4%	-0.4
pkg/apis/pipeline/v1/taskrun_validation.go	97.4%	96.9%	-0.5
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	95.7%	95.3%	-0.4
pkg/apis/pipeline/v1beta1/taskrun_validation.go	98.1%	97.5%	-0.6

tekton-robot · 2025-09-04T09:12:03Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipelinerun_validation.go	93.8%	93.4%	-0.4
pkg/apis/pipeline/v1/taskrun_validation.go	97.4%	96.9%	-0.5
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	95.7%	95.3%	-0.4
pkg/apis/pipeline/v1beta1/taskrun_validation.go	98.1%	97.5%	-0.6

waveywaves · 2025-09-04T11:48:03Z

/retest

tekton-robot · 2025-09-04T14:34:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vdemeester · 2025-09-04T14:35:04Z

/retest

khrm

/lgtm

tekton-robot · 2025-09-04T16:05:38Z

@khrm: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vdemeester · 2025-09-04T16:10:15Z

/lgtm

khrm · 2025-09-04T17:27:00Z

/cherry-pick release-v1.0.x

khrm · 2025-09-04T17:27:29Z

/cherry-pick release-v1.3.x

waveywaves · 2025-09-08T06:52:25Z

cherry pick automation hasn't been working, manually cherry picking them

github-project-automation bot added this to Tekton Community Roadmap Sep 2, 2025

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/bug Categorizes issue or PR as related to a bug. labels Sep 2, 2025

github-project-automation bot moved this to Todo in Tekton Community Roadmap Sep 2, 2025

tekton-robot requested review from dibyom and vdemeester September 2, 2025 19:14

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 2, 2025

waveywaves assigned vdemeester Sep 2, 2025

waveywaves force-pushed the fix-8824-allow-finalizer-updates-completed-pipelineruns branch from c9fa199 to c1731a2 Compare September 2, 2025 19:29

l-qing mentioned this pull request Sep 3, 2025

fix(validation): apply SetDefaults to both specs before comparison #9004

Closed

8 tasks

khrm reviewed Sep 4, 2025

View reviewed changes

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2025

waveywaves force-pushed the fix-8824-allow-finalizer-updates-completed-pipelineruns branch from 56787b8 to e48696a Compare September 4, 2025 08:52

waveywaves changed the title ~~fix: allow finalizer updates on completed PipelineRuns~~ fix: allow finalizer updates on completed TaskRun and PipelineRuns Sep 4, 2025

waveywaves added 2 commits September 4, 2025 10:58

waveywaves force-pushed the fix-8824-allow-finalizer-updates-completed-pipelineruns branch from e48696a to ce87dde Compare September 4, 2025 08:58

vdemeester approved these changes Sep 4, 2025

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2025

khrm reviewed Sep 4, 2025

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2025

tekton-robot merged commit eaf7dc1 into tektoncd:main Sep 4, 2025
64 of 69 checks passed

github-project-automation bot moved this from Todo to Done in Tekton Community Roadmap Sep 4, 2025

This was referenced Sep 8, 2025

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

Merged

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

Merged

Uh oh!

fix: allow finalizer updates on completed TaskRun and PipelineRuns #9011

fix: allow finalizer updates on completed TaskRun and PipelineRuns #9011

Uh oh!

Conversation

waveywaves commented Sep 2, 2025

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Sep 2, 2025

Uh oh!

tekton-robot commented Sep 2, 2025

Uh oh!

vdemeester commented Sep 2, 2025

Uh oh!

waveywaves commented Sep 3, 2025

Uh oh!

l-qing commented Sep 3, 2025

Uh oh!

khrm left a comment

Choose a reason for hiding this comment

Uh oh!

waveywaves commented Sep 4, 2025

Uh oh!

waveywaves commented Sep 4, 2025

Uh oh!

tekton-robot commented Sep 4, 2025

Uh oh!

tekton-robot commented Sep 4, 2025

Uh oh!

tekton-robot commented Sep 4, 2025

Uh oh!

waveywaves commented Sep 4, 2025

Uh oh!

tekton-robot commented Sep 4, 2025

Uh oh!

vdemeester commented Sep 4, 2025

Uh oh!

khrm left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Sep 4, 2025

Uh oh!

vdemeester commented Sep 4, 2025

Uh oh!

Uh oh!

khrm commented Sep 4, 2025

Uh oh!

khrm commented Sep 4, 2025

Uh oh!

waveywaves commented Sep 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants