-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add fastly_acl
hostcalls
#438
Conversation
764443e
to
76e6dc9
Compare
76e6dc9
to
6aae133
Compare
fn main() -> Result<(), Error> { | ||
// Temporary until fastly SDK is released which | ||
// includes the fastly::acl module. | ||
#[cfg(feature = "acl_hostcalls")] |
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.
If I'm following right, is the idea here to:
- Merge this PR
- Publish Viceroy
- Publish Rust SDK with ACLs support
- Do follow-up PR to update SDK version dependency and remove the
acl_hostcalls
feature guard
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, that's also how I was thinking the sequencing would be.
This feature guard can be removed once the new fastly
crate is published (the order of the previous steps shouldn't matter).
#[derive(Clone, Debug, Default)] | ||
pub struct AclConfig(pub(crate) acl::Acls); | ||
|
||
mod deserialization { |
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.
It seems fine, but I'm curious: is there a reason to put this into its own deserialization
module? And would it be worth putting it into its own lib/src/config/acl/deserialization.rs
?
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.
I choose this structure to match other similar sections (e.g backends
, device_detection
, dictionaries
, ...). So not a very compelling reason.
I'll change it to whichever organization you think best.
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.
I think this looks good! I just have two minor questions.
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 for doing this work. I've left a couple of comments regarding the json format, but I think I just convinced myself it was probably fine. I've also left a note to the effect that the goal here is to match production behaviour on a functional basis, not on a performance basis.
@@ -0,0 +1,8 @@ | |||
{ |
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.
I really like the fact that the file formats are close to the format used to send updates to the API. How do you feel about adding the "op"
field too, so that the formats are the same?
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, good idea. I've made that change in c18e3b6
{ "op": "update", "prefix": "192.168.0.0/16", "action": "BLOCK" }, | ||
{ "op": "create", "prefix": "23.23.23.23/32", "action": "ALLOW" }, | ||
{ "op": "update", "prefix": "1.2.3.4/32", "action": "ALLOW" }, | ||
{ "op": "update", "prefix": "1.2.3.4/8", "action": "ALLOW" } |
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.
It looks like this testcase includes the "op"
field, which is nice. Is it just ignored then? That's probably fine then!
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, it's ignored (along with any other extraneous fields).
Thanks for pointing out the inconsistency with this test case and the integration test data. In c18e3b6, I've corrected that along with adding better comments to better document the JSON structure.
/// over the acls entries. | ||
/// | ||
/// If the IP matches multiple ACL entries, then: | ||
/// - The most specific match is returned (longest mask), |
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.
This looks like it gives the same results as we'd expect to see in production. Obviously the performance characteristics will be different, but I don't believe that it is a goal of Viceroy to duplicate that.
This adds support for two new hostcalls:
fastly_acl::open
fastly_acl::lookup
ACLs can be mocked via
fastly.toml
by adding one or moreacls
tolocal_server
. Each acl entry is a reference to an external JSON file. Example:The JSON file contains a list of one or more ACL entries, in the same format as can be used to modify ACLs via api.fastly.com. Example:
The test-fixtures/src/bin/acl.rs file shows example guest usage.
Integration Tests
The
fastly
Rust crate containing support for ACLs has not yet been released. Because of that, theacl.rs
integration test uses a configuration feature to conditionally compile, with a default of not using thefastly::acl
feature:Viceroy/test-fixtures/src/bin/acl.rs
Lines 5 to 7 in 6aae133
To test against a local, unreleased crate:
test-fixtures/Cargo.toml
to point to the appropriate local cratesacl_hostcalls
feature:(cd test-fixtures && cargo build -F acl_hostcalls --target=wasm32-wasi)
Once a new version of the crate has been published, the conditional can be removed. I have tested against the internal/unreleased crate and verified that it passes.