You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/02/10 21:40:14 UTC
[GitHub] [trafficserver] dbhat opened a new pull request #7516: Adding TCP Info header support to header rewrite
dbhat opened a new pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516
Opening new pull request. This is squashed commit of closed PR #7441.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] dbhat commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
dbhat commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r605277149
##########
File path: doc/admin-guide/plugins/header_rewrite.en.rst
##########
@@ -491,6 +491,15 @@ SSN-TXN-COUNT
Returns the number of transactions between the Traffic Server proxy and the origin server from a single session.
Any value greater than zero indicates connection reuse.
+TCP-INFO
+~~~~~~~~
+::
+
+ cond %{<name>}
+ add-header @PropertyName "%{TCP-INFO}"
+
+This operation records TCP Info struct field values as an Internal remap as well as global header at the event hook specified by the condition. Supported hook conditions include TXN_START_HOOK, SEND_RESPONSE_HEADER_HOOK and TXN_CLOSE_HOOK in the Global plugin and REMAP_PSEUDO_HOOK, SEND_RESPONSE_HEADER_HOOK and TXN_CLOSE_HOOK in the Remap plugin. Conditions supported as request headers include TXN_START_HOOK and REMAP_PSEUDO_HOOK. The other conditions are supported as response headers. TCP Info fields currently recorded include rtt, rto, snd_cwnd and all_retrans. This operation is not supported on internal transactions.
+
Review comment:
Reworded to clarify.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-817208474
Cherry-picked to v9.1.x branch.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-811558350
[approve ci]
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r596629201
##########
File path: plugins/header_rewrite/resources.cc
##########
@@ -99,6 +104,24 @@ Resources::gather(const ResourceIDs ids, TSHttpHookID hook)
}
break;
+ case TS_HTTP_TXN_START_HOOK:
+ // Get TCP Info at transaction start
+ if (client_bufp && client_hdr_loc) {
Review comment:
Actually, the pointers are available. We can't read request headers, but can add headers.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r575511198
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1333,77 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ TSReturnCode tsSsn;
+ int fd;
+ struct tcp_info info;
+ socklen_t tcp_info_len = sizeof(info);
+ tsSsn = TSHttpTxnClientFdGet(res.txnp, &fd);
+ if (tsSsn != TS_SUCCESS || fd <= 0) {
+ TSDebug(PLUGIN_NAME, "error getting the client socket fd from ssn");
+ }
+ if (getsockopt(fd, IPPROTO_TCP, TCP_INFO, &info, &tcp_info_len) != 0) {
+ TSDebug(PLUGIN_NAME, "getsockopt(%d, TCP_INFO) failed: %s", fd, strerror(errno));
+ }
+
+ if (tsSsn == TS_SUCCESS) {
+ uint32_t rtt = info.tcpi_rtt;
+ uint32_t rto = info.tcpi_rto;
+ uint32_t snd_cwnd = info.tcpi_snd_cwnd;
Review comment:
why do you need these intermediary / temp variables? Just use info.tcp_rtt instead, it's much clearer, and less waste.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 edited a comment on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
ezelkow1 edited a comment on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-777898892
Since I wrote that lvalue test, I wonder if this is a curl difference popping up maybe? The test is fairly simple to just check for a regression where the `[L]` end of a header rewrite rule was being leaked back into the header it was writing. So thats all its doing is adding a header and making sure `[L]` isnt in there
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r596609684
##########
File path: plugins/header_rewrite/resources.cc
##########
@@ -87,6 +87,11 @@ Resources::gather(const ResourceIDs ids, TSHttpHookID hook)
TSDebug(PLUGIN_NAME, "\tAdding TXN client response status resource");
resp_status = TSHttpHdrStatusGet(bufp, hdr_loc);
}
+ if (client_bufp && client_hdr_loc) {
Review comment:
I'm not familiar with this area but this block doesn't seem to be correct. If I read the code correctly, `bufp` and `hdr_loc` point to headers that will be modified on current event (hook). Since this block is under `case TS_HTTP_SEND_RESPONSE_HDR_HOOK:`, those should point to response headers.
`TSHttpTxnClientRespGet` sets the right pointers but this block overwrites those with the ones for client request headers.
Removing this block fixes header_rewrite_l_value and header_rewrite_cond_ssn_txn_count at minimum.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r596098453
##########
File path: plugins/header_rewrite/parser.cc
##########
@@ -244,6 +244,14 @@ Parser::cond_is_hook(TSHttpHookID &hook) const
hook = TS_REMAP_PSEUDO_HOOK;
return true;
}
+ if ("TXN_START_HOOK" == _op) {
Review comment:
Yeh, but I'm thinking it should maybe be a warning or even an error if TXN_START_HOOK is used in a remap plugin.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] dbhat commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
dbhat commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r604630193
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1344,71 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ TSReturnCode tsSsn;
+ int fd;
+ struct tcp_info info;
+ socklen_t tcp_info_len = sizeof(info);
+ tsSsn = TSHttpTxnClientFdGet(res.txnp, &fd);
+ if (tsSsn != TS_SUCCESS || fd <= 0) {
Review comment:
Implemented (a) as follows:
```
if (TSHttpTxnIsInternal(res.txnp) == 1) {
TSDebug(PLUGIN_NAME, "No TCP-INFO available for internal transactions");
return;
}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-777896667
header_rewrite_l_value test failed once on the previous PR as well. It may happen intermittently.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] dbhat commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
dbhat commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r590669690
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1333,77 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
Review comment:
Decided to keep the check and insert "-" when TCP_INFO is not available
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 edited a comment on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
ezelkow1 edited a comment on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-777899538
if you check the autest, theres actually a fair amount of tests that fail all around there, not just lvalue, but this is that one tests output
` Reason: File differences
Gold File : /var/jenkins/workspace/autest-github/src/tests/gold_tests/pluginTest/header_rewrite/gold/header_rewrite-l_value.gold
Data File : /var/tmp/ausb-7516.14844/header_rewrite_l_value/_output/0-tr-Default/stream.stderr.txt
* Trying 127.0.0.1:61289...
* TCP_NODELAY set
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0* Connected to 127.0.0.1 (127.0.0.1) port 61289 (#0)
> GET http://www.example.com/ HTTP/1.1
> Host: www.example.com
> User-Agent: curl/7.65.0-DEV
> Accept: */*
> Proxy-Connection: Keep-Alive
> Proxy-Connection: keep-alive
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Date: Thu, 11 Feb 2021 23:10:39 GMT
< Age: 0
< Transfer-Encoding: chunked
< Proxy-Connection: keep-alive
- < Server: ATS/``
? ^^
+ < Server: ATS/10.0.0
? ^^^^^^
- < X-First: First
- < X-Last: Last
<
{ [5 bytes data]
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
* Connection #0 to host 127.0.0.1 left intact`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-810503823
> I'm also curious on how this information is intended to be used. If the intention is for mainly exposing the TCP-INFO to access logs, I wonder if it makes sense to consider using log tags instead of header-rewrite operator?
Yeh, primarily for logs. Moving it to the core is an option, but the thought was to avoid polluting the core more with a bunch of #ifdef's (and also makes it more difficult for people to make custom additions or changes). But if the consensus is that it'd be better to move this to the core, rather than a plugin, I'm fine with that too.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] dbhat commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
dbhat commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r596148150
##########
File path: plugins/header_rewrite/parser.cc
##########
@@ -244,6 +244,14 @@ Parser::cond_is_hook(TSHttpHookID &hook) const
hook = TS_REMAP_PSEUDO_HOOK;
return true;
}
+ if ("TXN_START_HOOK" == _op) {
Review comment:
Right now we include all hooks in [TSHttpHookID](https://github.com/apache/trafficserver/blob/d4fc16f64d3104d3682d496e47f2a53511fa77e0/include/ts/apidefs.h.in#L266) without filtering for ones that are applicable:
https://github.com/apache/trafficserver/blob/d4fc16f64d3104d3682d496e47f2a53511fa77e0/plugins/header_rewrite/header_rewrite.cc#L451
We do not have errors/warnings for non-applicable hooks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-811558050
CI jobs were not started somehow. Trying to start them manually. [approve ci]
Also, squashing commits is no longer required on author's side. It will be done automatically when a committer click the merge button.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r575510929
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1333,77 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ TSReturnCode tsSsn;
+ int fd;
+ struct tcp_info info;
+ socklen_t tcp_info_len = sizeof(info);
+ tsSsn = TSHttpTxnClientFdGet(res.txnp, &fd);
+ if (tsSsn != TS_SUCCESS || fd <= 0) {
+ TSDebug(PLUGIN_NAME, "error getting the client socket fd from ssn");
+ }
+ if (getsockopt(fd, IPPROTO_TCP, TCP_INFO, &info, &tcp_info_len) != 0) {
+ TSDebug(PLUGIN_NAME, "getsockopt(%d, TCP_INFO) failed: %s", fd, strerror(errno));
+ }
+
+ if (tsSsn == TS_SUCCESS) {
+ uint32_t rtt = info.tcpi_rtt;
+ uint32_t rto = info.tcpi_rto;
+ uint32_t snd_cwnd = info.tcpi_snd_cwnd;
+ uint32_t all_retx;
+#if !defined(freebsd) || defined(__GLIBC__)
+ all_retx = info.tcpi_retrans;
+#else
+ all_retx = info.__tcpi_retrans;
+#endif
+
+ if (tcp_info_len > 0) {
+ s += std::to_string(rtt);
Review comment:
Something like this is probably more efficient:
```
char buf[12 * 4 + 9]; // 4 uint32's + 4x "; " + '\0'
snprintf(buf, sizeof(buf), "%u; %u; %u; %u; ", rtt, rto, snd_cwnd, all_rtx);
s += buf;
```
Although, use PRIu32 in the printf.
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1333,77 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ TSReturnCode tsSsn;
+ int fd;
+ struct tcp_info info;
+ socklen_t tcp_info_len = sizeof(info);
+ tsSsn = TSHttpTxnClientFdGet(res.txnp, &fd);
+ if (tsSsn != TS_SUCCESS || fd <= 0) {
+ TSDebug(PLUGIN_NAME, "error getting the client socket fd from ssn");
+ }
+ if (getsockopt(fd, IPPROTO_TCP, TCP_INFO, &info, &tcp_info_len) != 0) {
+ TSDebug(PLUGIN_NAME, "getsockopt(%d, TCP_INFO) failed: %s", fd, strerror(errno));
+ }
+
+ if (tsSsn == TS_SUCCESS) {
+ uint32_t rtt = info.tcpi_rtt;
+ uint32_t rto = info.tcpi_rto;
+ uint32_t snd_cwnd = info.tcpi_snd_cwnd;
+ uint32_t all_retx;
+#if !defined(freebsd) || defined(__GLIBC__)
+ all_retx = info.tcpi_retrans;
+#else
+ all_retx = info.__tcpi_retrans;
+#endif
+
+ if (tcp_info_len > 0) {
+ s += std::to_string(rtt);
Review comment:
Something like this is probably more efficient:
```
char buf[12 * 4 + 9]; // 4x uint32's + 4x "; " + '\0'
snprintf(buf, sizeof(buf), "%u; %u; %u; %u; ", rtt, rto, snd_cwnd, all_rtx);
s += buf;
```
Although, use PRIu32 in the printf.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r575513606
##########
File path: doc/admin-guide/plugins/header_rewrite.en.rst
##########
@@ -491,6 +491,14 @@ SSN-TXN-COUNT
Returns the number of transactions between the Traffic Server proxy and the origin server from a single session.
Any value greater than zero indicates connection reuse.
+TCPINFO
+~~~~~~~~
+::
+ cond %{<name>}
+ add-header @PropertyName "<field_name>:{%{TCPINFO}}"
Review comment:
this looks weird, the syntax would be, I assume
```
add-header @PropertyName "%{TCPIINFO}"
```
no ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-810511497
> > I'm also curious on how this information is intended to be used. If the intention is for mainly exposing the TCP-INFO to access logs, I wonder if it makes sense to consider using log tags instead of header-rewrite operator?
>
> Yeh, primarily for logs. Moving it to the core is an option, but the thought was to avoid polluting the core more with a bunch of #ifdef's (and also makes it more difficult for people to make custom additions or changes). But if the consensus is that it'd be better to move this to the core, rather than a plugin, I'm fine with that too.
Makes sense. The only downside (that I can think of), for not having it as first class log tags is that it may not be "obvious" that this capability even exists, but, I guess the pros of being able to further customize it is definitely appealing. Also, I think if when we want to add this to log tags, it can co-exist with the header-rewrite operator too like a few other similar context info that we support in header-rewrite. Plus, there may be some legit use cases where someone may actually want to transport this info along to their downstream services for offline processing and supporting it via header-rewrite does make that part easier.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r596615696
##########
File path: plugins/header_rewrite/resources.cc
##########
@@ -99,6 +104,24 @@ Resources::gather(const ResourceIDs ids, TSHttpHookID hook)
}
break;
+ case TS_HTTP_TXN_START_HOOK:
+ // Get TCP Info at transaction start
+ if (client_bufp && client_hdr_loc) {
Review comment:
If I'm reading the code correctly, this condition would never be true because request header has not been read on `TS_HTTP_TXN_START_HOOK` yet.
https://docs.trafficserver.apache.org/en/9.0.x/developer-guide/plugins/hooks-and-transactions/index.en.html
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-811610722
> I thought it defaults to last used option
That's true, however, we disabled the other options on this reporisotry. So we can't merge PRs without squashing commits.
<img width="266" alt="image" src="https://user-images.githubusercontent.com/153144/113239233-4e138400-92e5-11eb-8378-a8124fe9edb6.png">
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] dbhat commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
dbhat commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r605276996
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1344,71 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ TSReturnCode tsSsn;
+ int fd;
+ struct tcp_info info;
+ socklen_t tcp_info_len = sizeof(info);
+ tsSsn = TSHttpTxnClientFdGet(res.txnp, &fd);
+ if (tsSsn != TS_SUCCESS || fd <= 0) {
Review comment:
Changed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop edited a comment on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop edited a comment on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-810503823
> I'm also curious on how this information is intended to be used. If the intention is for mainly exposing the TCP-INFO to access logs, I wonder if it makes sense to consider using log tags instead of header-rewrite operator?
Yeh, primarily for logs. Moving it to the core is an option, but the thought was to avoid polluting the core more with a bunch of #ifdef's (and also makes it more difficult for people to make custom additions or changes). But if the consensus is that it'd be better to move this to the core, rather than a plugin, I'm fine with that too.
Also, and this might be complicated, is that you may very likely want to have multiple TCPinfo log fields, for different hooks. Like, captured at the beginning of a session, and then at the end of each TXN.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-777834635
[approve ci autest]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-802620342
[approve ci autest]
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
sudheerv commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r605016439
##########
File path: doc/admin-guide/plugins/header_rewrite.en.rst
##########
@@ -491,6 +491,15 @@ SSN-TXN-COUNT
Returns the number of transactions between the Traffic Server proxy and the origin server from a single session.
Any value greater than zero indicates connection reuse.
+TCP-INFO
+~~~~~~~~
+::
+
+ cond %{<name>}
+ add-header @PropertyName "%{TCP-INFO}"
+
+This operation records TCP Info struct field values as an Internal remap as well as global header at the event hook specified by the condition. Supported hook conditions include TXN_START_HOOK, SEND_RESPONSE_HEADER_HOOK and TXN_CLOSE_HOOK in the Global plugin and REMAP_PSEUDO_HOOK, SEND_RESPONSE_HEADER_HOOK and TXN_CLOSE_HOOK in the Remap plugin. Conditions supported as request headers include TXN_START_HOOK and REMAP_PSEUDO_HOOK. The other conditions are supported as response headers. TCP Info fields currently recorded include rtt, rto, snd_cwnd and all_retrans. This operation is not supported on internal transactions.
+
Review comment:
Perhaps, reword it as
"This operation is not supported on transactions originated within Traffic Server (for e.g using the TS API TSHttpTxnIsInternal() with a ref to the API doc link)".
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] dbhat commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
dbhat commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r590670962
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1333,77 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
Review comment:
This one was not required since TCP_INFO was being checked elsewhere in the code. Added an _ifndef_:
```
// This is a bit of a hack, to get the more linux specific tcp_info struct ...
#if HAVE_STRUCT_LINUX_TCP_INFO
#ifndef _LINUX_TCP_H
#define _LINUX_TCP_H
#endif
#elif HAVE_NETINET_IN_H
#ifndef _NETINET_TCP_H
#define _NETINET_TCP_H
#endif
#endif
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] dbhat commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
dbhat commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-799814472
@zwoop: Done with changes, requesting review
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r575516613
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1333,77 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
Review comment:
Is there a missing set of #include's here somewhere ? Don't you need something similar to the include jive from the old tcpinfo plugin?
```
// This is a bit of a hack, to get the more linux specific tcp_info struct ...
#if HAVE_STRUCT_LINUX_TCP_INFO
#include <linux/tcp.h>
#elif HAVE_NETINET_IN_H
#include <netinet/tcp.h>
#endif
```
Of course, if you don't need the linux specific ones, only include the netinet/tcp.h include, but assuming this even compiles, something else is dragging in tcp.h? But depending on that is not good. It seems lulu.h includes netinet/in.h, so maybe add a netinet/tcp.h there too ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
sudheerv commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r605014663
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1344,71 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ TSReturnCode tsSsn;
+ int fd;
+ struct tcp_info info;
+ socklen_t tcp_info_len = sizeof(info);
+ tsSsn = TSHttpTxnClientFdGet(res.txnp, &fd);
+ if (tsSsn != TS_SUCCESS || fd <= 0) {
Review comment:
Looks good. A minor nit though is to change it to just ```if (TSHttpTxnIsInternal(res.txnp)) {``` (== 1 should be implied).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r575512844
##########
File path: doc/admin-guide/plugins/header_rewrite.en.rst
##########
@@ -935,6 +943,16 @@ cache updates may have been performed. This hook context provides a means to
modify aspects of the response sent to a client, while still caching the
original versions of those attributes delivered by the origin server.
+TXN_START_HOOK
+~~~~~~~~~~~~~~
+
+Rulesets are evaluated when |TS| receives a request and accepts it. This hook context indicates that a HTTP transaction is initiated.
Review comment:
At a minimum, I think we need to document that this would only work as a global plugin.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras removed a comment on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
ywkaras removed a comment on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-777834635
[approve ci autest]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r575513606
##########
File path: doc/admin-guide/plugins/header_rewrite.en.rst
##########
@@ -491,6 +491,14 @@ SSN-TXN-COUNT
Returns the number of transactions between the Traffic Server proxy and the origin server from a single session.
Any value greater than zero indicates connection reuse.
+TCPINFO
+~~~~~~~~
+::
+ cond %{<name>}
+ add-header @PropertyName "<field_name>:{%{TCPINFO}}"
Review comment:
this looks weird, the syntax would be, I assume
```
add-header @PropertyName %{TCPIINFO}
```
no ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
sudheerv commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r604354461
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1344,71 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ TSReturnCode tsSsn;
+ int fd;
+ struct tcp_info info;
+ socklen_t tcp_info_len = sizeof(info);
+ tsSsn = TSHttpTxnClientFdGet(res.txnp, &fd);
+ if (tsSsn != TS_SUCCESS || fd <= 0) {
Review comment:
To @zwoop 's point, I wonder how this would work for "internal" (PluginVC) transactions, as there is no real client socket involved for those transactions. We could either (a) not allow adding TCP-INFO for those transactions OR (b) ensure we handle it gracefully - ie fill it with some dummy values.
My preference is for (a) as long as we document 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r575510929
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1333,77 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ TSReturnCode tsSsn;
+ int fd;
+ struct tcp_info info;
+ socklen_t tcp_info_len = sizeof(info);
+ tsSsn = TSHttpTxnClientFdGet(res.txnp, &fd);
+ if (tsSsn != TS_SUCCESS || fd <= 0) {
+ TSDebug(PLUGIN_NAME, "error getting the client socket fd from ssn");
+ }
+ if (getsockopt(fd, IPPROTO_TCP, TCP_INFO, &info, &tcp_info_len) != 0) {
+ TSDebug(PLUGIN_NAME, "getsockopt(%d, TCP_INFO) failed: %s", fd, strerror(errno));
+ }
+
+ if (tsSsn == TS_SUCCESS) {
+ uint32_t rtt = info.tcpi_rtt;
+ uint32_t rto = info.tcpi_rto;
+ uint32_t snd_cwnd = info.tcpi_snd_cwnd;
+ uint32_t all_retx;
+#if !defined(freebsd) || defined(__GLIBC__)
+ all_retx = info.tcpi_retrans;
+#else
+ all_retx = info.__tcpi_retrans;
+#endif
+
+ if (tcp_info_len > 0) {
+ s += std::to_string(rtt);
Review comment:
Something like this is probably more efficient:
```
char buf[12 * 4 + 5]; // 4 uint32's
snprintf(buf, sizeof(buf), "%u; %u; %u; %u; ", rtt, rto, snd_cwnd, all_rtx);
s += buf;
```
Although, use PRIu32 in the printf.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-811573976
> CI jobs were not started somehow. Trying to start them manually. [approve ci]
>
> Also, squashing commits is no longer required on author's side. It will be done automatically when a committer click the merge button.
Hmm..I thought it defaults to last used option and not always ensure squash and commit option during merge. Assuming that is the case, I’d always remind the author to squash before merging (doesn’t really matter if it’s done manually or using the GitHub UI option)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-777898892
Since I wrote that lvalue test, I wonder if this is a curl difference popping up maybe? The test is failure simple to just check for a regression where the `[L]` end of a header rewrite rule was being leaked back into the header it was writing. So thats all its doing is adding a header and making sure `[L]` isnt in there
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r575510929
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1333,77 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ TSReturnCode tsSsn;
+ int fd;
+ struct tcp_info info;
+ socklen_t tcp_info_len = sizeof(info);
+ tsSsn = TSHttpTxnClientFdGet(res.txnp, &fd);
+ if (tsSsn != TS_SUCCESS || fd <= 0) {
+ TSDebug(PLUGIN_NAME, "error getting the client socket fd from ssn");
+ }
+ if (getsockopt(fd, IPPROTO_TCP, TCP_INFO, &info, &tcp_info_len) != 0) {
+ TSDebug(PLUGIN_NAME, "getsockopt(%d, TCP_INFO) failed: %s", fd, strerror(errno));
+ }
+
+ if (tsSsn == TS_SUCCESS) {
+ uint32_t rtt = info.tcpi_rtt;
+ uint32_t rto = info.tcpi_rto;
+ uint32_t snd_cwnd = info.tcpi_snd_cwnd;
+ uint32_t all_retx;
+#if !defined(freebsd) || defined(__GLIBC__)
+ all_retx = info.tcpi_retrans;
+#else
+ all_retx = info.__tcpi_retrans;
+#endif
+
+ if (tcp_info_len > 0) {
+ s += std::to_string(rtt);
Review comment:
Something like this is probably more efficient:
```
char buf[12 * 4 + 10]; // 4 uint32's
snprintf(buf, sizeof(buf), "%u; %u; %u; %u; ", rtt, rto, snd_cwnd, all_rtx);
s += buf;
```
Although, use PRIu32 in the printf.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-777899538
if you check the autest, theres actually a fair amount of tests that fail all around there, not just lvalue, but this is that one tests output
` Reason: File differences
Gold File : /var/jenkins/workspace/autest-github/src/tests/gold_tests/pluginTest/header_rewrite/gold/header_rewrite-l_value.gold
Data File : /var/tmp/ausb-7516.14844/header_rewrite_l_value/_output/0-tr-Default/stream.stderr.txt
* Trying 127.0.0.1:61289...
* TCP_NODELAY set
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0* Connected to 127.0.0.1 (127.0.0.1) port 61289 (#0)
> GET http://www.example.com/ HTTP/1.1
> Host: www.example.com
> User-Agent: curl/7.65.0-DEV
> Accept: */*
> Proxy-Connection: Keep-Alive
> Proxy-Connection: keep-alive
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Date: Thu, 11 Feb 2021 23:10:39 GMT
< Age: 0
< Transfer-Encoding: chunked
< Proxy-Connection: keep-alive
- < Server: ATS/``
? ^^
+ < Server: ATS/10.0.0
? ^^^^^^
- < X-First: First
- < X-Last: Last
<
{ [5 bytes data]
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
* Connection #0 to host 127.0.0.1 left intact`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r596633578
##########
File path: plugins/header_rewrite/resources.cc
##########
@@ -99,6 +104,24 @@ Resources::gather(const ResourceIDs ids, TSHttpHookID hook)
}
break;
+ case TS_HTTP_TXN_START_HOOK:
+ // Get TCP Info at transaction start
+ if (client_bufp && client_hdr_loc) {
+ TSDebug(PLUGIN_NAME, "\tAdding TXN Start header buffers");
+ bufp = client_bufp;
+ hdr_loc = client_hdr_loc;
+ }
+ break;
+
+ case TS_HTTP_TXN_CLOSE_HOOK:
+ // Get TCP Info at transaction close
+ if (client_bufp && client_hdr_loc) {
+ TSDebug(PLUGIN_NAME, "\tAdding TXN Close header buffers");
+ bufp = client_bufp;
Review comment:
I'd expect headers added on TS_HTTP_TXN_CLOSE_HOOK are available in client *response* header. This code seems like to add headers to client *request* headers.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] dbhat commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
dbhat commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r604630193
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1344,71 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ TSReturnCode tsSsn;
+ int fd;
+ struct tcp_info info;
+ socklen_t tcp_info_len = sizeof(info);
+ tsSsn = TSHttpTxnClientFdGet(res.txnp, &fd);
+ if (tsSsn != TS_SUCCESS || fd <= 0) {
Review comment:
Implemented (a) as follows:
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit merged pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
maskit merged pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] dbhat commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
dbhat commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-777060661
Requesting @bryancall for review
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-780089818
[approve ci autest]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r575512431
##########
File path: plugins/header_rewrite/parser.cc
##########
@@ -244,6 +244,14 @@ Parser::cond_is_hook(TSHttpHookID &hook) const
hook = TS_REMAP_PSEUDO_HOOK;
return true;
}
+ if ("TXN_START_HOOK" == _op) {
Review comment:
I don't remember how we deal with things like this, but this hook would not be viable when invoked as a remap plugin. TXN_START happens before that; meaning, this can only be used when running header_rewrite as a global plugin.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-777849238
[approve ci autest]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 edited a comment on pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
ezelkow1 edited a comment on pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#issuecomment-777899538
if you check the autest, theres actually a fair amount of tests that fail all around there, not just lvalue, but this is that one tests output
> Reason: File differences
Gold File : /var/jenkins/workspace/autest-github/src/tests/gold_tests/pluginTest/header_rewrite/gold/header_rewrite-l_value.gold
Data File : /var/tmp/ausb-7516.14844/header_rewrite_l_value/_output/0-tr-Default/stream.stderr.txt
* Trying 127.0.0.1:61289...
* TCP_NODELAY set
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0* Connected to 127.0.0.1 (127.0.0.1) port 61289 (#0)
> GET http://www.example.com/ HTTP/1.1
> Host: www.example.com
> User-Agent: curl/7.65.0-DEV
> Accept: */*
> Proxy-Connection: Keep-Alive
> Proxy-Connection: keep-alive
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Date: Thu, 11 Feb 2021 23:10:39 GMT
< Age: 0
< Transfer-Encoding: chunked
< Proxy-Connection: keep-alive
- < Server: ATS/``
? ^^
+ < Server: ATS/10.0.0
? ^^^^^^
- < X-First: First
- < X-Last: Last
<
{ [5 bytes data]
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
* Connection #0 to host 127.0.0.1 left intact
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r575511663
##########
File path: plugins/header_rewrite/factory.cc
##########
@@ -139,6 +139,8 @@ condition_factory(const std::string &cond)
c = new ConditionInbound();
} else if (c_name == "SSN-TXN-COUNT") {
c = new ConditionSessionTransactCount();
+ } else if (c_name == "TCPINFO") {
Review comment:
For consistency with other conditions, I think a name like
```
TCP-INFO
```
would be preferable. But I don't feel strongly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] dbhat commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
dbhat commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r590669153
##########
File path: plugins/header_rewrite/parser.cc
##########
@@ -244,6 +244,14 @@ Parser::cond_is_hook(TSHttpHookID &hook) const
hook = TS_REMAP_PSEUDO_HOOK;
return true;
}
+ if ("TXN_START_HOOK" == _op) {
Review comment:
If TXN_START is invoked in remap, the header value is not added.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r575514790
##########
File path: plugins/header_rewrite/conditions.cc
##########
@@ -1333,4 +1333,77 @@ ConditionSessionTransactCount::append_value(std::string &s, Resources const &res
TSDebug(PLUGIN_NAME, "Appending SSN-TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
s.append(value, length);
}
-}
\ No newline at end of file
+}
+
+void
+ConditionTcpInfo::initialize(Parser &p)
+{
+ Condition::initialize(p);
+ TSDebug(PLUGIN_NAME, "Initializing TCP Info");
+ MatcherType *match = new MatcherType(_cond_op);
+ std::string const &arg = p.get_arg();
+
+ match->set(strtol(arg.c_str(), nullptr, 10));
+ _matcher = match;
+}
+
+void
+ConditionTcpInfo::initialize_hooks()
+{
+ add_allowed_hook(TS_HTTP_TXN_START_HOOK);
+ add_allowed_hook(TS_HTTP_TXN_CLOSE_HOOK);
+ add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK);
+}
+
+bool
+ConditionTcpInfo::eval(const Resources &res)
+{
+ std::string s;
+
+ append_value(s, res);
+ bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+ TSDebug(PLUGIN_NAME, "Evaluating TCP-Info: %s - rval: %d", s.c_str(), rval);
+
+ return rval;
+}
+
+void
+ConditionTcpInfo::append_value(std::string &s, Resources const &res)
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
Review comment:
I wonder in the case of there being no TCP_INFO available on the platform (for whatever reason, seems odd on its own), should it not at least set something here, such as "-" ?
Alternatively, since we have eliminated Solaris, and only support macOS, Linux and FreeBSD, maybe we don't need this check at all, and assume that TCP_INFO is available ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] dbhat commented on a change in pull request #7516: Adding TCP Info header support to header rewrite
Posted by GitBox <gi...@apache.org>.
dbhat commented on a change in pull request #7516:
URL: https://github.com/apache/trafficserver/pull/7516#discussion_r596148150
##########
File path: plugins/header_rewrite/parser.cc
##########
@@ -244,6 +244,14 @@ Parser::cond_is_hook(TSHttpHookID &hook) const
hook = TS_REMAP_PSEUDO_HOOK;
return true;
}
+ if ("TXN_START_HOOK" == _op) {
Review comment:
I believe a warning would suffice.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org