chore: Refactor error messages for Notation signature verification #1730

binbin-li · 2024-08-20T08:57:04Z

Description

What this PR does / why we need it:

This PR focuses on the notation signature validation scenario, improving the clarity of error messages and providing more intuitive remediation steps for users.

For errors that are self-explanatory, I have not included remediation steps.

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

It will fix some cases of the issue: #1321

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Test A
Test B

Checklist:

Does the affected code have corresponding tests?
Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
Does this introduce breaking changes that would require an announcement or bumping the major version?
Do all new files have appropriate license header?

Post Merge Requirements

MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

feat: refresher interface fix: formatting and license test: kuberefresher test: kuberefresher helpers refactor: IsRefreshable KMP interface method fix: add IsRefreshable method refactor: kmp spec interval as string mod: rm todo mod: rm comment style: use embedded type feat: namespaced refresher interface test: mock factory & test refreshable logic refactor: use kubeRefreshNamespaced in controller mod: add license blocks refactor: kmp.spec.interval default to empty string disabling refresh tests: increase refresher coverage refactor(kmp): use factory for creating refresher objects This change introduces a RefresherFactory that is responsible for creating the appropriate refresher object based on a configuration that's passed in. This decouples the reconcile logic from the object creation, making the code more modular and easier to test mod: rename interval to refreshinterval & adds crd descriptions mod: remove verbose logging from refresher docs: kmp refresh interval samples doc: corrected resource name of akv samples fix: kmp spec json tag using incorrect values & add refresh interval description fix: add licenses & update interval name in tests mod: comments for exported interfaces test: improve refresher test coverage Signed-off-by: Joshua Duffney <[email protected]>

Signed-off-by: Joshua Duffney <[email protected]>

Bumps [anchore/sbom-action](https://github.com/anchore/sbom-action) from 0.17.0 to 0.17.1. - [Release notes](https://github.com/anchore/sbom-action/releases) - [Commits](anchore/sbom-action@d94f46e...ab9d16d) --- updated-dependencies: - dependency-name: anchore/sbom-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Joshua Duffney <[email protected]>

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3.26.0 to 3.26.1. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@eb055d7...29d86d2) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Joshua Duffney <[email protected]>

Signed-off-by: Joshua Duffney <[email protected]>

…ect#1697) Signed-off-by: Joshua Duffney <[email protected]>

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3.26.1 to 3.26.2. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@29d86d2...429e197) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Joshua Duffney <[email protected]>

Signed-off-by: Yi Zha <[email protected]> Signed-off-by: Joshua Duffney <[email protected]>

codecov · 2024-08-21T03:21:29Z

Codecov Report

Attention: Patch coverage is 90.74074% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/executor/core/executor.go	60.00%	2 Missing ⚠️
pkg/keymanagementprovider/keymanagementprovider.go	75.00%	1 Missing ⚠️
pkg/referrerstore/oras/oras.go	87.50%	1 Missing ⚠️
pkg/verifier/factory/factory.go	87.50%	1 Missing ⚠️

Files with missing lines	Coverage Δ
...controllers/clusterresource/verifier_controller.go	`90.24% <100.00%> (+10.24%)`	⬆️
...ntrollers/namespaceresource/verifier_controller.go	`90.00% <100.00%> (+10.00%)`	⬆️
pkg/controllers/utils/verifier.go	`100.00% <100.00%> (ø)`
pkg/verifier/notation/certstoresbytype.go	`92.59% <100.00%> (ø)`
pkg/verifier/notation/notation.go	`98.11% <100.00%> (+9.99%)`	⬆️
pkg/verifier/notation/truststore.go	`93.75% <100.00%> (+9.75%)`	⬆️
pkg/keymanagementprovider/keymanagementprovider.go	`94.20% <75.00%> (+1.47%)`	⬆️
pkg/referrerstore/oras/oras.go	`80.97% <87.50%> (+13.18%)`	⬆️
pkg/verifier/factory/factory.go	`89.65% <87.50%> (+24.13%)`	⬆️
pkg/executor/core/executor.go	`88.46% <60.00%> (-0.70%)`	⬇️

... and 9 files with indirect coverage changes

pkg/verifier/notation/notation.go

susanshi

LGTM , left a minor comment

pkg/controllers/clusterresource/verifier_controller.go

pkg/controllers/namespaceresource/verifier_controller.go

pkg/controllers/utils/verifier.go

pkg/keymanagementprovider/keymanagementprovider.go

pkg/executor/core/executor.go

pkg/referrerstore/oras/oras.go

pkg/verifier/factory/factory.go

pkg/verifier/notation/certstoresbytype.go

pkg/controllers/clusterresource/verifier_controller.go

pkg/verifier/notation/notation.go

pkg/verifier/notation/truststore.go

binbin-li · 2024-09-09T06:53:04Z

@yizha1 thanks a lot for your feedback, could you take another look? I've addressed all comments.

yizha1

Thanks @binbin-li

LGTM for the first round of improvements.

luisdlp

LGTM

…atify-project#1730) Signed-off-by: akashsinghal <[email protected]>

…atify-project#1730)

duffney and others added 15 commits August 16, 2024 05:55

mod: add const for refresherTypes

7442b8a

Signed-off-by: Joshua Duffney <[email protected]>

mod: Add cast checks & refresherType constants

aeaeacc

Signed-off-by: Joshua Duffney <[email protected]>

style: rename kmp.spec.refreshinterval to refreshInterval json tag

3b7e47c

Signed-off-by: Joshua Duffney <[email protected]>

fix: update samples with correct refreshInterval name

b566b2d

Signed-off-by: Joshua Duffney <[email protected]>

fix: new error for refresher.GetResult() cast check

8cb3f0d

Signed-off-by: Joshua Duffney <[email protected]>

chore: update helm charts (ratify-project#1702)

bed493d

Signed-off-by: Joshua Duffney <[email protected]>

feat: add timestamp and traceId to verification response (ratify-proj…

2a36491

…ect#1697) Signed-off-by: Joshua Duffney <[email protected]>

chore: add the governance doc link to readme.md (ratify-project#1713)

7a89eac

Signed-off-by: Yi Zha <[email protected]> Signed-off-by: Joshua Duffney <[email protected]>

Merge branch 'dev' into issue-1131/refresher

a8177a5

Merge pull request ratify-project#1625 from duffney/issue-1131/refresher

ad92eb8

refactor: refactor error messages

78ea559

binbin-li requested review from akashsinghal, jimmyraywv, luisdlp, susanshi and toddysm as code owners August 20, 2024 08:57

Merge remote-tracking branch 'upstream/dev' into refactor-error-msg

7ff80d1

binbin-li force-pushed the refactor-error-msg branch from 86da15d to 7ff80d1 Compare August 20, 2024 09:00

binbin-li marked this pull request as draft August 20, 2024 13:39

Merge remote-tracking branch 'upstream/dev' into refactor-error-msg

fa90085

test: fix broken tests

dfaaa0d

binbin-li force-pushed the refactor-error-msg branch 4 times, most recently from 4745ef8 to 644e8bb Compare August 22, 2024 08:53

binbin-li added 2 commits August 29, 2024 02:56

Merge remote-tracking branch 'upstream/dev' into refactor-error-msg

83be113

chore: refactor msg

8e58648

susanshi reviewed Aug 29, 2024

View reviewed changes

pkg/verifier/notation/notation.go Outdated Show resolved Hide resolved

susanshi previously approved these changes Aug 29, 2024

View reviewed changes

chore: address comments

eb1148c

binbin-li dismissed susanshi’s stale review via eb1148c August 30, 2024 01:30

yizha1 reviewed Aug 30, 2024

View reviewed changes

binbin-li force-pushed the refactor-error-msg branch from 8c69f5e to c258392 Compare September 1, 2024 08:41

chore: address comments

65617a1

binbin-li force-pushed the refactor-error-msg branch from c258392 to 65617a1 Compare September 2, 2024 03:16

Merge remote-tracking branch 'upstream/dev' into refactor-error-msg

242be02

junczhu mentioned this pull request Sep 2, 2024

chore: refactor Err msg in notation verifier #1774

Closed

7 tasks

yizha1 reviewed Sep 3, 2024

View reviewed changes

chore: address comments

d821a90

binbin-li force-pushed the refactor-error-msg branch from 14db495 to d821a90 Compare September 4, 2024 06:37

junczhu reviewed Sep 4, 2024

View reviewed changes

pkg/controllers/clusterresource/verifier_controller.go Show resolved Hide resolved

yizha1 reviewed Sep 6, 2024

View reviewed changes

binbin-li and others added 2 commits September 9, 2024 06:51

docs: address comments

fe92a9e

Merge branch 'dev' into refactor-error-msg

ebb2217

yizha1 approved these changes Sep 9, 2024

View reviewed changes

binbin-li and others added 3 commits September 9, 2024 16:12

Merge branch 'dev' into refactor-error-msg

4bf283d

Merge branch 'dev' into refactor-error-msg

45c29a2

chore: address comments

4f48d2f

luisdlp approved these changes Sep 9, 2024

View reviewed changes

Merge branch 'dev' into refactor-error-msg

06faa58

binbin-li enabled auto-merge (squash) September 9, 2024 15:41

binbin-li merged commit ab8d001 into ratify-project:dev Sep 9, 2024
19 checks passed

akashsinghal pushed a commit to akashsinghal/ratify that referenced this pull request Sep 13, 2024

chore: Refactor error messages for Notation signature verification (r…

698138f

…atify-project#1730) Signed-off-by: akashsinghal <[email protected]>

binbin-li added a commit to binbin-li/ratify that referenced this pull request Sep 14, 2024

chore: Refactor error messages for Notation signature verification (r…

56c7160

…atify-project#1730)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Refactor error messages for Notation signature verification #1730

chore: Refactor error messages for Notation signature verification #1730

binbin-li commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading

susanshi left a comment

binbin-li commented Sep 9, 2024

yizha1 left a comment

luisdlp left a comment

chore: Refactor error messages for Notation signature verification #1730

chore: Refactor error messages for Notation signature verification #1730

Conversation

binbin-li commented Aug 20, 2024 • edited Loading

Description

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Type of change

How Has This Been Tested?

Checklist:

Post Merge Requirements

codecov bot commented Aug 21, 2024 • edited Loading

Codecov Report

susanshi left a comment

Choose a reason for hiding this comment

binbin-li commented Sep 9, 2024

yizha1 left a comment

Choose a reason for hiding this comment

luisdlp left a comment

Choose a reason for hiding this comment

binbin-li commented Aug 20, 2024 •

edited

Loading

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

codecov bot commented Aug 21, 2024 •

edited

Loading