fix(signing): aggregate no-signer-configured errors into MarkSigned decision#1834
fix(signing): aggregate no-signer-configured errors into MarkSigned decision#1834pujitha24 wants to merge 2 commits into
Conversation
…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>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
tekton-robot
added
the
size/S
There was a problem hiding this comment.
⚠️ 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 toMarkSigned. - 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.
| 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 |
There was a problem hiding this comment.
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>
tekton-robot
added
size/M
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
merrstays nil, Sign() falls throughto 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
merraggregate 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.