Skip to content

Update Syntax06#1245

Merged
tgreenx merged 3 commits intozonemaster:developfrom
tgreenx:update-syntax06
Dec 4, 2023
Merged

Update Syntax06#1245
tgreenx merged 3 commits intozonemaster:developfrom
tgreenx:update-syntax06

Conversation

@tgreenx
Copy link
Contributor

@tgreenx tgreenx commented Jun 27, 2023

Purpose

This PR updates Syntax06 implementation in several aspects. I guess these were missed in the latest implementation update (#803) following the specification update (zonemaster/zonemaster#788).

Context

https://zonemaster.fr/en/result/24e61ae29cc4987e (see Syntax section)

Follow-up on #803

Changes

Deviations (from specification as of v2023.1):

  • RNAME_RFC822_VALID was being outputted even if previous RNAME_MAIL_DOMAIN_INVALID were seen.
  • RNAME_MAIL_DOMAIN_LOCALHOST and RNAME_MAIL_ILLEGAL_CNAME coverage was missing in unitary tests. Note that RNAME_MAIL_ILLEGAL_CNAME is still untested.

Corrections:

  • RNAME_MAIL_DOMAIN_INVALID will now be outputted once per mail domain.
  • Refactoring.

How to test this PR

Unit tests should pass. Also:

Now:

$ git log -2 --oneline
239e0f4 (HEAD -> update-syntax06, origin/update-syntax06) Update Syntax06 implementation and unitary tests
76b9dd9 (upstream/develop, develop) Merge pull request #1243 from zonemaster/master

$ zonemaster-cli --test=syntax/syntax06 com.gl --level=INFO --raw
   0.00 INFO     GLOBAL_VERSION   version=v4.7.0
   3.13 NOTICE   RNAME_MAIL_DOMAIN_INVALID   domain=cylnuw65dpky2iccpfjlennewaxz2voitry446wzkorpxyafryoq.mx-verification.google.com.

Before:

$ git log -1 --oneline
8ae6f4b (HEAD -> master, tag: v4.7.0, upstream/master, upstream/HEAD) Merge pull request #1242 from zonemaster/develop

$ zonemaster-cli --test=syntax/syntax06 com.gl --level=INFO --raw
   0.00 INFO     GLOBAL_VERSION   version=v4.7.0
   2.03 INFO     RNAME_RFC822_VALID   rname=gl-admin@tele.gl
   2.60 NOTICE   RNAME_MAIL_DOMAIN_INVALID   domain=cylnuw65dpky2iccpfjlennewaxz2voitry446wzkorpxyafryoq.mx-verification.google.com.
   2.89 NOTICE   RNAME_MAIL_DOMAIN_INVALID   domain=cylnuw65dpky2iccpfjlennewaxz2voitry446wzkorpxyafryoq.mx-verification.google.com.
   3.02 NOTICE   RNAME_MAIL_DOMAIN_INVALID   domain=cylnuw65dpky2iccpfjlennewaxz2voitry446wzkorpxyafryoq.mx-verification.google.com.
   3.16 NOTICE   RNAME_MAIL_DOMAIN_INVALID   domain=cylnuw65dpky2iccpfjlennewaxz2voitry446wzkorpxyafryoq.mx-verification.google.com.
   3.16 NOTICE   RNAME_MAIL_DOMAIN_INVALID   domain=cylnuw65dpky2iccpfjlennewaxz2voitry446wzkorpxyafryoq.mx-verification.google.com.

@tgreenx tgreenx added T-Bug Type: Bug in software or error in test case description A-TestCase Area: Test case specification or implementation of test case V-Patch Versioning: The change gives an update of patch in version. labels Jun 27, 2023
@tgreenx tgreenx added this to the v2023.2 milestone Jun 27, 2023
@tgreenx tgreenx requested review from a user, hannaeko, marc-vanderwal, matsduf and mattias-p June 27, 2023 14:32
@tgreenx tgreenx changed the base branch from master to develop June 27, 2023 14:33
ghost
ghost previously approved these changes Jun 29, 2023
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine

@matsduf
Copy link
Contributor

matsduf commented Jun 29, 2023

The SOA mail address for com.gl is gl-admin@tele.gl. The mail domain, as I see it, is tele.gl.

Then when mx for tele.gl is looked up, you get a list of mail server names:

$ dig tele.gl mx +short | cut -f2 -d' '
aspmx.l.google.com.
alt4.aspmx.l.google.com.
alt1.aspmx.l.google.com.
alt3.aspmx.l.google.com.
alt2.aspmx.l.google.com.
cylnuw65dpky2iccpfjlennewaxz2voitry446wzkorpxyafryoq.mx-verification.google.com.

Those should not be called "mail domains".

The last one gives NXDOMAIN.

@tgreenx tgreenx dismissed ghost ’s stale review via 9aa8687 June 29, 2023 12:37
@tgreenx
Copy link
Contributor Author

tgreenx commented Jun 29, 2023

Those should not be called "mail domains".

I have renamed variables and mentions of "mail domain" to "mail server" in the code, except in the message tags as those require an update to the specification first (but I don't think it's worth it). See commit 9aa8687.

@matsduf
Copy link
Contributor

matsduf commented Jun 29, 2023

I have renamed variables and mentions of "mail domain" to "mail server" in the code, except in the message tags as those require an update to the specification first (but I don't think it's worth it). See commit 9aa8687.

The specification needs a larger update, so I agree with you. I will set up test zones for the test case.

Copy link
Contributor

@marc-vanderwal marc-vanderwal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine; I just have a few style suggestions.

@tgreenx tgreenx requested review from a user and marc-vanderwal July 4, 2023 13:50
marc-vanderwal
marc-vanderwal previously approved these changes Jul 4, 2023
@tgreenx
Copy link
Contributor Author

tgreenx commented Nov 2, 2023

@matsduf @PNAX please re-review

@matsduf
Copy link
Contributor

matsduf commented Dec 4, 2023

As I read the specification, RNAME_RFC822_VALID should be outputted in the nice case, but here nothing is outputted.

$ zonemaster-cli --test=syntax/syntax06 --level=INFO --raw sunet.se
   0.00 INFO     GLOBAL_VERSION  version=v4.7.3

tgreenx and others added 3 commits December 4, 2023 14:34
Deviations (from specification v2023.1):
  - RNAME_RFC822_VALID was being outputted even if previous RNAME_MAIL_DOMAIN_INVALID were seen.
  - RNAME_MAIL_DOMAIN_LOCALHOST and RNAME_MAIL_ILLEGAL_CNAME coverage was missing in unitary tests. Note that RNAME_MAIL_ILLEGAL_CNAME is still untested as of this commit.

Corrections:
  - RNAME_MAIL_DOMAIN_INVALID will be outputted once per mail domain.
  - Refactoring.
Refactoring
Renaming of variables and mentions of "mail domain" to "mail server" (besides message tags)
Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>
@tgreenx
Copy link
Contributor Author

tgreenx commented Dec 4, 2023

As I read the specification, RNAME_RFC822_VALID should be outputted in the nice case, but here nothing is outputted.

$ zonemaster-cli --test=syntax/syntax06 --level=INFO --raw sunet.se
   0.00 INFO     GLOBAL_VERSION  version=v4.7.3

That's because this PR was based on old develop. I rebased, and now it should work as expected:

$ git log -1 --oneline
5d68ba59 (HEAD -> update-syntax06, origin/update-syntax06) Apply suggestions from code review

$ zonemaster-cli --test=syntax/syntax06 --level=INFO --raw sunet.se
   0.00 INFO     GLOBAL_VERSION  version=v4.7.3
  42.43 INFO     RNAME_RFC822_VALID  rname=hostmaster@sunet.se

@tgreenx tgreenx merged commit 7f2c5cd into zonemaster:develop Dec 4, 2023
@tgreenx tgreenx deleted the update-syntax06 branch December 4, 2023 14:56
@matsduf
Copy link
Contributor

matsduf commented Mar 17, 2024

Release testing: Looks fine.

@matsduf matsduf added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-TestCase Area: Test case specification or implementation of test case S-ReleaseTested Status: The PR has been successfully tested in release testing T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants