-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_http_server: add dynamic config size validation #12438
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
base: arni/gateway_http_server/max_request_body_size_test
Are you sure you want to change the base?
apollo_http_server: add dynamic config size validation #12438
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
TzahiTaub
left a comment
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.
@TzahiTaub reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware).
crates/apollo_http_server_config/src/config.rs line 140 at r1 (raw file):
} Ok(()) }
As discussed, please unify. The validation here is sufficient.
Suggestion:
fn max_size_validations(http_server_config: &HttpServerConfig) -> Result<(), ValidationError> {
let max_request_body_size = http_server_config.static_config.max_request_body_size;
validate_dynamic_config_bounds(&http_server_config.dynamic_config, max_request_body_size)
}
pub fn validate_dynamic_config_bounds(
dynamic_config: &HttpServerDynamicConfig,
max_request_body_size: usize,
) -> Result<(), ValidationError> {
if max_request_body_size <= dynamic_config.max_sierra_program_size {
return Err(ValidationError::new(
"max_request_body_size must be greater than max_sierra_program_size",
));
}
Ok(())
}crates/apollo_http_server/src/http_server.rs line 0 at r1 (raw file):
Revert changes in this file
a9eb32e to
108c567
Compare
707b7e5 to
04c835d
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
a3f16e7 to
4961fee
Compare
108c567 to
f091655
Compare
4961fee to
e2088c0
Compare
|
Previously, TzahiTaub (Tzahi) wrote…
Done. |
e2088c0 to
60de1e9
Compare
ArniStarkware
left a comment
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.
@ArniStarkware made 1 comment.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @TzahiTaub).
crates/apollo_http_server_config/src/config.rs line 140 at r1 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
As discussed, please unify. The validation here is sufficient.
Done.

Note
Low Risk
Small, self-contained config validation and test changes; risk is limited to rejecting previously-accepted invalid configurations at startup/validation time.
Overview
Adds a cross-field config validation on
HttpServerConfigto ensurestatic_config.max_request_body_sizeis greater thandynamic_config.max_sierra_program_size, returning aValidationErrorwhen violated.Introduces a focused unit test (
config_test.rs) and wires it into the crate’s test module setup to prevent regressions.Written by Cursor Bugbot for commit 60de1e9. This will update automatically on new commits. Configure here.