Attempt AAAA before A

[jacksontj]

This is common-practice for systems. Generally if a domain has ipv6 it also has ipv4, so if we query ipv4 always we effectively never use ipv6.

Ideally this would be (1) configurable or (2) smart enough to know if we have a ipv6 capability

[k8s-ci-robot]

Hi @jacksontj. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jacksontj
To complete the pull request process, please assign elvinefendi
You can assign the PR to them by writing /assign @elvinefendi in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[aledbf]

/ok-to-test

[jacksontj]

FYI this is a continuation of #3882

Basically in that PR; summary:

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

With the addition I had in the description (this might be problematic if we don't know if we are ipv6 or ipv6). In my case I have an ipv6-only cluster; so it failed with A record resolution, similarly if you had an ipv4 only cluster AAAA records would cause failure. So either we need our nginx stuff to fallback or we need to know what our capabilities are within lua.

[codecov-io]

Codecov Report

Merging #4551 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4551      +/-   ##
==========================================
+ Coverage   59.34%   59.37%   +0.02%     
==========================================
  Files          89       89              
  Lines        6823     6823              
==========================================
+ Hits         4049     4051       +2     
+ Misses       2338     2337       -1     
+ Partials      436      435       -1

Impacted Files	Coverage Δ
internal/ingress/metric/collectors/process.go	90.42% <0%> (+2.12%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe4f178...09646ea. Read the comment docs.

[ElvinEfendi]

@jacksontj so the main problem with the current implementation is we might miss IPv6 addresses of a domain being resolved if it has IPv4? Changing the order as you do in this PR fixes that, no?

You need to adjust test in https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/ingress-nginx/4551/pull-ingress-nginx-test-lua/1171440714668576773

[jacksontj]

This is more of a general ipv4 vs ipv6 issue. To clarify I'll talk about it in 3 modes:

1. ipv4 - only
   In this case we never want a AAAA response, as we can't talk to it

2. ipv6 - only
   In this case we never want a A response, as we can't talk to it

3. ipv4 and ipv6
   In this case we can handle either AAAA or A responses, generally this is implemented as preferring the AAAA response over the A response.

The way this lua stuff is configured right now it has the same query lookup function (and config) regardless of controller environment. If the controller only has ipv4 or ipv6 (not both) then its impossible to get it working (without custom overrides of the lua scripts through volume mounts). Ideally this would be configured on the controller so you could define the lookup order and preference-- but I didn't see other config options that do that so I was unable to just copy it.

[ElvinEfendi]

If the controller only has ipv4 or ipv6 (not both) then its impossible to get it working

This should not be the case. The resolver currently tries to resolve A record first and if not successful it queries AAAA.

ingress-nginx/rootfs/etc/nginx/lua/util/dns.lua

Line 80 in 846ff00

for _, qtype in ipairs(QTYPES_TO_CHECK) do

is the loop responsible for that. You can see that in case there's no address resolved for A type query, it'll continue and try AAAA.

Given we never resolve DNS in-line with requests, I don't see any issue with current approach. That being said I agree with trying AAAA before A.

[jacksontj]

This should not be the case. The resolver currently tries to resolve A record first and if not successful it queries AAAA.

I was referring to the controller having only ipv4 or ipv6. So as an example, if the destination has both IPv4 and IPv6 and the controller has ipv6 only the current setup of A then AAAA will return an A record -- which the controller can't route to (since it has no ipv6 address). Similarly if the controller was ipv4 only with this patch you'd get AAAA record which can't be connected to.

Which puts me back to my previous comment:

Ideally this would be configured on the controller so you could define the lookup order and preference-- but I didn't see other config options that do that so I was unable to just copy it.

[aledbf]

Closing. Please reopen if you plan to continue working on this. Thanks

[k8s-ci-robot]

@jacksontj: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-ingress-nginx-e2e-1-14	09646ea	link	/test pull-ingress-nginx-e2e-1-14
pull-ingress-nginx-e2e-1-13	09646ea	link	/test pull-ingress-nginx-e2e-1-13
pull-ingress-nginx-e2e-1-12	09646ea	link	/test pull-ingress-nginx-e2e-1-12
pull-ingress-nginx-chart-lint	09646ea	link	/test pull-ingress-nginx-chart-lint
pull-ingress-nginx-test-lua	09646ea	link	/test pull-ingress-nginx-test-lua
pull-ingress-nginx-e2e-1-17	09646ea	link	/test pull-ingress-nginx-e2e-1-17
pull-ingress-nginx-e2e-1-16	09646ea	link	/test pull-ingress-nginx-e2e-1-16
pull-ingress-nginx-e2e-1-15	09646ea	link	/test pull-ingress-nginx-e2e-1-15

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.