Skip to content
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.

Sign up for GitHub

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 RL response headers in correct phase #144

Merged
merged 2 commits into from
Nov 13, 2024
Merged

Add RL response headers in correct phase #144

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36a5c42
Add response headers in correct phase
adam-cattermole Nov 13, 2024
9f280cc
Return the response_headers to add at the filter level
adam-cattermole Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/filter/http_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,13 @@ impl Context for Filter {

match op_res {
Ok(operation) => {
if GrpcService::process_grpc_response(operation, resp_size).is_ok() {
if GrpcService::process_grpc_response(
operation,
resp_size,
&mut self.response_headers_to_add,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a code smell... this seems to invert the indirection between the state machine possibly progressing and the result of such a progress... Not a huge deal, I don't think this is even a first. "Just" mentioning it...

The way I think about it: something happens, this has a "result" (i.e. some output) and that output is dealt with. Which in this particular case would be adding the headers to this Vec, but the action knows nothing about that Vec, instead it merely returns the entries to add to a caller that then knows what to do with them.

Copy link
Member Author

@adam-cattermole adam-cattermole Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so you think it'd be preferable to have some kind of GrpcResult containing the response headers to let the filter deal with that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind for now... we probably would want to go to the drawing board at some point and design this a little more thoroughly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this.. I realise we would likely change a lot of things here, but how about this alternative in latest commit?

)
.is_ok()
{
// call the next op
match self.operation_dispatcher.borrow_mut().next() {
Ok(some_op) => {
Expand Down
9 changes: 6 additions & 3 deletions src/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ impl GrpcService {
pub fn process_grpc_response(
operation: Rc<Operation>,
resp_size: usize,
response_headers_to_add: &mut Vec<(String, String)>,
) -> Result<(), StatusCode> {
let failure_mode = operation.get_failure_mode();
if let Some(res_body_bytes) =
Expand All @@ -62,9 +63,11 @@ impl GrpcService {
match GrpcMessageResponse::new(operation.get_service_type(), &res_body_bytes) {
Ok(res) => match operation.get_service_type() {
ServiceType::Auth => AuthService::process_auth_grpc_response(res, failure_mode),
ServiceType::RateLimit => {
RateLimitService::process_ratelimit_grpc_response(res, failure_mode)
}
ServiceType::RateLimit => RateLimitService::process_ratelimit_grpc_response(
res,
failure_mode,
response_headers_to_add,
),
},
Err(e) => {
warn!(
Expand Down
12 changes: 4 additions & 8 deletions src/service/rate_limit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::service::GrpcService;
use log::warn;
use protobuf::{Message, RepeatedField};
use proxy_wasm::hostcalls;
use proxy_wasm::types::{Bytes, MapType};
use proxy_wasm::types::Bytes;

pub const RATELIMIT_SERVICE_NAME: &str = "envoy.service.ratelimit.v3.RateLimitService";
pub const RATELIMIT_METHOD_NAME: &str = "ShouldRateLimit";
Expand Down Expand Up @@ -38,6 +38,7 @@ impl RateLimitService {
pub fn process_ratelimit_grpc_response(
rl_resp: GrpcMessageResponse,
failure_mode: FailureMode,
response_headers_to_add: &mut Vec<(String, String)>,
) -> Result<(), StatusCode> {
match rl_resp {
GrpcMessageResponse::RateLimit(RateLimitResponse {
Expand Down Expand Up @@ -65,14 +66,9 @@ impl RateLimitService {
response_headers_to_add: additional_headers,
..
}) => {
// TODO: This should not be sent to the upstream!
additional_headers.iter().for_each(|header| {
hostcalls::add_map_value(
MapType::HttpResponseHeaders,
header.get_key(),
header.get_value(),
)
.unwrap()
response_headers_to_add
.push((header.get_key().to_owned(), header.get_value().to_owned()))
});
Ok(())
}
Expand Down
12 changes: 6 additions & 6 deletions tests/rate_limited.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,12 @@ fn it_passes_additional_headers() {
)
.expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer))
.returning(Some(&grpc_response))
.execute_and_expect(ReturnType::None)
.unwrap();

module
.call_proxy_on_response_headers(http_context, 0, false)
.expect_log(Some(LogLevel::Debug), Some("#2 on_http_response_headers"))
.expect_add_header_map_value(
Some(MapType::HttpResponseHeaders),
Some("test"),
Expand All @@ -363,12 +369,6 @@ fn it_passes_additional_headers() {
Some("other"),
Some("header value"),
)
.execute_and_expect(ReturnType::None)
.unwrap();

module
.call_proxy_on_response_headers(http_context, 0, false)
.expect_log(Some(LogLevel::Debug), Some("#2 on_http_response_headers"))
.execute_and_expect(ReturnType::Action(Action::Continue))
.unwrap();
}
Expand Down
Loading