Limit picod request body size for prevent DoS attack#326
Conversation
|
@RainbowMango: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Welcome @RainbowMango! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
There was a problem hiding this comment.
Pull request overview
This PR updates PicoD’s Gin server setup to enforce a maximum HTTP request body size globally (not only for authenticated routes), reducing DoS risk from oversized payloads.
Changes:
- Add a global Gin middleware that wraps
c.Request.Bodywithhttp.MaxBytesReader(..., MaxBodySize)and setengine.MaxMultipartMemory = MaxBodySize. - Remove the request body size enforcement from
AuthMiddleware, keeping auth focused on JWT verification.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/picod/server.go | Adds global request body size limiting middleware and aligns multipart parsing memory cap with MaxBodySize. |
| pkg/picod/auth.go | Removes body-size enforcement from authentication middleware. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request moves the request body size limit enforcement from the authentication middleware to a global middleware in the server setup. Feedback includes a request to remove redundant blank lines in auth.go, a suggestion to generalize a comment in server.go to avoid hardcoded values, and a recommendation to lower MaxMultipartMemory to prevent high memory consumption during multipart uploads.
Signed-off-by: RainbowMango <qdurenhongcai@gmail.com>
RainbowMango
force-pushed
the
pr_limit_picod_request_body
branch
from
5d37153 to
0322b15
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #326 +/- ##
==========================================
+ Coverage 47.57% 47.92% +0.35%
==========================================
Files 30 30
Lines 2819 2865 +46
==========================================
+ Hits 1341 1373 +32
- Misses 1338 1344 +6
- Partials 140 148 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
@FAUST-BENCHOU: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm |
What type of PR is this?
/kind cleanup
/kind security
What this PR does / why we need it:
This pull request refactors how the maximum request body size is enforced in the application. The body size limit logic has been moved from the authentication middleware to a new global middleware, ensuring that all incoming requests are protected from large payloads, not just those passing through authentication.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This PR is split from #250 to address the comments of #250 (comment).
Does this PR introduce a user-facing change?: