You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "SolidWallOfCode (via GitHub)" <gi...@apache.org> on 2023/04/18 04:02:49 UTC
[GitHub] [trafficserver] SolidWallOfCode opened a new pull request, #9618: Use BWF for SNI tunnel configuration.
SolidWallOfCode opened a new pull request, #9618:
URL: https://github.com/apache/trafficserver/pull/9618
This replaces the hand rolled substitution code for configuring a tunnel destination with `BufferWriter` formatting.
* This results in much less code.
* The code is more general - any or all of the substitutions can be used in a single configuration.
* Additional substitutions can be added with just a few lines of very simple code.
* Better performances - in most cases no allocation is done to generate the rewritten destination string.
* Many elements are removed, because there is no special handling for each supported substitution.
* BWF optimizes for the literal / no substitutions case without requiring the client to check.
Compatibility change - capture groups must be referred to use numeric arguments, e.g. `{1}` and not `$1`. This makes more than ten groups trivial to support with less in line ambiguity (e.g. "{1}2" vs. "$12").
I also moved the embarrassingly large header inline methods to the source file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#discussion_r1175723353
##########
tests/gold_tests/tls/tls_tunnel.test.py:
##########
@@ -96,19 +96,19 @@
"- fqdn: bob.*.com",
" tunnel_route: localhost:{0}".format(server_foo.Variables.SSL_Port),
"- fqdn: '*.match.com'",
- " tunnel_route: $1.testmatch:{0}".format(server_foo.Variables.SSL_Port),
+ ''' tunnel_route: "{{1}}.testmatch:{0}"'''.format(server_foo.Variables.SSL_Port),
"- fqdn: '*.ok.*.com'",
- " tunnel_route: $2.example.$1:{0}".format(server_foo.Variables.SSL_Port),
+ ''' tunnel_route: "{{2}}.example.{{1}}:{0}"'''.format(server_foo.Variables.SSL_Port),
"- fqdn: ''", # No SNI sent
" tunnel_route: localhost:{0}".format(server_bar.Variables.SSL_Port),
"- fqdn: 'incoming.port.com'",
- " tunnel_route: backend.incoming.port.com:{inbound_local_port}",
+ ''' tunnel_route: "backend.incoming.port.com:{inbound_local_port}"''',
"- fqdn: 'proxy.protocol.port.com'",
- " tunnel_route: backend.proxy.protocol.port.com:{proxy_protocol_port}",
+ ''' tunnel_route: "backend.proxy.protocol.port.com:{proxy_protocol_port}"''',
"- fqdn: '*.*.incoming.port.com'",
- " tunnel_route: backend.$1.$2.incoming.port.com:{inbound_local_port}",
+ ''' tunnel_route: "backend.{1}.{2}.incoming.port.com:{inbound_local_port}"''',
Review Comment:
Another thing to consider is, for backwards compatibility, do a pre-pass that converts "[$]([0-9]+)" to "{\1}".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] closed pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #9618: Use BWF for SNI tunnel configuration.
URL: https://github.com/apache/trafficserver/pull/9618
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 commented on pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "ezelkow1 (via GitHub)" <gi...@apache.org>.
ezelkow1 commented on PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#issuecomment-1554933183
[approve ci centos]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#discussion_r1175722290
##########
tests/gold_tests/tls/tls_tunnel.test.py:
##########
@@ -96,19 +96,19 @@
"- fqdn: bob.*.com",
" tunnel_route: localhost:{0}".format(server_foo.Variables.SSL_Port),
"- fqdn: '*.match.com'",
- " tunnel_route: $1.testmatch:{0}".format(server_foo.Variables.SSL_Port),
+ ''' tunnel_route: "{{1}}.testmatch:{0}"'''.format(server_foo.Variables.SSL_Port),
"- fqdn: '*.ok.*.com'",
- " tunnel_route: $2.example.$1:{0}".format(server_foo.Variables.SSL_Port),
+ ''' tunnel_route: "{{2}}.example.{{1}}:{0}"'''.format(server_foo.Variables.SSL_Port),
"- fqdn: ''", # No SNI sent
" tunnel_route: localhost:{0}".format(server_bar.Variables.SSL_Port),
"- fqdn: 'incoming.port.com'",
- " tunnel_route: backend.incoming.port.com:{inbound_local_port}",
+ ''' tunnel_route: "backend.incoming.port.com:{inbound_local_port}"''',
"- fqdn: 'proxy.protocol.port.com'",
- " tunnel_route: backend.proxy.protocol.port.com:{proxy_protocol_port}",
+ ''' tunnel_route: "backend.proxy.protocol.port.com:{proxy_protocol_port}"''',
"- fqdn: '*.*.incoming.port.com'",
- " tunnel_route: backend.$1.$2.incoming.port.com:{inbound_local_port}",
+ ''' tunnel_route: "backend.{1}.{2}.incoming.port.com:{inbound_local_port}"''',
Review Comment:
For the first point, the braces are removed by the substitution, just as is done currently. It would be expected that a correctly formatted string would not have any braces in the final result.
For the error cases, I pondered that. What happens current if "$3" is present but there are insufficient match groups? I would need to make a tweak to libswoc to provide access to the parsed format, after which the logic could check for invalid substitutions for names and match groups.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#discussion_r1177149632
##########
iocore/net/SNIActionPerformer.cc:
##########
@@ -92,3 +94,111 @@ SNI_IpAllow::TestClientSNIAction(char const *servrername, IpEndpoint const &ep,
{
return ip_addrs.contains(swoc::IPAddr(ep));
}
+
+TunnelDestination::bwf_map_type TunnelDestination::bwf_map;
+
+TunnelDestination::TunnelDestination(const std::string_view &dest, SNIRoutingType type, YamlSNIConfig::TunnelPreWarm prewarm,
+ const std::vector<int> &alpn)
+ : destination(dest), type(type), tunnel_prewarm(prewarm), alpn_ids(alpn)
+{
+ // Get a view of the port text. If there is a substitution there, @a port will end up empty while
+ // rest (stuff after port) will be non-empty. If both are empty there is no port specified at all.
+ std::string_view port_text, rest;
+ swoc::IPEndpoint::tokenize(destination, nullptr, &port_text, &rest);
+ dynamic_port_p = port_text.empty() && !rest.empty();
+
+ Debug("ssl_sni", "port is %s", dynamic_port_p ? "true" : "false");
Review Comment:
Would you like to add the string `dynamic`after the `is ` though? I think `port is dynamic: true` is a little more informative than `port is true` .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#discussion_r1173897404
##########
iocore/net/P_SNIActionPerformer.h:
##########
@@ -92,180 +95,62 @@ class HTTP2BufferWaterMark : public ActionItem
class TunnelDestination : public ActionItem
{
- // ID of the configured variable. This will be used to know which function
- // should be called when processing the tunnel destination.
- enum OpId : int32_t {
- MATCH_GROUPS, // Deal with configured groups.
- MAP_WITH_RECV_PORT, // Use port from inbound local
- MAP_WITH_PROXY_PROTOCOL_PORT, // Use port from the proxy protocol
- MAX // Always at the end and do not change the value of the above items.
+ /// Context used for name binding on tunnel destination.
+ struct BWContext {
+ Context const &_action_ctx; ///< @c ActionItem context.
+ SSLNetVConnection *_vc; ///< Connection object.
};
- static constexpr std::string_view MAP_WITH_RECV_PORT_STR = "{inbound_local_port}";
- static constexpr std::string_view MAP_WITH_PROXY_PROTOCOL_PORT_STR = "{proxy_protocol_port}";
-
-public:
- TunnelDestination(const std::string_view &dest, SNIRoutingType type, YamlSNIConfig::TunnelPreWarm prewarm,
- const std::vector<int> &alpn)
- : destination(dest), type(type), tunnel_prewarm(prewarm), alpn_ids(alpn)
+ /// The container for bound names.
+ using bwf_map_type = swoc::bwf::ContextNames<BWContext const>;
+ /// Argument pack for capture groups.
+ class CaptureArgs : public swoc::bwf::ArgPack
{
- // Check for port variable specification. Note that this is checked before
- // the match group so that the corresponding function can be applied before
- // the match group expansion(when the var_start_pos is still accurate).
- auto recv_port_start_pos = destination.find(MAP_WITH_RECV_PORT_STR);
- auto pp_port_start_pos = destination.find(MAP_WITH_PROXY_PROTOCOL_PORT_STR);
- bool has_recv_port_var = recv_port_start_pos != std::string::npos;
- bool has_pp_port_var = pp_port_start_pos != std::string::npos;
- if (has_recv_port_var && has_pp_port_var) {
- Error("Invalid destination \"%.*s\" in SNI configuration - Only one port variable can be specified.",
- static_cast<int>(destination.size()), destination.data());
- } else if (has_recv_port_var) {
- fnArrIndexes.push_back(OpId::MAP_WITH_RECV_PORT);
- var_start_pos = recv_port_start_pos;
- } else if (has_pp_port_var) {
- fnArrIndexes.push_back(OpId::MAP_WITH_PROXY_PROTOCOL_PORT);
- var_start_pos = pp_port_start_pos;
- }
- // Check for match groups as well.
- if (destination.find_first_of('$') != std::string::npos) {
- fnArrIndexes.push_back(OpId::MATCH_GROUPS);
- }
- }
- ~TunnelDestination() override {}
+ public:
+ CaptureArgs(std::optional<ActionItem::Context::CapturedGroupViewVec> const &groups);
- int
- SNIAction(TLSSNISupport *snis, const Context &ctx) const override
- {
- // Set the netvc option?
- SSLNetVConnection *ssl_netvc = dynamic_cast<SSLNetVConnection *>(snis);
- const char *servername = snis->get_sni_server_name();
- if (ssl_netvc) {
- if (fnArrIndexes.empty()) {
- ssl_netvc->set_tunnel_destination(destination, type, !TLSTunnelSupport::PORT_IS_DYNAMIC, tunnel_prewarm);
- Debug("ssl_sni", "Destination now is [%s], fqdn [%s]", destination.c_str(), servername);
- } else {
- bool port_is_dynamic = false;
- auto fixed_dst{destination};
- // Apply mapping functions to get the final destination.
- for (auto fnArrIndex : fnArrIndexes) {
- // Dispatch to the correct tunnel destination port function.
- fixed_dst = fix_destination[fnArrIndex](fixed_dst, var_start_pos, ctx, ssl_netvc, port_is_dynamic);
- }
- ssl_netvc->set_tunnel_destination(fixed_dst, type, port_is_dynamic, tunnel_prewarm);
- Debug("ssl_sni", "Destination now is [%s], configured [%s], fqdn [%s]", fixed_dst.c_str(), destination.c_str(), servername);
- }
+ virtual std::any capture(unsigned idx) const override;
- if (type == SNIRoutingType::BLIND) {
- ssl_netvc->attributes = HttpProxyPort::TRANSPORT_BLIND_TUNNEL;
- }
+ /// Call out from formatting when a replace group is referenced.
+ virtual swoc::BufferWriter &print(swoc::BufferWriter &w, swoc::bwf::Spec const &spec, unsigned idx) const override;
- // ALPN
- for (int id : alpn_ids) {
- ssl_netvc->enableProtocol(id);
- }
- }
+ /// Number of arguments in the pack.
+ virtual unsigned count() const override;
- return SSL_TLSEXT_ERR_OK;
- }
+ private:
+ /// Empty group for when no captures exist.
+ static inline const ActionItem::Context::CapturedGroupViewVec NO_GROUPS;
+ ActionItem::Context::CapturedGroupViewVec const &_groups;
+ };
-private:
- static bool
- is_number(std::string_view s)
- {
- return !s.empty() &&
- std::find_if(std::begin(s), std::end(s), [](std::string_view::value_type c) { return !std::isdigit(c); }) == std::end(s);
- }
+ static constexpr std::string_view MAP_WITH_RECV_PORT_STR = "inbound_local_port";
+ static constexpr std::string_view MAP_WITH_PROXY_PROTOCOL_PORT_STR = "proxy_protocol_port";
Review Comment:
In master, specifying both the `inbound_local_port` and `proxy_protocol_port` results in an error. Looking at the changes it seems all available substitutions are applied by default. Is it possible to preserve the error for this misconfigured case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#discussion_r1177146180
##########
tests/gold_tests/tls/tls_tunnel.test.py:
##########
@@ -96,19 +96,19 @@
"- fqdn: bob.*.com",
" tunnel_route: localhost:{0}".format(server_foo.Variables.SSL_Port),
"- fqdn: '*.match.com'",
- " tunnel_route: $1.testmatch:{0}".format(server_foo.Variables.SSL_Port),
+ ''' tunnel_route: "{{1}}.testmatch:{0}"'''.format(server_foo.Variables.SSL_Port),
"- fqdn: '*.ok.*.com'",
- " tunnel_route: $2.example.$1:{0}".format(server_foo.Variables.SSL_Port),
+ ''' tunnel_route: "{{2}}.example.{{1}}:{0}"'''.format(server_foo.Variables.SSL_Port),
"- fqdn: ''", # No SNI sent
" tunnel_route: localhost:{0}".format(server_bar.Variables.SSL_Port),
"- fqdn: 'incoming.port.com'",
- " tunnel_route: backend.incoming.port.com:{inbound_local_port}",
+ ''' tunnel_route: "backend.incoming.port.com:{inbound_local_port}"''',
"- fqdn: 'proxy.protocol.port.com'",
- " tunnel_route: backend.proxy.protocol.port.com:{proxy_protocol_port}",
+ ''' tunnel_route: "backend.proxy.protocol.port.com:{proxy_protocol_port}"''',
"- fqdn: '*.*.incoming.port.com'",
- " tunnel_route: backend.$1.$2.incoming.port.com:{inbound_local_port}",
+ ''' tunnel_route: "backend.{1}.{2}.incoming.port.com:{inbound_local_port}"''',
Review Comment:
> What happens current if "$3" is present but there are insufficient match groups?
Currently the `$3` would be left as is for this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] commented on pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#issuecomment-1683214210
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#discussion_r1173897404
##########
iocore/net/P_SNIActionPerformer.h:
##########
@@ -92,180 +95,62 @@ class HTTP2BufferWaterMark : public ActionItem
class TunnelDestination : public ActionItem
{
- // ID of the configured variable. This will be used to know which function
- // should be called when processing the tunnel destination.
- enum OpId : int32_t {
- MATCH_GROUPS, // Deal with configured groups.
- MAP_WITH_RECV_PORT, // Use port from inbound local
- MAP_WITH_PROXY_PROTOCOL_PORT, // Use port from the proxy protocol
- MAX // Always at the end and do not change the value of the above items.
+ /// Context used for name binding on tunnel destination.
+ struct BWContext {
+ Context const &_action_ctx; ///< @c ActionItem context.
+ SSLNetVConnection *_vc; ///< Connection object.
};
- static constexpr std::string_view MAP_WITH_RECV_PORT_STR = "{inbound_local_port}";
- static constexpr std::string_view MAP_WITH_PROXY_PROTOCOL_PORT_STR = "{proxy_protocol_port}";
-
-public:
- TunnelDestination(const std::string_view &dest, SNIRoutingType type, YamlSNIConfig::TunnelPreWarm prewarm,
- const std::vector<int> &alpn)
- : destination(dest), type(type), tunnel_prewarm(prewarm), alpn_ids(alpn)
+ /// The container for bound names.
+ using bwf_map_type = swoc::bwf::ContextNames<BWContext const>;
+ /// Argument pack for capture groups.
+ class CaptureArgs : public swoc::bwf::ArgPack
{
- // Check for port variable specification. Note that this is checked before
- // the match group so that the corresponding function can be applied before
- // the match group expansion(when the var_start_pos is still accurate).
- auto recv_port_start_pos = destination.find(MAP_WITH_RECV_PORT_STR);
- auto pp_port_start_pos = destination.find(MAP_WITH_PROXY_PROTOCOL_PORT_STR);
- bool has_recv_port_var = recv_port_start_pos != std::string::npos;
- bool has_pp_port_var = pp_port_start_pos != std::string::npos;
- if (has_recv_port_var && has_pp_port_var) {
- Error("Invalid destination \"%.*s\" in SNI configuration - Only one port variable can be specified.",
- static_cast<int>(destination.size()), destination.data());
- } else if (has_recv_port_var) {
- fnArrIndexes.push_back(OpId::MAP_WITH_RECV_PORT);
- var_start_pos = recv_port_start_pos;
- } else if (has_pp_port_var) {
- fnArrIndexes.push_back(OpId::MAP_WITH_PROXY_PROTOCOL_PORT);
- var_start_pos = pp_port_start_pos;
- }
- // Check for match groups as well.
- if (destination.find_first_of('$') != std::string::npos) {
- fnArrIndexes.push_back(OpId::MATCH_GROUPS);
- }
- }
- ~TunnelDestination() override {}
+ public:
+ CaptureArgs(std::optional<ActionItem::Context::CapturedGroupViewVec> const &groups);
- int
- SNIAction(TLSSNISupport *snis, const Context &ctx) const override
- {
- // Set the netvc option?
- SSLNetVConnection *ssl_netvc = dynamic_cast<SSLNetVConnection *>(snis);
- const char *servername = snis->get_sni_server_name();
- if (ssl_netvc) {
- if (fnArrIndexes.empty()) {
- ssl_netvc->set_tunnel_destination(destination, type, !TLSTunnelSupport::PORT_IS_DYNAMIC, tunnel_prewarm);
- Debug("ssl_sni", "Destination now is [%s], fqdn [%s]", destination.c_str(), servername);
- } else {
- bool port_is_dynamic = false;
- auto fixed_dst{destination};
- // Apply mapping functions to get the final destination.
- for (auto fnArrIndex : fnArrIndexes) {
- // Dispatch to the correct tunnel destination port function.
- fixed_dst = fix_destination[fnArrIndex](fixed_dst, var_start_pos, ctx, ssl_netvc, port_is_dynamic);
- }
- ssl_netvc->set_tunnel_destination(fixed_dst, type, port_is_dynamic, tunnel_prewarm);
- Debug("ssl_sni", "Destination now is [%s], configured [%s], fqdn [%s]", fixed_dst.c_str(), destination.c_str(), servername);
- }
+ virtual std::any capture(unsigned idx) const override;
- if (type == SNIRoutingType::BLIND) {
- ssl_netvc->attributes = HttpProxyPort::TRANSPORT_BLIND_TUNNEL;
- }
+ /// Call out from formatting when a replace group is referenced.
+ virtual swoc::BufferWriter &print(swoc::BufferWriter &w, swoc::bwf::Spec const &spec, unsigned idx) const override;
- // ALPN
- for (int id : alpn_ids) {
- ssl_netvc->enableProtocol(id);
- }
- }
+ /// Number of arguments in the pack.
+ virtual unsigned count() const override;
- return SSL_TLSEXT_ERR_OK;
- }
+ private:
+ /// Empty group for when no captures exist.
+ static inline const ActionItem::Context::CapturedGroupViewVec NO_GROUPS;
+ ActionItem::Context::CapturedGroupViewVec const &_groups;
+ };
-private:
- static bool
- is_number(std::string_view s)
- {
- return !s.empty() &&
- std::find_if(std::begin(s), std::end(s), [](std::string_view::value_type c) { return !std::isdigit(c); }) == std::end(s);
- }
+ static constexpr std::string_view MAP_WITH_RECV_PORT_STR = "inbound_local_port";
+ static constexpr std::string_view MAP_WITH_PROXY_PROTOCOL_PORT_STR = "proxy_protocol_port";
Review Comment:
In the current code, specifying both the `inbound_local_port` and `proxy_protocol_port` results in an error. Looking at the changes it seems all available substitutions are applied by default. Is it possible to preserve the error for this misconfigured case?
##########
tests/gold_tests/tls/tls_tunnel.test.py:
##########
@@ -96,19 +96,19 @@
"- fqdn: bob.*.com",
" tunnel_route: localhost:{0}".format(server_foo.Variables.SSL_Port),
"- fqdn: '*.match.com'",
- " tunnel_route: $1.testmatch:{0}".format(server_foo.Variables.SSL_Port),
+ ''' tunnel_route: "{{1}}.testmatch:{0}"'''.format(server_foo.Variables.SSL_Port),
"- fqdn: '*.ok.*.com'",
- " tunnel_route: $2.example.$1:{0}".format(server_foo.Variables.SSL_Port),
+ ''' tunnel_route: "{{2}}.example.{{1}}:{0}"'''.format(server_foo.Variables.SSL_Port),
"- fqdn: ''", # No SNI sent
" tunnel_route: localhost:{0}".format(server_bar.Variables.SSL_Port),
"- fqdn: 'incoming.port.com'",
- " tunnel_route: backend.incoming.port.com:{inbound_local_port}",
+ ''' tunnel_route: "backend.incoming.port.com:{inbound_local_port}"''',
"- fqdn: 'proxy.protocol.port.com'",
- " tunnel_route: backend.proxy.protocol.port.com:{proxy_protocol_port}",
+ ''' tunnel_route: "backend.proxy.protocol.port.com:{proxy_protocol_port}"''',
"- fqdn: '*.*.incoming.port.com'",
- " tunnel_route: backend.$1.$2.incoming.port.com:{inbound_local_port}",
+ ''' tunnel_route: "backend.{1}.{2}.incoming.port.com:{inbound_local_port}"''',
Review Comment:
Is it valid for `tunnel_route` url to have the `{}` characters? I am not sure if we support url that contains the `{}` at all in ATS since they are considered unsafe and need to be percent-encoded. But if so, there would be issues for:
- When `{abc}` is in the url but `abc` is an invalid name for bwf so would become `~abc~`
- When `{3}` is specified in the url but only two match groups are available.
##########
iocore/net/SNIActionPerformer.cc:
##########
@@ -92,3 +94,111 @@ SNI_IpAllow::TestClientSNIAction(char const *servrername, IpEndpoint const &ep,
{
return ip_addrs.contains(swoc::IPAddr(ep));
}
+
+TunnelDestination::bwf_map_type TunnelDestination::bwf_map;
+
+TunnelDestination::TunnelDestination(const std::string_view &dest, SNIRoutingType type, YamlSNIConfig::TunnelPreWarm prewarm,
+ const std::vector<int> &alpn)
+ : destination(dest), type(type), tunnel_prewarm(prewarm), alpn_ids(alpn)
+{
+ // Get a view of the port text. If there is a substitution there, @a port will end up empty while
+ // rest (stuff after port) will be non-empty. If both are empty there is no port specified at all.
+ std::string_view port_text, rest;
+ swoc::IPEndpoint::tokenize(destination, nullptr, &port_text, &rest);
+ dynamic_port_p = port_text.empty() && !rest.empty();
+
+ Debug("ssl_sni", "port is %s", dynamic_port_p ? "true" : "false");
Review Comment:
-> `"port is dynamic: %s"` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#discussion_r1177146042
##########
iocore/net/P_SNIActionPerformer.h:
##########
@@ -92,180 +95,62 @@ class HTTP2BufferWaterMark : public ActionItem
class TunnelDestination : public ActionItem
{
- // ID of the configured variable. This will be used to know which function
- // should be called when processing the tunnel destination.
- enum OpId : int32_t {
- MATCH_GROUPS, // Deal with configured groups.
- MAP_WITH_RECV_PORT, // Use port from inbound local
- MAP_WITH_PROXY_PROTOCOL_PORT, // Use port from the proxy protocol
- MAX // Always at the end and do not change the value of the above items.
+ /// Context used for name binding on tunnel destination.
+ struct BWContext {
+ Context const &_action_ctx; ///< @c ActionItem context.
+ SSLNetVConnection *_vc; ///< Connection object.
};
- static constexpr std::string_view MAP_WITH_RECV_PORT_STR = "{inbound_local_port}";
- static constexpr std::string_view MAP_WITH_PROXY_PROTOCOL_PORT_STR = "{proxy_protocol_port}";
-
-public:
- TunnelDestination(const std::string_view &dest, SNIRoutingType type, YamlSNIConfig::TunnelPreWarm prewarm,
- const std::vector<int> &alpn)
- : destination(dest), type(type), tunnel_prewarm(prewarm), alpn_ids(alpn)
+ /// The container for bound names.
+ using bwf_map_type = swoc::bwf::ContextNames<BWContext const>;
+ /// Argument pack for capture groups.
+ class CaptureArgs : public swoc::bwf::ArgPack
{
- // Check for port variable specification. Note that this is checked before
- // the match group so that the corresponding function can be applied before
- // the match group expansion(when the var_start_pos is still accurate).
- auto recv_port_start_pos = destination.find(MAP_WITH_RECV_PORT_STR);
- auto pp_port_start_pos = destination.find(MAP_WITH_PROXY_PROTOCOL_PORT_STR);
- bool has_recv_port_var = recv_port_start_pos != std::string::npos;
- bool has_pp_port_var = pp_port_start_pos != std::string::npos;
- if (has_recv_port_var && has_pp_port_var) {
- Error("Invalid destination \"%.*s\" in SNI configuration - Only one port variable can be specified.",
- static_cast<int>(destination.size()), destination.data());
- } else if (has_recv_port_var) {
- fnArrIndexes.push_back(OpId::MAP_WITH_RECV_PORT);
- var_start_pos = recv_port_start_pos;
- } else if (has_pp_port_var) {
- fnArrIndexes.push_back(OpId::MAP_WITH_PROXY_PROTOCOL_PORT);
- var_start_pos = pp_port_start_pos;
- }
- // Check for match groups as well.
- if (destination.find_first_of('$') != std::string::npos) {
- fnArrIndexes.push_back(OpId::MATCH_GROUPS);
- }
- }
- ~TunnelDestination() override {}
+ public:
+ CaptureArgs(std::optional<ActionItem::Context::CapturedGroupViewVec> const &groups);
- int
- SNIAction(TLSSNISupport *snis, const Context &ctx) const override
- {
- // Set the netvc option?
- SSLNetVConnection *ssl_netvc = dynamic_cast<SSLNetVConnection *>(snis);
- const char *servername = snis->get_sni_server_name();
- if (ssl_netvc) {
- if (fnArrIndexes.empty()) {
- ssl_netvc->set_tunnel_destination(destination, type, !TLSTunnelSupport::PORT_IS_DYNAMIC, tunnel_prewarm);
- Debug("ssl_sni", "Destination now is [%s], fqdn [%s]", destination.c_str(), servername);
- } else {
- bool port_is_dynamic = false;
- auto fixed_dst{destination};
- // Apply mapping functions to get the final destination.
- for (auto fnArrIndex : fnArrIndexes) {
- // Dispatch to the correct tunnel destination port function.
- fixed_dst = fix_destination[fnArrIndex](fixed_dst, var_start_pos, ctx, ssl_netvc, port_is_dynamic);
- }
- ssl_netvc->set_tunnel_destination(fixed_dst, type, port_is_dynamic, tunnel_prewarm);
- Debug("ssl_sni", "Destination now is [%s], configured [%s], fqdn [%s]", fixed_dst.c_str(), destination.c_str(), servername);
- }
+ virtual std::any capture(unsigned idx) const override;
- if (type == SNIRoutingType::BLIND) {
- ssl_netvc->attributes = HttpProxyPort::TRANSPORT_BLIND_TUNNEL;
- }
+ /// Call out from formatting when a replace group is referenced.
+ virtual swoc::BufferWriter &print(swoc::BufferWriter &w, swoc::bwf::Spec const &spec, unsigned idx) const override;
- // ALPN
- for (int id : alpn_ids) {
- ssl_netvc->enableProtocol(id);
- }
- }
+ /// Number of arguments in the pack.
+ virtual unsigned count() const override;
- return SSL_TLSEXT_ERR_OK;
- }
+ private:
+ /// Empty group for when no captures exist.
+ static inline const ActionItem::Context::CapturedGroupViewVec NO_GROUPS;
+ ActionItem::Context::CapturedGroupViewVec const &_groups;
+ };
-private:
- static bool
- is_number(std::string_view s)
- {
- return !s.empty() &&
- std::find_if(std::begin(s), std::end(s), [](std::string_view::value_type c) { return !std::isdigit(c); }) == std::end(s);
- }
+ static constexpr std::string_view MAP_WITH_RECV_PORT_STR = "inbound_local_port";
+ static constexpr std::string_view MAP_WITH_PROXY_PROTOCOL_PORT_STR = "proxy_protocol_port";
Review Comment:
@brbzull0 Do you think we should support this? If so, the doc would need to be updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#discussion_r1175719239
##########
iocore/net/P_SNIActionPerformer.h:
##########
@@ -92,180 +95,62 @@ class HTTP2BufferWaterMark : public ActionItem
class TunnelDestination : public ActionItem
{
- // ID of the configured variable. This will be used to know which function
- // should be called when processing the tunnel destination.
- enum OpId : int32_t {
- MATCH_GROUPS, // Deal with configured groups.
- MAP_WITH_RECV_PORT, // Use port from inbound local
- MAP_WITH_PROXY_PROTOCOL_PORT, // Use port from the proxy protocol
- MAX // Always at the end and do not change the value of the above items.
+ /// Context used for name binding on tunnel destination.
+ struct BWContext {
+ Context const &_action_ctx; ///< @c ActionItem context.
+ SSLNetVConnection *_vc; ///< Connection object.
};
- static constexpr std::string_view MAP_WITH_RECV_PORT_STR = "{inbound_local_port}";
- static constexpr std::string_view MAP_WITH_PROXY_PROTOCOL_PORT_STR = "{proxy_protocol_port}";
-
-public:
- TunnelDestination(const std::string_view &dest, SNIRoutingType type, YamlSNIConfig::TunnelPreWarm prewarm,
- const std::vector<int> &alpn)
- : destination(dest), type(type), tunnel_prewarm(prewarm), alpn_ids(alpn)
+ /// The container for bound names.
+ using bwf_map_type = swoc::bwf::ContextNames<BWContext const>;
+ /// Argument pack for capture groups.
+ class CaptureArgs : public swoc::bwf::ArgPack
{
- // Check for port variable specification. Note that this is checked before
- // the match group so that the corresponding function can be applied before
- // the match group expansion(when the var_start_pos is still accurate).
- auto recv_port_start_pos = destination.find(MAP_WITH_RECV_PORT_STR);
- auto pp_port_start_pos = destination.find(MAP_WITH_PROXY_PROTOCOL_PORT_STR);
- bool has_recv_port_var = recv_port_start_pos != std::string::npos;
- bool has_pp_port_var = pp_port_start_pos != std::string::npos;
- if (has_recv_port_var && has_pp_port_var) {
- Error("Invalid destination \"%.*s\" in SNI configuration - Only one port variable can be specified.",
- static_cast<int>(destination.size()), destination.data());
- } else if (has_recv_port_var) {
- fnArrIndexes.push_back(OpId::MAP_WITH_RECV_PORT);
- var_start_pos = recv_port_start_pos;
- } else if (has_pp_port_var) {
- fnArrIndexes.push_back(OpId::MAP_WITH_PROXY_PROTOCOL_PORT);
- var_start_pos = pp_port_start_pos;
- }
- // Check for match groups as well.
- if (destination.find_first_of('$') != std::string::npos) {
- fnArrIndexes.push_back(OpId::MATCH_GROUPS);
- }
- }
- ~TunnelDestination() override {}
+ public:
+ CaptureArgs(std::optional<ActionItem::Context::CapturedGroupViewVec> const &groups);
- int
- SNIAction(TLSSNISupport *snis, const Context &ctx) const override
- {
- // Set the netvc option?
- SSLNetVConnection *ssl_netvc = dynamic_cast<SSLNetVConnection *>(snis);
- const char *servername = snis->get_sni_server_name();
- if (ssl_netvc) {
- if (fnArrIndexes.empty()) {
- ssl_netvc->set_tunnel_destination(destination, type, !TLSTunnelSupport::PORT_IS_DYNAMIC, tunnel_prewarm);
- Debug("ssl_sni", "Destination now is [%s], fqdn [%s]", destination.c_str(), servername);
- } else {
- bool port_is_dynamic = false;
- auto fixed_dst{destination};
- // Apply mapping functions to get the final destination.
- for (auto fnArrIndex : fnArrIndexes) {
- // Dispatch to the correct tunnel destination port function.
- fixed_dst = fix_destination[fnArrIndex](fixed_dst, var_start_pos, ctx, ssl_netvc, port_is_dynamic);
- }
- ssl_netvc->set_tunnel_destination(fixed_dst, type, port_is_dynamic, tunnel_prewarm);
- Debug("ssl_sni", "Destination now is [%s], configured [%s], fqdn [%s]", fixed_dst.c_str(), destination.c_str(), servername);
- }
+ virtual std::any capture(unsigned idx) const override;
- if (type == SNIRoutingType::BLIND) {
- ssl_netvc->attributes = HttpProxyPort::TRANSPORT_BLIND_TUNNEL;
- }
+ /// Call out from formatting when a replace group is referenced.
+ virtual swoc::BufferWriter &print(swoc::BufferWriter &w, swoc::bwf::Spec const &spec, unsigned idx) const override;
- // ALPN
- for (int id : alpn_ids) {
- ssl_netvc->enableProtocol(id);
- }
- }
+ /// Number of arguments in the pack.
+ virtual unsigned count() const override;
- return SSL_TLSEXT_ERR_OK;
- }
+ private:
+ /// Empty group for when no captures exist.
+ static inline const ActionItem::Context::CapturedGroupViewVec NO_GROUPS;
+ ActionItem::Context::CapturedGroupViewVec const &_groups;
+ };
-private:
- static bool
- is_number(std::string_view s)
- {
- return !s.empty() &&
- std::find_if(std::begin(s), std::end(s), [](std::string_view::value_type c) { return !std::isdigit(c); }) == std::end(s);
- }
+ static constexpr std::string_view MAP_WITH_RECV_PORT_STR = "inbound_local_port";
+ static constexpr std::string_view MAP_WITH_PROXY_PROTOCOL_PORT_STR = "proxy_protocol_port";
Review Comment:
Is that misconfigured? Currently it's a misconfiguration because one of them will not be applied and therefore the behavior is not as expected. With this change all instances are replaced and therefore there is no surprise if both are used. I don't see any reason to forbid it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9618: Use BWF for SNI tunnel configuration.
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#discussion_r1175718248
##########
iocore/net/SNIActionPerformer.cc:
##########
@@ -92,3 +94,111 @@ SNI_IpAllow::TestClientSNIAction(char const *servrername, IpEndpoint const &ep,
{
return ip_addrs.contains(swoc::IPAddr(ep));
}
+
+TunnelDestination::bwf_map_type TunnelDestination::bwf_map;
+
+TunnelDestination::TunnelDestination(const std::string_view &dest, SNIRoutingType type, YamlSNIConfig::TunnelPreWarm prewarm,
+ const std::vector<int> &alpn)
+ : destination(dest), type(type), tunnel_prewarm(prewarm), alpn_ids(alpn)
+{
+ // Get a view of the port text. If there is a substitution there, @a port will end up empty while
+ // rest (stuff after port) will be non-empty. If both are empty there is no port specified at all.
+ std::string_view port_text, rest;
+ swoc::IPEndpoint::tokenize(destination, nullptr, &port_text, &rest);
+ dynamic_port_p = port_text.empty() && !rest.empty();
+
+ Debug("ssl_sni", "port is %s", dynamic_port_p ? "true" : "false");
Review Comment:
Yes. The value is the literal string "true" or the literal string "false" from the ternary operator. This is a common pattern for printing boolean values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org