-
Notifications
You must be signed in to change notification settings - Fork 41
Make protocol-less references point to policy providers #1282
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
Conversation
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
migmartri
left a comment
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.
Thanks @jiparis, is there any improvements we should do in the contract validation now?
For example, if no providers are configured, say that file:// or https:// needs to be used?
I want to make sure people don't point to remote policies by mistake if we can prevent it.
Currently, any attempt of creating a contract with a policy from external providers, will raise an error if providers are not configured or policy is not found. For example, when trying to create this contract, with no providers: We could try to improve the error message. |
I think so, let's say that 99% of users will not have enabled the provider setup. In that case I'd make sure to tell them to use file:// or https://, what do you think? |
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
migmartri
left a comment
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!
| ref := attachment.GetRef() | ||
| filePath, err := ensureScheme(ref, fileScheme) | ||
| if err != nil { | ||
| return nil, err |
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.
please wrap the errors.
| var err error | ||
| // support both | ||
| if httpRef, err = ensureScheme(ref, httpScheme, httpsScheme); err != nil { | ||
| return nil, err |
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.
ditto about wrap
| func (l *HTTPSLoader) Load(_ context.Context, attachment *v1.PolicyAttachment) (*v1.Policy, error) { | ||
| ref := attachment.GetRef() | ||
|
|
||
| var httpRef string |
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.
these two variables could be replaced if you run the following stanza in two lines
if httpRef, err = ensureScheme(ref, httpScheme, httpsScheme); err != nil {
httpRef, err := ensureScheme(ref, httpScheme, httpsScheme)
if err != nil {
This PR is a followup of #1267 to make policy providers the default when no protocol is specified.
This simplifies the operation in organizations with built-in policies provided by an external provider.
Note. This PR maintains backwards compatibility for existing contracts. For example, for
attestation add:It also improves some error messages during contract validation: