Skip to content

Conversation

@sumitkmr2
Copy link
Contributor

Commit Message: Add UTs for array type in FieldChecker.
Additional Description: This is the first part of support for non-primitive types. Enums, maps and Any type will follow. For arrays, there's no additional change needed in the CheckField() method. The field paths for array's child fields would be configured just like simple message's child fields and the proto_scrubber library also passes the path argument to CheckField just like simple message. ProtoApiScrubber would not support index level scrubbing i.e., the scrubbing rules in the filter config can't be defined array index wise. e.g., for a field request.api_keys_list[*].current_key would be configured as request.api_keys_list.current_key.
Risk Level: None
Testing: UTs are added.
Docs Changes: None for now. The documentation (once published) will contain guidelines for configuring array types.
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
Copy link

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

see: more, trace.

Signed-off-by: Sumit Kumar <sumitkmr@google.com>
@sumitkmr2 sumitkmr2 marked this pull request as ready for review November 22, 2025 15:37
@sumitkmr2 sumitkmr2 requested a review from adisuissa as a code owner November 22, 2025 15:37
@adisuissa adisuissa self-assigned this Nov 24, 2025
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks.
I've left a couple of style comments that can be addressed in a followup PR.

addRestriction(config, method, "metadata.history.edits", FieldType::Request, true);

// Repeated Message: "chapters" -> No Rule (Should result in Partial to scrub children)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove unnecessary empty lines.

ProtoApiScrubberConfig config;
std::string method = "/library.BookService/UpdateBook";

// Top-level repeated primitive: "tags" -> Remove
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style-nit: comments start with a capital letter and end with '.'

@adisuissa adisuissa merged commit 7dd8848 into envoyproxy:main Nov 24, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants