[ProtoApiScrubber] Fixing header and trailer propagation in field checker by sumitkmr2 · Pull Request #42293 · envoyproxy/envoy

sumitkmr2 · 2025-11-27T14:29:01Z

Commit Message: Fixing header and trailer propagation in field checker
Additional Description: The headers and trailers were not properly propagated to the field checker and hence, the match tree couldn't use these inputs. Rest of the envoy attributes (https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes) are automatically available for matching and hence, additional plumbing is not required.
Risk Level: None
Testing: UTs and ITs added.
Docs Changes: None.
Release Notes: None.
Platform Specific Features: None.
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Signed-off-by: sumitkmr2 <sumitkmr@google.com>

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

repokitteh-read-only · 2025-11-27T14:29:07Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #42293 was opened by sumitkmr2.

see: more, trace.

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

sumitkmr2 · 2025-12-12T11:06:28Z

/retest

sumitkmr2 · 2025-12-12T11:55:25Z

@adisuissa I'm working on fixing the Envoy/Check failure. It's taking time as the tests pass but the envoy step fails. Request you to please review the code and test changes in the meantime.

phlax · 2025-12-12T12:47:16Z

@sumitkmr2 gcc failure looks real (even if flaking)

adisuissa

Thanks, overall LGTM

adisuissa · 2025-12-15T19:40:02Z

  FieldChecker(const ScrubberContext scrubber_context,
-               const Envoy::StreamInfo::StreamInfo* stream_info, const std::string& method_name,
+               const Envoy::StreamInfo::StreamInfo* stream_info,
+               const Http::RequestHeaderMap* request_headers,


OOC why is there a need to move between OptRef and pointers (for al the headers maps)?

The use of pointers here was intended to handle the optional nature of the headers and trailers, as the matching_data_ setters require valid references (so I needed a way to check for existence before dereferencing). I used pointers as a mechanism for nullable checks.

However, I agree that passing OptRef directly is more idiomatic for Envoy and avoids converting between types. I have updated the constructor signature to accept OptRef instead of raw pointers.

adisuissa · 2025-12-15T19:47:42Z

 public:
  // Validates whether the input type for the matcher is in the list of supported input types.
-  // Currently, only CEL input type (i.e., HttpAttributesCelMatchInput) is supported.
+  // ProtoApiScrubber filter supports all types of data inputs and hence, it returns


Interesting. So what will the behavior (during a request processing by the ProtoApiScrubber filter) be if the matcher type is DestinationIPInput?

It would result in a no-match. This is documented nicely in matching API documentation but I couldn't find it as of now. Right now, I can only find this: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/matching/matching_api#inputs-and-matching-algorithms
I'll try to find the documentation which clearly stated that if you use response type inputs in the request flow, it would result in a no-match.
In essence, if someone is using matching API, we can be assured that they are aware of such nuances as it's clearly documented in envoy documentation.

adisuissa · 2025-12-15T19:51:31Z

      : scrubber_context_(scrubber_context), matching_data_(*stream_info),
-        method_name_(method_name), filter_config_ptr_(filter_config) {}
+        method_name_(method_name), filter_config_ptr_(filter_config) {
+    // Populate matching data with whatever is available.


FWIW, the design here seems a bit leaky (a single field checker for both request and response, but used differently for each). I guess that is ok for now, but if further divergence from behavior needs to be done for request/response, it may be best to divide the two paths to different classes.

Yes, you are right. Although around 90-95% of the code is common for request and response, there's one part where they diverge. I'll keep that in mind in case that divergence becomes bigger but in principle, it would most probably not diverge as it's core job is to do scrubbing given a protobuf message. In case, the divergence becomes bigger, we might try to make it configurable instead.

Also, please note that it's not like request headers are only for request path, request headers can very well be utilized by response path.

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

sumitkmr2

Addressed comments ,thanks!

sumitkmr2 · 2025-12-16T14:50:13Z

      : scrubber_context_(scrubber_context), matching_data_(*stream_info),
-        method_name_(method_name), filter_config_ptr_(filter_config) {}
+        method_name_(method_name), filter_config_ptr_(filter_config) {
+    // Populate matching data with whatever is available.


Yes, you are right. Although around 90-95% of the code is common for request and response, there's one part where they diverge. I'll keep that in mind in case that divergence becomes bigger but in principle, it would most probably not diverge as it's core job is to do scrubbing given a protobuf message. In case, the divergence becomes bigger, we might try to make it configurable instead.

Also, please note that it's not like request headers are only for request path, request headers can very well be utilized by response path.

sumitkmr2 · 2025-12-16T14:55:57Z

 public:
  // Validates whether the input type for the matcher is in the list of supported input types.
-  // Currently, only CEL input type (i.e., HttpAttributesCelMatchInput) is supported.
+  // ProtoApiScrubber filter supports all types of data inputs and hence, it returns


It would result in a no-match. This is documented nicely in matching API documentation but I couldn't find it as of now. Right now, I can only find this: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/matching/matching_api#inputs-and-matching-algorithms
I'll try to find the documentation which clearly stated that if you use response type inputs in the request flow, it would result in a no-match.
In essence, if someone is using matching API, we can be assured that they are aware of such nuances as it's clearly documented in envoy documentation.

sumitkmr2 · 2025-12-16T15:05:18Z

  FieldChecker(const ScrubberContext scrubber_context,
-               const Envoy::StreamInfo::StreamInfo* stream_info, const std::string& method_name,
+               const Envoy::StreamInfo::StreamInfo* stream_info,
+               const Http::RequestHeaderMap* request_headers,


The use of pointers here was intended to handle the optional nature of the headers and trailers, as the matching_data_ setters require valid references (so I needed a way to check for existence before dereferencing). I used pointers as a mechanism for nullable checks.

However, I agree that passing OptRef directly is more idiomatic for Envoy and avoids converting between types. I have updated the constructor signature to accept OptRef instead of raw pointers.

Signed-off-by: sumitkmr2 <sumitkmr@google.com>

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

sumitkmr2 · 2025-12-17T10:36:57Z

@sumitkmr2 gcc failure looks real (even if flaking)

@phlax Yes, they were real failures caused by improper test cleanup. I've fixed those now.

adisuissa

LGTM

…cker (envoyproxy#42293) Signed-off-by: Sumit Kumar <sumitkmr@google.com> Signed-off-by: sumitkmr2 <sumitkmr@google.com> Signed-off-by: Xuyang Tao <taoxuy@google.com>

…cker (envoyproxy#42293) Signed-off-by: Sumit Kumar <sumitkmr@google.com> Signed-off-by: sumitkmr2 <sumitkmr@google.com> Signed-off-by: Gustavo <grnmeira@gmail.com>

sumitkmr2 added 20 commits November 18, 2025 11:21

Add request scrubbing logic in filter

282a0bf

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Minor change

7bcab92

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Address comments

3183c11

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Add UTs for method validation

deaeb65

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Format fixes

e2ec8d6

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Add Integration Tests

ce62547

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Fix indentation

208b17c

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Merge branch 'main' into it

3d79c3b

Signed-off-by: sumitkmr2 <sumitkmr@google.com>

Addressing comments

9b4ee08

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Try fixing tests

5e8a288

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Format fixes

86b5b2f

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Try fixing test

67d3b30

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Fix test

ab59d36

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Address comments

78425a9

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Address comments

0340ac6

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Allow generic matchers

410dfd7

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

rename

36ba1bb

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Merge branch 'main' into it

f96906f

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Merge branch 'match' into it2

3de1ba4

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Fixing header and trailer propagation in field checker

5476bf1

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Build fix

bd595a7

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

sumitkmr2 mentioned this pull request Dec 12, 2025

[ProtoApiScrubber] Allow generic matchers to be configured in filter config. #42276

Merged

sumitkmr2 added 3 commits December 12, 2025 07:35

Merge main

019ab93

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Increase coverage

0757fe2

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Fix typo

5558951

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

sumitkmr2 changed the title ~~Fixing header and trailer propagation in field checker~~ [ProtoApiScrubber] Fixing header and trailer propagation in field checker Dec 12, 2025

sumitkmr2 marked this pull request as ready for review December 12, 2025 11:54

sumitkmr2 requested a review from adisuissa as a code owner December 12, 2025 11:54

phlax assigned adisuissa Dec 12, 2025

adisuissa reviewed Dec 15, 2025

View reviewed changes

Addressing comments

6b8fb31

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

sumitkmr2 commented Dec 16, 2025

View reviewed changes

sumitkmr2 added 6 commits December 17, 2025 09:54

Merge branch 'main' into it2

6b78100

Signed-off-by: sumitkmr2 <sumitkmr@google.com>

Fix test

3807dad

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Fix gcc test failure due to race condition

017fce3

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Comment fix

8455157

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Try fixing gcc failure

b3b4853

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Try fixing gcc failure

2a9177f

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

adisuissa approved these changes Dec 17, 2025

View reviewed changes

adisuissa merged commit a7d4e04 into envoyproxy:main Dec 17, 2025
25 checks passed

Conversation

sumitkmr2 commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Nov 27, 2025

Uh oh!

sumitkmr2 commented Dec 12, 2025

Uh oh!

sumitkmr2 commented Dec 12, 2025

Uh oh!

phlax commented Dec 12, 2025

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumitkmr2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumitkmr2 commented Dec 17, 2025

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sumitkmr2 commented Nov 27, 2025 •

edited

Loading