upstream_proxy_protocol: Introduce custom TLV support #37591

timflannagan · 2024-12-09T20:32:41Z

Commit Message:

This commit introduces support for injecting custom TLVs into the Proxy Protocol v2 (PP2) header for upstream transport sockets. This enables xDS control planes to build upstream PP2 headers with greater flexibility. Previously, upstream PP2 headers only passed through TLVs from downstream connections when using the Proxy Protocol listener, limiting customization.

With this change, users can define custom TLVs by configuring the custom_tlvs field in the upstream_proxy_protocol transport socket config, or specifying host metadata in a well-known namespace in order to provide dynamic and more granular control over PP2 header content. For example:

      transport_socket:
        name: envoy.transport_sockets.upstream_proxy_protocol
        typed_config:
          "@type": type.googleapis.com/envoy.extensions.transport_sockets.proxy_protocol.v3.ProxyProtocolUpstreamTransport
          config:
            version: V2
            added_tlvs:
              - type: 150
                value: Zm9v
              - type: 151
                value: YmFy

And

clusters:
      - name: httpbin
        load_assignment:
          ...
          endpoints:
          - lbEndpoints:
            - metadata:
                filter_metadata:
                  envoy.transport_socket_match:
                    outbound-proxy: true
                typed_filter_metadata:
                  envoy.transport_sockets.proxy_protocol:
                    "@type": type.googleapis.com/envoy.config.core.v3.ProxyProtocolConfig
                    added_tlvs:
                      - type: 0xD7
                        value: b3Zy
                      - type: 0xD8
                        value: bmV3
               ...

By decoupling upstream PP2 customization from downstream listener config, this unlocks more flexible use cases for Proxy Protocol in upstream paths.

Additional Description: N/A
Risk Level: Low
Testing: Unit & integration
Docs Changes: Include in the protobuf docs
Release Notes: Included in the changelog as a new feature
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue] #18520
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2024-12-09T20:32:48Z

Hi @timflannagan, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #37591 was opened by timflannagan.

see: more, trace.

repokitteh-read-only · 2024-12-09T20:32:53Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #37591 was opened by timflannagan.

see: more, trace.

timflannagan · 2024-12-09T20:41:12Z

For more discussion on some of the implementation choices, see timflannagan#1 for an initial round of review. Also, I haven't written C++ in a while so I'm happy to make any style-related changes.

timflannagan · 2024-12-09T23:05:47Z

Looks like integration tests are failing. I'll fix that tomorrow. At a glance, the assertions are expecting a certain ordering for the custom TLVs that are being defined, and there's deterministic output issues.

@type

This commit introduces support for injecting custom TLVs into the Proxy Protocol v2 (PP2) header for upstream transport sockets. This enables xDS control planes to build upstream PP2 headers with greater flexibility. Previously, upstream PP2 headers only passed through TLVs from downstream connections when using the Proxy Protocol listener, limiting customization. With this change, users can define custom TLVs by specifying host metadata in a well-known namespace, providing dynamic, granular control over PP2 header content. For example: ```yaml clusters: - name: httpbin load_assignment: ... endpoints: - lbEndpoints: - metadata: filter_metadata: envoy.transport_socket_match: outbound-proxy: true typed_filter_metadata: envoy.transport_sockets.proxy_protocol: "@type": type.googleapis.com/envoy.extensions.transport_sockets.proxy_protocol.v3.CustomTlvMetadata entries: - type: 0x96 value: Zm9v # foo - type: 0x97 value: YmFy # bar ... ``` By decoupling upstream PP2 customization from downstream listener config, this unlocks more flexible use cases for Proxy Protocol in upstream paths. Earlier approaches considered extending upstream_proxy_protocol to support TLV configuration but were rejected due to added control plane complexity. Similarly, reusing the envoy.network.proxy_protocol_options namespace was evaluated but required significant refactoring and risk. Signed-off-by: timflannagan <timflannagan@gmail.com>

ggreenway

/wait

api/envoy/extensions/transport_sockets/proxy_protocol/v3/upstream_proxy_protocol.proto

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc

Signed-off-by: timflannagan <timflannagan@gmail.com>

changelogs/current.yaml

timflannagan · 2024-12-11T17:12:13Z

Looks like the format check is failing:

ERROR: From ./source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc
ERROR: ./source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc:217: Don't use UnpackTo() directly, use MessageUtil::unpackToNoThrow() instead
ERROR: check format failed. diff has been applied'

I don't see that function defined anywhere, but I see several PRs that seem relevant here and I think I can figure out how to silence that linting violation. Does that linting rule need to be updated as well?

Signed-off-by: timflannagan <timflannagan@gmail.com>

…tream-tlvs

mattklein123 · 2024-12-12T04:07:10Z

/api lgtm

timflannagan · 2024-12-12T15:46:28Z

@ggreenway Can you take another pass when you get a second? I need to look into getting the codecov check green, but the rest of the CI checks looked good last run.

Signed-off-by: timflannagan <timflannagan@gmail.com>

ggreenway

/wait

api/envoy/config/core/v3/proxy_protocol.proto

source/extensions/common/proxy_protocol/proxy_protocol_header.cc

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc

ggreenway · 2025-01-07T21:53:49Z

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h

@@ -52,6 +53,9 @@ class UpstreamProxyProtocolSocket : public TransportSockets::PassthroughSocket,
  const UpstreamProxyProtocolStats& stats_;
  const bool pass_all_tlvs_;
  absl::flat_hash_set<uint8_t> pass_through_tlvs_{};


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.

Signed-off-by: timflannagan <timflannagan@gmail.com>

ggreenway

/wait

api/envoy/config/core/v3/proxy_protocol.proto

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc

test/extensions/common/proxy_protocol/proxy_protocol_header_test.cc

ggreenway · 2025-01-09T17:52:09Z

test/extensions/common/proxy_protocol/proxy_protocol_header_test.cc

+  std::vector<Envoy::Network::ProxyProtocolTLV> custom_tlvs = {
+      {0x8, std::vector<unsigned char>(65536, 'a')},
+  };
+  EXPECT_LOG_CONTAINS("warn", "Generating Proxy Protocol V2 header: TLVs exceed length limit 65535",


This case was probably unreachable before this change, but now a bad config could result in a connection not generating a PP header at all. I think this case needs to be handled better, probably with the connection being aborted, or alternately generate a PP header omitting the TLV that is too long. cc @botengyao who added the TLV passthrough code.

Yeah, this becomes more problematic with these changes. I think I'd lean towards truncating the custom (and passthrough) TLVs for the upstream PP2 header. At a minimum, I think we'd need to log and possible document this behavior so users aren't caught over guard. cc @yuval-k in case you have any thoughts as well.

#37591 (comment)

Signed-off-by: timflannagan <timflannagan@gmail.com>

…tream-tlvs

timflannagan · 2025-01-16T20:37:31Z

source/extensions/common/proxy_protocol/proxy_protocol_header.cc

+  // Filter out TLVs that would exceed the 65535 limit.
+  uint64_t extension_length = 0;
+  bool skipped_tlvs = false;
+  for (auto&& tlv : combined_tlv_vector) {
+    uint64_t new_size = extension_length + PROXY_PROTO_V2_TLV_TYPE_LENGTH_LEN + tlv.value.size();
+    if (new_size > std::numeric_limits<uint16_t>::max()) {
+      ENVOY_LOG_MISC(warn, "Skipping TLV type {} because adding it would exceed the 65535 limit.",
+                     tlv.type);
+      skipped_tlvs = true;
+      continue;
+    }
+    extension_length = new_size;
+    final_tlvs.push_back(tlv);
  }


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

if (!Common::ProxyProtocol::generateV2Header(options, header_buffer_, pass_all_tlvs_,

pass_through_tlvs_, custom_tlvs)) {

// There is a warn log in generateV2Header method.

stats_.v2_tlvs_exceed_max_length_.inc();

expects this function to return false to increment the stats.

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.

phlax · 2025-01-22T10:34:09Z

ping @ggreenway i think this is awaiting further review

@timflannagan requires main merge

phlax · 2025-01-22T10:35:30Z

api/envoy/config/core/v3/proxy_protocol.proto

+  //                        - type: 0xD8
+  //                          value: bmV3
+  //
+  // **Precedence behavior**:


wondering if this should be a header

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.

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 ...

for ref this is the Proxy Protocol ref page https://www.envoyproxy.io/docs/envoy/latest/configuration/listeners/listener_filters/proxy_protocol.html

@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.

not a blocker - but please at least fix the config indents

thanks! really appreciated

Fixed the indentation issues in b343e67. I also took a stab at the literalinclude approach in 7c5d4ed after mirroring the examples from #35532.

yeah - using literalinclude is what is super appreciated - its much more maintainable

probs we should add some docs somewhere - i have been pondering a plan to work on the tech debt here

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.

// The PROXY protocol transport socket allows carrying connection metadata (such as // the original source IP address) to upstream services in either a human-readable // (Version 1) or binary (Version 2) header. // // Behavior // ======== // When the PROXY protocol transport socket is configured, Envoy can prepend a PROXY // protocol header to connections going upstream. Depending on the protocol version, // additional metadata such as custom Type-Length-Value (TLV) entries may also be sent. // // Configuration // ============= // .. code-block:: yaml // // transport_socket: // name: envoy.transport_sockets.proxy_protocol // typed_config: // "@type": type.googleapis.com/envoy.config.core.v3.ProxyProtocolConfig // version: V2 // pass_through_tlvs: // match_type: INCLUDE_ALL // // Passing & Adding TLVs // --------------------- // - *pass_through_tlvs*: // Controls which TLVs from the downstream PROXY protocol request are forwarded // to the upstream connection. // - *added_tlvs*: // Defines new TLVs to add into the header for the upstream request. TLVs defined // at a host level take precedence over those defined at the transport socket level.

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.

phlax · 2025-01-22T10:41:23Z

re unpackToNoThrow ...

I don't see that function defined anywhere, but I see several PRs that seem relevant here and I think I can figure out how to silence that linting violation. Does that linting rule need to be updated as well?

it would seem it does

cc @tyxia @alyssawilk

alyssawilk · 2025-01-22T14:54:17Z

yeah looks like that rule should have been updated with https://github.com/envoyproxy/envoy/pull/32775/files

#38138 should fix

Signed-off-by: timflannagan <timflannagan@gmail.com>

…tream-tlvs

phlax · 2025-01-22T19:10:21Z

/docs

repokitteh-read-only · 2025-01-22T19:10:27Z

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/37591/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: a #37591 (comment) was created by @phlax.

see: more, trace.

phlax

for configs - we strongly encourage using a literalinclude with a full bootstrap yaml, as these are linted and tested variously for validity.

this avoids bitrot and is much more helpful for end users

we also enforce least indent on yaml formatting which is missed in code-block s

if we shift the example to a reference page its ~easier, but it can be done for api also

api/envoy/config/core/v3/proxy_protocol.proto

Signed-off-by: timflannagan <timflannagan@gmail.com>

…tream-tlvs

Signed-off-by: timflannagan <timflannagan@gmail.com>

timflannagan requested review from ggreenway, soulxu and alyssawilk as code owners December 9, 2024 20:32

repokitteh-read-only bot added the api label Dec 9, 2024

repokitteh-read-only bot assigned mattklein123 Dec 9, 2024

timflannagan changed the title ~~Introduce custom TLV support for upstream PP2 headers~~ proxy_protocol: Introduce custom TLV support for upstream PP2 headers Dec 9, 2024

timflannagan force-pushed the feat/custom-pp2-upstream-tlvs branch from ba3c628 to 09d7ce1 Compare December 10, 2024 18:42

ggreenway requested changes Dec 10, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 10, 2024

ggreenway self-assigned this Dec 10, 2024

pp2: Add custom TLV entries field to protocol protocol config API

d0a3b62

Signed-off-by: timflannagan <timflannagan@gmail.com>

repokitteh-read-only bot removed the waiting label Dec 11, 2024

timflannagan commented Dec 11, 2024

View reviewed changes

changelogs/current.yaml Outdated Show resolved Hide resolved

timflannagan added 2 commits December 11, 2024 17:48

pp2: Silence linting violation

abc5088

Signed-off-by: timflannagan <timflannagan@gmail.com>

Merge remote-tracking branch 'upstream/main' into feat/custom-pp2-ups…

bdc38a9

…tream-tlvs

mattklein123 previously approved these changes Dec 12, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Dec 12, 2024

mattklein123 removed their assignment Dec 12, 2024

timflannagan dismissed mattklein123’s stale review via bdc38a9 December 12, 2024 15:45

repokitteh-read-only bot added the api label Dec 12, 2024

timflannagan requested a review from ggreenway December 12, 2024 15:46

pp2: Add additional unit tests

6d94e1d

Signed-off-by: timflannagan <timflannagan@gmail.com>

ggreenway requested changes Jan 7, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 7, 2025

timflannagan added 2 commits January 7, 2025 22:02

*: Rename custom_tlvs -> added_tlvs in proxy_protocol.proto

1c02dc1

Signed-off-by: timflannagan <timflannagan@gmail.com>

api: Revisit added_tlvs proto field documentation to clarify behavior

e50c959

Signed-off-by: timflannagan <timflannagan@gmail.com>

repokitteh-read-only bot removed the waiting label Jan 8, 2025

timflannagan added 5 commits January 8, 2025 17:11

source,test: Refactor added_tlvs code

f1ed670

Signed-off-by: timflannagan <timflannagan@gmail.com>

api: Attempt to fix docs sanity check

5ec9dd8

Signed-off-by: timflannagan <timflannagan@gmail.com>

api: Fix docs precheck

118792b

Signed-off-by: timflannagan <timflannagan@gmail.com>

pp2: Add ASSERT, move duplicate test case, remove TODOs

c5a5619

Signed-off-by: timflannagan <timflannagan@gmail.com>

pp2: Consolidate tests to add explicit precedence & override cases

7e5454a

Signed-off-by: timflannagan <timflannagan@gmail.com>

ggreenway requested changes Jan 9, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 9, 2025

timflannagan added 2 commits January 16, 2025 20:30

pp2: Refactor code, address review comments, etc

ad46d23

Signed-off-by: timflannagan <timflannagan@gmail.com>

Merge remote-tracking branch 'upstream/main' into feat/custom-pp2-ups…

3122ab3

…tream-tlvs

repokitteh-read-only bot removed the waiting label Jan 16, 2025

timflannagan commented Jan 16, 2025

View reviewed changes

phlax reviewed Jan 22, 2025

View reviewed changes

timflannagan added 2 commits January 22, 2025 19:03

Fix format errors

a6de035

Signed-off-by: timflannagan <timflannagan@gmail.com>

Merge remote-tracking branch 'upstream/main' into feat/custom-pp2-ups…

c1e9f63

…tream-tlvs

phlax reviewed Jan 22, 2025

View reviewed changes

api/envoy/config/core/v3/proxy_protocol.proto Outdated Show resolved Hide resolved

timflannagan added 5 commits January 22, 2025 20:26

api: Fix YAML identation in PP2 code block

b343e67

Signed-off-by: timflannagan <timflannagan@gmail.com>

api: Use literalinclude for added_tlvs code example

7c5d4ed

Signed-off-by: timflannagan <timflannagan@gmail.com>

configs: Fix proxy_protocol.yaml format errors

876af00

Signed-off-by: timflannagan <timflannagan@gmail.com>

Merge remote-tracking branch 'upstream/main' into feat/custom-pp2-ups…

977bb4b

…tream-tlvs

Fix invalid proxy_protocol.yaml config

5ebdb87

Signed-off-by: timflannagan <timflannagan@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upstream_proxy_protocol: Introduce custom TLV support #37591

upstream_proxy_protocol: Introduce custom TLV support #37591

timflannagan commented Dec 9, 2024 •

edited

Loading

repokitteh-read-only bot commented Dec 9, 2024

repokitteh-read-only bot commented Dec 9, 2024

timflannagan commented Dec 9, 2024

timflannagan commented Dec 9, 2024

ggreenway left a comment

timflannagan commented Dec 11, 2024

mattklein123 commented Dec 12, 2024

timflannagan commented Dec 12, 2024

ggreenway left a comment

ggreenway Jan 7, 2025

ggreenway left a comment

ggreenway Jan 9, 2025

timflannagan Jan 9, 2025 •

edited

Loading

timflannagan Jan 16, 2025

timflannagan Jan 16, 2025

phlax commented Jan 22, 2025

phlax Jan 22, 2025

timflannagan Jan 22, 2025

phlax Jan 22, 2025

phlax Jan 22, 2025

timflannagan Jan 22, 2025

phlax Jan 22, 2025

phlax Jan 22, 2025

timflannagan Jan 22, 2025

phlax Jan 22, 2025

timflannagan Jan 22, 2025

phlax commented Jan 22, 2025

alyssawilk commented Jan 22, 2025

phlax commented Jan 22, 2025

repokitteh-read-only bot commented Jan 22, 2025

phlax left a comment •

edited

Loading

	if (!Common::ProxyProtocol::generateV2Header(options, header_buffer_, pass_all_tlvs_,
	pass_through_tlvs_, custom_tlvs)) {
	// There is a warn log in generateV2Header method.
	stats_.v2_tlvs_exceed_max_length_.inc();

upstream_proxy_protocol: Introduce custom TLV support #37591

Are you sure you want to change the base?

upstream_proxy_protocol: Introduce custom TLV support #37591

Conversation

timflannagan commented Dec 9, 2024 • edited Loading

repokitteh-read-only bot commented Dec 9, 2024

repokitteh-read-only bot commented Dec 9, 2024

timflannagan commented Dec 9, 2024

timflannagan commented Dec 9, 2024

ggreenway left a comment

Choose a reason for hiding this comment

timflannagan commented Dec 11, 2024

mattklein123 commented Dec 12, 2024

timflannagan commented Dec 12, 2024

ggreenway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timflannagan Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phlax commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phlax commented Jan 22, 2025

alyssawilk commented Jan 22, 2025

phlax commented Jan 22, 2025

repokitteh-read-only bot commented Jan 22, 2025

phlax left a comment • edited Loading

Choose a reason for hiding this comment

timflannagan commented Dec 9, 2024 •

edited

Loading

timflannagan Jan 9, 2025 •

edited

Loading

phlax left a comment •

edited

Loading