json: Add protobuf wrapper for streamer by filipcacky · Pull Request #47088 · envoyproxy/envoy

filipcacky · 2026-08-31T15:32:45Z

Commit Message: json: Add protobuf wrapper for streamer
Additional Description:

Adds Envoy::Json::MessageStreamer, which wraps a BufferStreamer::Level and serializes a Protobuf::Message by walking it with reflection.

The MessageStreamer supports redacting fields annotated with [(udpa.annotations.sensitive) = true] with the same output format as MessageUtil::redact.

Risk Level: Low
Testing: Added unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

repokitteh-read-only · 2026-08-31T15:32:52Z

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: #47088 was opened by filipcacky.

see: more, trace.

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

filipcacky · 2026-08-31T15:53:32Z

#46911 (comment)

Added emitMapEntry.

#46911 (comment)

Added struct Options and renamed to preserve_proto_field_names, consistent with Protobuf::util::JsonPrintOptions.

#46911 (comment)

Added examples for all types to config and cleaned up the redactions a bit, since they were all over the place. I'm thinking the redactions should probably copy over Redact test cases from test/common/protobuf/utility_test.cc.

cc @jmarantz

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

jmarantz

This looks great! Mostly minor nits and requests for more comments.

/wait

jmarantz · 2026-09-01T14:35:39Z

+public:
+  struct Options {
+    // Whether to emit a leading @type naming the message.
+    bool emit_type_url = false;


Not putting the "_" suffix on struct member variables is consistent with the Google style guide.

However the Envoy style guide calls this out as an explicit deviation: https://github.com/envoyproxy/envoy/blob/main/STYLE.md#deviations-from-google-c-style-guidelines

I raised #47030 few days ago specifically because of this :D Apparently both styles are acceptable. I dislike the suffix on public interfaces, but I'll fix. I suppose its better than complaining about it in an issue and then doing the same x)

It's reasonable to object to the style guide. I happen to agree with you stylistically.

However that discussion should be in the form of a PR to change the style guide, without other functional changes :)

jmarantz · 2026-09-01T14:36:09Z

+    // Whether the keys are the proto field names or the lowerCamelCase ProtoJSON defaults to.
+    // https://protobuf.dev/programming-guides/json/#field-names
+    bool preserve_proto_field_names = false;
+    // Whether the fields the API marks sensitive are emitted, or replaced the way


optional nit: add blank link betweeen option member variables and their doc.

jmarantz · 2026-09-01T14:42:47Z

+                        bool ancestor_is_sensitive);
+
+  // Emits the redacted form of `field`'s value, a replacement for text and the type's default for
+  // anything else. Messages are handled by walking them, not redacting as whole.


I think I understand, but maybe it's worthwhile expanding on this a little.

e.g.

Leaf messages that are redacted are handled directly here. Messages with hiearchy (repeated fields and maps) are recursed fully, and the redactions occur only at leaves.

Is that right?

Yep! Fixed.

jmarantz · 2026-09-01T14:44:13Z

+    bool field_is_sensitive_{false};
+  };
+
+  void nextElement(Frame& frame);


Add brief doc for nextElement. I think for nextElement and startField I'd like to know from looking at this header if it's also managing a stack owned by 'this'. Maybe call that out?

I assume it is, otherwise nextElement would be 'const'.

Note that these are both non-const methods and they take a mutable Frame objects. Can you just dive into the semantics a little more?

Added comment and renamed to emitNextElement/emitNextField, also realized that next_field_ was interpreted as "current field" during nextElement, so i cleaned that up a bit too.

Both are entry points to emit Something, which may push on the stack, emitNextElement just handles repeated fields and maps.

jmarantz · 2026-09-01T14:45:29Z

+                         BufferStreamer::Level& level);
+
+  // Emits one value of `field`, or pushes a frame for it. `index` is which element of a repeated
+  // field to emit, or -1 for a field that is not repeated.


WDYT of making a named constant for -1 here?

Changed to std::optional, emitValue then doesn't need the same comment about the sentinel and IMO looks nicer.

jmarantz · 2026-09-01T14:56:04Z

+         field.type() != Field::TYPE_BYTES;
+}
+
+ProtobufTypes::MessagePtr redactedCopy(const Protobuf::Message& message,


do we need to copy the entire message to redact a field from it?

Couldn't we just stream out the message and redact at the leaf level?

Maybe this is because we have MessageUtil::redactAll we don't want to mess with, or maybe you have already gotten to the leaf level by the time this is called, in which case you can add comments :)

Added a comment.

Everything is redacted leaf by leaf while streaming except some well-known types (the TODO message) and TypedStructs, which need to be reified via the type_url, so they go through MessageUtil::redact.

jmarantz · 2026-09-01T15:08:24Z

@@ -0,0 +1,311 @@
+#include "source/common/buffer/buffer_impl.h"


how is the coverage looking for this test?

96.4 %, added a test.

What's missing? This is a whole bunch of new code, replacing complex old code, which is going to be hard to hand-validate so we are utterly reliant on unit tests covering every corner.

Come to think of it, for validation purposes, can you do differential fuzzing with protobuf's protojson generator?

At this point just the early return for next() if stack_ is empty, which IIRC was needed for the streamed request.

Fuzzing as in generating random configurations? Probably yes although I've no idea how yet. I did run every /config_dump test side by side in the other PR, should also do the same with MessageUtil::redact.

jmarantz · 2026-09-01T17:04:26Z

For ensuring semantic coverage of the xDS data model I'd recommend @htuch and @adisuissa take a look at the proto streamer.

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

…ed fields instead of -1 sentinel Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

filipcacky · 2026-09-02T12:21:03Z

/retest

adisuissa · 2026-09-02T12:52:48Z

For ensuring semantic coverage of the xDS data model I'd recommend @htuch and @adisuissa take a look at the proto streamer.

Challenging to say if the same semantic coverage is acheived by both the proto-streamer and the current proto dumper. I don't see any red-flags, and assuming that there are enough tests covering different edge cases (specifically redacted fields), then it may be ok.
If one wants to take the extra mile, a fuzzer can be added to compare different config dumps outputs.

High-level comment: please add a tracking issue that has the results (that were published in #46911), and add the issue to the PR description.

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

filipcacky added 4 commits August 31, 2026 15:06

json: Add proto wrapper

055b5e9

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

json: Proto speed test

3fb897f

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

json: Make proto streamer redaction aware

a3d7c55

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

proto_streamer: Nicer comments

8e3bece

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

filipcacky added 3 commits August 31, 2026 17:51

Add emitMapEntry

c04bd46

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

Cleanup redactions; Add examples

2d14be3

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

proto_streamer: Mimic Protobuf::util::JsonPrintOptions api

dd25caa

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

filipcacky force-pushed the proto_streamer branch from af237aa to dd25caa Compare August 31, 2026 15:52

ravenblackx assigned wbpcode Aug 31, 2026

filipcacky marked this pull request as ready for review August 31, 2026 16:04

Fix spellchecker complaining about base64

f9c6e0e

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

jmarantz reviewed Sep 1, 2026

View reviewed changes

filipcacky added 7 commits September 1, 2026 23:33

spaces

48ef739

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

std::deque -> std::stack

8510ed6

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

Make REFLECTION_GET and emitValue accept std::optional for non-repeat…

b708c31

…ed fields instead of -1 sentinel Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

startField/nextElement -> emitNextField/emitNextElement

23c9eca

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

Comments on redactions

05d735c

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

Add test to improve coverage

28735a2

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

trailing _

2633f6e

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

wbpcode assigned jmarantz Sep 2, 2026

Reword emitRedactedValue comment, mainly to satisfy spell checker

4799206

Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>

		@@ -0,0 +1,311 @@
		#include "source/common/buffer/buffer_impl.h"

Uh oh!

Conversation

filipcacky commented Aug 31, 2026

Uh oh!

repokitteh-read-only Bot commented Aug 31, 2026

Uh oh!

filipcacky commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz 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!

Uh oh!

Uh oh!

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!

jmarantz commented Sep 1, 2026

Uh oh!

filipcacky commented Sep 2, 2026

Uh oh!

adisuissa commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

filipcacky commented Aug 31, 2026 •

edited

Loading