Add support for IPV6 resolvers

[aledbf]

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):

fixes #3881

[jacksontj]

Do we need this format in the config file? If not then a cleaner solution might be https://github.com/kubernetes/ingress-nginx/compare/master...jacksontj:issue_3881?expand=1 ? (I'm not familiar enough with this codebase to know if others require it in the non-bracket form).

[jacksontj]

I applied this patch (through a configmap) on my cluster, but it seems that there are more ipv6 compatibility issues with this lua script:

2019/03/10 00:45:09 [error] 41#41: *235 [lua] dns.lua:78: resolve(): no A record resolved, context: ngx.timer

[jacksontj]

the problem line now is that its doing an A query only -- https://github.com/kubernetes/ingress-nginx/blob/master/rootfs/etc/nginx/lua/util/dns.lua#L54

so here the descision needs to be made if we do A, AAAA, or fail between them. For my use-case (as a workaround) I'm using this script and I've just changed the type to AAAA all the time -- not ideal, but unblocks me :) )

[aledbf]

@ElvinEfendi I think this is the opportunity to migrate to http://kong.github.io/lua-resty-dns-client/modules/resty.dns.client.html#resolve

[ElvinEfendi]

As @jacksontj suggested above I think this should be done in the controller (I had an impression we already do it in template.go).

As to no A record resolved error, it should be easy to add support for it.

@ElvinEfendi I think this is the opportunity to migrate to http://kong.github.io/lua-resty-dns-client/modules/resty.dns.client.html#resolve

I looked at the implementation of that library again, and it's really unnecessarily (for this project) complicated.

[aledbf]

I looked at the implementation of that library again, and it's really unnecessarily (for this project) complicated.

Ok, any suggestion how to fix this issue?

As to no A record resolved error, it should be easy to add support for it.

How? (attempting to resolve AAAA after failing A if the dns is ipv6)

[ElvinEfendi]

How? (attempting to resolve AAAA after failing A if the dns is ipv6)

Yes. Basically extract that logic out into separate function i.e query(host, qtype) and call it first with r.TYPE_A and if unsuccessful then call it again with r.TYPE_AAAA.

And even better we can probably deduce which one of r.TYPE_A or r.TYPE_AAAA to try first by looking at nameservers (i.e when all entries in the nameservers list is IPv6 then try r.TYPE_AAAA first).

[jacksontj]

In general the resolver result (A or AAAA) is independent of the ip version of the resolver address. Meaning a resolver at 1.2.3.4 can return AAAA and ::1 can return A. Generally things try AAAA then fall back, and it's not uncommon to have an option to control this behavior (for example the go dialer has tcp, tcp4, and tcp6). Sine the first part should be done on the go side I'll open a PR with the change I have

…

On Thu, Mar 14, 2019, 7:46 AM Elvin Efendi ***@***.***> wrote: How? (attempting to resolve AAAA after failing A if the dns is ipv6) Yes. Basically extract that logic out into separate function i.e query(host, qtype) and call it first with r.TYPE_A and if unsuccessful then call it again with r.TYPE_AAAA. And even better we can probably deduce which one of r.TYPE_A or r.TYPE_AAAA to try first by looking at nameservers (i.e when all entries in the nameservers list is IPv6 then try r.TYPE_AAAA first). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3882 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABi3nbaujat-QOv6GqQ-TuV0O58HO2AKks5vWmDIgaJpZM4bm-_e> .

[jacksontj]

Created #3895 for the resolver config fix.

[ElvinEfendi]

I'd not log an error here, because it might be expected that there's no TYPE_A record, so we will keep logging error.

Instead I suggest you collect this and the error below and log them in the end only if both TYPE_A and TYPE_AAAA queries fail

[ElvinEfendi]

@aledbf implementation looks good (just few comments), you will need to edit the unit test file to adjust the expected error messages.

[ElvinEfendi]

/lgtm

[k8s-ci-robot]

[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:

• OWNERS [ElvinEfendi,aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ElvinEfendi]

Right now we short circuit and does not check for IPv6 address when IPv4 exists. I wonder if it would be better to always check for both and combine the result before returning.

@aledbf @jacksontj thoughts?

[jacksontj]

I believe I covered this in #3882 (comment)

TLDR; should be ipv6 with fallback to ipv4; optionally have config to control that lookup priority.