From 13a70c1b48c95d3306dafa90194459ba6c8bd5ed Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Thu, 21 Jul 2016 08:31:29 -0700 Subject: [PATCH 1/3] Revert "TS-4622 Use port from SRV response when connecting to origin (#773)" This reverts commit d91457b8cf27beff88d32fb33a37423fb656b01a. Github UI squashed the tests and the code change-- we want them separate --- ci/tsqa/tests/test_hostdb.py | 186 +---------------------------------- proxy/http/HttpTransact.cc | 7 +- 2 files changed, 2 insertions(+), 191 deletions(-) diff --git a/ci/tsqa/tests/test_hostdb.py b/ci/tsqa/tests/test_hostdb.py index b79fa4ef99f..614a238b189 100644 --- a/ci/tsqa/tests/test_hostdb.py +++ b/ci/tsqa/tests/test_hostdb.py @@ -97,13 +97,9 @@ def handle(self): 'Connection: keep-alive\r\n' 'X-Server-Ip: {server_ip}\r\n' 'X-Server-Port: {server_port}\r\n' - 'X-Client-Ip: {client_ip}\r\n' - 'X-Client-Port: {client_port}\r\n' '\r\n'.format( server_ip=self.request.getsockname()[0], - server_port=self.request.getsockname()[1], - client_ip=self.request.getpeername()[0], - client_port=self.request.getpeername()[1], + server_port=self.request.getsockname()[0], )) self.request.sendall(resp) @@ -403,183 +399,3 @@ def test_serve_stail_for(self): # even though the hostdb.lookup_timeout is set to 1 (meaning it should be ~1s) #print end - end_working #self.assertTrue(end - start >= 2) - - -class TestHostDBSRV(helpers.EnvironmentCase): - '''Tests for SRV records within hostdb - - Tests: - - SRV record - - port overriding - - http/https lookups - - fallback to non SRV - ''' - @classmethod - def setUpEnv(cls, env): - cls.dns_sock = socket.socket (socket.AF_INET, socket.SOCK_DGRAM) - cls.dns_sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) - cls.dns_sock.bind(('', 0)) # bind to all interfaces on an ephemeral port - dns_port = cls.dns_sock.getsockname()[1] - - # set up dns resolver - cls.responses = { - 'www.foo.com.': dnslib.server.RR.fromZone("foo.com. 1 A 127.0.0.3\nfoo.com. 1 A 127.0.0.2"), - 'www.stale_for.com.': dnslib.server.RR.fromZone("foo.com. 1 A 127.0.0.1"), - } - - cls.dns_server = dnslib.server.DNSServer( - StubDNSResolver(cls.responses), - port=dns_port, - address="localhost", - ) - cls.dns_server.start_thread() - - cls.configs['records.config']['CONFIG'].update({ - 'proxy.config.http.response_server_enabled': 2, # only add server headers when there weren't any - 'proxy.config.hostdb.lookup_timeout': 1, - 'proxy.config.http.connect_attempts_max_retries': 1, - 'proxy.config.diags.debug.enabled': 1, - 'proxy.config.diags.debug.tags': 'hostdb', - 'proxy.config.dns.resolv_conf': os.path.join(env.layout.prefix, 'resolv'), - 'proxy.config.hostdb.serve_stale_for': 2, - 'proxy.config.hostdb.ttl_mode': 0, - 'proxy.config.http_ui_enabled': 3, - 'proxy.config.dns.nameservers': '127.0.0.1:{0}'.format(dns_port), - 'proxy.config.srv_enabled': 1, - }) - - cls.socket_servers = [] - ss_dns_results = [] - - for x in xrange(0, 3): - ss = tsqa.endpoint.SocketServerDaemon(EchoServerIpHandler) - ss.start() - ss.ready.wait() - cls.socket_servers.append(ss) - ss_dns_results.append(dnslib.server.RR( - '_http._tcp.www.foo.com.', - dnslib.dns.QTYPE.SRV, - rdata = dnslib.dns.SRV( - priority=10, - weight=10, - port=ss.port, - target='127.0.0.{0}.'.format(x + 1), # note: NUM_REALS must be < 253 - ), - ttl=1, - )) - cls.responses['_http._tcp.www.foo.com.'] = ss_dns_results - - cls.configs['remap.config'].add_line('map http://www.foo.com/ http://www.foo.com/') - cls.configs['remap.config'].add_line('map /_hostdb/ http://{hostdb}') - - def _hostdb_entries(self): - # mapping of name -> entries - ret = {} - showall_ret = requests.get('http://127.0.0.1:{0}/_hostdb/showall?format=json'.format( - self.configs['records.config']['CONFIG']['proxy.config.http.server_ports'] - ), timeout=1) - return showall_ret.text - - for item in showall_ret: - ret[item['hostname']] = item - - return ret - - def test_ports(self): - '''Test port functionality of SRV responses - - SRV responses include ports-- so we want to ensure that we are correctly - overriding the port based on the response - ''' - expected_set = set([s.port for s in self.socket_servers]) - - actual_set = set() - for x in xrange(0, 10): - # test one that works - ret = requests.get( - 'http://www.foo.com/', - proxies=self.proxies, - ) - self.assertEqual(ret.status_code, 200) - actual_set.add(int(ret.headers['X-Server-Port'])) - - self.assertEqual(expected_set, actual_set) - - # TODO: fix, seems broken... - @helpers.unittest.expectedFailure - def test_priority(self): - '''Test port functionality of SRV responses - - SRV responses include ports-- so we want to ensure that we are correctly - overriding the port based on the response - ''' - time.sleep(3) # TODO: clear somehow? waiting for expiry is lame - - NUM_REQUESTS = 10 - orig_responses = self.responses['_http._tcp.www.foo.com.'] - try: - self.responses['_http._tcp.www.foo.com.'][0].rdata.priority=1 - - request_distribution = {} - for x in xrange(0, NUM_REQUESTS): - # test one that works - ret = requests.get( - 'http://www.foo.com/', - proxies=self.proxies, - ) - self.assertEqual(ret.status_code, 200) - port = int(ret.headers['X-Server-Port']) - if port not in request_distribution: - request_distribution[port] = 0 - request_distribution[port] += 1 - - # since one has higher priority, we want to ensure that it got all requests - self.assertEqual( - request_distribution[self.responses['_http._tcp.www.foo.com.'][0].rdata.port], - NUM_REQUESTS, - ) - - finally: - self.responses['_http._tcp.www.foo.com.'] = orig_responses - - # TODO: fix, seems broken... - @helpers.unittest.expectedFailure - def test_weight(self): - '''Test port functionality of SRV responses - - SRV responses include ports-- so we want to ensure that we are correctly - overriding the port based on the response - ''' - time.sleep(3) # TODO: clear somehow? waiting for expiry is lame - - NUM_REQUESTS = 100 - orig_responses = self.responses['_http._tcp.www.foo.com.'] - try: - self.responses['_http._tcp.www.foo.com.'][0].rdata.weight=100 - - request_distribution = {} - for x in xrange(0, NUM_REQUESTS): - # test one that works - ret = requests.get( - 'http://www.foo.com/', - proxies=self.proxies, - ) - self.assertEqual(ret.status_code, 200) - port = int(ret.headers['X-Server-Port']) - if port not in request_distribution: - request_distribution[port] = 0 - request_distribution[port] += 1 - - # since the first one has a significantly higher weight, we expect it to - # take ~10x the traffic of the other 2 - self.assertTrue( - request_distribution[self.responses['_http._tcp.www.foo.com.'][0].rdata.port] > - (NUM_REQUESTS / len(self.responses['_http._tcp.www.foo.com.'])) * 2, - 'Expected significantly more traffic on {0} than the rest: {1}'.format( - self.responses['_http._tcp.www.foo.com.'][0].rdata.port, - request_distribution, - ), - ) - - finally: - self.responses['_http._tcp.www.foo.com.'] = orig_responses diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index 74fbf0637dc..c67055bbd09 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -1787,12 +1787,7 @@ 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()); - // 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_lookup_success) { - 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. - } + s->server_info.dst_addr.port() = htons(s->hdr_info.client_request.port_get()); // now we can set the port. ats_ip_copy(&s->request_data.dest_ip, &s->server_info.dst_addr); get_ka_info_from_host_db(s, &s->server_info, &s->client_info, &s->host_db_info); From 1eebb832faade9ae876b7834b3e9842bb8178c4f Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Thu, 30 Jun 2016 10:46:26 -0700 Subject: [PATCH 2/3] TS-4622 use port from SRV response for origin connections This closes #773 --- proxy/http/HttpTransact.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index c67055bbd09..74fbf0637dc 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -1787,7 +1787,12 @@ 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. + // 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_lookup_success) { + 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. + } ats_ip_copy(&s->request_data.dest_ip, &s->server_info.dst_addr); get_ka_info_from_host_db(s, &s->server_info, &s->client_info, &s->host_db_info); From 7ffa6cbc9a64ecfe6c73875aa9669bb3e6bcb26e Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Thu, 21 Jul 2016 08:21:25 -0700 Subject: [PATCH 3/3] Add some initial SRV tsqa tests --- ci/tsqa/tests/test_hostdb.py | 186 ++++++++++++++++++++++++++++++++++- 1 file changed, 185 insertions(+), 1 deletion(-) diff --git a/ci/tsqa/tests/test_hostdb.py b/ci/tsqa/tests/test_hostdb.py index 614a238b189..b79fa4ef99f 100644 --- a/ci/tsqa/tests/test_hostdb.py +++ b/ci/tsqa/tests/test_hostdb.py @@ -97,9 +97,13 @@ def handle(self): 'Connection: keep-alive\r\n' 'X-Server-Ip: {server_ip}\r\n' 'X-Server-Port: {server_port}\r\n' + 'X-Client-Ip: {client_ip}\r\n' + 'X-Client-Port: {client_port}\r\n' '\r\n'.format( server_ip=self.request.getsockname()[0], - server_port=self.request.getsockname()[0], + server_port=self.request.getsockname()[1], + client_ip=self.request.getpeername()[0], + client_port=self.request.getpeername()[1], )) self.request.sendall(resp) @@ -399,3 +403,183 @@ def test_serve_stail_for(self): # even though the hostdb.lookup_timeout is set to 1 (meaning it should be ~1s) #print end - end_working #self.assertTrue(end - start >= 2) + + +class TestHostDBSRV(helpers.EnvironmentCase): + '''Tests for SRV records within hostdb + + Tests: + - SRV record + - port overriding + - http/https lookups + - fallback to non SRV + ''' + @classmethod + def setUpEnv(cls, env): + cls.dns_sock = socket.socket (socket.AF_INET, socket.SOCK_DGRAM) + cls.dns_sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + cls.dns_sock.bind(('', 0)) # bind to all interfaces on an ephemeral port + dns_port = cls.dns_sock.getsockname()[1] + + # set up dns resolver + cls.responses = { + 'www.foo.com.': dnslib.server.RR.fromZone("foo.com. 1 A 127.0.0.3\nfoo.com. 1 A 127.0.0.2"), + 'www.stale_for.com.': dnslib.server.RR.fromZone("foo.com. 1 A 127.0.0.1"), + } + + cls.dns_server = dnslib.server.DNSServer( + StubDNSResolver(cls.responses), + port=dns_port, + address="localhost", + ) + cls.dns_server.start_thread() + + cls.configs['records.config']['CONFIG'].update({ + 'proxy.config.http.response_server_enabled': 2, # only add server headers when there weren't any + 'proxy.config.hostdb.lookup_timeout': 1, + 'proxy.config.http.connect_attempts_max_retries': 1, + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'hostdb', + 'proxy.config.dns.resolv_conf': os.path.join(env.layout.prefix, 'resolv'), + 'proxy.config.hostdb.serve_stale_for': 2, + 'proxy.config.hostdb.ttl_mode': 0, + 'proxy.config.http_ui_enabled': 3, + 'proxy.config.dns.nameservers': '127.0.0.1:{0}'.format(dns_port), + 'proxy.config.srv_enabled': 1, + }) + + cls.socket_servers = [] + ss_dns_results = [] + + for x in xrange(0, 3): + ss = tsqa.endpoint.SocketServerDaemon(EchoServerIpHandler) + ss.start() + ss.ready.wait() + cls.socket_servers.append(ss) + ss_dns_results.append(dnslib.server.RR( + '_http._tcp.www.foo.com.', + dnslib.dns.QTYPE.SRV, + rdata = dnslib.dns.SRV( + priority=10, + weight=10, + port=ss.port, + target='127.0.0.{0}.'.format(x + 1), # note: NUM_REALS must be < 253 + ), + ttl=1, + )) + cls.responses['_http._tcp.www.foo.com.'] = ss_dns_results + + cls.configs['remap.config'].add_line('map http://www.foo.com/ http://www.foo.com/') + cls.configs['remap.config'].add_line('map /_hostdb/ http://{hostdb}') + + def _hostdb_entries(self): + # mapping of name -> entries + ret = {} + showall_ret = requests.get('http://127.0.0.1:{0}/_hostdb/showall?format=json'.format( + self.configs['records.config']['CONFIG']['proxy.config.http.server_ports'] + ), timeout=1) + return showall_ret.text + + for item in showall_ret: + ret[item['hostname']] = item + + return ret + + def test_ports(self): + '''Test port functionality of SRV responses + + SRV responses include ports-- so we want to ensure that we are correctly + overriding the port based on the response + ''' + expected_set = set([s.port for s in self.socket_servers]) + + actual_set = set() + for x in xrange(0, 10): + # test one that works + ret = requests.get( + 'http://www.foo.com/', + proxies=self.proxies, + ) + self.assertEqual(ret.status_code, 200) + actual_set.add(int(ret.headers['X-Server-Port'])) + + self.assertEqual(expected_set, actual_set) + + # TODO: fix, seems broken... + @helpers.unittest.expectedFailure + def test_priority(self): + '''Test port functionality of SRV responses + + SRV responses include ports-- so we want to ensure that we are correctly + overriding the port based on the response + ''' + time.sleep(3) # TODO: clear somehow? waiting for expiry is lame + + NUM_REQUESTS = 10 + orig_responses = self.responses['_http._tcp.www.foo.com.'] + try: + self.responses['_http._tcp.www.foo.com.'][0].rdata.priority=1 + + request_distribution = {} + for x in xrange(0, NUM_REQUESTS): + # test one that works + ret = requests.get( + 'http://www.foo.com/', + proxies=self.proxies, + ) + self.assertEqual(ret.status_code, 200) + port = int(ret.headers['X-Server-Port']) + if port not in request_distribution: + request_distribution[port] = 0 + request_distribution[port] += 1 + + # since one has higher priority, we want to ensure that it got all requests + self.assertEqual( + request_distribution[self.responses['_http._tcp.www.foo.com.'][0].rdata.port], + NUM_REQUESTS, + ) + + finally: + self.responses['_http._tcp.www.foo.com.'] = orig_responses + + # TODO: fix, seems broken... + @helpers.unittest.expectedFailure + def test_weight(self): + '''Test port functionality of SRV responses + + SRV responses include ports-- so we want to ensure that we are correctly + overriding the port based on the response + ''' + time.sleep(3) # TODO: clear somehow? waiting for expiry is lame + + NUM_REQUESTS = 100 + orig_responses = self.responses['_http._tcp.www.foo.com.'] + try: + self.responses['_http._tcp.www.foo.com.'][0].rdata.weight=100 + + request_distribution = {} + for x in xrange(0, NUM_REQUESTS): + # test one that works + ret = requests.get( + 'http://www.foo.com/', + proxies=self.proxies, + ) + self.assertEqual(ret.status_code, 200) + port = int(ret.headers['X-Server-Port']) + if port not in request_distribution: + request_distribution[port] = 0 + request_distribution[port] += 1 + + # since the first one has a significantly higher weight, we expect it to + # take ~10x the traffic of the other 2 + self.assertTrue( + request_distribution[self.responses['_http._tcp.www.foo.com.'][0].rdata.port] > + (NUM_REQUESTS / len(self.responses['_http._tcp.www.foo.com.'])) * 2, + 'Expected significantly more traffic on {0} than the rest: {1}'.format( + self.responses['_http._tcp.www.foo.com.'][0].rdata.port, + request_distribution, + ), + ) + + finally: + self.responses['_http._tcp.www.foo.com.'] = orig_responses