Adds specification of default query and response handling by matsduf · Pull Request #1000 · zonemaster/zonemaster

matsduf · 2021-10-24T22:16:08Z

Purpose

This PR adds a specification for the default setting of the DNS queries that the test cases emit. Test cases are to refer to this specification and use the default setting or specify the deviation.

By having a shared specification the test cases will be more consistent.
The test case specifications do not have to specify the DNS query when default values are used.

The specification also has default handling of the response, and that will give consistent handling in the test cases.

matsduf · 2021-11-01T12:58:40Z

@vlevigneron, do you have a comment on this PR?

mattias-p

I really like this document!

The filename is misspelled. Also, maybe call it DNSQueryAndResponseDefaults.md?

matsduf

The document is renamed as suggested. Also the headline is changed accordingly.

matsduf · 2021-11-15T18:13:03Z

@vlevigneron, could you review?

mattias-p

LGTM

matsduf · 2021-12-10T14:16:50Z

@mattias-p, updated after our discussion. Please review again.

matsduf · 2022-01-04T15:06:38Z

@mattias-p, @vlevigneron, @PNAX and @tgreenx, please review this.

ghost

This looks good and would definitely remove some redundancy across tests specifications.

Just a question on the handling of an EDNS or DNSSEC response. If the OPT flag or the DO flag are missing, the default handling says that a warning should be raised. However the comment suggest this warning could be downgraded to a DEBUG message.
Are responses without the OPT flag or DO flag valid responses? If so why raising a warning?
Or if they are invalid, should we only display a DEBUG message?
Or am I missing something else?

matsduf · 2022-01-04T16:23:48Z

Just a question on the handling of an EDNS or DNSSEC response. If the OPT flag or the DO flag are missing, the default handling says that a warning should be raised. However the comment suggest this warning could be downgraded to a DEBUG message.

Bad wording. I have rephrased it. Please re-reveiw.

Are responses without the OPT flag or DO flag valid responses? If so why raising a warning? Or if they are invalid, should we only display a DEBUG message? Or am I missing something else?

Not really. Either the OPT should be included, or FORMERR is expected. The handling should be set in the test case specification, but at least a debug message should be issued.

ghost

Thanks for the explanation. Then indeed it seems reasonable to let the test case implementation deal with the missing flag.

matsduf · 2022-01-26T21:01:22Z

@mattias-p, @PNAX and @mattias-p, please re-review.

mattias-p · 2022-01-27T15:58:53Z

+  * The first CNAME in the chain must match *Query Name*.
+  * The last record in the chain must either be a CNAME or a record matching
+    *Query Type*.
+  * For each owner name of the CNAME records in the chain there must not be any
+    additional CNAME records in the answer section (only one CNAME record per
+    owner name).


I'm curious about an edge case here. Let's say we're following a CNAME chain and the query name is example.com. The answer section contains the following CNAME records:

error.com 3600 IN CNAME example.com example.com 3600 IN CNAME other.com

The way I read the specification there's one chain in the answer section, and the first CNAME is error.com, which is different from the query name, so none of these records should be fetched. Is that the intended behavior?

I believe the "The first CNAME in the chain must match Query Name." item should be removed from the list of criteria and added as a definition instead.

I'm curious about an edge case here. Let's say we're following a CNAME chain and the query name is example.com. The answer section contains the following CNAME records:

error.com 3600 IN CNAME example.com example.com 3600 IN CNAME other.com

The way I read the specification there's one chain in the answer section, and the first CNAME is error.com, which is different from the query name, so none of these records should be fetched. Is that the intended behavior?

Yes.

I believe the "The first CNAME in the chain must match Query Name." item should be removed from the list of criteria and added as a definition instead.

I disagree, but I will change the wording somewhat to make intended behavior clear.

ghost · 2022-01-27T17:05:12Z

-|AA flag      | unset        | yes  |                              |
-|TC flag      | unset        | yes  |                              |
+|OpCode       | "query"      | X    |                              |
+|QR flag      | unset        | X    | Always unset in a query      |


Why specify here in the comment that the parameter is unset since it is already fixed to unset?

To underline that fact.

And this fact is not as important for other fixed unset parameters? (I don't know, it's just I was wondering why only this one had the comment).

It is just a comment. The choice of the other fixed, and default, values could also be commented, but I have not done that.

matsduf · 2022-01-27T17:05:25Z

@mattias-p, I have updated based on your comment. Please re-review.

@tgreenx, I have done som updates based on comments from @mattias-p. Please re-review.

matsduf · 2022-01-27T17:07:23Z

@PNAX, please re-review.

ghost · 2022-01-27T17:09:14Z


 * Authority and additional sections are by default ignored.

+The test case specification may prescribe that CNAME records are handled in an a


typo: an a -> a

ghost · 2022-01-27T17:09:53Z

+  * The chain is, by default, considered to be valid if the following criteria
+    are met:
+    * It must be possible to arrange all CNAME records from the answer section
+      into a contiguous logical chain with a posisble addition of a non-CNAME


typo: posisble -> possible

ghost · 2022-01-27T17:12:15Z

I think I said it before, but I think this is a good document to have that we can refer to.

matsduf · 2022-01-27T17:17:29Z

@PNAX, please re-review.

matsduf · 2022-01-28T08:13:02Z

@tgreenx, please re-review.

mattias-p

Awesome!

First version

5a4642c

matsduf added the A-TestCase Area: Test case specification or implementation of test case label Oct 24, 2021

matsduf added this to the v2022.1 milestone Oct 24, 2021

matsduf requested a review from vlevigneron October 24, 2021 22:16

matsduf changed the title ~~First version~~ Adds specification default DNS query and default handling of DNS response for the test case specifications Oct 24, 2021

matsduf changed the title ~~Adds specification default DNS query and default handling of DNS response for the test case specifications~~ Adds specification of default DNS query and default handling of DNS response Oct 26, 2021

matsduf changed the title ~~Adds specification of default DNS query and default handling of DNS response~~ Adds specification of default query and response handling Oct 26, 2021

matsduf requested a review from mattias-p November 15, 2021 10:55

mattias-p reviewed Nov 15, 2021

View reviewed changes

Adds clarifications and editorial updates

ae2063b

matsduf commented Nov 15, 2021

View reviewed changes

matsduf requested a review from mattias-p November 15, 2021 18:12

mattias-p previously approved these changes Nov 15, 2021

View reviewed changes

mattias-p reviewed Dec 2, 2021

View reviewed changes

Comment thread docs/specifications/tests/DNSQueryAndResponseDefaults.md Outdated

Adds more clarifications and editorial updates

778be96

matsduf dismissed mattias-p’s stale review via 778be96 December 10, 2021 14:10

matsduf requested a review from mattias-p December 10, 2021 14:16

matsduf requested review from a user and tgreenx January 4, 2022 15:05

This was referenced Jan 4, 2022

Update specification of Zone01 - #1028 #1032

Merged

New specification of Nameserver11 - #993 #1031

Merged

ghost reviewed Jan 4, 2022

View reviewed changes

Comment thread docs/specifications/tests/DNSQueryAndResponseDefaults.md Outdated

Comment thread docs/specifications/tests/DNSQueryAndResponseDefaults.md Outdated

Editorial updates

1eb4ce1

matsduf requested a review from a user January 4, 2022 16:24

ghost previously approved these changes Jan 4, 2022

View reviewed changes

tgreenx previously approved these changes Jan 26, 2022

View reviewed changes

Comment thread docs/specifications/tests/DNSQueryAndResponseDefaults.md Outdated

Comment thread docs/specifications/tests/DNSQueryAndResponseDefaults.md Outdated

Fixes spelling error and adds editorial updates

88a5994

matsduf dismissed tgreenx’s stale review via 88a5994 January 26, 2022 20:59

matsduf requested a review from tgreenx January 26, 2022 20:59

tgreenx previously approved these changes Jan 27, 2022

View reviewed changes

mattias-p reviewed Jan 27, 2022

View reviewed changes

The handling of CNAME records is more tightly prescribed.

ccc9b7c

matsduf dismissed tgreenx’s stale review via ccc9b7c January 27, 2022 17:03

matsduf requested review from mattias-p and tgreenx January 27, 2022 17:03

ghost reviewed Jan 27, 2022

View reviewed changes

matsduf added 2 commits January 27, 2022 18:15

Editorial updates

9b5c617

Editorial updates

6ede3db

tgreenx previously approved these changes Jan 27, 2022

View reviewed changes

Comment thread docs/specifications/tests/DNSQueryAndResponseDefaults.md Outdated

Clarifies the comparison of owner name and record type

3130520

matsduf dismissed tgreenx’s stale review via 3130520 January 28, 2022 08:11

matsduf requested review from a user and tgreenx January 28, 2022 08:12

Adds lower level headlines and some editorial updates

6ee326b

mattias-p approved these changes Jan 28, 2022

View reviewed changes

tgreenx approved these changes Jan 28, 2022

View reviewed changes

matsduf merged commit f00b86b into zonemaster:develop Jan 28, 2022

matsduf deleted the add-specification-for-queries-and-responses branch January 28, 2022 21:10


		* Authority and additional sections are by default ignored.

		The test case specification may prescribe that CNAME records are handled in an a

Uh oh!

Conversation

matsduf commented Oct 24, 2021

Purpose

Uh oh!

matsduf commented Nov 1, 2021

Uh oh!

mattias-p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matsduf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matsduf commented Nov 15, 2021

Uh oh!

mattias-p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matsduf commented Dec 10, 2021

Uh oh!

matsduf commented Jan 4, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matsduf commented Jan 4, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matsduf commented Jan 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matsduf commented Jan 27, 2022

Uh oh!

matsduf commented Jan 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 27, 2022

Uh oh!

matsduf commented Jan 27, 2022

Uh oh!

Uh oh!

matsduf commented Jan 28, 2022

Uh oh!