You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/10/14 16:14:01 UTC
[GitHub] [apisix] Firstsawyou opened a new pull request #2424: feat: In dns parse, the value of `resolvers` is allowed to be empty
Firstsawyou opened a new pull request #2424:
URL: https://github.com/apache/apisix/pull/2424
fix #2422
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2424: feat: In dns parse, the value of `resolvers` is allowed to be empty
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2424:
URL: https://github.com/apache/apisix/pull/2424#discussion_r505151332
##########
File path: t/core/utils.t
##########
@@ -68,3 +68,19 @@ qr/random seed \d+\ntwice: false/
GET /t
--- no_error_log
[error]
+
Review comment:
Here we do not need to set the `dns_resolver` in `config.yaml`. Through the `set_resolver` function in the ʻutils` file. We can set `resolvers` to be empty for testing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2424: feat: In dns parse, when the parameter `resolvers` is empty, the local dns is used as `resolvers` by default
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2424:
URL: https://github.com/apache/apisix/pull/2424#discussion_r505373084
##########
File path: t/core/utils.t
##########
@@ -68,3 +68,50 @@ qr/random seed \d+\ntwice: false/
GET /t
--- no_error_log
[error]
+
+
+
+=== TEST 3: resolvers is not empty
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local resolvers = {"8.8.8.8"}
+ core.utils.set_resolver(resolvers)
+ local ip_info, err = core.utils.dns_parse("baidu.com", resolvers)
Review comment:
already changed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2424: feat: In dns parse, the value of `resolvers` is allowed to be empty
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2424:
URL: https://github.com/apache/apisix/pull/2424#discussion_r505144557
##########
File path: t/core/utils.t
##########
@@ -68,3 +68,19 @@ qr/random seed \d+\ntwice: false/
GET /t
--- no_error_log
[error]
+
Review comment:
added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis merged pull request #2424: feat: In dns parse, when the parameter `resolvers` is empty, the local dns is used as `resolvers` by default
Posted by GitBox <gi...@apache.org>.
membphis merged pull request #2424:
URL: https://github.com/apache/apisix/pull/2424
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2424: feat: In dns parse, when the parameter `resolvers` is empty, the local dns is used as `resolvers` by default
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2424:
URL: https://github.com/apache/apisix/pull/2424#discussion_r505310823
##########
File path: apisix/core/utils.lua
##########
@@ -100,11 +101,16 @@ local function dns_parse(resolvers, domain)
return nil, "unsupport DNS answer"
end
- return dns_parse(resolvers, answer.cname)
+ return dns_parse(answer.cname, resolvers)
Review comment:
I think we can remove the argument `resolvers` here
##########
File path: t/core/utils.t
##########
@@ -68,3 +68,50 @@ qr/random seed \d+\ntwice: false/
GET /t
--- no_error_log
[error]
+
+
+
+=== TEST 3: resolvers is not empty
Review comment:
bad title
##########
File path: t/core/utils.t
##########
@@ -68,3 +68,50 @@ qr/random seed \d+\ntwice: false/
GET /t
--- no_error_log
[error]
+
+
+
+=== TEST 3: resolvers is not empty
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local resolvers = {"8.8.8.8"}
+ core.utils.set_resolver(resolvers)
+ local ip_info, err = core.utils.dns_parse("baidu.com", resolvers)
+ if not ip_info then
+ core.log.error("failed to parse domain: ", host, ", error: ",err)
+ end
+ ngx.say(core.json.encode(ip_info))
+ }
+ }
+--- request
+GET /t
+--- response_body eval
+qr/"address":.+,"name":"baidu.com"/
+--- no_error_log
+[error]
+--- LAST
+
+
+=== TEST 4: resolvers is empty
Review comment:
bad title
##########
File path: t/core/utils.t
##########
@@ -68,3 +68,50 @@ qr/random seed \d+\ntwice: false/
GET /t
--- no_error_log
[error]
+
+
+
+=== TEST 3: resolvers is not empty
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local resolvers = {"8.8.8.8"}
+ core.utils.set_resolver(resolvers)
+ local ip_info, err = core.utils.dns_parse("baidu.com", resolvers)
Review comment:
please change it to `github.com`
##########
File path: apisix/init.lua
##########
@@ -177,7 +178,7 @@ end
local function parse_domain(host)
- local ip_info, err = core.utils.dns_parse(dns_resolver, host)
+ local ip_info, err = core.utils.dns_parse(host, dns_resolver)
Review comment:
ditto
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2424: feat: In dns parse, the value of `resolvers` is allowed to be empty
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2424:
URL: https://github.com/apache/apisix/pull/2424#discussion_r505147591
##########
File path: t/core/utils.t
##########
@@ -68,3 +68,19 @@ qr/random seed \d+\ntwice: false/
GET /t
--- no_error_log
[error]
+
Review comment:
No, I means the `config.yaml` file
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2424: feat: In dns parse, the value of `resolvers` is allowed to be empty
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2424:
URL: https://github.com/apache/apisix/pull/2424#discussion_r505151519
##########
File path: t/core/utils.t
##########
@@ -68,3 +68,19 @@ qr/random seed \d+\ntwice: false/
GET /t
--- no_error_log
[error]
+
Review comment:
Here we do not need to set the `dns_resolver` in `config.yaml`. Through the `set_resolver` function in the `utils` file. We can set `resolvers` to be empty for testing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2424: feat: In dns parse, when the parameter `resolvers` is empty, the local dns is used as `resolvers` by default
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2424:
URL: https://github.com/apache/apisix/pull/2424#discussion_r505342745
##########
File path: t/core/utils.t
##########
@@ -68,3 +68,50 @@ qr/random seed \d+\ntwice: false/
GET /t
--- no_error_log
[error]
+
+
+
+=== TEST 3: resolvers is not empty
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local resolvers = {"8.8.8.8"}
+ core.utils.set_resolver(resolvers)
+ local ip_info, err = core.utils.dns_parse("baidu.com", resolvers)
+ if not ip_info then
+ core.log.error("failed to parse domain: ", host, ", error: ",err)
+ end
+ ngx.say(core.json.encode(ip_info))
+ }
+ }
+--- request
+GET /t
+--- response_body eval
+qr/"address":.+,"name":"baidu.com"/
+--- no_error_log
+[error]
+--- LAST
+
+
+=== TEST 4: resolvers is empty
Review comment:
already changed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2424: feat: In dns parse, when the parameter `resolvers` is empty, the local dns is used as `resolvers` by default
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2424:
URL: https://github.com/apache/apisix/pull/2424#discussion_r505341254
##########
File path: apisix/init.lua
##########
@@ -177,7 +178,7 @@ end
local function parse_domain(host)
- local ip_info, err = core.utils.dns_parse(dns_resolver, host)
+ local ip_info, err = core.utils.dns_parse(host, dns_resolver)
Review comment:
removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on pull request #2424: feat: In dns parse, when the parameter `resolvers` is empty, the local dns is used as `resolvers` by default
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on pull request #2424:
URL: https://github.com/apache/apisix/pull/2424#issuecomment-709157760
@moonming ping
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2424: feat: In dns parse, the value of `resolvers` is allowed to be empty
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2424:
URL: https://github.com/apache/apisix/pull/2424#discussion_r505079428
##########
File path: t/core/utils.t
##########
@@ -68,3 +68,19 @@ qr/random seed \d+\ntwice: false/
GET /t
--- no_error_log
[error]
+
Review comment:
The test case did not cover empty configuration of resolver
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org