add json to metadata response side support #29460
Conversation
Signed-off-by: cqi1217 <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: cqi1217 <[email protected]>
There was a problem hiding this comment.
Thanks!
Left a couple of minor API comments.
Assigning @JuniorHsu as code-owner.
/assign @JuniorHsu
| MatchRules request_rules = 1 [(validate.rules).message = {required: true}]; | ||
| MatchRules request_rules = 1; | ||
|
|
||
| // Rules to match json body of responses |
There was a problem hiding this comment.
nit: add '.' at the end of the sentence (also for the comment of request_rules)
api/envoy/extensions/filters/http/json_to_metadata/v3/json_to_metadata.proto
Show resolved
Hide resolved
|
/wait |
Signed-off-by: cqi1217 <[email protected]>
Signed-off-by: cqi1217 <[email protected]>
|
/retest |
Signed-off-by: cqi1217 <[email protected]>
Signed-off-by: cqi1217 <[email protected]>
| @@ -177,54 +191,68 @@ bool Filter::addMetadata(const std::string& meta_namespace, const std::string& k | |||
| } | |||
|
|
|||
| void Filter::finalizeDynamicMetadata(Http::StreamFilterCallbacks& filter_callback, | |||
| const StructMap& struct_map, bool& processing_finished_flag) { | |||
| const StructMap& struct_map, bool& processing_finished_flag, | |||
| bool shouldClearRouteCache) { | |||
There was a problem hiding this comment.
should_clear_route_cache here and there
| NiceMock<Envoy::StreamInfo::MockStreamInfo> stream_info_; | ||
| envoy::config::core::v3::Metadata dynamic_metadata_; | ||
| std::shared_ptr<FilterConfig> config_; | ||
| std::shared_ptr<Filter> filter_; | ||
| Buffer::OwnedImpl buffer_; | ||
| Http::TestRequestHeaderMapImpl incoming_headers_{ | ||
| {":path", "/ping"}, {":method", "GET"}, {"Content-Type", "application/json"}}; | ||
| Http::TestResponseHeaderMapImpl response_headers_{ | ||
| {":path", "/ping"}, {":method", "GET"}, {"Content-Type", "application/json"}}; |
There was a problem hiding this comment.
let's remove path/method and add status 200 to match common sense
| @@ -143,6 +173,30 @@ TEST_F(FilterTest, BasicStringMatch) { | |||
| EXPECT_EQ(getCounterValue("json_to_metadata.rq_invalid_json_body"), 0); | |||
| } | |||
|
|
|||
| TEST_F(FilterTest, BasicResponseStringMatch) { | |||
| initializeFilter(request_config_yaml_); | |||
Signed-off-by: cqi1217 <[email protected]>
Signed-off-by: cqi1217 <[email protected]>
|
try merge the main to see if ci passes? |
|
seems it is still failing |
|
|
||
| // Process the case without body, i.e., on_missing is applied for all rules. | ||
| void handleAllOnMissing(const Rules& rules, bool& processing_finished_flag); | ||
| void handleAllOnMissing(const Rules& rules, bool& processing_finished_flag, | ||
| Http::StreamFilterCallbacks& filter_callback, bool shouldClearRouteCache); |
There was a problem hiding this comment.
snake style in *.h file
also input variable should in front of output variable. processing_finished_flag and filter_callback are probably need to be after should_clear_route_cache
https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
Signed-off-by: cqi1217 <[email protected]>
Signed-off-by: cqi1217 <[email protected]>
|
@KBaichoo could you take a look? thanks |
|
sorry this wasn't assigned to Kevin so wouldn't show up on his PR reminders - fixed that today |
api/envoy/extensions/filters/http/json_to_metadata/v3/json_to_metadata.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: Cai Qi <[email protected]>
Signed-off-by: Cai Qi <[email protected]>
|
addressed comments |
Commit Message:
Currently the json to metadata filter can only extract value from request body, this PR adds the support to extract value from response body.
Related PR: #28394
Additional Description:
Risk Level:low
Testing:unit tests
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]