doc: proposal for error message improvements #1662
Conversation
Signed-off-by: Yi Zha <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
Signed-off-by: Yi Zha <[email protected]>
Signed-off-by: Yi Zha <[email protected]>
Signed-off-by: Yi Zha <[email protected]>
Signed-off-by: Yi Zha <[email protected]>
yizha1
requested review from
akashsinghal,
binbin-li,
jimmyraywv,
luisdlp,
susanshi and
toddysm
as code owners
| For the above first example, the error message in the Ratify log can be improved to: | ||
|
|
||
| ```text | ||
| REPOSITORY_OPERATION_FAILURE: Failed to resolve the artifact descriptor: HEAD "https://roacr.azurecr.io/v2/net-monitor/manifests/v2": GET "https://roacr.azurecr.io/oauth2/token? |
There was a problem hiding this comment.
This example is also confusing. Why are they two HTTP requests (HEAD and GET) and how is this helping the user understand what the reason is (if this is meant to explain the reason)?
Also, having everything lowercase makes those messages really hard to read because it is not clear where a sentence ends and starts.
There was a problem hiding this comment.
@toddysm This message (HEAD and GET) was returned by ACR in this example. The ACR responded with an error and the response message was printed out. If we do not print out server response, we do not know what happened and the reason. In this case, the 401 unauthorized was also included in the message.
There was a problem hiding this comment.
Passing through errors is not a user-friendly practice. We need to catch the error from ACR and wrap it into something meaningful\ for the user of Rarify. The user of Ratify may have no knowledge of where the artifact is and even not know what artifact descriptor is. We need to provide meaningful message for the user of Ratyify and not the user of ACR.
|
@fseldow , could you take a look thanks! |
| "isSuccess": false, | ||
| "message": "NO_VERIFIER_REPORT: Failed to verify artifact docker.io/library/hello-world@sha256:1408fec50309afee38f3535383f5b09419e6dc0925bc69891e79d84cc4cdce6: | ||
| "errorReason": "No signature verification report is found." | ||
| "remediation": "The artifact was either not signed and should not be trusted, signed but missing a Verifier configuration for verification, or needs to be signed if it should be." |
There was a problem hiding this comment.
have one quick question. If we found the artifact was not signed, why the error reason is no signature verification report is found. It sounds more like verification failed or not signed
There was a problem hiding this comment.
That's good call. I think this proposal is mainly focusing on the overall format of the error, like what info should be printed in the log. For the specific message for each error, we'll handle it one by one.
There was a problem hiding this comment.
@fseldow Updated. Please review it again.
| "isSuccess": false, | ||
| "message": "NO_VERIFIER_REPORT: Failed to verify artifact docker.io/library/hello-world@sha256:1408fec50309afee38f3535383f5b09419e6dc0925bc69891e79d84cc4cdce6: | ||
| "errorReason": "No signature verification report is found." | ||
| "remediation": "The artifact was either not signed and should not be trusted, signed but missing a Verifier configuration for verification, or needs to be signed if it should be." |
There was a problem hiding this comment.
another question, how can customer get the remediation msg? via kubectl logs ratify or constraint template violation msg?
There was a problem hiding this comment.
@fseldow as you said there are two ways, I prefer policy controller to customize the constraint template to use the remediation field, since it is not easy for users to identify the correct logs among many with kubectl logs
@binbin-li I think Ratify can also improve the current default constraint template to include the remediation message.
There was a problem hiding this comment.
@fseldow Updated. Please review it again.
There was a problem hiding this comment.
#1669 I updated this issue to add tracking remediation in the CT.
Signed-off-by: Yi Zha <[email protected]>
Signed-off-by: Yi Zha <[email protected]>
Created the issue: #1681 |
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):Proposal for issue #1321
Type of change
Please delete options that are not relevant.
mainbranch)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
Checklist:
Post Merge Requirements
Helm Chart Change