parses addresses in all the sections #10

kiddkai · 2016-04-15T07:37:44Z

This seems like a breaking change to this module. Basically, what this PR do is returns all the sections in the dns response. Previously, it just returns the answer section.

This change based on our requirement that when we do a SRV request to our consul server
to get what services are available, and what are the ip addresses + port for that specific services. It returns us:


$ dig @127.0.0.1 -p 8600 test_service.service.consul SRV

; <<>> DiG 9.8.3-P1 <<>> @127.0.0.1 -p 8600 test_service.service.consul SRV
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 32661
;; flags: qr aa rd; QUERY: 1, ANSWER: 2, AUTHORITY: 0, ADDITIONAL: 2
;; WARNING: recursion requested but not available

;; QUESTION SECTION:
;test_service.service.consul.   IN      SRV

;; ANSWER SECTION:
test_service.service.consul. 0  IN      SRV     1 1 3003 zekaizhengs-mbp.corpad.net.local.node.dc1.consul.
test_service.service.consul. 0  IN      SRV     1 1 3000 zekaizhengs-mbp.corpad.net.local.node.dc1.consul.

;; ADDITIONAL SECTION:
zekaizhengs-mbp.corpad.net.local.node.dc1.consul. 0 IN A 169.254.160.242
zekaizhengs-mbp.corpad.net.local.node.dc1.consul. 0 IN A 169.254.160.242

;; Query time: 0 msec
;; SERVER: 127.0.0.1#8600(127.0.0.1)
;; WHEN: Thu Apr 14 15:45:26 2016
;; MSG SIZE  rcvd: 363

But the current lua-resty-dns module actually just returns us the hostname + port for the query. And from the dig result shows that the additional section already have the information I need. So in order
to make less dns request like this project: https://github.com/vlipco/srv-router/blob/master/conf/srv_router.lua#L84

I create this pr for the change. Which may breaks the current modules exists which using this module.

kiddkai · 2016-04-15T07:38:08Z

Related Topic: https://groups.google.com/forum/#!topic/openresty/MtDSntxnkck

agentzh · 2016-04-15T19:07:29Z

@kiddkai Backward compatibility of the API is important. Maybe we should add new options to allow the caller to request records in those additional sections (like "authority" and "additional" sections)? Instead of creating nested tables, maybe we can just append those records to the existing answers table but with a special field like section being set?

kiddkai · 2016-04-15T22:35:44Z

Good point, so just add a new option called 'parse_all_sections(Boolean)' ? In this case will be pretty easy for the API consumers. And it will just have the same behaviour as the current PR.

The other way is provide more options like give "give me the results for some specific section". And the response parser will do the work.

I prefer the parse_all_sections solution since the record size of the dns response is limited. And the internal implementation can keep really simple. And we have a new "section" field each answer which can let the consumer do the filter work.

What do you think @agentzh

agentzh · 2016-04-15T23:00:51Z

@kiddkai Can we just reuse the existing options table for the query and tcp_query methods? Maybe we should allow specifying individual sections? Choosing between all sections and just the answers section looks not flexible enough to me :) Thoughts?

kiddkai · 2016-04-15T23:11:00Z

Yup, reusing the options table is what I planing to do. I can implement to let it supports specifying individual sections. The hard part for me maybe is just the naming for the new options.

For me as a API consumer of this module. Either returning answers or answers from all sections is good enough for my use case. It's true that some developer may want something specific from the response.

kiddkai · 2016-04-15T23:12:40Z

If go go down the path to implement supporting specify individual section. What options you think we should add to the query method?

agentzh · 2016-04-16T18:33:24Z

@kiddkai How about additional_section = true | false and authority_section = true | false?

kiddkai · 2016-04-16T21:38:25Z

Sounds good, will do that change.

kiddkai · 2016-04-18T01:47:13Z

Hi @agentzh

I Just add additional_section and authority_section to both tcp_query and query.

A dumb question. I want to add tests for the changes, do you know how I can generates the mock dns response? I can use tcpdump to get the dns responses. But don't know how to put it into the t file.
Will happy to know more about it.

Cheers

kiddkai · 2016-04-18T01:52:10Z

Another thing need to do maybe is bump up the _VERSION.

kiddkai · 2016-04-19T06:40:25Z

No worries, just get my head around perl. Will add mock tests for this functional.

agentzh · 2016-04-22T19:29:36Z

@kiddkai Well, you don't really need to use Perl for writing tests. The tests are declarative. See the tutorials here if you have problems:

https://openresty.gitbooks.io/programming-openresty/content/testing/index.html

kiddkai · 2016-04-22T21:56:43Z

Hi @agentzh, thank you for the suggestion. I've already make the TestDNS file support mocking the srv record and other sections response 4days ago. But I don't have chance to update my PR yet. Quite a busy week. I will apply the changes I made to my PR in the werkend.

It's a great fun to write the DNS mocking feature. Really like the design of testing in openresty.

agentzh · 2016-04-22T23:08:53Z

@kiddkai Awesome! Looking forward to your updated PR. Glad to hear that you enjoyed it :)

kiddkai · 2016-04-26T01:18:36Z

Hey @agentzh , just updated. Now test covers the new features I added in this repo. I guess everything is done for this PR 👯

agentzh · 2016-04-26T20:06:06Z

@kiddkai Thanks for your hard work!

@doujiang24 Will you have a quick look at this PR? If nothing is found, I'll merge this :)

guanglinlv · 2016-04-27T01:27:00Z

lib/resty/dns/resolver.lua

+        elseif ans.section == SECTION_AR and additional_section then
+          table.insert(result, ans)
+        end
+    end


@kiddkai ,good job!

the size of answers is explicitly, it's better to pre-allocate the result using new_tab.

table.insert is expensive, it's better to replace using index.

Hey @guanglinlv , no problems, will allocate the table first for all the records.

also my code intent here is also wrong, thanks for pointing out.

@kiddkai , nice job!

we usually use for i = 1, #answers do instead iparis

yup, understand that. because I saw the ipars is already optimised by the luajit, so
I use it.

http://wiki.luajit.org/NYI#libraries_table-library

Hi @doujiang24, just get back to the code and did a double check. When we start doing _filter answers, the answer is already becomes a list of table and doesn't specify how many answers in each section. I guess if we want to pass the "how many items in each section" we need to change
the shape of the answers which comes from parse_response. And make a nested hash table which looks like:

{ ['answers'] = { ... }, ['authority'] = { ... }, ['additional'] = { ... } }

Then we will be able to know how many items in each section. Otherwise it will become little bit
hacky to pre-allocate the table using new_tab since the state of how many answers in each section didn't pass around in the original design.

Cheers

agentzh · 2016-05-10T19:14:50Z

README.markdown

@@ -1,4 +1,4 @@
-Name
+  Name


Why this change?

I guess is my vim auto format this, will change it back .

kiddkai · 2016-05-18T01:40:06Z

Hi @agentzh @doujiang24 , just updated the PR. Currently, it still parses all the records in all sections. But it will not insert the answer into the result table unless user enables it. So current implementation will not have extra filter process 🐒.

@agentzh I also fixes the README.md, yay 💃

agentzh · 2016-05-18T03:33:07Z

@kiddkai Thanks for the updates. I'll look at it again.

Tieske · 2016-05-31T07:07:55Z

Any update on this? Would be great to get this in.

agentzh · 2016-05-31T18:30:35Z

@Tieske Still working on the 1.9.15.1 release. Will look into this after the formal release is out.

I hope that 1.9.15.2 can include this.

Tieske · 2016-05-31T20:22:32Z

👍

was hoping for 1.9.15.1 😄

agentzh · 2016-06-01T19:09:52Z

lib/resty/dns/resolver.lua

-        insert(answers, ans)
+        ans.section = section
+
+        if not should_skip then


Instead of always parsing the unwanted sections, we should just stop parsing directly?

agentzh · 2016-06-01T19:17:10Z

@kiddkai Please check out my latest comments. Also, please rebase to the latest master. Thank you!

kiddkai · 2016-06-14T01:33:33Z

Hi @agentzh , just updated my pr. You can review it again to see if there's anything is bad :D.

boostbob · 2016-07-05T13:06:07Z

very cool, it's useful for me.

agentzh · 2016-07-06T21:29:14Z

@kiddkai Thanks for your updates! I'll have another look soon :)

agentzh · 2016-07-28T04:43:09Z

lib/resty/dns/resolver.lua

+            insert(answers, ans)
+        end
+
+        ans.section = section



The Lua variables type_hi and type_lo below are not declared as local and thus are examples of global variable abuses.

The lua-releng tool provided by the nginx-devel-utils github repository does find all such global variable misuses:

Checking use of Lua global variables in file lib/resty/dns/resolver.lua... op no. line instruction args ; code 31 [335] SETGLOBAL 13 -4 ; type_hi 36 [336] SETGLOBAL 13 -5 ; type_lo 38 [337] GETGLOBAL 14 -4 ; type_hi 41 [337] GETGLOBAL 14 -5 ; type_lo 48 [343] SETGLOBAL 14 -8 ; class_hi 53 [344] SETGLOBAL 14 -10 ; class_lo 55 [345] GETGLOBAL 15 -8 ; class_hi 58 [345] GETGLOBAL 15 -10 ; class_lo

agentzh · 2016-07-28T04:53:41Z

@kiddkai Found some more small issues, as in my comments above. Well, since the remaining issues are minor, I'll fix them locally myself. Just FYI :) Thanks for your work on this!

kiddkai · 2016-07-28T04:54:55Z

Fixing them, will fix them soon

agentzh · 2016-07-28T05:03:00Z

lib/resty/dns/resolver.lua

@@ -65,6 +69,10 @@ local _M = {
    TYPE_SRV    = TYPE_SRV,
    TYPE_SPF    = TYPE_SPF,
    CLASS_IN    = CLASS_IN,
+    SECTION_QD  = SECTION_QD,


This SECTON_QD constant is unused internally in the implementation nor documented in README. Better remove it?

Yeah, is not being used anywhere. I can remove it

kiddkai · 2016-07-28T05:04:03Z

Hi @agentzh I just fixes all of the styling issue you just mentioned, so feel free to review it again. Don't know why still have the line 572 issue shows there. I am sure that is two blank line there now ~

agentzh · 2016-07-28T05:07:52Z

lib/resty/dns/resolver.lua

+                        if not answers then
+                          return nil, err
+                        end
+                        return answers


Why eliminating the original tail call to _tcp_query above? Seems like we should just do

return _tcp_query(self, query, id, opts)

here?

true, just update it

agentzh · 2016-07-28T05:40:34Z

@kiddkai Just merged a modified version of your branch into master. some more edits in the docs. Thanks!

kiddkai · 2016-07-28T05:41:57Z

YAY, cool

kiddkai force-pushed the master branch from f78d754 to c3eb7f4 Compare April 18, 2016 01:43

kiddkai force-pushed the master branch from c3eb7f4 to 7d41c28 Compare April 26, 2016 01:16

guanglinlv reviewed Apr 27, 2016
View reviewed changes

kiddkai force-pushed the master branch from 7d41c28 to 228abae Compare April 29, 2016 00:07

agentzh reviewed May 10, 2016
View reviewed changes

Tieske mentioned this pull request May 17, 2016

feat(core) embedded dns resolver Kong/kong#1225

Closed

kiddkai force-pushed the master branch 2 times, most recently from 23c452d to be43352 Compare May 18, 2016 01:31

agentzh reviewed Jun 1, 2016
View reviewed changes

kiddkai force-pushed the master branch 3 times, most recently from a57c9a2 to c75d606 Compare June 14, 2016 01:29

agentzh reviewed Jul 28, 2016
View reviewed changes

kiddkai force-pushed the master branch from c75d606 to 988a500 Compare July 28, 2016 05:00

agentzh reviewed Jul 28, 2016
View reviewed changes

kiddkai force-pushed the master branch from 988a500 to 27a59c1 Compare July 28, 2016 05:06

agentzh reviewed Jul 28, 2016
View reviewed changes

parses addresses in all the sections

c74e91a

kiddkai force-pushed the master branch from 27a59c1 to c74e91a Compare July 28, 2016 05:10

agentzh closed this Jul 28, 2016

Tieske mentioned this pull request Sep 13, 2016

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

Merged

7 tasks

parses addresses in all the sections #10

parses addresses in all the sections #10

Conversation

kiddkai commented Apr 15, 2016

kiddkai commented Apr 15, 2016

agentzh commented Apr 15, 2016

kiddkai commented Apr 15, 2016

agentzh commented Apr 15, 2016 • edited Loading

kiddkai commented Apr 15, 2016

kiddkai commented Apr 15, 2016

agentzh commented Apr 16, 2016

kiddkai commented Apr 16, 2016

kiddkai commented Apr 18, 2016 • edited Loading

kiddkai commented Apr 18, 2016

kiddkai commented Apr 19, 2016

agentzh commented Apr 22, 2016

kiddkai commented Apr 22, 2016

agentzh commented Apr 22, 2016

kiddkai commented Apr 26, 2016

agentzh commented Apr 26, 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

kiddkai Apr 28, 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

kiddkai commented May 18, 2016

agentzh commented May 18, 2016

Tieske commented May 31, 2016

agentzh commented May 31, 2016

Tieske commented May 31, 2016

Choose a reason for hiding this comment

agentzh commented Jun 1, 2016

kiddkai commented Jun 14, 2016

boostbob commented Jul 5, 2016

agentzh commented Jul 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agentzh commented Jul 28, 2016

kiddkai commented Jul 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiddkai commented Jul 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agentzh commented Jul 28, 2016

kiddkai commented Jul 28, 2016

agentzh commented Apr 15, 2016 •

edited

Loading

kiddkai commented Apr 18, 2016 •

edited

Loading

kiddkai Apr 28, 2016 •

edited

Loading