-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix request.body rego builtin sporadic unexpected EOF #31
Conversation
WalkthroughThe recent modifications aim to enhance the flexibility and configurability of the Beskar plugin system and streamline the request handling in the routing package. These changes involve adding new configuration options, modifying method signatures to accommodate additional parameters, and refining the request body processing mechanism for improved efficiency. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- internal/pkg/router/builtin.go (3 hunks)
Additional comments: 5
internal/pkg/router/builtin.go (5)
- 151-154: The check for
nil
orhttp.NoBody
on the request body is a good practice to avoid unnecessary processing or potential nil dereferences.- 156-156: Initializing a new buffer to read the request body is a standard approach and is correctly implemented.
- 165-171: Reading the buffered data into a
bytes.Reader
and then converting it to an AST value is done correctly.- 174-174: Resetting the request body with
io.NopCloser
wrapped around thebytes.Reader
is a clean way to allow for potential re-reads without the risk of an EOF error from a closed body.- 176-176: Returning the new AST term created from the request body is the expected outcome of this function.
cea17b6
to
3c0282d
Compare
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
internal/pkg/config/default/beskar.yaml
is excluded by:!**/*.yaml
Files selected for processing (5)
- internal/pkg/beskar/plugin.go (6 hunks)
- internal/pkg/beskar/registry.go (2 hunks)
- internal/pkg/config/beskar.go (1 hunks)
- internal/pkg/router/builtin.go (4 hunks)
- internal/pkg/router/router.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- internal/pkg/router/builtin.go
Additional comments: 13
internal/pkg/router/router.go (4)
- 34-34: The addition of the
bodyLimit
field to theRegoRouter
struct is consistent with the PR's objective to manage the request body size.- 63-68: The
WithBodyLimit
function is correctly implemented to set thebodyLimit
field in theRegoRouter
struct.- 79-79: Initializing the
bodyLimit
field to 8192 in theNew
function is a sensible default, aligning with the PR's intent to manage request body sizes.- 98-100: The
Decision
method now correctly uses thebodyLimit
field from theRegoRouter
instance, which should help in handling the request body size as intended.internal/pkg/config/beskar.go (2)
- 34-36: The
Router
struct with theBodyLimit
attribute has been correctly added to theBeskarConfig
struct, allowing configuration of the request body limit.- 45-45: The new
Router
field in theBeskarConfig
struct is properly defined and follows the expected structure for configuration files.internal/pkg/beskar/plugin.go (5)
- 31-31: The import statement for the
config
package is correctly added to use the new configuration options in the plugin logic.- 122-122: The
register
function now correctly takes an additionalbeskarConfig *config.BeskarConfig
parameter, which is necessary for passing the new configuration options.- 150-150: The
initRouter
function within theplugin
struct is correctly modified to take an additionalbodyLimit int64
parameter, aligning with the PR's objectives.- 164-164: The
initRouter
function is correctly updated to handle the newbodyLimit
parameter during plugin updates.- 353-361: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [347-358]
The
initRouter
function is correctly updated to include thebodyLimit
parameter when initializing the router with the new configuration options.internal/pkg/beskar/registry.go (2)
- 161-161: The additional parameter
br.beskarConfig
is correctly passed to theregister
method call within thegossip.PluginInstance
case block, ensuring the new configuration is used when registering plugins.- 185-185: The
register
method is consistently called with the newbr.beskarConfig
parameter in another instance within thegossip.PluginInstance
case block.
Summary by CodeRabbit