Skip to content

fix(pipelinesascode): guard nil Settings in validate - #4065

Open
pujitha24 wants to merge 1 commit into
tektoncd:mainfrom
pujitha24:auto/issue-4057
Open

fix(pipelinesascode): guard nil Settings in validate#4065
pujitha24 wants to merge 1 commit into
tektoncd:mainfrom
pujitha24:auto/issue-4057

Conversation

@pujitha24

Copy link
Copy Markdown
Contributor

Changes

Fix a validating-webhook panic ("assignment to entry in nil map") that is
triggered whenever TektonConfig.Spec.Platforms.{Kubernetes,OpenShift}.PipelinesAsCode
is disabled, via either pipelinesAsCode.enable: false or the deprecated
spec.addon.enablePipelinesAsCode: false.

SetDefaults sets PACSettings.Settings to nil on the disable path (both
platform branches), but (*PACSettings).validate unconditionally passes
ps.Settings into the vendored pipelines-as-code SyncConfig, which
writes into that map unconditionally (getHubCatalogs, default.go:21).
A nil map panics on write.

The fix guards ps.Settings to an empty map before calling SyncConfig,
in (*PACSettings).validate — the single method used by both the
OpenShiftPipelinesAsCode.Validate and TektonConfig.Validate (Kubernetes
and OpenShift branches) entry points, so one guard covers all three call
sites.

A regression test reproduces the exact panic reported in the issue
(PACSettings{Settings: nil} through OpenShiftPipelinesAsCode.Validate)
and asserts it now returns cleanly.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Release Notes

Fix a validating webhook panic when disabling PipelinesAsCode in
TektonConfig (via `pipelinesAsCode.enable: false` or the deprecated
`addon.enablePipelinesAsCode: false`), which previously rejected the
update and blocked unrelated TektonConfig field changes as well.

AI assistance: this change was drafted with Claude Code.

Fixes #4057

Disabling PipelinesAsCode in TektonConfig, via either
platforms.<kubernetes|openshift>.pipelinesAsCode.enable: false or the
deprecated addon.enablePipelinesAsCode: false, is rejected by the
validating webhook, which panics with "assignment to entry in nil
map" on every attempt. Because the whole object is rejected, this
also blocks unrelated TektonConfig field updates while the disable is
requested.

SetDefaults nils out PACSettings.Settings on the disable path (both
the Kubernetes and OpenShift branches in tektonconfig_defaults.go),
but (*PACSettings).validate unconditionally forwards ps.Settings into
the vendored pipelines-as-code SyncConfig, which writes into that map
unconditionally in getHubCatalogs (default.go:21). A nil map panics
on write.

Guard ps.Settings to an empty map before calling SyncConfig, in
(*PACSettings).validate. This is the single method used by both the
OpenShiftPipelinesAsCode.Validate and TektonConfig.Validate
(Kubernetes and OpenShift branches) entry points, so one guard covers
all three call sites; no other caller of SyncConfig or
PACSettings.validate was found unguarded.

Added TestValidateNilSettings, which reproduces the exact panic
reported in the issue (PACSettings{Settings: nil} through
OpenShiftPipelinesAsCode.Validate) before the fix, and passes after
it. Verified by temporarily reverting only the guard: the test panics
with the same stack (getHubCatalogs -> SyncConfig ->
PACSettings.validate -> OpenShiftPipelinesAsCode.Validate) reported in
the issue; with the guard restored it passes.

Validation:
- go test ./pkg/apis/operator/v1alpha1/... (pass)
- go test -race ./pkg/apis/operator/v1alpha1/... (pass)
- go build ./pkg/apis/... (pass)
- make lint-go PKG=./pkg/apis/operator/v1alpha1/... (0 issues)
- gofmt -l on changed files (clean)
- No API types changed, so codegen is unaffected and was not run.
- A full-repo `go build ./...` could not be completed locally: this
  sandbox has very little free disk space, unrelated to this change,
  and the build exhausts it while compiling large unrelated
  dependency trees. The affected package builds, vets, and tests
  cleanly in isolation.

Report: tektoncd#4057

Assisted-by: Claude Sonnet 5
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 9, 2026
@tekton-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign jkhelil after the PR has been reviewed.
You can assign the PR to them by writing /assign @jkhelil in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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

@tekton-robot tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 9, 2026
@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 26.71%. Comparing base (9df99c4) to head (3e55c4d).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4065   +/-   ##
=======================================
  Coverage   26.70%   26.71%           
=======================================
  Files         465      465           
  Lines       25010    25012    +2     
=======================================
+ Hits         6679     6681    +2     
  Misses      17601    17601           
  Partials      730      730           
Flag Coverage Δ
unit-tests 26.71% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@theakshaypant theakshaypant left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pujitha24 Were you able to verify the nil panic is no longer happening on a live deployment?

Unrelated to the changes, what is a mutex in Go, and when would you use one?

@pujitha24

Copy link
Copy Markdown
Contributor Author

No, I haven't run this against a live deployment. What I verified is the regression test (TestValidateNilSettings in openshiftpipelinesascode_validation_test.go), which reproduces the exact panic path — OpenShiftPipelinesAsCode.Validate with PACSettings.Settings: nil — and confirms it now returns cleanly instead of panicking, plus the rest of the package's test suite and make test lint passing. If a live-cluster check is needed before merge I'm happy to do that, just say so.

On the mutex question: sync.Mutex gives mutual exclusion so only one goroutine holds the lock at a time — you'd use one when multiple goroutines read/write the same shared state (a map, counter, etc.) and you need to serialize access to avoid a data race. Wasn't needed here since ps.Settings is local to a single validation call, not shared across goroutines.

@theakshaypant theakshaypant left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pujitha24 Would be very helpful if you can perform the check deploying your changes, maybe even on kind.

Additionally, can you provide an inline review of the changes in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot disable PipelinesAsCode in TektonConfig: validating webhook panics with "assignment to entry in nil map"

3 participants