TS-4622 Use port from SRV response when connecting to origin #773

jacksontj · 2016-06-30T17:48:01Z

No description provided.

This closes apache#773

jpeach · 2016-06-30T17:57:55Z

-1, this breaks TSHttpTxnServerAddrSet.

atsci · 2016-06-30T17:58:50Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/388/ for details.

atsci · 2016-06-30T18:03:50Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/284/ for details.

jpeach · 2016-06-30T19:13:38Z

@jacksontj Maybe the right place to do this is when HttpSM::set_next_state() handles SM_ACTION_DNS_LOOKUP?

jacksontj · 2016-07-18T16:22:27Z

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.

jacksontj · 2016-07-19T16:47:36Z

@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.

atsci · 2016-07-19T16:58:18Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/455/ for details.

atsci · 2016-07-19T16:58:58Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/349/ for details.

JamesPeach · 2016-07-20T00:22:48Z

proxy/http/HttpTransact.cc

+    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.
+  }


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.

I'll change the conditional-- that definitely makes sense.

atsci · 2016-07-20T14:48:11Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/458/ for details.

atsci · 2016-07-20T14:54:01Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/354/ for details.

jacksontj · 2016-07-20T14:58:04Z

@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?

jpeach · 2016-07-21T01:55:39Z

👍 This looks good to me and I think it will be safe w/ "TsHttpTxnServerAddrSet()".

This closes apache#773

atsci · 2016-07-21T02:08:25Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/464/ for details.

atsci · 2016-07-21T02:13:51Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/360/ for details.

atsci · 2016-07-21T02:15:11Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/465/ for details.

This closes apache#773

atsci · 2016-07-21T14:48:14Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/466/ for details.

atsci · 2016-07-21T14:54:08Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/361/ for details.

atsci · 2016-07-21T15:27:46Z

Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/362/ for details.

…pache#773)" This reverts commit d91457b. Github UI squashed the tests and the code change-- we want them separate

This closes apache#773

* asf/master: (44 commits) TS-4614: avoid e->schedule_in for dummy event. This closes apache#766. Use devtoolset-3 for ATS 7 and later, when available Updated some build instructions TS-4311 Removes support for SPDY completely TS-4683 Adds better error handling on config problems Add TSQA tests for https SRV records TS-4615 set SRV scheme based on next_hop_scheme Add some initial SRV tsqa tests TS-4622 use port from SRV response for origin connections Revert "TS-4622 Use port from SRV response when connecting to origin (apache#773)" TS-4622 Use port from SRV response when connecting to origin (apache#773) TS-4688 handle DNS compression labels in SRV responses (apache#812) cachekey/pattern.h: clang format TS-4667 Uses the WKS in the gzip plugin TS-4653: esi plugin - disable HTTP_COOKIE variable by default and implement a whitelist mechanism to allow the specified cookies for it Original code and idea contributed by Chris Rohlf TS-4680: thread safe initialization in TS*DirGet() functions TS-4652 Fixes log field initialization, previously crippled TS-4595: TSRuntimeDirGet TS-4689 Assert if Machine UUID init fails TS-4674: Remove useless assert statement (apache#809) ...

Make a separate process to await upon the needed log message so the Ready condition of the ts process doesn't race between finding the log message and noticing the process ended.

jacksontj changed the title ~~TS-4622 potential fix~~ TS-4622 User port from SRV response when connecting to origin Jun 30, 2016

jacksontj force-pushed the TS-4622 branch from 76bf602 to e2fe549 Compare June 30, 2016 17:49

jacksontj added a commit to jacksontj/trafficserver that referenced this pull request Jun 30, 2016

TS-4622 potential fix

e2fe549

This closes apache#773

zwoop added the DNS label Jun 30, 2016

zwoop added this to the 7.0.0 milestone Jun 30, 2016

zwoop assigned jpeach Jun 30, 2016

jacksontj changed the title ~~TS-4622 User port from SRV response when connecting to origin~~ TS-4622 Use port from SRV response when connecting to origin Jul 19, 2016

JamesPeach reviewed Jul 20, 2016
View reviewed changes

jacksontj force-pushed the TS-4622 branch from b87be91 to a77ac35 Compare July 21, 2016 01:58

jacksontj added a commit to jacksontj/trafficserver that referenced this pull request Jul 21, 2016

TS-4622 potential fix

ba1d671

This closes apache#773

jacksontj force-pushed the TS-4622 branch from a77ac35 to 1c435c9 Compare July 21, 2016 01:59

jacksontj added a commit to jacksontj/trafficserver that referenced this pull request Jul 21, 2016

TS-4622 use port from SRV response for origin connections

1c435c9

This closes apache#773

jacksontj force-pushed the TS-4622 branch from 1c435c9 to 2c9be07 Compare July 21, 2016 02:01

jacksontj added a commit to jacksontj/trafficserver that referenced this pull request Jul 21, 2016

TS-4622 use port from SRV response for origin connections

2c9be07

This closes apache#773

TS-4622 use port from SRV response for origin connections

b056b56

This closes apache#773

jacksontj force-pushed the TS-4622 branch from 2c9be07 to b056b56 Compare July 21, 2016 14:39

Add some initial SRV tsqa tests

f0f6948

jacksontj merged commit d91457b into apache:master Jul 21, 2016

jacksontj added a commit to jacksontj/trafficserver that referenced this pull request Jul 21, 2016

TS-4622 use port from SRV response for origin connections

1eebb83

This closes apache#773

jacksontj mentioned this pull request Jul 21, 2016

TS-4622 #820

Merged

TS-4622 Use port from SRV response when connecting to origin #773

TS-4622 Use port from SRV response when connecting to origin #773

Uh oh!

Conversation

jacksontj commented Jun 30, 2016

Uh oh!

jpeach commented Jun 30, 2016

Uh oh!

atsci commented Jun 30, 2016

Uh oh!

atsci commented Jun 30, 2016

Uh oh!

jpeach commented Jun 30, 2016

Uh oh!

jacksontj commented Jul 18, 2016

Uh oh!

jacksontj commented Jul 19, 2016

Uh oh!

atsci commented Jul 19, 2016

Uh oh!

atsci commented Jul 19, 2016

Uh oh!

JamesPeach Jul 20, 2016

Choose a reason for hiding this comment

Uh oh!

jacksontj Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atsci commented Jul 20, 2016

Uh oh!

atsci commented Jul 20, 2016

Uh oh!

jacksontj commented Jul 20, 2016

Uh oh!

jpeach commented Jul 21, 2016

Uh oh!

atsci commented Jul 21, 2016

Uh oh!

atsci commented Jul 21, 2016

Uh oh!

atsci commented Jul 21, 2016

Uh oh!

atsci commented Jul 21, 2016

Uh oh!

atsci commented Jul 21, 2016

Uh oh!

atsci commented Jul 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jacksontj Jul 20, 2016 •

edited

Loading