fix: return NODATA instead of NXDOMAIN for known names - #729
fix: return NODATA instead of NXDOMAIN for known names#729BTF-Kabir-2020 wants to merge 1 commit into
Conversation
91b576d to
68ce2f9
Compare
|
LGTM, but your new test is failing |
|
Thanks — the integration failure was the MX dig on the short name timing out because it was forwarded upstream. Pushed a fix: return NODATA when the name exists locally before forwarding, and assert NXDOMAIN against \does-not-exist.dns.podman. |
Luap99
left a comment
There was a problem hiding this comment.
That does not seem correct to me, doing the lookup after we know what request type we got seems wrong. We should not be messing with requests we do not understand and just keep forwarding them.
In particular doing this extra lookup for PTR or MX requests is simply wrong.
We already performed the lookup in reply_ip sop IMO this should be contained there in one place and not do a second lookup later
I do not know where you picked up MX, but since we do not handle at all atm we should not just randomly return NOERROR without an answer section. The MX handling has nothing to do with the issue.
Also the commits should be squashed.
And we are not part in the new podman org here but I would still refer to https://github.com/podman-container-tools/community/blob/main/LLM_POLICY.md
These AI PR descriptions add no value, just paste your commit message, how you tested that is irrelevant since we see what test is being added in the diff and only that counts anyway.
| // Name exists in our zone. Even with no answers of this type (e.g. AAAA | ||
| // for an IPv4-only container), return NOERROR/NODATA — not a miss that | ||
| // would fall through to NXDOMAIN. |
There was a problem hiding this comment.
this comment makes no sense in the current version since you did not actually handle this at all in reply_ip
| // No match found, forwarding below. | ||
| } | ||
|
|
||
| // TODO: handle MX here like docker does |
There was a problem hiding this comment.
this should not be removed, you do not handle that at all
reply_ip already returns after a successful backend lookup, so an IPv4-only name queried for AAAA gets NOERROR with an empty answer instead of falling through as a miss. Add a regression test for that. Fixes containers#679 Signed-off-by: BTF Kabir <PouyaGH2080@gmail.com>
c5baccf to
3ca9058
Compare
|
@Luap99 thanks for the review. Dropped the extra lookup and the MX stuff — that was the wrong approach. Left the NODATA behavior in reply_ip only and trimmed the test to AAAA. Squashed to one commit. |
|
Mhh, how can we fix an issue if there are not code changes here? What exactly is the current status vs what is described in the issue? Clearly if we already return NOERROR for A/AAAA requests that means the issues description must be wrong. What requests is |
|
@Luap99 fair point, sorry for the confusion. I went back to the issue and the code. reply_ip already returns Some(req) once the backend lookup succeeds, so an ipv4-only name queried for AAAA already gets NOERROR with an empty answer on main. My PR doesn't change that, it just adds a regression test for it. So the AAAA part of the issue looks like it was already fixed. The NXDOMAIN from the issue output comes from the MX query I believe. host (bind-tools) sends A, AAAA and MX by default. aardvark doesn't handle MX so those get forwarded upstream, and upstream answers NXDOMAIN for names in our zone. That part still doesn't match the expected behavior. So I'm not sure what the best path is. Either I make MX (and other unhandled types) return NODATA for names we know, like Docker does, or we close this PR as stale and keep the regression test. I can do either, just let me know which direction you prefer. |
fix: treat empty AAAA answers as NODATA for known names
reply_ip already returns after a successful backend lookup, so an
IPv4-only name queried for AAAA gets NOERROR with an empty answer
instead of falling through as a miss. Add a regression test for that.
Fixes #679