add support for ExternalName service type in dynamic mode #2804
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
rootfs/etc/nginx/lua/util/dns.lua
Outdated
|
|
||
| function _M.resolve(host) | ||
| local r, err = resolver:new{ | ||
| nameservers = {"8.8.8.8", {"8.8.4.4", 53} }, |
There was a problem hiding this comment.
We have the DNS servers to use in the controller. Maybe we can pass that during the initialization?
There was a problem hiding this comment.
Mhm - this will not work for us as it stands - we need to resolve private dns names here (split horizon dns, corporate environment)
There was a problem hiding this comment.
Mhm - this will not work for us as it stands - we need to resolve private dns names here (split horizon dns, corporate environment)
That's why I suggested we use the inherited dns name servers the container starts
There was a problem hiding this comment.
Thanks for early feedbacks, this was just a quick copy paste from the lua-resty-dns docs, hence WIP :)
We have the DNS servers to use in the controller. Maybe we can pass that during the initialization?
makes sense yeah, I'll do that
k8s-ci-robot
added
size/L
rootfs/etc/nginx/lua/util/dns.lua
Outdated
| for _, ans in ipairs(answers) do | ||
| if ans.address then | ||
| cache:set(host, ans.address, ans.ttl) | ||
| return ans.address |
There was a problem hiding this comment.
I wonder if we should use all A records here when there are more than one. Can not think of a reason why we would not wanna do that.
ElvinEfendi
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign @aledbf |
|
The PR is ready for reviews now. |
|
/test all |
| return { host } | ||
| end | ||
|
|
||
| local answers, err, _tries = r:query(host, { qtype = r.TYPE_A }, {}) |
There was a problem hiding this comment.
what happens if the host is a CNAME?
There was a problem hiding this comment.
imagine following:
> ingress-nginx (external-nama-in-dynamic-mode)$ dig www.shopify.com
www.shopify.com. 2441 IN CNAME origin.www.shopify.com.
origin.www.shopify.com. 2432 IN CNAME brochure-elb-567677451.us-east-1.elb.amazonaws.com.
brochure-elb-567677451.us-east-1.elb.amazonaws.com. 42 IN A 54.165.90.249
brochure-elb-567677451.us-east-1.elb.amazonaws.com. 42 IN A 50.17.21.142
brochure-elb-567677451.us-east-1.elb.amazonaws.com. 42 IN A 34.235.254.128
then it will return all three IP addresses.
if the cname target does not have A record then it will be empty
There was a problem hiding this comment.
👍
(I should read the resty dns API before asking 😉 )
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Adds support for service of type ExternalName in dynamic mode. It's worth to mention that in dynamic mode when DNS setting changes for the externalname one won't have to reload Nginx, as soon as the TTL expires, Lua code will resolve the domain again.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #2797Special notes for your reviewer: