Support bootstrap configs in text proto format, and improve errors.#1593
Conversation
This commit allows `.pb_text` configs to be parsed as text, and improves the parse failure error message to include (1) text or binary mode, and (2) the message type being parsed. I was trying to use the new `--bootstrap-path` flag to get away from JSON so the config could have comments, and got stuck for a while before realizing the protobuf was being parsed in _binary_ mode.
|
@jmillikin-stripe will defer to @htuch but I thought there was some reason we didn't want to support text PB (vs. JSON/YAML). He can advise. If we do stick with this we will need a test. |
|
Yes, since text PB is not standardized (it's not even consistent across languages in some corner cases), we decided to use YAML as the standardized textual representation of proto. That said, this is a small PR to give us support for this, which might be useful in some test situation, so sure, why not. We won't be officially supporting it though. |
htuch
left a comment
There was a problem hiding this comment.
Yeah, just add a simple test (preferably just in utility_test.cc, I don't want to change any of the integration tests from YAML or duplicate there).
|
BTW, text PB is super ugly for the opaque filter configs, take a look at https://gist.github.com/htuch/aa273c9bf01ba9c6b18353ec8799695e#file-listeners_generated-pb. JSON/YAML representations hide this. |
|
|
||
| envoy::api::v2::Bootstrap proto_from_file; | ||
| MessageUtil::loadFromFile(filename, proto_from_file); | ||
| EXPECT_TRUE(TestUtility::protoEqual(bootstrap, proto_from_file)); |
There was a problem hiding this comment.
nit: please also test the exception case with EXPECT_THROW_WITH_MESSAGE by passing a bogus .pb_text file.
|
OK, tests added.
As one datapoint: For proto3, text format consistency across {C++, Go, Python, Java} is Good Enough™ for what we'll be using it for, and has strong advantages over YAML (simpler semantics/grammar) and JSON (comments). It's also more practical to generate than binary protobufs for environments that can't easily have |
|
Dev experience note: Tests won't build on my MacOS machine because it's missing some Linux tools ( @htuch, I understand a lot of the current Bazel weirdness is due to constraints from fitting the Envoy build into Google's external infra. At some point I'd like to see if it could be simplified so it's easier to build dependencies in a more Bazel-y way. |
|
@jmillikin-stripe we're open to improvements to the build, as long as they don't involve creating a parallel Bazel build system for external deps (e.g. our own custom injected BUILD files via A pure Bazel dependency tree would be amazing, but I think it's a bit of a long term goal, given that dependencies seem to keep growing and very few projects are Bazel native. That said, if we could move some parts out of the recursive make step to Bazel for dependencies where there is native Bazel support already (e.g. BoringSSL), which you previously expressed interest in, definitely open to that. We should have a better CI setup soon without the Travis resource limits, so that would open up the opportunity to having less done in prebuilts in CI. |
Description: It looks like Bazel doesn't pick up the credentials env variable if it is already running. Unfortunately, there isn't an easy way to test this hypothesis without merging a commit to main. Risk Level: LOW Testing: N/A Signed-off-by: Ulf Adams <ulf@engflow.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: It looks like Bazel doesn't pick up the credentials env variable if it is already running. Unfortunately, there isn't an easy way to test this hypothesis without merging a commit to main. Risk Level: LOW Testing: N/A Signed-off-by: Ulf Adams <ulf@engflow.com> Signed-off-by: JP Simard <jp@jpsim.com>
**Description** This PR create a benchmark test for AIGW withe diffrent `mcp-session-encryption-iterations` Run this benchmark with following: ```console go test -timeout=15m -run='^$$' -bench=. -benchmem -benchtime=1x ./tests/bench/... 2025/12/03 13:50:12 starting DUMB MCP Streamable-HTTP server on :8080 at /mcp goos: darwin goarch: arm64 pkg: github.com/envoyproxy/ai-gateway/tests/bench cpu: Apple M1 Pro BenchmarkMCP/BaseLine-10 1 31305583 ns/op 49736 B/op 720 allocs/op BenchmarkMCP/Iterations_100-10 1 1394125 ns/op 22640 B/op 300 allocs/op PASS ok github.com/envoyproxy/ai-gateway/tests/bench 5.394s ``` ~~This result that the default `SessionCrypto` cost a lot, we'd better improve it or change the default setting.~~ cc @nacx --------- Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: Ignasi Barrera <nacx@apache.org> Co-authored-by: Ignasi Barrera <nacx@apache.org> Co-authored-by: Ignasi Barrera <ignasi@tetrate.io>
This commit allows
.pb_textconfigs to be parsed as text, and improvesthe parse failure error message to include (1) text or binary mode, and
(2) the message type being parsed.
I was trying to use the new
--bootstrap-pathflag to get away fromJSON so the config could have comments, and got stuck for a while before
realizing the protobuf was being parsed in binary mode.