-
Notifications
You must be signed in to change notification settings - Fork 34
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.
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
Make root hints configurable, take 2 #1134
Conversation
…her use cases. * Removes the hardcoded list of root hints * Adds a subroutine to read an externa file
* Corrected the algorithm to be able to handle file name "0" * Removed duplcated dot.
This is a step towards making the root hints overridable using the fake data interface.
Allow granularly importing Util module symbols from modules that depend on Util module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good suggestion. It is better to use normal hint file format. To utilize this CLI and Backend need to be updated.
t/util.t
Outdated
{ | ||
name => 'Forbidden $INCLUDE', | ||
hints => "\n\$INCLUDE /etc/motd", | ||
error => qr//, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could check for the error message in the unit test if the presence of forbidden directives is checked before parsing the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. That's better.
I converted this PR back to draft status because I plan to update the test instruction. |
@mvw-afnic I've made updates for your comments. Do you want to review again? |
Hi @mattias-p , I saw you replied to my comments but I don’t see any changes since my first review. Did you push to your branch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
v2022.2 release testingThis works as expected. However when it comes to deal with wrong files, I encountered several behaviors:
|
There are sever limitations in the error handling of Net::DNS. It is known. Another alternative would be to write our own verification script. |
Purpose
In current code the root hints are hard coded. This PR opens for use cases where private root must be used, see #850. The driving use case for this PR is the possibility to use it for unit tests.
Context
Fixes #850.
This PR is based on and replaces #1132. The main difference is in the approach to APIs.
This PR leverages the existing fake addresses API for programmatic configurability, parses IANA root hints files and leaves application user interface design up to the application.
This PR only extends Engine's programmatic interface. Applications need to be adapted to make use of this new feature. E.g. zonemaster-cli and zonemaster-backend. Those interface changes are made separately.
Changes
named.root
file taken directly from IANA.How to test this PR
It would be super nice to automate this test but that is out of scope for this PR.