return NXDOMAIN instead of NOTIMP #589
Conversation
|
wouldn't it have been easier to list the query types that we do implement? |
|
also, you should use the canonical set representation (#518). |
|
Hmm. querying the weave.works domain, I have yet to find a query type that doesn't result in an NXDOMAIN. Perhaps that is what DNS servers typically do? How did you come up with the list of unimplemented types? |
|
I haven't found much information on when to return NOTIMP, just this RFC that recommends using it for rejecting ANY queries... I will reduce the set to just this query type... |
inercia
force-pushed
the
weave-588
branch
2 times, most recently
from
a99ae96
to
95e8b1a
Compare
|
That RFC is a very recent draft. So it's rather flimsy evidence. Is NXDOMAIN a valid response for all query types? The duplication between queryHandler and rdnsHandler is getting out of hand. |
|
We have two options: a) we can just reply with NXDOMAIN for all query types or b) reply with NOTIMPL to ANY queries. I think the safest option is a), but it is up to you @rade |
|
I am fine with (a), provided NXDOMAIN is a valid response for all query types. Is it? |
|
From my understanding of the DNS RFC, I think NXDOMAIN is the general error code (unless otherwise specified). The only documented evidence of the need of NOTIMP is that draft RFC... |
|
ok. In which case let's just go with NXDOMAIN for everything. Would be good to add a smoke tests that checks for the (absence of) the behaviour I reported in #588. Re the the duplication between queryHandler and rdnsHandler, I can take a stab at that after this has been merged. |
|
Just want to confirm my understanding of what we've decided needs to be done for this PR - I think it becomes a one line change to return NXDOMAIN instead of NOTIMP, and a smoke test to that effect? |
|
@awh Yes, returning NXDOMAIN instead of NOTIMP is trivial... no idea of what the smoke test would involve though... |
Something that simply asserts that |
|
you could just run what's in #588, asserting that you get only get one line as an answer / get no NOTIMPs. |
|
@inercia ping |
|
@rade Just updated, with some integration tests now... |
|
Hmm. Do we have to use |
|
I'm afraid we can't: |
Well, as you can see from #588, it does return some error codes, but ignores NXDOMAIN. No idea whether that's a feature of the resolver library or
The smoke tests are meant to operate at a very high level, close to what we expect users/applications to do. Few users/apps query DNS directly; they go through the resolver library. Hence |
|
The code still returns NOTIMP for ANY queries. I thought we had agreed we'd always return NXDOMAIN. |
| assert_dig_on() { | ||
| ok=$(dig_on "$1" "$2" "+short $3") | ||
| assert "echo $ok" "$4" | ||
| } |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
| # assert_dns_status <host> <container> <name> <query> <status> | ||
| assert_dns_status() { | ||
| res=$(dig_on "$1" "$2" "$3" | grep -q "status: $4" && echo "FOUND") | ||
| assert "echo $res" "FOUND" |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
I thought we had agreed to return NOTIMP on ANY queries... anyway, I will remove anything that has to do with NOTIMP |
rade
changed the title
return NXDOMAIN instead of NOTIMP Fixes #588.
Fixes #588 by returning NOTIMP for some unimplemented query types, NXDOMAIN otherwise