-
Notifications
You must be signed in to change notification settings - Fork 79
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
Expose protobufs in public API #1345
Conversation
Signed-off-by: Craig Disselkoen <[email protected]>
cedar-policy/src/api.rs
Outdated
@@ -1927,6 +2016,11 @@ impl PolicySet { | |||
}) | |||
} | |||
|
|||
/// Build the [`PolicySet`] from just the AST information | |||
pub fn from_ast(ast: ast::PolicySet) -> Result<Self, PolicySetError> { |
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.
Should this be pub
?
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.
Correct, it shouldn't be
@@ -102,8 +102,8 @@ message EntityRecordKind { | |||
} | |||
|
|||
enum OpenTag { | |||
OpenAttributes = 0; | |||
ClosedAttributes = 1; | |||
OpenAttributes = 0; |
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.
A general note about the protobuf impl. Does this mean that OpenAttributes
on records in the schema would become a publicly accessible feature when we stabilize protobuf? What other internal details might be exposed this way?
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.
Interesting question.
Protobuf is not self-describing. In this PR, public-API users only see a blob of bytes, and if one of those bits indicates open-attributes vs closed-attributes, they would never know. But if anyone actually relies on our .proto
files to encode/decode themselves, then yes, they see these attributes.
I believe the Lean protobuf parser errors if it sees OpenAttributes.
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.
Arguably if we want to stabilize protobufs before OpenAttributes, we should remove OpenAttributes from the protobuf format.
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.
Yeah, I'd agree with that. And also
- attributes on actions
- never and empty set types
- Action Entity type
- and permissive validation mode (behind it's own feature)
Signed-off-by: Craig Disselkoen <[email protected]>
Signed-off-by: Craig Disselkoen <[email protected]>
Description of changes
To date, the protobuf serialization/deserialization experimental feature is only available to direct consumers of cedar-policy-core and cedar-policy-validator, and callers are required to manually call
from()
/into()
to convert into the appropriate struct incedar_policy_core::ast::proto
orcedar_policy_validator::proto
.This PR introduces a new
Protobuf
trait, exposed in our public API (guarded by theprotobufs
feature flag, of course), which allows consumers of the public API to serialize/deserialize various public-API types likePolicy
andSchema
using protobufs. All necessary conversions are performed under-the-hood, and the types in theproto
modules are not exposed publicly.Issue #, if available
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec
(choose one, and delete the other options):I confirm that
docs.cedarpolicy.com
(choose one, and delete the other options):