fix(signing): aggregate no-signer-configured errors into MarkSigned decision by pujitha24 · Pull Request #1834 · tektoncd/chains

pujitha24 · 2026-07-25T22:39:10Z

Motivation:
When no signer (e.g. no x509.pem/cosign.key) is configured for a
required signer type, ObjectSigner.Sign() logs "No signer %s
configured for %s" and continues, but never records this as a
failure. Since the aggregate merr stays nil, Sign() falls through
to annotations.MarkSigned(), which sets
chains.tekton.dev/signed=true even though nothing was signed. That
annotation is treated as terminal, so the TaskRun/PipelineRun is
never retried once a signer becomes available. This is the exact
scenario reported in #858: no signing key configured, yet Chains
marks the run as signed.

Note: user-visible behavior for legitimate Signer: "none"
configurations (intentional opt-out of signing for an artifact
type) is unchanged - Artifact.Enabled() short-circuits before the
signer lookup for that case, so this only affects the genuine
misconfiguration path.

Approach:
Append an error to the existing merr aggregate in the
"no signer configured" branch of ObjectSigner.Sign(), matching the
same multierror.Append pattern already used a few lines below for
storage failures. This makes Sign() return an error and take the
existing HandleRetry path instead of MarkSigned, so the resource is
retried on the next reconciliation rather than being permanently
marked signed.

Validation:
Added a table-driven case to TestSigner_Sign that points SecretPath
at a directory with no key material, reproducing the no-signer
scenario, and asserts the object is NOT marked signed. Verified the
new case fails without the fix (IsSigned()=true, wanted false) and
passes with it.

go build ./...
go test ./pkg/chains/...
make golangci-lint PKG=./pkg/chains/...

All pass with zero failures and zero lint issues.

Fixes #858

Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com

AI assistance: this change was drafted with Claude Code.

…ecision Motivation: When no signer (e.g. no x509.pem/cosign.key) is configured for a required signer type, ObjectSigner.Sign() logs "No signer %s configured for %s" and continues, but never records this as a failure. Since the aggregate `merr` stays nil, Sign() falls through to annotations.MarkSigned(), which sets chains.tekton.dev/signed=true even though nothing was signed. That annotation is treated as terminal, so the TaskRun/PipelineRun is never retried once a signer becomes available. This is the exact scenario reported in tektoncd#858: no signing key configured, yet Chains marks the run as signed. Note: user-visible behavior for legitimate `Signer: "none"` configurations (intentional opt-out of signing for an artifact type) is unchanged - Artifact.Enabled() short-circuits before the signer lookup for that case, so this only affects the genuine misconfiguration path. Approach: Append an error to the existing `merr` aggregate in the "no signer configured" branch of ObjectSigner.Sign(), matching the same multierror.Append pattern already used a few lines below for storage failures. This makes Sign() return an error and take the existing HandleRetry path instead of MarkSigned, so the resource is retried on the next reconciliation rather than being permanently marked signed. Validation: Added a table-driven case to TestSigner_Sign that points SecretPath at a directory with no key material, reproducing the no-signer scenario, and asserts the object is NOT marked signed. Verified the new case fails without the fix (IsSigned()=true, wanted false) and passes with it. go build ./... go test ./pkg/chains/... make golangci-lint PKG=./pkg/chains/... All pass with zero failures and zero lint issues. Fixes tektoncd#858 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

tekton-robot · 2026-07-25T22:39:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign puneetpunamiya after the PR has been reviewed.
You can assign the PR to them by writing /assign @puneetpunamiya 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:

OWNERS

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

linux-foundation-easycla · 2026-07-25T22:39:16Z

The committers listed above are authorized under a signed CLA.

✅ login: pujitha24 / name: Pujitha Paladugu (3ec3cf8, 67a31db)

Copilot

⚠️ Not ready to approve

The new missing-signer error aggregation can balloon into many duplicate aggregated errors/log spam for multi-object artifacts unless the per-object loop exits on first missing signer.

Pull request overview

This PR fixes a misconfiguration edge case in Chains signing where missing signer configuration could still result in chains.tekton.dev/signed=true, preventing future retries once a signer/key becomes available.

Changes:

Aggregate “no signer configured” as an error in ObjectSigner.Sign() so signing failures take the retry path instead of falling through to MarkSigned.
Add a unit test case to reproduce the “no signer configured” scenario and assert the object is not marked signed.

File summaries

File	Description
pkg/chains/signing.go	Appends missing-signer errors into the existing multierror aggregate so signing failures trigger retries instead of `MarkSigned`.
pkg/chains/signing_test.go	Adds a regression test case covering the missing-signer scenario and validates the object is not marked signed.

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Low

Note

Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pujitha24 · 2026-07-26T00:06:00Z

 			if !ok {
 				logger.Warnf("No signer %s configured for %s", signerType, signableType.Type())
+				merr = multierror.Append(merr, fmt.Errorf("no signer %s configured for %s", signerType, signableType.Type()))
 				continue


Good catch, thank you. Since signerType := signableType.Signer(cfg) only depends on signableType/cfg and not on the individual object, the missing-signer condition really is invariant across every object extracted for that signable type. I changed the continue to break on this branch, so we now record at most one "no signer configured" error per signable type per Sign() call and stop creating payloads for the remaining objects of that type, instead of repeating the warning/error for each one. It still falls through to the existing retry/annotation handling right after the loop, so that behavior is unchanged.

go build ./..., go test ./pkg/chains/..., and make golangci-lint PKG=./pkg/chains/... all pass with this change.

Break out of the per-object loop on the first missing-signer occurrence instead of continuing, since the signer type is determined by the signable type/config and does not vary per object. This avoids appending duplicate identical errors to the aggregate and skips unnecessary payload creation for the remaining objects of that signable type. Addressed per Copilot review feedback on PR tektoncd#1834. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

codecov-commenter · 2026-07-28T09:02:41Z

Codecov Report

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1834      +/-   ##
==========================================
+ Coverage   61.93%   62.14%   +0.20%     
==========================================
  Files          64       64              
  Lines        4067     4068       +1     
==========================================
+ Hits         2519     2528       +9     
+ Misses       1266     1261       -5     
+ Partials      282      279       -3

Flag	Coverage Δ
unit-tests	`62.14% <ø> (+0.20%)`	⬆️

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.

anithapriyanatarajan · 2026-07-28T09:40:04Z

@pujitha24 Thank you for this PR. Could you squash the commits and sign-off ?

ngelman1 · 2026-07-28T14:58:52Z

Thank you @pujitha24
I tested on a kind cluster using the task from the original issue and got:

{
    "chains.tekton.dev/retries": "3",
    "chains.tekton.dev/signed": "failed",

and logs also showed "msg":"error configuring x509 signer: no valid private key found, looked for: [x509.pem, cosign.key]"

Looks good to me

pujitha24 · 2026-07-31T17:48:15Z

Thanks @anithapriyanatarajan — squashed the two commits into one (6f8aa11b1), keeping the Signed-off-by trailer that was already on both. go build ./..., go test ./pkg/chains/..., and make golangci-lint PKG=./pkg/chains/... all still pass.

@ngelman1 thanks for testing this against the original issue #858 scenario!

Copilot AI review requested due to automatic review settings July 25, 2026 22:39

tekton-robot requested a review from ab-ghosh July 25, 2026 22:39

tekton-robot requested a review from ngelman1 July 25, 2026 22:39

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 25, 2026

Copilot started reviewing on behalf of pujitha24 July 25, 2026 22:39 View session

Copilot AI reviewed Jul 25, 2026

View reviewed changes

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 26, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(signing): aggregate no-signer-configured errors into MarkSigned decision - #1834

fix(signing): aggregate no-signer-configured errors into MarkSigned decision#1834
pujitha24 wants to merge 2 commits into
tektoncd:mainfrom
pujitha24:auto/issue-858

pujitha24 commented Jul 25, 2026

Uh oh!

tekton-robot commented Jul 25, 2026

Uh oh!

linux-foundation-easycla Bot commented Jul 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

pujitha24 Jul 26, 2026

Uh oh!

codecov-commenter commented Jul 28, 2026

Uh oh!

anithapriyanatarajan commented Jul 28, 2026

Uh oh!

ngelman1 commented Jul 28, 2026

Uh oh!

pujitha24 commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

pujitha24 commented Jul 25, 2026

Uh oh!

tekton-robot commented Jul 25, 2026

Uh oh!

linux-foundation-easycla Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

⚠️ Not ready to approve

Review details

Uh oh!

pujitha24 Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 28, 2026

Codecov Report

Uh oh!

anithapriyanatarajan commented Jul 28, 2026

Uh oh!

ngelman1 commented Jul 28, 2026

Uh oh!

pujitha24 commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

linux-foundation-easycla Bot commented Jul 25, 2026 •

edited

Loading