[ProtoApiScrubber] Add Integration Tests by sumitkmr2 · Pull Request #42121 · envoyproxy/envoy

sumitkmr2 · 2025-11-19T13:18:39Z

Commit Message: Add Integration tests.
Additional Description: Currently, the tests are added using envoy_cc_extension as envoy_extension_cc_test fails to recognize the extension as it's not registered. Once the filter's core functionality is complete, we'll do the registration and update the build rule. Currently, only request integration tests are added as response flow is not ready. Response ITs will be added once response flow code is checked-in.
Risk Level: None.
Testing: 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>

repokitteh-read-only · 2025-11-19T13:18:44Z

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: #42121 was opened by sumitkmr2.

see: more, trace.

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

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

sumitkmr2 · 2025-11-20T17:00:39Z

/retest

adisuissa

Thanks, overall LGTM.
Left a few minor comments.

adisuissa · 2025-11-21T13:50:18Z

+                                          1: 0
+)EOF";
+
+class ProtoApiScrubberIntegrationTest : public HttpProtocolIntegrationTest {


What's the reason for extending HttpProtocolIntegrationTest and not HttpIntegrationTest? Does it require AsyncLB?

Actually, I'm not sure about the exact reason. I followed proto_message_extraction and grpc_field_extraction filters and both use HttpProtocolIntegrationTest. In my view, I think it's better for extensibility when we add support for non-grpc requests carrying protobuf payloads as well. In that case, tests can easily be extended with a one-line change. Let me know if you think this is too-much future proofing and needs to be removed.

adisuissa · 2025-11-21T13:57:21Z

+                          typed_config:
+                            "@type": type.googleapis.com/xds.type.matcher.v3.CelMatcher
+                            expr_match:
+                              {2}


Can the expr_match be added in a non copy-paste method?
The reason is that it now depends on the spaces in the config, which makes it brittle to maintain.

Done. Added in a json format so that indentation is not a problem. Tried doing in a builder fashion but that wasn't much readable (i.e., one can't figure out easily the config while looking at the builder). Hope that json format is ok.

adisuissa · 2025-11-21T13:57:37Z

+  descriptor_set:
+    data_source:
+      filename: {0}
+{1}


Same as expr_match above.

adisuissa · 2025-11-21T14:01:01Z

+      restrictions_yaml = fmt::format(
+          R"EOF(
+  restrictions:
+    method_restrictions:


Does message_restrictions also need integrations tests?
Also does the method_restriction need to be validated?

Does message_restrictions also need integrations tests?

Yes, it does. @pranavrautgoogle is working on it and the PR will be out once this PR gets merged.

Also does the method_restriction need to be validated?

Yes, it is already getting validated in the filter_config.cc as of today.

adisuissa · 2025-11-21T14:02:48Z

+// TEST GROUP 1: PASS THROUGH
+// ============================================================================
+
+TEST_P(ProtoApiScrubberIntegrationTest, UnaryRequestPassesThrough) {


Please add a one-liner comment describing what the goal of each test.

adisuissa · 2025-11-21T14:09:19Z

+  ASSERT_TRUE(response->waitForEndStream());
+}
+
+TEST_P(ProtoApiScrubberIntegrationTest, ScrubNestedField) {


Please add a test that attempts to scrub a field that is not found in the data.

By not found in the data., do you mean not found in the descriptors? eg, a newer client library passing a field to an old envoy build? If yes, that case is not handled yet in the code. Planning to add that in next few days. The expectation in that case is that it passes through without scrubbing. I'll add the tests along with it in the UTs itself.

If you mean that it's not found in the filter config (ie, no restrictions configured for the field), then that case is already covered in the UTs and it passes through without scrubbing.

adisuissa · 2025-11-21T14:14:00Z

+      getFilterConfig(apikeysDescriptorPath(), kCreateApiKeyMethod, "key.display_name"));
+  initialize();
+
+  auto original_proto = makeCreateApiKeyRequest(R"pb(


Will it be also useful to test the following cases:

more than 1 key exists (say another_name in addition to display_name) and only one (display_name) is scrubbed?

not sure if it's valid but a case where there's a double-nested field with the same name key { key: { display_name: "sensitive" } }.

These may already be addressed by the unit tests, and in that case, that should be sufficient.

The first one is already covered in the field checker UT.
The second one is interesting and is not covered anywhere, thanks for pointing out! It's a valid case - the field path for respective fields would be key, key.key and key.key.display_name. It's not possible to repro with the ApiKeys service that we are using currently for testing here in IT. I'll that case in the field checker UT where it is much simpler to repro that case. SG?

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

sumitkmr2

Addressed comments, thanks! Will add the requested tests in the field checker UT in a separate PR to avoid conflicts as there are a lot of pending changes going on by myself and Pranav so trying to keep the scope of each PR limited to as less number of files as possible.

sumitkmr2 · 2025-11-23T06:58:42Z

+      restrictions_yaml = fmt::format(
+          R"EOF(
+  restrictions:
+    method_restrictions:


Does message_restrictions also need integrations tests?

Yes, it does. @pranavrautgoogle is working on it and the PR will be out once this PR gets merged.

Also does the method_restriction need to be validated?

Yes, it is already getting validated in the filter_config.cc as of today.

sumitkmr2 · 2025-11-23T10:23:23Z

+                          typed_config:
+                            "@type": type.googleapis.com/xds.type.matcher.v3.CelMatcher
+                            expr_match:
+                              {2}


Done. Added in a json format so that indentation is not a problem. Tried doing in a builder fashion but that wasn't much readable (i.e., one can't figure out easily the config while looking at the builder). Hope that json format is ok.

sumitkmr2 · 2025-11-23T10:23:29Z

+  descriptor_set:
+    data_source:
+      filename: {0}
+{1}


sumitkmr2 · 2025-11-23T10:54:22Z

+// TEST GROUP 1: PASS THROUGH
+// ============================================================================
+
+TEST_P(ProtoApiScrubberIntegrationTest, UnaryRequestPassesThrough) {


sumitkmr2 · 2025-11-23T11:05:44Z

+  ASSERT_TRUE(response->waitForEndStream());
+}
+
+TEST_P(ProtoApiScrubberIntegrationTest, ScrubNestedField) {


By not found in the data., do you mean not found in the descriptors? eg, a newer client library passing a field to an old envoy build? If yes, that case is not handled yet in the code. Planning to add that in next few days. The expectation in that case is that it passes through without scrubbing. I'll add the tests along with it in the UTs itself.

If you mean that it's not found in the filter config (ie, no restrictions configured for the field), then that case is already covered in the UTs and it passes through without scrubbing.

sumitkmr2 · 2025-11-23T11:25:19Z

+      getFilterConfig(apikeysDescriptorPath(), kCreateApiKeyMethod, "key.display_name"));
+  initialize();
+
+  auto original_proto = makeCreateApiKeyRequest(R"pb(


The first one is already covered in the field checker UT.
The second one is interesting and is not covered anywhere, thanks for pointing out! It's a valid case - the field path for respective fields would be key, key.key and key.key.display_name. It's not possible to repro with the ApiKeys service that we are using currently for testing here in IT. I'll that case in the field checker UT where it is much simpler to repro that case. SG?

sumitkmr2 · 2025-11-23T11:30:22Z

+                                          1: 0
+)EOF";
+
+class ProtoApiScrubberIntegrationTest : public HttpProtocolIntegrationTest {


Actually, I'm not sure about the exact reason. I followed proto_message_extraction and grpc_field_extraction filters and both use HttpProtocolIntegrationTest. In my view, I think it's better for extensibility when we add support for non-grpc requests carrying protobuf payloads as well. In that case, tests can easily be extended with a one-line change. Let me know if you think this is too-much future proofing and needs to be removed.

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

adisuissa

Thanks, overall LGTM.
Can you please run this under tsan with runs_per_test=1000 to reduce the chances of introducing flaky tests?

adisuissa · 2025-11-24T14:15:11Z

+const std::string kCreateApiKeyMethod = "/apikeys.ApiKeys/CreateApiKey";
+
+// CEL Matcher Config (Protobuf Text Format) which evaluates to TRUE.
+const std::string kCelAlwaysTrue = R"pb(


nit: preffer constexpr absl::string_view

adisuissa · 2025-11-24T14:17:57Z

+
+      // Format the full config.
+      full_config_text = fmt::format(R"pb(
+      filtering_mode: OVERRIDE


nit: use yaml instead of this (no need for double curly brackets everywhere, and makes the code a bit more concise - example).

Yeah, it was in yaml before but changed it to json due to spacing constraints while doing replacement of parameters {0}, {1}, etc. as discussed here.
In json, spacing doesn't matter so it's not brittle now.

Another approach is using a different approach than format replace.
What about using a format replacement for simple elements (strings, numbers), and using the protobuf methods (e.g., PackFrom()) for non-simple elements?

I've refactored the code to make it more configurable and readable at the same time. Let me know if this looks better now. Also, I'll be adding more Integration tests with different input types and different CEL expressions, etc. in follow-up CLs.

I think we are back to square 1 with this, but as you will be the codeowner on this and it is only test code, then I can live with that.

adisuissa · 2025-11-24T14:19:01Z

+  EXPECT_EQ("3", grpc_status->value().getStringView()); // 3 = Invalid Argument
+}
+
+INSTANTIATE_TEST_SUITE_P(Protocols, ProtoApiScrubberIntegrationTest,


style-nit: please move right after the class declaration.

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

sumitkmr2

Addressed comments, thanks!
Also, ran the following command to test flakiness:

envoy$ bazel test   --config=tsan   --runs_per_test=1000   --cache_test_results=no   --test_output=errors    //test/extensions/filters/http/proto_api_scrubber:integration_test
INFO: Invocation ID: d9aed359-0255-4f37-964a-ce8f86721e2f
INFO: Analyzed target //test/extensions/filters/http/proto_api_scrubber:integration_test (0 packages loaded, 20 targets configured).
INFO: Found 1 test target...
Target //test/extensions/filters/http/proto_api_scrubber:integration_test up-to-date:
  *****/execroot/envoy/bazel-out/k8-fastbuild/bin/test/extensions/filters/http/proto_api_scrubber/integration_test
INFO: Elapsed time: 786.311s, Critical Path: 114.53s
INFO: 1001 processes: 1 internal, 1000 linux-sandbox.
INFO: Build completed successfully, 1001 total actions

Executed 1 out of 1 test: 1 test passes.

sumitkmr2 · 2025-11-25T05:14:27Z

+
+      // Format the full config.
+      full_config_text = fmt::format(R"pb(
+      filtering_mode: OVERRIDE


Yeah, it was in yaml before but changed it to json due to spacing constraints while doing replacement of parameters {0}, {1}, etc. as discussed here.
In json, spacing doesn't matter so it's not brittle now.

sumitkmr2 · 2025-11-25T06:26:15Z

+  EXPECT_EQ("3", grpc_status->value().getStringView()); // 3 = Invalid Argument
+}
+
+INSTANTIATE_TEST_SUITE_P(Protocols, ProtoApiScrubberIntegrationTest,


sumitkmr2 · 2025-11-25T06:26:52Z

+const std::string kCreateApiKeyMethod = "/apikeys.ApiKeys/CreateApiKey";
+
+// CEL Matcher Config (Protobuf Text Format) which evaluates to TRUE.
+const std::string kCelAlwaysTrue = R"pb(


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

sumitkmr2

Addressed pending comment.

sumitkmr2 · 2025-11-26T16:45:18Z

+
+      // Format the full config.
+      full_config_text = fmt::format(R"pb(
+      filtering_mode: OVERRIDE


I've refactored the code to make it more configurable and readable at the same time. Let me know if this looks better now. Also, I'll be adding more Integration tests with different input types and different CEL expressions, etc. in follow-up CLs.

adisuissa

Thanks, merging.

adisuissa · 2025-12-02T16:20:53Z

+
+      // Format the full config.
+      full_config_text = fmt::format(R"pb(
+      filtering_mode: OVERRIDE


I think we are back to square 1 with this, but as you will be the codeowner on this and it is only test code, then I can live with that.

adisuissa · 2025-12-02T16:21:38Z

+      getFilterConfig(apikeysDescriptorPath(), kCreateApiKeyMethod, "key.display_name"));
+  initialize();
+
+  auto original_proto = makeCreateApiKeyRequest(R"pb(


Signed-off-by: Sumit Kumar <sumitkmr@google.com> Signed-off-by: sumitkmr2 <sumitkmr@google.com> Signed-off-by: Gustavo <grnmeira@gmail.com>

sumitkmr2 added 6 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>

sumitkmr2 changed the title It [ProtoApiScrubber] Add Integration Tests Nov 19, 2025

Fix indentation

208b17c

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

sumitkmr2 marked this pull request as ready for review November 19, 2025 14:27

sumitkmr2 requested a review from adisuissa as a code owner November 19, 2025 14:27

sumitkmr2 mentioned this pull request Nov 20, 2025

[ProtoApiScrubber] Add response scrubber to the filter. #42145

Merged

Merge branch 'main' into it

3d79c3b

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

adisuissa reviewed Nov 21, 2025

View reviewed changes

wbpcode assigned adisuissa Nov 23, 2025

Addressing comments

9b4ee08

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

sumitkmr2 commented Nov 23, 2025

View reviewed changes

sumitkmr2 added 4 commits November 24, 2025 08:10

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>

adisuissa reviewed Nov 24, 2025

View reviewed changes

Address comments

78425a9

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

sumitkmr2 commented Nov 25, 2025

View reviewed changes

Address comments

0340ac6

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

sumitkmr2 commented Nov 26, 2025

View reviewed changes

sumitkmr2 mentioned this pull request Nov 26, 2025

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

Merged

adisuissa approved these changes Dec 2, 2025

View reviewed changes

adisuissa merged commit 5aecf40 into envoyproxy:main Dec 2, 2025
24 checks passed

Conversation

sumitkmr2 commented Nov 19, 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 19, 2025

Uh oh!

sumitkmr2 commented Nov 20, 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!

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!

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!

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!

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!

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

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

sumitkmr2 commented Nov 19, 2025 •

edited

Loading

sumitkmr2 Nov 26, 2025 •

edited

Loading

sumitkmr2 Nov 26, 2025 •

edited

Loading