-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
http: json_to_metadata filter #28394
Conversation
Signed-off-by: kuochunghsu <[email protected]>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
/wait-any |
Signed-off-by: kuochunghsu <[email protected]>
api/envoy/extensions/filters/http/json_to_metadata/v3/json_to_metadata.proto
Outdated
Show resolved
Hide resolved
api/envoy/extensions/filters/http/json_to_metadata/v3/json_to_metadata.proto
Outdated
Show resolved
Hide resolved
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.
Here's a first pass, thanks for working on this
/wait
api/envoy/extensions/filters/http/json_to_metadata/v3/json_to_metadata.proto
Outdated
Show resolved
Hide resolved
api/envoy/extensions/filters/http/json_to_metadata/v3/json_to_metadata.proto
Outdated
Show resolved
Hide resolved
api/envoy/extensions/filters/http/json_to_metadata/v3/json_to_metadata.proto
Outdated
Show resolved
Hide resolved
api/envoy/extensions/filters/http/json_to_metadata/v3/json_to_metadata.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
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.
Here's another pass
/wait
api/envoy/extensions/filters/http/json_to_metadata/v3/json_to_metadata.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
/lgtm api |
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
ci error is probably unrelated. would retest tmr
|
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.
Generally looks good besides few remaining points.
/assign-from @envoyproxy/senior-maintainers
EXPECT_EQ(0UL, test_server_->counter("json_to_metadata.rq_invalid_json_body")->value()); | ||
} | ||
|
||
TEST_P(JsonToMetadataIntegrationTest, GoOverWaterMarkBeforeEndStream) { |
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.
Test is failing in arm, and windows, looks legit:
[ RUN ] Protocols/JsonToMetadataIntegrationTest.GoOverWaterMarkBeforeEndStream/IPv4_HttpDownstream_HttpUpstreamHttpParserNghttp2NoDeferredProcessing
[2023-08-04 05:43:16.081][62580][critical][backtrace] [./source/server/backtrace.h:104] Caught Segmentation fault, suspect faulting address 0xbeb39f8000000010
[2023-08-04 05:43:16.081][62580][critical][backtrace] [./source/server/backtrace.h:91] Backtrace (use tools/stack_decode.py to get line numbers):
[2023-08-04 05:43:16.081][62580][critical][backtrace] [./source/server/backtrace.h:92] Envoy version: 0/1.28.0-dev/test/RELEASE/BoringSSL
[2023-08-04 05:43:16.081][62580][critical][backtrace] [./source/server/backtrace.h:96] #0: __kernel_rt_sigreturn [0xffffb8aad78c]
[2023-08-04 05:43:16.092][62580][critical][backtrace] [./source/server/backtrace.h:96] #1: Envoy::IntegrationCodecClient::sendData() [0xa8eb80]
[2023-08-04 05:43:16.103][62580][critical][backtrace] [./source/server/backtrace.h:96] #2: Envoy::(anonymous namespace)::JsonToMetadataIntegrationTest::runTest() [0xa6f160]
[2023-08-04 05:43:16.114][62580][critical][backtrace] [./source/server/backtrace.h:96] #3: Envoy::(anonymous namespace)::JsonToMetadataIntegrationTest_GoOverWaterMarkBeforeEndStream_Test::TestBody() [0xa72538]
[2023-08-04 05:43:16.125][62580][critical][backtrace] [./source/server/backtrace.h:96] #4: testing::internal::HandleExceptionsInMethodIfSupported<>() [0x1caa7b8]
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.
It crashes in the testing framework here with dangling raw point inner_encoder_
envoy/source/common/http/codec_wrappers.h
Line 82 in b1ea062
inner_encoder_->encodeData(data, end_stream); |
(gdb) p inner_encoder_
$4 = (Envoy::Http::RequestEncoder *) 0x2578bd512570
(gdb) info vtbl inner_encoder_
warning: can't find linker symbol for virtual table for `Envoy::Http::RequestEncoder' value
I suspect I abused the test framework :P
Moreover, I substitue the prepended filter by wait-for-whole-request-and-response-filter
to simulate the buffer behavior and it still crashes, which kinda proves the crash is not related to
json to metadata filter.
I'm going to remove the redundant test to move forward.
@envoyproxy/senior-maintainers assignee is @alyssawilk |
/wait |
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Can you take a look @RyanTheOptimist ? Thank you |
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.
Looks great!
/unassign @markdroth /assign @adisuissa For API review since Roth is out. |
@adisuissa FWIW roth did approve once but we need a fresh stamp. Thanks! |
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.
Following the approval here it is again:
/lgtm api
Commit Message:
This filter is configured with rules that will be matched against requests. Each rule has either a series of selectors and can be triggered either when the json attribute is present or missing.
When a rule is triggered, dynamic metadata will be added based on the configuration of the rule. If present, it’s value is extracted and used along with the specified key as metadata. If missing, on missing case is triggered and the value specified is used for adding metadata.
The metadata can then be used for load balancing decisions, consumed from logs, etc.
A typical use case for this filter is to dynamically match a specified json attribute of requests with rate limit. For this, a given json payload attribute’s value would be extracted and attached to the request as dynamic metadata which would then be used to match a rate limit action on metadata.
The Json to metadata filter stops iterating to next filter until we have the whole payload to extract the Json attributes or encounter error.
Additional Description:
Risk Level: low
Testing: unit
Docs Changes: new doc for filter
Release Notes: current.yaml
[Optional Fixes #Issue] #27645