feat(core) implement internal dns with loadbalancing, replace dnsmasq #1587

Tieske · 2016-09-02T14:48:51Z

Summary

implements an internal dns resolver, removes the dnsmasq dependency.

Features

uses the balancer_by_lua directive
dns resolution is cached in Lua memory
loadbalancing; lookups are rotated (round-robin) over the entire dns record. For example if the upstream url resolves to an A record with 3 entries, Kong will automatically round-robin the requests over those
resolver also resolves SRV records and takes into account the ports in those cases
the new connect method also resolves ports. So auxiliary systems can now also be configured using SRV records. The effect is that for example, the LDAP plugin, when connecting to a host named "myHost" and port "123", will be looked up against an SRV record as well. If the SRV record returns 3 hosts with ports, those will be used in round robin mode (the provided port '123' will be overridden by the dns results)

todo

add tests to dns.lua and publish it
check necessity to update cli / db name resolving, would be nice to use SRV records here as well
implement set_more_retries as discussed here
- still to fix: hacky migrations on retries
implement global patch for the connect method as discussed here see patch the global tcp.connect function to use the internal dns resolver #1598
implement dog-pile-effect prevention when a dns record expires
make keepalive setting configurable

additional todos, not making it here...

make the upstream block configurable (optional)
implement applying healthdata, as discussed here (optional)
implement the new balancer method setTimeouts() (optional)
implement parsing the additional section of the dns records (optional)

related to

extracted from 'upstreams' feat(core) upstreams #1541
replaces 'dns' feat(core) embedded dns resolver #1225

instructions

To use this branch the dns.lua module must be manually installed

thibaultcha · 2016-09-02T18:36:12Z

Cool stuff 👏

thibaultcha · 2016-09-02T18:39:04Z

kong/plugins/galileo/buffer.lua

@@ -111,7 +112,7 @@ _send = function(premature, self, to_send)
  local client = http.new()
  client:set_timeout(self.connection_timeout)

-  local ok, err = client:connect(self.host, self.port)
+  local ok, err = connect(client, self.host, self.port)


We could implement this in our globalpatches module now, to avoid diverging from the tcpsock regular API.

How do you see that? the connect method is metatable property of the socket userdata. I don't think we should mess with that. Or did you have something different in mind?

Maybe:

-- globalpatches.lua local tcp = ngx.socket.tcp _G.ngx.socket.tcp = function(...) local sock = tcp(...) local connect = sock.connect sock.connect = function(...) return dns.connect(sock, ...) end return sock end

did you try that? I'd expect sock to be a userdata, so the line sock.connect = ... will fail.

openresty/resty-cli#10

Might just work

> resty -e "local s = ngx.socket.tcp(); print(type(s));s.connect = function() end" table

does work, but was rejected by resty-cli

What do you mean 'was rejected by resty-cli'?

thibaultcha · 2016-09-02T18:59:22Z

Should we make proxy_next_upstream and proxy_next_upstream_tries configurable? They could be configured on an per-API basis just like proxy_pass = $upstream_host; (which btw, I think should be updated).

thibaultcha · 2016-09-02T19:03:30Z

check necessity to update cli / db name resolving, would be nice to use SRV records here as well

Is there any reason why we couldn't do that? resty-cli runs in a timer context and we do not mind about blocking operations. I believe we should be able to resolve /etc/hosts entries as well as use dns.lua atop tcpsock:connect() (see my note about patching it in globalpatches too).

thibaultcha · 2016-09-02T20:52:58Z

kong/templates/nginx_kong.lua

+    balancer_by_lua_block {
+        kong.balancer()
+    }
+    keepalive 60;


Note for polishing this PR: make this configurable.
Also worth considering allowing users to add custom upstream blocks (managed by our template) to allow for different upstream connection pools maybe? With different keepalive settings.

With different keepalive settings.

I think that making it globally configurable in the configuration file would be enough for now.

sonicaghi · 2016-09-02T20:55:08Z

loadbalancing; lookups are rotated (round-robin) over the entire dns record

Would be cool if the LB will be customizable with custom policies in the future.

Tieske · 2016-09-02T22:54:51Z

Is there any reason why we couldn't do that? resty-cli runs in a timer context and we do not mind about blocking operations. I believe we should be able to resolve /etc/hosts entries as well as use dns.lua atop tcpsock:connect() (see my note about patching it in globalpatches too).

If patching the the connect method in globalpatches works, then it would be easy.

patch the global tcp.connect function to use the internal dns resolver

subnetmarco · 2016-09-12T06:49:16Z

Currently having this error:

Missing dependencies for kong:
dns == 0.1


Error: Could not satisfy dependency: dns == 0.1
make: *** [install] Error 1

…migration process

…rs an empty table if nothing is specified instead of the previous `nil`. So if table length is 0, drop it and revert to defaults.

subnetmarco · 2016-10-11T22:16:06Z

bin/kong

@@ -1,16 +1,12 @@
-#!/usr/bin/env resty
+#!/usr/bin/env resty -c 65535


This would override whatever ulimit I have already set - do we really need it?

removed it, latest dns.lua master doesn't use it anymore

subnetmarco · 2016-10-11T22:20:10Z

kong/core/balancer.lua

+-- @param target the table with the target details
+-- @return balancer if found, or nil if not found, or nil+error on error
+local get_balancer = function(target)
+  return nil  -- TODO: place holder, forces dns use to first fix regression


@Tieske said we can merge with this TODO in place, because it's fixed in the other upstream PR.

indeed, see https://github.com/Mashape/kong/blob/feature/upstreams2/kong/core/balancer.lua

subnetmarco · 2016-10-11T22:26:16Z

kong/core/handler.lua

+      local ok, err = balancer_execute(balancer_address)
+      if not ok then
+        ngx.log(ngx.ERR, "failed the initial dns/balancer resolve: ", err)
+        return ngx.exit(500)


We can use return responses.send_HTTP_INTERNAL_SERVER_ERROR(err).

subnetmarco · 2016-10-11T22:30:09Z

kong/dao/migrations/cassandra.lua

+
+      for _, row in ipairs(rows) do
+        if not row.retries then  -- only if retries is not set already
+          local _, err = dao.apis:update(row, { id = row.id }, {full = true})


Bug: this is not doing anything. It's probably missing a row.retries = 5 before executing the update(..) method.

subnetmarco · 2016-10-11T22:31:56Z

kong/dao/schemas/apis.lua

@@ -163,7 +163,8 @@ return {
    request_path = {type = "string", unique = true, func = check_request_path},
    strip_request_path = {type = "boolean", default = false},
    upstream_url = {type = "url", required = true, func = validate_upstream_url_protocol},
-    preserve_host = {type = "boolean", default = false}
+    preserve_host = {type = "boolean", default = false},
+    retries = {type = "number", default = 5, func = check_retries},


Maybe the previous row.retries = 5 in the Cassandra migration was not set because the expectation was that re-updating the same entity would populate the default values.

Not sure if this really happens, and if it doesn't, should it be fixed? This needs to be tested.

Just checked it and it works as it currently is. Adding an api on 0.9.2, then restarting Kong with this branch (running the migrations), results in this api output;

Mashapes-MacBook-Pro-2:kong thijs$ http get localhost:8001/apis HTTP/1.1 200 OK Access-Control-Allow-Origin: * Connection: keep-alive Content-Type: application/json; charset=utf-8 Date: Wed, 12 Oct 2016 18:10:31 GMT Server: kong/0.9.2 Transfer-Encoding: chunked { "data": [ { "created_at": 1476295721000, "id": "13af5ca7-a617-4e22-92b7-540fe9607e65", "name": "tieske", "preserve_host": false, "request_path": "/", "retries": 5, "strip_request_path": false, "upstream_url": "http://www.thijsschreijer.nl" } ], "total": 1 }

Retries default is now set.

^^^ being with Cassandra configured as the database

subnetmarco · 2016-10-11T22:33:32Z

kong/kong.lua

+    local ok, err = balancer_execute(addr)
+    if not ok then
+      ngx.log(ngx.ERR, "failed to retry the balancer/resolver: ", err)
+      return ngx.exit(500)


We should use responses.send_HTTP..

subnetmarco · 2016-10-11T22:33:37Z

kong/kong.lua

+  local ok, err = set_current_peer(addr.ip, addr.port)
+  if not ok then
+    ngx.log(ngx.ERR, "failed to set the current peer: ", err)
+    return ngx.exit(500)


We should use responses.send_HTTP..

subnetmarco · 2016-10-11T22:37:04Z

We need to add more integration tests that check if SRV records are properly resolved, for example.

…valents

Tieske · 2016-10-12T17:48:30Z

keepalive is now configurable (one global setting)

pgieniec5tar · 2016-10-14T21:12:58Z

@Tieske one thing I wanted to bring up here, for ELB's in AWS, they will resolve to an ip address, however that underlying ip can eventually change.

I noticed one of the features is dns resolution is cached in Lua memory, but it should be looked up every few seconds to mitigate this.

Here is a much better post describing the issue and solution:
http://gc-taylor.com/blog/2011/11/10/nginx-aws-elb-name-resolution-resolvers/

Tieske · 2016-10-24T15:33:48Z

@pgieniec5tar apparently (what I read from the post) is that nginx does not honour the ttl by simply holding on to the IP address indefinitely.

This dns resolver branch will honour ttl so as long as the proper values are set, it will renew the ip addresses (and update its in memory cache) when the record expires.

initial merge commit, untested. Probably needs some fixes still.

subnetmarco · 2016-10-25T17:55:27Z

@Tieske can you please fix the conflicts? I would like to execute some benchmarks too on the latest code.

reverting some previously breaking changes

…. See comments for caveats.

…ging `next`

Tieske and others added 3 commits September 1, 2016 14:37

initial dns commit

2b9486d

fixed unit tests after extracting feature/dns2 branch

42c73b0

updated all connections to use local Kong dns resolver, except cli + db

6c3ab98

thibaultcha added the pr/status/needs review label Sep 2, 2016

thibaultcha reviewed Sep 2, 2016
View reviewed changes

thibaultcha added pr/status/ongoing-discussion and removed pr/status/needs review labels Sep 2, 2016

thibaultcha added this to the 0.10 milestone Sep 2, 2016

thibaultcha added pr/wip A work in progress PR opened to receive feedback and removed pr/status/ongoing-discussion labels Sep 2, 2016

thibaultcha reviewed Sep 2, 2016
View reviewed changes

thibaultcha changed the title ~~feature(core) implement internal dns with loadbalancing, replace dnsmasq~~ feat(core) implement internal dns with loadbalancing, replace dnsmasq Sep 2, 2016

Tieske and others added 3 commits September 5, 2016 14:37

patch the global tcp.connect function to use the internal dns resolver

67431fd

Merge pull request #1598 from Mashape/fix/dns2-globalpatch

03693df

patch the global tcp.connect function to use the internal dns resolver

Merge branch 'next' of https://github.com/Mashape/kong into feature/dns2

5f60bf0

Tieske mentioned this pull request Sep 6, 2016

Fix/dns2 branch implements retries #1600

Merged

Tieske and others added 2 commits September 8, 2016 12:24

implements the api property retries

2927eda

Merge branch 'next' of https://github.com/Mashape/kong into feature/dns2

9916ef4

Tieske added 3 commits September 12, 2016 16:34

fix double drop-column statement and added commenst regarding 2 step …

1a4d03b

…migration process

fix a remnant from merging next. The config file parsing now delive…

ee654dd

…rs an empty table if nothing is specified instead of the previous `nil`. So if table length is 0, drop it and revert to defaults.

fix linter errors

42277d1

fix linter error

5d38d4c

Tieske mentioned this pull request Oct 11, 2016

feat(core) upstreams #1735

Merged

7 tasks

subnetmarco reviewed Oct 11, 2016

View reviewed changes

Tieske added 3 commits October 11, 2016 22:43

make keepalive setting configurable

f06576b

removed connections setting from shebang

6e3e3d7

replaced ngx.exit calls with the proper kong.tools.responses equi…

85c0c43

…valents

Merge branch 'next' of https://github.com/Mashape/kong into feature/dns2

f2469c7

Tieske added 3 commits October 15, 2016 10:33

updated comments

4d93ede

updated dns.lua dependency to release 0.2.0

6565d3f

update dns.lua dependency to 0.2.1

b138dc2

Merge branch 'next' of https://github.com/Mashape/kong into feature/dns2

c085643

initial merge commit, untested. Probably needs some fixes still.

Tieske mentioned this pull request Oct 25, 2016

implement DNS record type priority setting #1770

Closed

Tieske added 10 commits October 27, 2016 09:37

Merge branch 'next' of https://github.com/Mashape/kong into feature/dns2

ece41c7

reverting some previously breaking changes

fix merge issues

773afac

multiple small updates after reviewing and a bug fix

6d9194f

fix typo

e6ebd98

fix cassandra-abstraction failing to parse version string correctly

fa11a9c

fix tests, implemented pure lua semaphore only to be used for testing…

4d9921e

…. See comments for caveats.

Merge branch 'next' of https://github.com/Mashape/kong into feature/dns2

d876c50

Merge branch 'next' of https://github.com/Mashape/kong into feature/dns2

b3d8eef

Merge branch 'next' of https://github.com/Mashape/kong into feature/dns2

74e4c13

remove pgmoon-mashape reference accidentally reintroduced while mer…

3aa85a7

…ging `next`

Tieske merged commit e121325 into next Nov 4, 2016

Tieske deleted the feature/dns2 branch November 4, 2016 12:09

thibaultcha mentioned this pull request Jan 12, 2017

Release version 0.10 #1971

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core) implement internal dns with loadbalancing, replace dnsmasq #1587

feat(core) implement internal dns with loadbalancing, replace dnsmasq #1587

Tieske commented Sep 2, 2016 •

edited by p0pr0ck5

Loading

thibaultcha commented Sep 2, 2016

thibaultcha Sep 2, 2016

Tieske Sep 2, 2016

thibaultcha Sep 2, 2016

Tieske Sep 2, 2016

thibaultcha Sep 2, 2016

Tieske Sep 2, 2016

thibaultcha Sep 3, 2016 •

edited

Loading

Tieske Sep 3, 2016

thibaultcha commented Sep 2, 2016

thibaultcha commented Sep 2, 2016

thibaultcha Sep 2, 2016

subnetmarco Oct 11, 2016

sonicaghi commented Sep 2, 2016

Tieske commented Sep 2, 2016

subnetmarco commented Sep 12, 2016

subnetmarco Oct 11, 2016

Tieske Oct 12, 2016

subnetmarco Oct 11, 2016

Tieske Oct 12, 2016

subnetmarco Oct 11, 2016

Tieske Oct 12, 2016

subnetmarco Oct 11, 2016

subnetmarco Oct 11, 2016 •

edited

Loading

Tieske Oct 12, 2016

Tieske Oct 12, 2016

subnetmarco Oct 11, 2016

subnetmarco Oct 11, 2016

subnetmarco commented Oct 11, 2016

Tieske commented Oct 12, 2016

pgieniec5tar commented Oct 14, 2016

Tieske commented Oct 24, 2016

subnetmarco commented Oct 25, 2016 •

edited

Loading

		@@ -1,16 +1,12 @@
		#!/usr/bin/env resty
		#!/usr/bin/env resty -c 65535

feat(core) implement internal dns with loadbalancing, replace dnsmasq #1587

feat(core) implement internal dns with loadbalancing, replace dnsmasq #1587

Conversation

Tieske commented Sep 2, 2016 • edited by p0pr0ck5 Loading

Summary

Features

todo

additional todos, not making it here...

related to

instructions

thibaultcha commented Sep 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha Sep 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha commented Sep 2, 2016

thibaultcha commented Sep 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonicaghi commented Sep 2, 2016

Tieske commented Sep 2, 2016

subnetmarco commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subnetmarco Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subnetmarco commented Oct 11, 2016

Tieske commented Oct 12, 2016

pgieniec5tar commented Oct 14, 2016

Tieske commented Oct 24, 2016

subnetmarco commented Oct 25, 2016 • edited Loading

Tieske commented Sep 2, 2016 •

edited by p0pr0ck5

Loading

thibaultcha Sep 3, 2016 •

edited

Loading

subnetmarco Oct 11, 2016 •

edited

Loading

subnetmarco commented Oct 25, 2016 •

edited

Loading