Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
upstream_proxy_protocol: Introduce custom TLV support #37591
base: main
Are you sure you want to change the base?
upstream_proxy_protocol: Introduce custom TLV support #37591
Changes from 23 commits
09d7ce1
d0a3b62
abc5088
bdc38a9
6d94e1d
2406b71
9dbb9ff
37f6be1
9c5ac97
da65ca7
b656b36
d23df4f
1c02dc1
e50c959
f1ed670
5ec9dd8
118792b
c5a5619
7e5454a
ad46d23
3122ab3
a6de035
c1e9f63
b343e67
7c5d4ed
876af00
977bb4b
5ebdb87
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if this should be a header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any examples on how to do that? I tried following https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#sections, but
ci/do_ci.sh docs
was failing when I ran it and I can't seem to find where the generated files live on my local box.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generated files live in bazel ether - unless you build them directly (eg
bazel build //docs:rst
inside the container)you can view the rendered docs eg here:
https://storage.googleapis.com/envoy-pr/3122ab3/docs/api-v3/config/core/v3/proxy_protocol.proto.html#envoy-v3-api-msg-config-core-v3-proxyprotocolpassthroughtlvs
thinking more about my heading suggestion, im wondering if we should move some of this to a reference page and link it - eg the config example
i tried setting the "heading" to h5 - it kinda works but probs we shouldnt have headings inside the descriptions - they should be relatively terse
ill comment on the config now ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for ref this is the Proxy Protocol ref page https://www.envoyproxy.io/docs/envoy/latest/configuration/listeners/listener_filters/proxy_protocol.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phlax I like the reference page idea looking more at the current docset. Is this something we can tackle as a follow-up or is this a blocker in your mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a blocker - but please at least fix the config indents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! really appreciated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the indentation issues in b343e67. I also took a stab at the literalinclude approach in 7c5d4ed after mirroring the examples from #35532.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah - using
literalinclude
is what is super appreciated - its much more maintainableprobs we should add some docs somewhere - i have been pondering a plan to work on the tech debt here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think the reference docs or some high-level field documentation in the prot is still worth chasing -- I just have limited capacity right now. At a minimum, I think it would be good to have a header that explains pass_through_tlvs & added_tlvs and their interlope as there's some nuance in their behavior.
And then the individual added_tlvs field documentation could introduce the two config options and the precedence behavior. I'm not sure what's best w.r.t organization so I stopped messing around with it locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truncated custom or passthrough TLVs that exceed the max length in ad46d23. There were some header unit tests that were failing as a result as
envoy/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc
Lines 106 to 109 in 3122ab3
I went back and forth on whether returning false here would be ambiguous when writing the output header was successful, but I didn't want to extend the function signature to return (bool written, bool skipped_tlvs) to handle this edge case at the call sites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't related to this PR, but instead of this (and your new config) being copied by value here, the normal pattern is to hold a shared_ptr to a Config object that has already converted the protobuf config to the internal represenation, and done validation such as no duplicates.