Refactor the plugin API #202
Conversation
…nvironment import
…etail to the plugin.
…sed by the context.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- crates/wasm-host/src/context.rs (1 hunks)
- crates/wasm-host/src/from.rs (1 hunks)
- crates/wasm-sdk/src/errors.rs (3 hunks)
- wit/redis.wit (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- crates/wasm-host/src/context.rs
- crates/wasm-host/src/from.rs
- wit/redis.wit
Additional comments: 8
crates/wasm-sdk/src/errors.rs (8)
- 3-3: Renaming
Errortoerrorand using::anyhow::anyhowdirectly simplifies error creation, but it's crucial to ensure this change is consistently applied across the entire codebase to avoid confusion. The previous discussion indicates thatanyhow::Erroris still used in some places, which might be intentional for wrapping errors. However, it's important to document this decision clearly to maintain clarity in error handling practices.- 16-24: The renaming of
ParamErrortoEnvVarErrorand the update of its variants make the error names more descriptive and aligned with their specific use cases. This change enhances code readability and makes error handling more intuitive.- 63-73: Adding
ConfigErrorwith new variants improves the specificity of configuration-related errors. This addition follows the same pattern asEnvVarError, making the error handling more consistent across different aspects of the plugin API.- 103-105: The conversion from
std::string::FromUtf8ErrortoConfigErrorfor handling invalid Unicode errors is a good practice, ensuring that errors from different sources can be uniformly handled within the plugin API.- 118-127: The addition of
RemoteStateErrorwith new variants, includingInvalidInteger, enhances the granularity of error handling for remote state operations. This change is in line with the PR's objective to improve the developer experience by making error handling more explicit and intuitive.- 130-145: The conversion from
crate::wit::bulwark::plugin::redis::ErrortoRemoteStateErroris well-implemented, covering all relevant cases from the Redis error domain. This ensures that errors from Redis operations are correctly mapped to the plugin API's error handling model.- 150-154: The conversion from
std::string::FromUtf8ErrortoRemoteStateErrorfor handling invalid Unicode errors in the context of remote state is consistent with similar conversions for other error types, maintaining uniformity in error handling.- 158-161: The conversion from
ParseCounterErrortoRemoteStateErrorfor handling invalid integer errors is a thoughtful addition, ensuring that parsing errors are correctly categorized within the broader error handling model of the plugin API.
I'm assuming you're referring to this comment: // TODO: handle overflow errors by expiring everything on overflow and returning nil?That's related to the possibility that we could increment a counter enough that the counter overflows. However, I just tested it out and Redis already guards against overflows natively. We just have to handle that error correctly in the script: |
Here's the error given by overflow in script execution: I might just leave it as-is because the error is returned in a |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/wasm-host/src/context.rs (1 hunks)
Additional comments: 8
crates/wasm-host/src/context.rs (8)
- 16-38: The
PluginContextstruct is well-defined, encapsulating all necessary components for plugin execution, including WASI contexts, HTTP contexts, I/O buffers, configurations, permissions, and Redis information. This struct serves as a central point for managing plugin execution environments, ensuring that plugins have access to the resources and configurations they need while adhering to security constraints.- 40-46: The
RedisInfostruct provides a clear and concise way to manage Redis connections and Lua script registries. This struct is crucial for enabling plugins to interact with Redis, supporting rate limiting, circuit breaking, and other features that require Redis operations.- 48-65: The
ScriptRegistrystruct organizes predefined Lua scripts for Redis operations, such as rate limiting and circuit breaking. This approach centralizes script management, making it easier to maintain and update scripts as needed. It's a good practice to keep these scripts within the codebase for better version control and auditability.- 216-231: The implementation of the
WasiViewtrait forPluginContextis straightforward and correctly delegates to the internalwasi_tableandwasi_ctxfields. This implementation ensures that the plugin context can be used wherever a WASI view is required, facilitating interactions with WASI syscalls.- 234-252: The implementation of the
WasiHttpViewtrait, specifically thesend_requestmethod, correctly enforces HTTP domain permissions before dispatching outgoing requests. This is a critical security feature that prevents plugins from making unauthorized HTTP requests. The use ofverify_http_domainsfor permission checks is appropriate and aligns with the security model.- 322-1029: The implementations of Redis operations (e.g.,
get,set,del,incr,incr_by,sadd,smembers,srem,expire,expire_at,incr_rate_limit,check_rate_limit,incr_breaker,check_breaker) are comprehensive and cover a wide range of Redis functionalities required by plugins. Each method correctly enforces remote state key permissions usingverify_remote_state_prefixes, ensuring that plugins can only access allowed keys. The use of asynchronous futures and the?operator for error handling is consistent and idiomatic in Rust async programming. These implementations provide a solid foundation for plugins to interact with Redis in a secure and controlled manner.- 1032-1051: The
verify_http_domainsfunction correctly parses the authority part of an HTTP request and checks if it's allowed based on the configured permissions. The use ofUrl::parsefor parsing and the domain extraction logic is appropriate. This function plays a crucial role in enforcing HTTP domain permissions, ensuring that plugins can only make requests to allowed domains.- 1053-1069: The
verify_remote_state_prefixesfunction ensures that access to any remote state key adheres to the configured permissions. The logic for checking if a key starts with any of the allowed prefixes is correct and straightforward. This function is essential for maintaining the security and integrity of remote state access by plugins.
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've just finished my final pass over this review.
Like I said in an earlier pass, this refactor looks quite solid and has a lot of great improvements. The main points I noted were mostly just related to edge cases with Redis, and those have already been addressed, just to be extra careful.
Otherwise, I just had 2 additional comments in this final pass.
Thanks for all of the hard work you're doing on maintaining this project! Like I expressed when reviewing last May, I'm a big fan of it.
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/wasm-sdk-macros/src/lib.rs (9 hunks)
Check Runs (2)
Clippy completed (6)
Test suite (stable, ubuntu-latest) completed (6)
Additional comments: 8
crates/wasm-sdk-macros/src/lib.rs (8)
- 6-6: The constant
WIT_PATHis defined usingconcat!withenv!("CARGO_MANIFEST_DIR")to specify the path to the WIT definitions. Ensure that the WIT files are indeed located in the specified directory relative to theCARGO_MANIFEST_DIR. This is crucial for thewit_bindgenmacro to correctly locate and use the WIT files.- 18-32: The documentation for the
bulwark_pluginattribute provides a clear overview of the handler functions and their sequence. However, it's important to ensure that the documentation is kept up-to-date with any changes to the handler functions or their behavior. Regularly reviewing and updating documentation is essential for maintainability and developer experience.- 61-67: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [64-78]
The error handling for unexpected trait names in the
bulwark_pluginmacro is well-implemented, providing clear feedback to the developer. However, consider adding more detailed guidance or suggestions in the error message on how to resolve the issue, such as specifying the correct trait name or checking for typos.
- 87-91: The list of handler names is hardcoded. This approach is straightforward but may lead to maintenance challenges if new handlers are added or existing ones are renamed. Consider a more dynamic way of managing handler names, possibly through metadata or configuration, to ease future updates.
- 133-202: The generation of no-op default handlers for missing handler functions is a smart feature, ensuring that all required handlers have some implementation. However, ensure that these default implementations are documented and understood by developers, especially regarding their behavior and when it's appropriate to override them with custom logic.
- 213-219: The use of
wit_bindgen::generate!macro to generate bindings based on WIT definitions is crucial for integrating with the WASI ecosystem. Verify that the specifiedworldandpathare correct and that the generated bindings are being utilized as expected in the plugin system.- 418-419: The implementation of the
HttpHandlerstrait for the struct type is a key part of the plugin system. Ensure that all handler functions are correctly implemented and that the trait's requirements are fully satisfied. This is crucial for the correct operation of plugins within the Bulwark system.- 432-587: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [435-607]
The
handlermacro is designed to simplify the creation of event handlers. Ensure that the macro is correctly applied to all relevant functions and that it properly transforms them into Bulwark event handlers. Additionally, verify that the error handling within the generated functions is adequate and that any errors are appropriately logged or reported.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/wasm-sdk-macros/src/lib.rs (9 hunks)
Check Runs (2)
Test suite (stable, ubuntu-latest) completed (6)
Clippy completed (6)
Additional comments: 5
crates/wasm-sdk-macros/src/lib.rs (5)
- 18-32: The documentation for the
bulwark_pluginattribute clearly outlines the purpose and usage of each handler. This is a good practice as it enhances code readability and helps developers understand the expected behavior of their implementations.- 133-202: The implementation of no-op default handlers is a smart approach to ensure that all required handlers have some form of implementation, even if they are not explicitly defined by the developer. This can prevent runtime errors due to missing handlers. However, ensure that this behavior is clearly documented so developers are aware that these default implementations exist.
- 213-219: The use of
wit_bindgen::generate!macro to generate bindings based on the WIT definitions is crucial for the interoperability between Rust and WASM. Ensure that the WIT path and the runtime path are correctly configured and that they point to the expected locations within the project structure.Verification successful
Given the confirmation that the WIT directory exists, the next step is to verify that it contains the expected WIT definitions and to ensure the runtime path is correctly configured. To proceed, I will generate a script to list the contents of the WIT directory.
To further verify that the WIT directory contains the expected WIT definitions, the next step is to list the contents of the actual WIT directory to which the symbolic link points.
The verification process has confirmed that the WIT directory exists and is properly configured with the expected WIT definitions, as evidenced by the presence of multiple
.witfiles within the directory. This addresses the review comment's concern about ensuring the WIT path points to the expected locations within the project structure. While the correctness of the runtime path configuration is implied by the structured approach to managing WIT definitions, it cannot be directly verified through script execution. Based on the information gathered, the review comment is validated.* 284-366: The implementation of `TryFrom` for converting an `IncomingRequest` to Bulwark's `Request` type is comprehensive, covering various aspects of an HTTP request. Pay special attention to error handling and ensure that all potential errors are accounted for and that the error messages provide enough context for debugging. Additionally, consider the performance implications of copying and converting data, especially for large requests. * 369-415: Similar to the `IncomingRequest` conversion, the `TryFrom` implementation for `IncomingResponse` to `Response` also requires careful consideration of error handling and performance. Ensure consistency in how errors are handled between request and response conversions and consider adding benchmarks to measure the performance impact of these conversions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the WIT path exists and contains the expected WIT definitions. if [ -d "crates/wasm-sdk-macros/wit" ]; then echo "WIT directory exists." else echo "WIT directory does not exist. Please check the path." fiLength of output: 92
Script:
#!/bin/bash # List the contents of the WIT directory to verify it contains the expected WIT definitions. ls -l crates/wasm-sdk-macros/witLength of output: 110
Script:
#!/bin/bash # List the contents of the actual WIT directory to verify it contains the expected WIT definitions. ls -l witLength of output: 414
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/wasm-sdk/src/redis.rs (1 hunks)
Check Runs (2)
Clippy completed (6)
Test suite (stable, ubuntu-latest) completed (6)
Files skipped from review as they are similar to previous changes (1)
- crates/wasm-sdk/src/redis.rs
|
I'm going to ignore the bot recommendations related to |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/wasm-sdk-macros/src/lib.rs (9 hunks)
Check Runs (2)
Clippy completed (6)
Test suite (stable, ubuntu-latest) completed (6)
Additional comments: 7
crates/wasm-sdk-macros/src/lib.rs (7)
- 6-6: The introduction of the
WIT_PATHconstant is a good practice for managing paths, ensuring consistency and maintainability.- 133-202: The implementation of no-op default handlers is a robust way to ensure that all required handlers have some form of implementation, even if it's just a placeholder. This approach enhances the macro's usability and developer experience.
- 213-219: The use of
wit_bindgen::generate!macro to generate bindings based on the WIT definitions is a significant enhancement. It ensures that the Rust code and the WIT definitions are tightly coupled, reducing the risk of mismatches.- 284-366: The implementation of
TryFromfor converting anIncomingRequestto Bulwark'sRequesttype is comprehensive. However, consider the performance implications of copying and converting data, especially for large requests. Ensure that all potential errors are accounted for and that the error messages provide enough context for debugging.- 369-415: Similar to the
IncomingRequestconversion, theTryFromimplementation forIncomingResponsetoResponsealso requires careful consideration of error handling and performance. Ensure consistency in how errors are handled between request and response conversions.- 418-419: The implementation of the
HttpHandlerstrait for the struct type is a key part of integrating the macro-generated code with the rest of the system. This ensures that the custom handlers and the no-op defaults are correctly recognized and utilized.- 432-587: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [435-607]
The
handlermacro attribute's implementation is well-structured, ensuring that the associated functions are correctly transformed into Bulwark event handlers. The detailed error handling and flushing of IO streams are thoughtful touches that enhance reliability.
| match Self::send_block_request_message(self.sender.clone()).await { | ||
| Ok(response) => { | ||
| // Normally we initiate feedback after the response phase, but if we're blocking the request | ||
| // in the request phase, we're also skipping the response phase and we need to do it here |
sporkmonger
added
the
size/xx-large
This is a major redesign of the plugin API. This aligns Bulwark's plugin API to the WASI 0.2 types.
Redesign goal
Parameterize the handlers so that plugin developers no longer have to make awkward function calls to pull in values that make more sense as parameters. (e.g.
get_request()) Similarly, handlers now have return values that make their intended output more explicit. A secondary goal is enabling future performance improvement by allowing WASM instances to be recycled instead of having to create and tear them down on every request.Major changes in this PR
wasmtimeupdates necessary for WASI 0.2 (supercedes Upgrade to wasmtime 16 #191)ProcessorContextwith a lifetime that lines up with the request/response cycleCaveats
wasi:http/outgoing-handlerAPI is wired up and implemented, it doesn't seem to be getting exposed correctly in the compiled.wasmoutput. I don't know why, but I'm inclined to treat it as a bug to fix after this PR lands rather than as part of it, unless the cause of the bug gets spotted during review. This PR has already been in-progress for way too long.Summary by CodeRabbit
New Features
wasmtimereleases.buildcrate for constructing plugins with enhanced error handling.Decisioninstances and managing plugin information.Refactor
Documentation
Chores
.gitignorerules across several directories.Style