tcp-proxy: support proxy-protocol tlvs #38131
Conversation
|
Hi @jronak, 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. |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
There was a problem hiding this comment.
@jronak could you fix DCO please
also there are quite a lot of other failures - some might be flakes but most look real
some of the format issues can be resolved by applying diffs from CI artefacts here https://github.com/envoyproxy/envoy/actions/runs/12901724937
jronak
force-pushed
the
ronakj/tcp-proxy-tlvs
branch
2 times, most recently
from
934a2cc to
44d5fa0
Compare
|
@phlax fixed fmt and failed tests seems to be flaky. Could you trigger a re-run? |
|
you can also ... /retest (please check that failures dont look real before retesting) |
|
/retest |
source/common/tcp_proxy/tcp_proxy.cc
Outdated
|
|
||
| if (!config.proxy_protocol_tlvs().empty()) { | ||
| proxy_protocol_tlvs_ = | ||
| Extensions::Common::ProxyProtocol::parseTLVs(config.proxy_protocol_tlvs()); |
There was a problem hiding this comment.
Generally we try pretty hard to not add deps from core to extensions. Would it be possible to instead configure which TLVs we put into ProxyProtocolFilterState, and then simply copy all when we hit the tcp proxy session?
/wait-any
There was a problem hiding this comment.
I'm not sure if I fully understand, but I believe this implementation achieves the same functionality:
- Configuring TLVs to be populated in the proxy protocol filter state in the configuration.
- Copying the parsed TLVs for each TCP proxy session.
I agree that we should avoid dependencies from core to extensions. To address this, we could:
- Move the parseTLV method into core/tcp-proxy.
- Create a new source/common/proxy_protocol module to host the parseTLV method, making it accessible without introducing unwanted dependencies. This might be better as this method will be needed for usage in HTTP connection manager.
Does this approach align with what you had in mind?
I have updated the PR to use approach 2 for easier iteration, open to your feedback
Signed-off-by: Ronak Jain <[email protected]>
Signed-off-by: Ronak Jain <[email protected]>
jronak
force-pushed
the
ronakj/tcp-proxy-tlvs
branch
from
aece64a to
acbd222
Compare
Additional Description: Adds support for passing Proxy Protocol TLVs in the Proxy Protocol header in tcp_proxy filter
Risk Level: low
Testing: unit test
Docs Changes: tcp-proxy proto config
Release Notes: none