Open
Conversation
- `Zonemaster::Engine::Recursor::_resolve_cname()` now returns undef when the CNAME resolution fails, along with a new message tag: `CNAME_UNRESOLVABLE` - Update unit tests: move the content of scenario `TOO-LONG-CNAME-CHAIN` to a new scenario `TOO-MANY-CNAME`, and update scenario `TOO-LONG-CNAME-CHAIN` to check for tag `CNAME_CHAIN_TOO_LONG` instead
tgreenx
requested review from
MichaelTimbert,
marc-vanderwal,
matsduf,
mattias-p and
tolvmannen
tgreenx
added
V-Patch
Comment on lines
+274
to
+276
| local $TODO = "Need to create zones with those errors: "; | ||
| my @missing_tags = qw( CNAME_UNRESOLVABLE ); | ||
| warn $TODO, "\n\t", join("\n\t", @missing_tags), "\n"; |
Contributor
There was a problem hiding this comment.
You could write the test already, even if you haven’t set up the infrastructure yet. It will fail, but because it’s in a TODO block, it is expected to fail, and therefore this failure will not fail the test suite itself. And if it passes, you’ll get a message about an “unexpectedly passing TODO test” reminding you to remove the TODO block.
Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>
| return ( $p, $state ); | ||
| # Catch-all; unforeseen problem in CNAME resolution | ||
| Zonemaster::Engine->logger->add( CNAME_UNRESOLVABLE => { name => $name, type => $type, target => $target } ); | ||
| return ( undef, $state ); |
Contributor
There was a problem hiding this comment.
We’ll reach that case when the recursor is trying to resolve a name that is an alias to a subdomain of that name, and there is no resource record of the requested type at the alias’ target. For example, the break-me.test domain name in the zone file below:
$ORIGIN test.
;; SOA records, NS records, etc.
break-me IN CNAME sub.break-me
;; sub.break-me is an empty non-terminal
*.sub.break-me IN AAAA ::
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This PR slightly updates the CNAME resolution of Engine's recursor. See the
Changessection below.Updated test scenarios are defined in zonemaster/zonemaster#1477.
Context
zonemaster/zonemaster#1477
Changes
Zonemaster::Engine::Recursor::_resolve_cname()now returns undef when the CNAME resolution fails, along with a new message tag:CNAME_UNRESOLVABLETOO-LONG-CNAME-CHAINto a new scenarioTOO-MANY-CNAME, and update scenarioTOO-LONG-CNAME-CHAINto check for tagCNAME_CHAIN_TOO_LONGinsteadHow to test this PR
Unit tests are updated and should pass.