use name normalization method #1132

hannaeko · 2023-10-24T13:27:45Z

Purpose

Use new name normalization method instead of Basic00.

Context

zonemaster/zonemaster-engine#1040 introduce a new method to normalize domain names before testing. Let's use it.

It also fixes #1032.

Changes

Normalize domain name before saving to database
Use normalized domain for history search
Normalize domain name before fetching data from parent (fixes issue that cause that method to not work for idna domains, see Cannot lookup data for IDN domain zonemaster-gui#426)

Note: With this PR, domains are normalized before being saved to database, meaning that domains that contain ulabel will be converted to alabel before saving. That might break history for the affected domains. Should we provide a migration script to normalize all the domains in the database?

How to test this PR

Test that the domain validation is working

% ./script/zmb --server http://127.0.0.1:5000 start_domain_test --domain '' | jq .error.data -c
[{"message":"Domain name is empty.","path":"/domain"}]

% ./script/zmb --server http://127.0.0.1:5000 start_domain_test --domain , | jq .error.data -c
[{"path":"/domain","message":"Domain name has an ASCII label (\",\") with a character not permitted."}]

Test that the validation is also working for nameserver

% ./script/zmb --server http://127.0.0.1:5000 start_domain_test --domain example.org --nameserver ':0.0.0.0' | jq .error.data -c
[{"path":"/nameservers/0/ns","message":"Domain name is empty."}]

Test that fetching data from parent is working for idna domains

% curl 'http://localhost:5000' --data '{"jsonrpc":"2.0","id":0,"method":"get_data_from_parent_zone","params":{"domain":"café.fr"}}'
{"jsonrpc":"2.0","result":{"ds_list":[],"ns_list":[{"ns":"ns1.parkingcrew.net","ip":"13.248.158.159"},{"ns":"ns2.parkingcrew.net","ip":"76.223.21.9"}]},"id":0}

Try lauching a test using a GUI, check that it is working correctly.
Then check history, check that is is working correctly.

lib/Zonemaster/Backend/DB.pm

lib/Zonemaster/Backend/Validator.pm

hannaeko · 2023-11-06T09:33:58Z

Waiting for #1092 to update the DB migration script.

mattias-p

LGTM

lib/Zonemaster/Backend/DB.pm

matsduf

Press return to run 'make distcheck'...
"/usr/local/bin/perl" "-Iinc" "-MExtUtils::Manifest=fullcheck" -e fullcheck
Not in MANIFEST: t/db_ddl.t
Not in MANIFEST: t/queue.t

The error comes from older PRs, not this one. Fixed in #1136.

Error not from this PR.

matsduf · 2023-11-27T14:34:23Z

Rå$ttgift.se is captured as having errors ("Domain name has a non-ASCII label ("Rå$ttgift") which is not a valid U-label.") but Rå_ttgift.se passes without error. I assume this is an error in Engine.

matsduf · 2023-11-27T14:35:24Z

It does not work to use zmb for IDN names with U-label, see #1138.

marc-vanderwal · 2023-12-04T14:17:39Z

Waiting for #1092 to update the DB migration script.

That PR has now been merged.

hannaeko · 2023-12-05T14:27:25Z

I have updated the migration script. I am currently running it against a snapshot of zonemaster.net (1.9M tests) to see how long it takes.

mattias-p

LGTM. I have one question though.

mattias-p · 2023-12-05T14:54:06Z

share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl

+    while ( my $row = $sth1->fetchrow_hashref ) {
+        my $hash_id = $row->{hash_id};
+        my $raw_params = decode_json($row->{params});
+        my $domain = $raw_params->{domain};


We're extracting domain from params and putting in its own field, but we're still keeping a copy inside params. Is this intentional? Should this be further cleaned up in the future?

The domain is still in the params, whether it could / should be removed is a good question, but outside of this PR.

Here I am taking the domain from the params instead its own field for normalization to avoid encoding issues.

tgreenx

LGTM, although Travis can't test it.

matsduf · 2023-12-05T22:44:15Z

@blacksponge, What happens if there is a domain name that cannot be normalized, e.g. containing character not allowed or being a U-label that cannot be converted to A-label? I think I would suggest keeping the domain name as-is in the database.

hannaeko · 2023-12-06T08:59:13Z

@blacksponge, What happens if there is a domain name that cannot be normalized, e.g. containing character not allowed or being a U-label that cannot be converted to A-label? I think I would suggest keeping the domain name as-is in the database.

Good suggestion, I'll do that.

hannaeko · 2023-12-08T13:16:06Z

@blacksponge, What happens if there is a domain name that cannot be normalized, e.g. containing character not allowed or being a U-label that cannot be converted to A-label? I think I would suggest keeping the domain name as-is in the database.

Good suggestion, I'll do that.

I implemented that.

% perl share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl
Configured database engine: SQLite
Starting database migration

-> (1/2) Populating new result_entries table
Will update 0 rows
Progress update: 0 / 0

-> (2/2) Normalizing domain names
Will update 5 rows
Caught error while updating record, ignoring: Caught Zonemaster::Backend::Error::Internal in the `Zonemaster::Backend::DB::_normalize_domain` method: Normalizing domain returned errors. Context: ['Domain name has repeated dots.']
20%
40%
60%
80%
100%

Migration done

mattias-p · 2023-12-08T14:29:25Z

share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl

+            $db->dbh->do('UPDATE test_results SET domain = ?, params = ?, fingerprint = ? where hash_id = ?', undef, $domain, $params, $fingerprint, $hash_id);
+        };
+        if ($@) {
+            warn "Caught error while updating record, ignoring: $@\n";


Would it be helpful to explicitly include the hash id or perhaps the domain name itself in this message?

I updated it to include the hash id,

Configured database engine: SQLite Starting database migration -> (1/2) Populating new result_entries table Will update 0 rows Progress update: 0 / 0 -> (2/2) Normalizing domain names Will update 5 rows Caught error while updating record with hash id b212fc97d29d51f3, ignoring: Caught Zonemaster::Backend::Error::Internal in the `Zonemaster::Backend::DB::_normalize_domain` method: Normalizing domain returned errors. Context: ['Le nom de domaine contient plusieurs points successifs.'] 20% 40% 60% 80% 100% Migration done

matsduf · 2023-12-11T12:57:13Z

share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl

+        if ($@) {
+            warn "Caught error while updating record with hash id $hash_id, ignoring: $@\n";
+        }


Can you see any other error than the domain name cannot be normalized or some problem with the database engine? For the first error I would suggest a milder wording, something like that the domain name cannot be normalized and is kept in its original form. The user should be not in doubt that this can be ignored.

A database error should maybe even result in a die?

I was thinking of decoding related errors.

matsduf · 2023-12-11T12:58:40Z

share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl

+            $domain = Zonemaster::Backend::DB::_normalize_domain( $domain );
+
+            $db->dbh->do('UPDATE test_results SET domain = ?, params = ?, fingerprint = ? where hash_id = ?', undef, $domain, $params, $fingerprint, $hash_id);


If normalization fails, $domain will be set to empty, won't it? And then the database update will attempt to set an empty domain? And the database update will fail because the domain must not be empty?

No, it stops the execution and exit the eval block as there is a die in the _normalize_domain method.

So if there is one single domain name that cannot be normalized then no conversion can be done. I do not think that is reasonable.

Well not at all, I am updating the domains one by one, so if one domain fail it does not affect the others.

The eval block just handle the migration of one single domain.

ghost

lgtm

matsduf

It must be possible to ignore a domain name that cannot be normalized. I think the script should ignore such a domain name by default, and just keep that domain name unnormalized, and proceed with the conversion.

hannaeko · 2023-12-13T09:12:20Z

It must be possible to ignore a domain name that cannot be normalized. I think the script should ignore such a domain name by default, and just keep that domain name unnormalized, and proceed with the conversion.

This is exactly what it does.

mattias-p · 2024-01-15T14:17:28Z

v2023.2 Release Testing

I repeated the steps in the "How to test this PR" section on Rocky Linux 8.9 and found no problems.

hannaeko requested review from mattias-p, matsduf, a user, tgreenx and marc-vanderwal October 24, 2023 13:28

hannaeko added this to the v2023.2 milestone Oct 24, 2023

hannaeko added the V-Minor Versioning: The change gives an update of minor in version. label Oct 24, 2023

hannaeko mentioned this pull request Oct 24, 2023

Validate nameserver's ns parameter before passing it to Engine #783

Closed

marc-vanderwal reviewed Oct 25, 2023

View reviewed changes

lib/Zonemaster/Backend/DB.pm Outdated Show resolved Hide resolved

lib/Zonemaster/Backend/Validator.pm Outdated Show resolved Hide resolved

mattias-p previously approved these changes Nov 17, 2023

View reviewed changes

lib/Zonemaster/Backend/DB.pm Outdated Show resolved Hide resolved

tgreenx linked an issue Nov 21, 2023 that may be closed by this pull request

Creating test with empty domain gives confusing error message #1032

Open

hannaeko added 4 commits November 21, 2023 16:38

use name normalization method

b380c85

fix typo in comment

8b28668

use suggestion

a8ab03d

fix unit test

192a77d

hannaeko force-pushed the domain-normalization branch from 4497c2b to 192a77d Compare November 21, 2023 15:38

matsduf previously requested changes Nov 23, 2023

View reviewed changes

matsduf mentioned this pull request Nov 27, 2023

zmb does not handle IDN names correctly #1138

Open

matsduf previously approved these changes Nov 27, 2023

View reviewed changes

This was referenced Nov 29, 2023

Remove Basic00 Test Case zonemaster/zonemaster#1226

Merged

Remove Basic00 implementation zonemaster/zonemaster-engine#1308

Merged

tgreenx mentioned this pull request Dec 4, 2023

CI: update dependency and Perl versions #1140

Merged

update database migration script

6c85dbd

hannaeko dismissed stale reviews from matsduf and mattias-p via 6c85dbd December 5, 2023 14:24

die if errors occur in normaliz_domain method

a673796

mattias-p previously approved these changes Dec 5, 2023

View reviewed changes

tgreenx previously approved these changes Dec 5, 2023

View reviewed changes

marc-vanderwal previously approved these changes Dec 6, 2023

View reviewed changes

ghost mentioned this pull request Dec 7, 2023

CI: Ubuntu 22.04 comes with PostgreSQL 14 #1142

Merged

tgreenx mentioned this pull request Dec 7, 2023

Clickhouse: new database engine (experimental) #1094

Closed

ignore error in migration script

396bd31

hannaeko dismissed stale reviews from marc-vanderwal, tgreenx, and mattias-p via 396bd31 December 8, 2023 13:13

mattias-p previously approved these changes Dec 8, 2023

View reviewed changes

matsduf mentioned this pull request Dec 11, 2023

PRs for v2023.2 zonemaster/zonemaster#1232

Closed

add hash id in warning message

2359703

hannaeko dismissed mattias-p’s stale review via 2359703 December 11, 2023 10:48

marc-vanderwal approved these changes Dec 11, 2023

View reviewed changes

matsduf reviewed Dec 11, 2023

View reviewed changes

ghost approved these changes Dec 13, 2023

View reviewed changes

matsduf requested changes Dec 13, 2023

View reviewed changes

matsduf approved these changes Dec 13, 2023

View reviewed changes

hannaeko merged commit d5b4e57 into zonemaster:develop Dec 13, 2023
1 check passed

mattias-p added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jan 15, 2024

tgreenx mentioned this pull request Sep 17, 2024

Normalize zone name before test start zonemaster/zonemaster-engine#739

Closed

tgreenx linked an issue Sep 17, 2024 that may be closed by this pull request

Normalize zone name before test start zonemaster/zonemaster-engine#739

Closed

tgreenx mentioned this pull request Sep 17, 2024

Undelegated tests crash when the domain has a trailing dot zonemaster/zonemaster-engine#965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use name normalization method #1132

use name normalization method #1132

hannaeko commented Oct 24, 2023 •

edited

Loading

hannaeko commented Nov 6, 2023

mattias-p left a comment

matsduf left a comment •

edited

Loading

matsduf commented Nov 27, 2023

matsduf commented Nov 27, 2023

marc-vanderwal commented Dec 4, 2023

hannaeko commented Dec 5, 2023 •

edited

Loading

mattias-p left a comment

mattias-p Dec 5, 2023

hannaeko Dec 5, 2023

tgreenx left a comment

matsduf commented Dec 5, 2023 •

edited

Loading

hannaeko commented Dec 6, 2023

hannaeko commented Dec 8, 2023

mattias-p Dec 8, 2023 •

edited

Loading

hannaeko Dec 11, 2023

matsduf Dec 11, 2023

hannaeko Dec 12, 2023

matsduf Dec 11, 2023

hannaeko Dec 12, 2023 •

edited

Loading

matsduf Dec 13, 2023

hannaeko Dec 13, 2023

hannaeko Dec 13, 2023

ghost left a comment

matsduf left a comment

hannaeko commented Dec 13, 2023

mattias-p commented Jan 15, 2024 •

edited

Loading

		$domain = Zonemaster::Backend::DB::_normalize_domain( $domain );

		$db->dbh->do('UPDATE test_results SET domain = ?, params = ?, fingerprint = ? where hash_id = ?', undef, $domain, $params, $fingerprint, $hash_id);

use name normalization method #1132

use name normalization method #1132

Conversation

hannaeko commented Oct 24, 2023 • edited Loading

Purpose

Context

Changes

How to test this PR

hannaeko commented Nov 6, 2023

mattias-p left a comment

Choose a reason for hiding this comment

matsduf left a comment • edited Loading

Choose a reason for hiding this comment

matsduf commented Nov 27, 2023

matsduf commented Nov 27, 2023

marc-vanderwal commented Dec 4, 2023

hannaeko commented Dec 5, 2023 • edited Loading

mattias-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgreenx left a comment

Choose a reason for hiding this comment

matsduf commented Dec 5, 2023 • edited Loading

hannaeko commented Dec 6, 2023

hannaeko commented Dec 8, 2023

mattias-p Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannaeko Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

matsduf left a comment

Choose a reason for hiding this comment

hannaeko commented Dec 13, 2023

mattias-p commented Jan 15, 2024 • edited Loading

v2023.2 Release Testing

hannaeko commented Oct 24, 2023 •

edited

Loading

matsduf left a comment •

edited

Loading

hannaeko commented Dec 5, 2023 •

edited

Loading

matsduf commented Dec 5, 2023 •

edited

Loading

mattias-p Dec 8, 2023 •

edited

Loading

hannaeko Dec 12, 2023 •

edited

Loading

mattias-p commented Jan 15, 2024 •

edited

Loading