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 2022/01/05 08:00:36 UTC
[GitHub] [apisix] leslie-tsang opened a new pull request #6023: fix(ngx_tpl): sync ipv6 config to resolver
leslie-tsang opened a new pull request #6023:
URL: https://github.com/apache/apisix/pull/6023
### 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. -->
Sync `apisix.enable_ipv6` config to the generated `nginx.conf` resolver section
### Pre-submission checklist:
<!--
Please follow the PR manners:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. If you need to resolve merge conflicts after the PR is reviewed, please merge master but do not rebase
6. Use "request review" to notify the reviewer once you have resolved the review
7. Only reviewer can click "Resolve conversation" to mark the reviewer's review resolved
-->
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a change in pull request #6023: fix(ngx_tpl): sync ipv6 config to resolver
Posted by GitBox <gi...@apache.org>.
leslie-tsang commented on a change in pull request #6023:
URL: https://github.com/apache/apisix/pull/6023#discussion_r781956616
##########
File path: apisix/core/dns/client.lua
##########
@@ -130,7 +130,7 @@ end
function _M.new(opts)
- opts.ipv6 = true
+ opts.ipv6 = opts.ipv6 or true
Review comment:
> Why not = local_conf.apisix.enable_ipv6?
IMO, `= local_conf.apisix.enable_ipv6` is a breaking change, Imagine a scenario where a third party or serverless plugin uses this function to parse ipv6, this change will causes an unexpected behavior, I wander if this is a fictitious demand.
Do let me know WDYT. Thanks.
> And we need a test for it.
Sure, I will add it.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a change in pull request #6023: fix(ngx_tpl): sync ipv6 config to resolver
Posted by GitBox <gi...@apache.org>.
leslie-tsang commented on a change in pull request #6023:
URL: https://github.com/apache/apisix/pull/6023#discussion_r782636879
##########
File path: apisix/core/dns/client.lua
##########
@@ -130,7 +130,7 @@ end
function _M.new(opts)
- opts.ipv6 = true
+ opts.ipv6 = opts.ipv6 or true
Review comment:
OK, I will add test case for it in next PR
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a change in pull request #6023: fix(ngx_tpl): sync ipv6 config to resolver
Posted by GitBox <gi...@apache.org>.
leslie-tsang commented on a change in pull request #6023:
URL: https://github.com/apache/apisix/pull/6023#discussion_r782796211
##########
File path: apisix/core/dns/client.lua
##########
@@ -130,7 +132,22 @@ end
function _M.new(opts)
- opts.ipv6 = true
+ local local_conf = config_local.local_conf()
+
+ if opts.ipv6 == nil then
Review comment:
🤣 done
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6023: fix(ngx_tpl): sync ipv6 config to resolver
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6023:
URL: https://github.com/apache/apisix/pull/6023#discussion_r778689484
##########
File path: apisix/cli/ops.lua
##########
@@ -552,6 +552,7 @@ Please modify "admin_key" in conf/config.yaml .
admin_server_addr = admin_server_addr,
control_server_addr = control_server_addr,
prometheus_server_addr = prometheus_server_addr,
+ enable_ipv6 = yaml_conf.apisix.enable_ipv6,
Review comment:
The enable_ipv6 is already imported below
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6023: fix(ngx_tpl): sync ipv6 config to resolver
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6023:
URL: https://github.com/apache/apisix/pull/6023#discussion_r782635690
##########
File path: apisix/core/dns/client.lua
##########
@@ -130,7 +130,7 @@ end
function _M.new(opts)
- opts.ipv6 = true
+ opts.ipv6 = opts.ipv6 or true
Review comment:
We can use
```
if opts.ipv6 == nil then
...
end
```
so user can specify the ipv6 option to overwrite the default one.
AFAIK, we don't promise the DNS client always handles IPv6 query and the `enable_ipv6` is enabled by default, so it's not a break change. (Even if users don't consider it is a bug fix)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6023: fix(ngx_tpl): sync ipv6 config to resolver
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6023:
URL: https://github.com/apache/apisix/pull/6023#discussion_r782738419
##########
File path: apisix/core/dns/client.lua
##########
@@ -130,7 +132,22 @@ end
function _M.new(opts)
- opts.ipv6 = true
+ local local_conf = config_local.local_conf()
+
+ if opts.ipv6 == nil then
Review comment:
I spend some time digging into it. Look like it should be `enable_ipv6`, and it only works with the nameserver. See http://kong.github.io/lua-resty-dns-client/modules/resty.dns.client.html#init
This field is incorrect from the beginning...
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6023: fix(ngx_tpl): sync ipv6 config to resolver
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6023:
URL: https://github.com/apache/apisix/pull/6023#discussion_r781931661
##########
File path: apisix/core/dns/client.lua
##########
@@ -130,7 +130,7 @@ end
function _M.new(opts)
- opts.ipv6 = true
+ opts.ipv6 = opts.ipv6 or true
Review comment:
Why not `= local_conf.apisix.enable_ipv6`? And we need a test for it.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #6023: fix(ipv6): allow disabling IPv6 resolve
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #6023:
URL: https://github.com/apache/apisix/pull/6023
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org