You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by jacksontj <gi...@git.apache.org> on 2016/06/30 17:48:06 UTC
[GitHub] trafficserver pull request #773: TS-4622 potential fix
GitHub user jacksontj opened a pull request:
https://github.com/apache/trafficserver/pull/773
TS-4622 potential fix
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jacksontj/trafficserver TS-4622
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/773.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #773
----
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the issue:
https://github.com/apache/trafficserver/pull/773
@jpeach
I did some testing of my own and I can't reproduce any problems with `TsHttpTxnServerAddrSet()` at all. TSHttpTxnServerAddrSet() is supposed to be called before the DNS lookup. If called then the core bypasses the entire OS_DNS stuff which includes this port setting block in HttpTransact.
I tested calling the API on both the `TS_EVENT_HTTP_READ_REQUEST_HDR` and `TS_EVENT_HTTP_OS_DNS` hooks-- with no problem at all.
You mentioned in a previous comment that it was broken in your testing, can you share the code you were using to see it broken?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/773
\U0001f44d This looks good to me and I think it will be safe w/ "TsHttpTxnServerAddrSet()".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/349/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #773: TS-4622 Use port from SRV response when con...
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/773#discussion_r71536060
--- Diff: proxy/http/HttpTransact.cc ---
@@ -1783,7 +1783,13 @@ HttpTransact::OSDNSLookup(State *s)
// update some state variables with hostdb information that has
// been provided.
ats_ip_copy(&s->server_info.dst_addr, s->host_db_info.ip());
- s->server_info.dst_addr.port() = htons(s->hdr_info.client_request.port_get()); // now we can set the port.
+ // TODO ideally only if ports aren't defined in remap
+ // If the SRV response has a port number, we should honor it. Otherwise we do the port defined in remap
+ if (s->dns_info.srv_port) {
+ s->server_info.dst_addr.port() = htons(s->dns_info.srv_port);
+ } else {
+ s->server_info.dst_addr.port() = htons(s->hdr_info.client_request.port_get()); // now we can set the port.
+ }
--- End diff --
I'll change the conditional-- that definitely makes sense.
From looking at the API callout code-- it does seem like its already broken :/
It seems that `TSHttpTxnServerAddrSet()` is called before the DNS lookup-- and if so it bypasses the [dns lookup| https://github.com/apache/trafficserver/blob/master/proxy/http/HttpSM.cc#L7179]. So, I would think in this section of HttpTransact we would want to skip all port setting if `api_server_addr_set` is true.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/361/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 User port from SRV response when connectin...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/773
-1, this breaks ``TSHttpTxnServerAddrSet``.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the issue:
https://github.com/apache/trafficserver/pull/773
@jpeach `HttpTransact::OSDNSLookup` seems to be the correct place-- as it is called immediately after the DNS lookup is complete-- and is what sets the port numbers for the destination.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 User port from SRV response when connectin...
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the issue:
https://github.com/apache/trafficserver/pull/773
Yea, I'll have to look a bit more. Ideally its as close to that DNS lookup as possible, that way if some plugin wants to overwrite it I've done the change in core before they can (so that we won't be clobbering eachother). Have to look into it this week.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 User port from SRV response when connectin...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/284/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 User port from SRV response when connectin...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/388/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #773: TS-4622 Use port from SRV response when con...
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj closed the pull request at:
https://github.com/apache/trafficserver/pull/773
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/455/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/362/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/464/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/465/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/360/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 User port from SRV response when connectin...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/773
@jacksontj Maybe the right place to do this is when ``HttpSM::set_next_state()
`` handles ``SM_ACTION_DNS_LOOKUP``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #773: TS-4622 Use port from SRV response when con...
Posted by JamesPeach <gi...@git.apache.org>.
Github user JamesPeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/773#discussion_r71447768
--- Diff: proxy/http/HttpTransact.cc ---
@@ -1783,7 +1783,13 @@ HttpTransact::OSDNSLookup(State *s)
// update some state variables with hostdb information that has
// been provided.
ats_ip_copy(&s->server_info.dst_addr, s->host_db_info.ip());
- s->server_info.dst_addr.port() = htons(s->hdr_info.client_request.port_get()); // now we can set the port.
+ // TODO ideally only if ports aren't defined in remap
+ // If the SRV response has a port number, we should honor it. Otherwise we do the port defined in remap
+ if (s->dns_info.srv_port) {
+ s->server_info.dst_addr.port() = htons(s->dns_info.srv_port);
+ } else {
+ s->server_info.dst_addr.port() = htons(s->hdr_info.client_request.port_get()); // now we can set the port.
+ }
--- End diff --
If a plugin used ``TSHttpTxnServerAddrSet()``, then at this point ``dst_addr`` may be the address set by the plugin in which case we should not clobber it. However it is already being clobbered from the request URL, so maybe the cases I tested were already broken (I was using port 80 and 443).
Setting the port from the SRV record should probably be conditional on ``srv_lookup_success``.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/458/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/354/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/773
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/466/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---