New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Test zones for the CNAME function in Recursor.pm in Engine #1220

Merged

matsduf merged 16 commits into zonemaster:develop from matsduf:cname-resolver-test-zones

Jul 4, 2024

Contributor

matsduf commented Nov 15, 2023

Purpose

Test zone for unit tests for zonemaster/zonemaster-engine#1288

How to test this PR

Create unit tests based on these test zones when the PR in engine has been completed.


          Adds test scenarios for the CNAME function in Recursor.pm in Engine

a6f9a44

matsduf added this to the v2023.2 milestone

matsduf requested a review from tgreenx

November 15, 2023 18:44

matsduf mentioned this pull request

Add CNAME followage in Recursor zonemaster/zonemaster-engine#1288

Merged


          Merge branch 'develop' into cname-resolver-test-zones

cb33b87

marc-vanderwal reviewed

View reviewed changes

Contributor

marc-vanderwal left a comment

Looks fine. I just saw a few typos and I have one comment about a problem case.

docs/public/specifications/test-zones/Engine/README.md Outdated Show resolved Hide resolved

docs/public/specifications/test-zones/Engine/README.md Outdated Show resolved Hide resolved

docs/public/specifications/test-zones/Engine/README.md Outdated Show resolved Hide resolved

docs/public/specifications/test-zones/Engine/Recursor-PM/CNAME.md Outdated Show resolved Hide resolved

docs/public/specifications/test-zones/Engine/Recursor-PM/CNAME.md Outdated Show resolved Hide resolved

docs/public/specifications/test-zones/Engine/Recursor-PM/CNAME.md Outdated Show resolved Hide resolved

docs/public/specifications/test-zones/Engine/Recursor-PM/CNAME.md Show resolved Hide resolved

matsduf and others added 5 commits

November 22, 2023 22:02


          Apply suggestions from document review

4d45188

Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>


          Corrects miswriting

0c08dae


          Makes specification more precise in two scenarios

ab6af8b


          Updates zone specification to make it correct for NXDOMAIN and NODATA…

551f926

… cases


          Adds logging from dig output for the scenarios

matsduf commented

View reviewed changes

Contributor Author

matsduf left a comment •

edited

Loading

@marc-vanderwal, also see the dig output in test-zone-data/Engine/Recursor-PM/README.md in this PR.

docs/public/specifications/test-zones/Engine/Recursor-PM/CNAME.md Outdated Show resolved Hide resolved

docs/public/specifications/test-zones/Engine/Recursor-PM/CNAME.md Show resolved Hide resolved

marc-vanderwal reviewed

View reviewed changes

test-zone-data/Engine/Recursor-PM/README.md

		@@ -20,6 +20,446 @@ See [CNAME.md] for specification of the scenarios.

		## zonemaster-cli commands and their output for each test scenario

Contributor

marc-vanderwal Nov 23, 2023

But these are outputs from dig, not zonemaster-cli. And I suppose that they were obtained with CoreDNS.

Contributor Author

matsduf Nov 24, 2023 •

edited

Loading

The outputs are with dig against the CoreDNS configuration. They are there to show what Zonemaster would get if the unit tests query for those names with query type A.

I am waiting for zonemaster/zonemaster-engine#1288 to be done.

This is different since there is no test case to run, is there? I do not see how I could run zonemaster-cli for these scenarios.

Contributor

tgreenx Nov 28, 2023

zonemaster/zonemaster-engine#1288 has been updated to include the changes discussed in the work group. Note that two unit tests are failing due to an issue with CoreDNS, see my other comments.
You should now be able to test this PR properly.

test-zone-data/Engine/Recursor-PM/README.md Outdated Show resolved Hide resolved

matsduf added 2 commits

November 24, 2023 18:39


          Updates from review comments

b90cde2


          Merge branch 'develop' into cname-resolver-test-zones

5c3d6fa

matsduf requested a review from marc-vanderwal

November 24, 2023 17:41

Contributor Author

matsduf commented Nov 24, 2023

@marc-vanderwal and @tgreenx: Please re-review.

marc-vanderwal previously approved these changes

View reviewed changes

Contributor Author

matsduf commented Nov 28, 2023

I want to hear from @tgreenx before merging. It should match zonemaster/zonemaster-engine#1288.

tgreenx reviewed

View reviewed changes

test-zone-data/Engine/Recursor-PM/README.md

Comment on lines +288 to +298

+              ;; ANSWER SECTION:
+              looped-cname-in-zone-1.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-1.cname.recursor.engine.xa.
+              looped-cname-in-zone-1.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-1.cname.recursor.engine.xa.
+              looped-cname-in-zone-1.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-1.cname.recursor.engine.xa.
+              looped-cname-in-zone-1.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-1.cname.recursor.engine.xa.
+              looped-cname-in-zone-1.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-1.cname.recursor.engine.xa.
+              looped-cname-in-zone-1.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-1.cname.recursor.engine.xa.
+              looped-cname-in-zone-1.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-1.cname.recursor.engine.xa.
+              looped-cname-in-zone-1.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-1.cname.recursor.engine.xa.
+              looped-cname-in-zone-1.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-1.cname.recursor.engine.xa.
+              looped-cname-in-zone-1.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-1.cname.recursor.engine.xa.

Contributor

tgreenx Nov 28, 2023 •

edited

Loading

Why are there so many of the same CNAME record here? This is also what I get when running locally. Is it a bug in CoreDNS? There should be just one for this test scenario; otherwise Engine will output CNAME_MULTIPLE_FOR_NAME instead of CNAME_LOOP_INNER.

Contributor Author

matsduf Nov 28, 2023

I guess it is a bug in CoreDNS, but on the other hand, the CNAME records are identical and I feel they should be considered to be one. I will check.

Contributor Author

matsduf Nov 29, 2023

I have reported the bug to CoreDNS (coredns/coredns#6421). At least I think it is a bug. I suspect that CoreDNS "thinks" that it should follow CNAME from the response. Some sort of internal resolver.

I failed to find a work-around.

test-zone-data/Engine/Recursor-PM/README.md Outdated

Comment on lines 328 to 338

+              ;; ANSWER SECTION:
+              looped-cname-in-zone-2.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-2-a.cname.recursor.engine.xa.
+              looped-cname-in-zone-2-a.cname.recursor.engine.xa. 3600	IN CNAME looped-cname-in-zone-2-b.cname.recursor.engine.xa.
+              looped-cname-in-zone-2-b.cname.recursor.engine.xa. 3600	IN CNAME looped-cname-in-zone-2-a.cname.recursor.engine.xa.
+              looped-cname-in-zone-2-a.cname.recursor.engine.xa. 3600	IN CNAME looped-cname-in-zone-2-b.cname.recursor.engine.xa.
+              looped-cname-in-zone-2-b.cname.recursor.engine.xa. 3600	IN CNAME looped-cname-in-zone-2-a.cname.recursor.engine.xa.
+              looped-cname-in-zone-2-a.cname.recursor.engine.xa. 3600	IN CNAME looped-cname-in-zone-2-b.cname.recursor.engine.xa.
+              looped-cname-in-zone-2-b.cname.recursor.engine.xa. 3600	IN CNAME looped-cname-in-zone-2-a.cname.recursor.engine.xa.
+              looped-cname-in-zone-2-a.cname.recursor.engine.xa. 3600	IN CNAME looped-cname-in-zone-2-b.cname.recursor.engine.xa.
+              looped-cname-in-zone-2-b.cname.recursor.engine.xa. 3600	IN CNAME looped-cname-in-zone-2-a.cname.recursor.engine.xa.
+              looped-cname-in-zone-2-a.cname.recursor.engine.xa. 3600	IN CNAME looped-cname-in-zone-2-b.cname.recursor.engine.xa.

Contributor

tgreenx Nov 28, 2023

Same comment here. CNAME records are unexpectedly repeated.

Contributor Author

matsduf Nov 28, 2023

Same answer as above.

Contributor Author

matsduf Nov 29, 2023

I have also reported this, but here I found a work-around wht with dig gives the following result:

; <<>> DiG 9.18.18-0ubuntu0.22.04.1-Ubuntu <<>> @127.30.1.31 looped-cname-in-zone-2.cname.recursor.engine.xa
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 13110
;; flags: qr aa rd; QUERY: 1, ANSWER: 3, AUTHORITY: 1, ADDITIONAL: 1
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
; COOKIE: 7a7a75c08fe99560 (echoed)
;; QUESTION SECTION:
;looped-cname-in-zone-2.cname.recursor.engine.xa. IN A

;; ANSWER SECTION:
looped-cname-in-zone-2.cname.recursor.engine.xa. 3600 IN CNAME looped-cname-in-zone-2-a.cname.recursor.engine.xa.
looped-cname-in-zone-2-a.cname.recursor.engine.xa. 3600	IN CNAME looped-cname-in-zone-2-b.cname.recursor.engine.xa.
looped-cname-in-zone-2-b.cname.recursor.engine.xa. 3600	IN CNAME looped-cname-in-zone-2-a.cname.recursor.engine.xa.

;; AUTHORITY SECTION:
cname.recursor.engine.xa. 3600	IN	NS	ns1.cname.recursor.engine.xa.

;; Query time: 0 msec
;; SERVER: 127.30.1.31#53(127.30.1.31) (UDP)
;; WHEN: Wed Nov 29 13:07:05 UTC 2023
;; MSG SIZE  rcvd: 488

I will update.

test-zone-data/Engine/Recursor-PM/README.md

		@@ -20,6 +20,446 @@ See [CNAME.md] for specification of the scenarios.

		## zonemaster-cli commands and their output for each test scenario

Contributor

tgreenx Nov 28, 2023

zonemaster/zonemaster-engine#1288 has been updated to include the changes discussed in the work group. Note that two unit tests are failing due to an issue with CoreDNS, see my other comments.
You should now be able to test this PR properly.


          Adds work-around to give more sane DNS response

01eb8c3

matsduf dismissed marc-vanderwal’s stale review via

01eb8c3

November 29, 2023 13:43

Contributor Author

matsduf commented Nov 29, 2023

@marc-vanderwal and @tgreenx: Please re-review.

matsduf added 2 commits

November 30, 2023 16:46


          Adds three additional scenarios

42e2d36


          Adds three new scenarios, LOOPED-CNAME-IN-ZONE-3, WRONG-CNAME-OWNER-N…

82b3aec

…AME and EXTRA-CNAME-IN-ANSWER

Contributor Author

matsduf commented Dec 1, 2023

@marc-vanderwal and @tgreenx: Please re-review.

matsduf requested review from tgreenx and marc-vanderwal

December 1, 2023 12:18


          Merge branch 'develop' into cname-resolver-test-zones

609c23d

tgreenx reviewed

View reviewed changes

Contributor

tgreenx left a comment

Commit zonemaster/zonemaster-engine@32244e9 adds a message tag CNAME_START whenever the CNAME lookup functionality is used. This could be added to the list of expected message tags for each scenario.

docs/public/specifications/test-zones/Engine/Recursor-PM/CNAME.md Show resolved Hide resolved

docs/public/specifications/test-zones/Engine/Recursor-PM/CNAME.md Outdated Show resolved Hide resolved

matsduf modified the milestones: v2023.2, v2024.1


          Fixes typo

cba3a9a

Contributor Author

matsduf commented Dec 7, 2023

Commit zonemaster/zonemaster-engine@32244e9 adds a message tag CNAME_START whenever the CNAME lookup functionality is used. This could be added to the list of expected message tags for each scenario.

What is the expected output where I have put "??"? Could you check the table?

tgreenx reviewed

View reviewed changes

docs/public/specifications/test-zones/Engine/Recursor-PM/CNAME.md Outdated Show resolved Hide resolved


          Apply suggestions from document review

28b83b6

Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>

marc-vanderwal reviewed

View reviewed changes

docs/public/specifications/test-zones/Engine/Recursor-PM/CNAME.md Outdated Show resolved Hide resolved


          Update docs/public/specifications/test-zones/Engine/Recursor-PM/CNAME.md

29fe434

From review comment.

Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>

marc-vanderwal approved these changes

View reviewed changes

matsduf requested a review from tgreenx

May 22, 2024 11:53

tgreenx approved these changes

View reviewed changes

tgreenx modified the milestones: v2024.1, v2024.2

matsduf merged commit 946f837 into zonemaster:develop

matsduf deleted the cname-resolver-test-zones branch

July 4, 2024 14:55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet