Remove FFI interface from cedar-cli and create new create cedar-lean-ffi which can be used in both cedar-cli and cedar-drt.#628
Conversation
|
The goal of this refactor is to eliminate code dupilication between the I have refactored |
john-h-kastner-aws
left a comment
john-h-kastner-aws
left a comment
There was a problem hiding this comment.
Change looks good. CI isn't building this, so I'll assume for now that it passes all tests, but we should fix that
| Ok(()) | ||
| } | ||
| Err(e) => Err(e), | ||
| Err(e) => Err(e)?, |
There was a problem hiding this comment.
Err(e)? looks funny to me. Should be able to put the ? in the match like lean_context.validate(policyset, schema, mode)? and drop this arm. Same for the other changes in this file
There was a problem hiding this comment.
That's fair. The underlying err type changed and I was on auto-pilot to fix the errors. 😅
| cp -r include/google/ /usr/local/include/ | ||
| ``` | ||
|
|
||
| ### Build this CLI |
There was a problem hiding this comment.
| ### Build this CLI | |
| ### Build this Library |
| publish = false | ||
|
|
||
| [dependencies] | ||
| cedar-policy = { version = "4.4.0", features = ["protobufs"] } |
There was a problem hiding this comment.
I like not always requiring a local clone, but what's the plan for local development and the Github actions build? As written, this will always pull from crates.io
There was a problem hiding this comment.
If I understand correctly, it won't pull from crates.io if the crate that imports this library uses a local clone of cedar-policy?
There was a problem hiding this comment.
I guess the problem is if we add a new function to cedar-policy and want to call it in this crate. That will requiring depending on the local version for development and then the github version for CI until we do a new minor version release. Could be fine if you think this crate is small and stable enough for that not to come up.
There was a problem hiding this comment.
My biggest concern is that I want CI to be testing that this crate always build with the latest version of cedar-policy from main. If you think you have a way to set that up (or if building this into cedar-drt should do that automatically), then this isn't a problem
There was a problem hiding this comment.
I see two possible solutions.
1.) We add a use-local feature that will change the dependency to a local clone of cedar-policy.
by adding the following lines to Cargo.toml
[dependencies]
cedar-policy = { version = "4.4.0", features = ["protobufs"], optional = true }
[dependencies.cedar-policy-local]
package = "cedar-policy"
path = "../cedar/cedar-policy"
version = "4.4.0"
features = ["protobufs"]
optional = true
[features]
use-local = ["cedar-policy-local"]
default = ["cedar-policy"]
Normally, we build using crate.io (or whatever version the importing code uses). But allows us to do:
cargo build --features use-local --no-default-features
Note, as far as I can tell. I would need to modify every import of cedar_policy::* to use cedar_policy_local::* if the use-local feature is set.
2.) Require the CI or local developer to either manually edit the cargo or add a cargo patch that forces the use of a local clone of cedar-policy
# .cargo/config.toml
[patch.crates-io]
cedar-policy = { path = "../cedar/cedar-policy" }
Pros: No need to modify the actual Cargo.toml or rust code.
Cons: a bit clunkier to do during local development. Can be automated in CI by writing .cargo/config.toml in the CI actions as part of the setup.
There was a problem hiding this comment.
I think our CI for some repo in this org takes some variant of approach 2. Probably makes sense to be consistent -- if we go with something else here, update our other CIs to match
cdisselkoen
left a comment
cdisselkoen
left a comment
There was a problem hiding this comment.
Looks like a great change to me
| Ok(()) | ||
| } | ||
| Err(e) => Err(e), | ||
| Err(e) => Err(e)?, |
There was a problem hiding this comment.
Here's another place where we can move the ? up, in this case to line 62 probably
| publish = false | ||
|
|
||
| [dependencies] | ||
| cedar-policy = { version = "4.4.0", features = ["protobufs"] } |
There was a problem hiding this comment.
I think our CI for some repo in this org takes some variant of approach 2. Probably makes sense to be consistent -- if we go with something else here, update our other CIs to match
| let decision = match inner.decision.as_str() { | ||
| "allow" => Decision::Allow, | ||
| "deny" => Decision::Deny, | ||
| _ => return Err(FfiError::LeanDeserializationError), |
There was a problem hiding this comment.
Might be helpful to have this error carry the value that wasn't recognized
…ffi which can be used in both cedar-cli and cedar-drt. Signed-off-by: Charlie Murphy <mutmoth@amazon.com>
Co-authored-by: John Kastner <130772734+john-h-kastner-aws@users.noreply.github.com> Signed-off-by: Charlie Murphy <mutmoth@amazon.com>
Signed-off-by: Charlie Murphy <mutmoth@amazon.com>
Co-authored-by: Craig Disselkoen <craigdissel@gmail.com> Signed-off-by: Charlie Murphy <mutmoth@amazon.com>
…al cedar-cli and this cli Signed-off-by: Charlie Murphy <mutmoth@amazon.com>
Signed-off-by: Charlie Murphy <mutmoth@amazon.com>
Signed-off-by: Charlie Murphy <mutmoth@amazon.com>
| [package] | ||
| name = "cedar-lean-cli" | ||
| edition = "2021" | ||
| version = "0.1.0" |
There was a problem hiding this comment.
should this start at 4.4.0 as well?
3 participants
Refactor
cedar-clito create a new cratecedar-lean-ffiwhich provides a reusable set of Rust-bindings to the Lean formalization of Cedar.Additionally, fixes a bug in cedar-cli which caused the cli to parse json format schemas using the cedar-schema parser.
The new crate should be a superset of the needs for both
cedar-cliandcedar-drt.