Skip to content

Conversation

@lzx404243
Copy link
Collaborator

This PR adds support for the tunnel_route configuration that contains both the regex match group number and the port variable specification, such as the following

- fqdn: "node.*.example.com"
  tunnel_route: "backend.$1.example.com:{proxy_protocol_port}"

Prior to this PR, only one of the match group specification or port variable specification can be used in the tunnel_route configuration.

@bneradt bneradt added the TLS label Apr 10, 2023
@bneradt bneradt added this to the 10.0.0 milestone Apr 10, 2023
Copy link
Contributor

@bneradt bneradt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this @lzx404243.

@lzx404243 lzx404243 marked this pull request as ready for review April 10, 2023 16:31
@bryancall bryancall requested a review from cmcfarlen April 10, 2023 22:06
bneradt
bneradt previously approved these changes Apr 14, 2023
Copy link
Contributor

@cmcfarlen cmcfarlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation could better match the code and I feel the code should handle the case where both port strings appear (even if that means a warning or error).

@SolidWallOfCode
Copy link
Member

I'll defer to @brbzull0 and @cmcfarlen but otherwise I think it's fine.

@lzx404243 lzx404243 force-pushed the tunnel_route_match_group_and_port branch from 2cab5f2 to d3142a0 Compare April 14, 2023 18:40
@lzx404243 lzx404243 requested a review from cmcfarlen April 14, 2023 18:51
cmcfarlen
cmcfarlen previously approved these changes Apr 14, 2023
Copy link
Contributor

@cmcfarlen cmcfarlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new code to add the function is a little hard to read imo.

Copy link
Contributor

@cmcfarlen cmcfarlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for cleaning that up.

@bneradt bneradt merged commit 0772610 into apache:master Apr 14, 2023
JosiahWI pushed a commit to JosiahWI/trafficserver that referenced this pull request Jul 19, 2023
…t variable (apache#9594) (apache#804)

Conflicts:
	doc/admin-guide/files/sni.yaml.en.rst
	iocore/net/P_SNIActionPerformer.h
	tests/gold_tests/tls/tls_tunnel.test.py

Co-authored-by: Zhengxi Li <zli11@yahooinc.com>
@maskit maskit mentioned this pull request Aug 15, 2024
91 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants