-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: make notation verifier installation optional on ratify installation #1719
fix: make notation verifier installation optional on ratify installation #1719
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
f71a81e
to
43b82b9
Compare
discussed in the PR reviewing meeting, we should have a warning ( if possible) if there is no verifier is configured. Please also validate when there are no verifiers configured, Ratify produces a verification response with FAILed status. |
we don't want to block the installation on 0 verifiers, we will try our test to warn if possible. |
Here is the result of testing different scenarios:
|
43b82b9
to
2e9f6c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@susanshi Should we only check for cosign and notation here? Or sbom should also be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all external plugin should be included too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious: does this block print out a warning on helm install?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Helm doesn't provide a straightforward way to issue a warning that doesn't halt execution, initially I was just printing a string based on the same condition on top of the verifier.yaml file. But it doesn't work, because Helm treated the printed warning message as part of the Kubernetes resource definition.
WE can leverage Helm's NOTES.txt file, which is specifically designed for displaying messages after a successful installation or rendering. While this doesn't show up directly during the template rendering phase, it will ensure the user sees the message right after installation.
2e9f6c4
to
d8febbb
Compare
charts/ratify/templates/NOTES.txt
Outdated
@@ -0,0 +1,7 @@ | |||
{{- if not (or .Values.notation.enabled .Values.cosign.enabled .Values.sbom.enabled) }} | |||
NOTES: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
charts/ratify/templates/NOTES.txt
Outdated
@@ -0,0 +1,6 @@ | |||
{{- if not (or .Values.notation.enabled .Values.cosign.enabled .Values.sbom.enabled) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can add check on vulnerabilityreport as well
58646e2
to
5447f92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
…ion (ratify-project#1719) Signed-off-by: akashsinghal <[email protected]>
Description
What this PR does / why we need it:
It enables Ratify to not install notation by default. It gives the option that the notation verifier installation is disabled at the time Ratify installs.
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):Fixes #1451
Type of change
Please delete options that are not relevant.
main
branch)How Has This Been Tested?
The following scenarios are being tested to make sure the right report is generated:
a) When both notation and cosign are enabled:
1. when the image is signed by both notation and cosign:
2. when the image is signed by just notation:
3. when the image is signed by just cosign:
4. when the image is unsigned:
b) When only notation is enabled:
1. when the image is signed by both notation and cosign:
2. when the image is signed by just notation:
3. when the image is signed by just cosign:
4. when the image is unsigned
c) When only cosign is enabled:
1. when the image is signed by both notation and cosign:
2. when the image is signed by just notation:
3. when the image is signed by just cosign:
4. when the image is unsigned
d) When neither notation nor cosign is enabled:
1. when the image is signed by both notation and cosign:
2. when the image is signed by just notation:
3. when the image is signed by just cosign:
4. when the image is unsigned
Checklist:
Post Merge Requirements
Helm Chart Change