Skip to content

Conversation

@mattias-p
Copy link
Member

@mattias-p mattias-p commented Oct 9, 2025

Purpose

Ignore and deprecate profile properties that complicate query parameter semantics for no good reason, and allows user configuration to cause Zonemaster to misbehave for no good reason.

Context

Fixes the remainder of #1148.

Changes

Ignore and deprecate these profile properties:

  • resolver.defaults.igntc
  • resolver.defaults.recurse
  • resolver.defaults.usevc

How to test this PR

Here is a profile file that sets all four properties to unreasonable values:

{"resolver":{"defaults":{"igntc":true,"recurse":true,"usevc":true}}}

I don't have any examples of zones that break with that profile, but it does break ASN lookups without the changes in this PR.

@mattias-p mattias-p added this to the v2025.2 milestone Oct 9, 2025
@mattias-p mattias-p changed the base branch from master to develop October 9, 2025 17:41
@matsduf
Copy link
Contributor

matsduf commented Oct 10, 2025

I cannot find a good documentation of what fallback really means. I find "Use new fallback mechanism (try EDNS, then do TCP)" and that seems wrong. Then fallback should be FALSE by default.

@mattias-p
Copy link
Member Author

mattias-p commented Oct 10, 2025

I feel the semantics of fallback is out of scope for this particular PR.
I suggest that the desired semantics should be described in Default handling of a DNS Response. Or, I guess they are already?

@matsduf
Copy link
Contributor

matsduf commented Oct 10, 2025

This PR suggests to set fallback to always TRUE which is problematic.

Zonemaster/Engine/Profile.pm says:

  resolver.defaults.igntc
    A boolean. If false, UDP queries that get responses with the "TC" flag
    set will be automatically resent over TCP. Default false.

Yes, we want resolver.defaults.igntc to be FALSE by default, but it should be possible to be TRUE in some test cases. That is defined in Default handling of a DNS Response.

Now we come to fallback.

  resolver.defaults.fallback
    A boolean. If true, UDP queries that get responses with the "TC" flag
    set will be automatically resent over TCP or using EDNS. Default true.

    In ldns-1.7.0 (NLnet Labs), in case of truncated answer when UDP is
    used, the same query is resent with EDNS0 and TCP (if needed). If you
    want the original answer (with TC bit set) and avoid this kind of
    replay, set this flag to false.

We want resolver.defaults.fallback to be always FALSE because there should be no magic of converting non-EDNS queries as EDNS queries. That should be done in a controlled manner by creating a new query.

@mattias-p
Copy link
Member Author

Here is some context for why the default "fallback" value is true: #460

@matsduf
Copy link
Contributor

matsduf commented Oct 15, 2025

Here is some context for why the default "fallback" value is true: #460

I do not see it in #460. Can you please point it out?

Is that due to some strange behavior in LDNS that makes resolver.defaults.igntc not do what it should? Setting resolver.defaults.igntc should be enough.

@matsduf
Copy link
Contributor

matsduf commented Oct 20, 2025

@mattias-p, please set correct V-XXX and RC-XXX labels on it.

RD flag, protocol and TC upgrade policy for a given query should be
determined by code, not config.
Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

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

Else it looks fine.

=head2 resolver.defaults.igntc
A boolean. Default false. Ignored. Deprecated and planned for removal in v2026.1. Remove it from your profile file.
Copy link
Contributor

Choose a reason for hiding this comment

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

After removal the profile will break, won't it? Maybe give longer time, v2026.2?

Copy link
Contributor

Choose a reason for hiding this comment

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

For this one I'm fine with either.

@tgreenx tgreenx added V-Patch Versioning: The change gives an update of patch in version. RC-Deprecations Release category: Deprecations. labels Nov 6, 2025
=head2 resolver.defaults.igntc
A boolean. Default false. Ignored. Deprecated and planned for removal in v2026.1. Remove it from your profile file.
Copy link
Contributor

Choose a reason for hiding this comment

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

For this one I'm fine with either.

@tgreenx
Copy link
Contributor

tgreenx commented Nov 6, 2025

@mattias-p, please set correct V-XXX and RC-XXX labels on it.

@mattias-p @matsduf I've set RC-Deprecations and V-Patch (maybe V-Minor instead? I'm not sure).

@matsduf
Copy link
Contributor

matsduf commented Nov 7, 2025

@mattias-p @matsduf I've set RC-Deprecations and V-Patch (maybe V-Minor instead? I'm not sure).

That looks correct. When the deprecated is removed it will be a breaking change, but not now. And no functionality is added.

@mattias-p mattias-p merged commit 0e1adfa into zonemaster:develop Nov 13, 2025
3 checks passed
@MichaelTimbert MichaelTimbert added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Dec 8, 2025
@MichaelTimbert
Copy link
Contributor

Tested on Rocky 8 and work fine.

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

Labels

RC-Deprecations Release category: Deprecations. S-ReleaseTested Status: The PR has been successfully tested in release testing 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.

4 participants