-
Notifications
You must be signed in to change notification settings - Fork 5.5k
ext_authz: added ability to detect partial request body data #6583
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
Changes from 10 commits
7236a7a
7cfd880
fbeba65
944ff2b
12e3e10
d69fceb
9a2f45b
950e7cf
6e196c2
cebcc3c
2b3b7dd
20a3ce9
31a2079
6f1f781
568b9c7
9d3255a
d2f0bc4
3893170
b1ce1f6
7507afe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,12 +107,15 @@ void CheckRequestUtils::setHttpRequest( | |
| auto mutable_headers = httpreq.mutable_headers(); | ||
| headers.iterate( | ||
| [](const Envoy::Http::HeaderEntry& e, void* ctx) { | ||
| Envoy::Protobuf::Map<Envoy::ProtobufTypes::String, Envoy::ProtobufTypes::String>* | ||
| mutable_headers = static_cast< | ||
| Envoy::Protobuf::Map<Envoy::ProtobufTypes::String, Envoy::ProtobufTypes::String>*>( | ||
| ctx); | ||
| (*mutable_headers)[std::string(e.key().getStringView())] = | ||
| std::string(e.value().getStringView()); | ||
| // Skip any client EnvoyAuthPartialBody header, which could interfere with internal use. | ||
| if (e.key().getStringView() != Http::Headers::get().EnvoyAuthPartialBody.get()) { | ||
| Envoy::Protobuf::Map<Envoy::ProtobufTypes::String, Envoy::ProtobufTypes::String>* | ||
| mutable_headers = | ||
| static_cast<Envoy::Protobuf::Map<Envoy::ProtobufTypes::String, | ||
| Envoy::ProtobufTypes::String>*>(ctx); | ||
| (*mutable_headers)[std::string(e.key().getStringView())] = | ||
| std::string(e.value().getStringView()); | ||
| } | ||
| return Envoy::Http::HeaderMap::Iterate::Continue; | ||
| }, | ||
| mutable_headers); | ||
|
|
@@ -124,6 +127,10 @@ void CheckRequestUtils::setHttpRequest( | |
| std::string data(length, 0); | ||
| buffer->copyOut(0, length, &data[0]); | ||
| httpreq.set_body(std::move(data)); | ||
|
|
||
| // Add in a header to detect when a partial body is used. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may also want to check that this header does not exist when the request is not a partial message. WDYT?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not exactly sure what you mean here. Only send the header if the body was actually truncated? Or only send the header if the The metadata header is currently only there if Now, if you mean only write the header if the config option If you think that the metadata header may get in people's way when
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if the client request contains
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I'll skip that header during the copy. |
||
| (*mutable_headers)[Http::Headers::get().EnvoyAuthPartialBody.get()] = | ||
| length != buffer->length() ? "1" : "0"; | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
Personally I would do "true" and "false" here vs. "0" and "1" to clearly indicate boolean, but I don't feel strongly about it. WDYT? A more general question would be why use a header at all vs. adding a boolean field to the request proto?
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.
I suggested using headers here for a couple of reasons. Please see #6583 (comment) for details.
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.
@mattklein123
true|falsevs0|1- I have no preference. The bool values are much clearer, so I can change that.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.
If you have all already discussed headers vs. field and determined header is better that's fine with me, but yeah let's change to true/false if you don't mind. Thank you!
/wait
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.
@mattklein123 changed. Thanks.