Bug #120085 for Net-DNS: No brackets around multi-line RRs any more?

Fri Feb 03 13:35:22 2017 rt [...] illuminated.co.uk - Ticket created

Subject:

No brackets around multi-line RRs any more?

Consider the following record, generated in our tests: DB<9> x $rr 0 Net::DNS::RR::SOA=HASH(0x19da0e28) 'expire' => 604800 'minimum' => 10800 'mname' => Net::DNS::DomainName1035=HASH(0x1a253618) 'label' => ARRAY(0x1a27e1b0) 0 'ns' 1 'example' 2 'org' 'owner' => Net::DNS::DomainName1035=HASH(0x1a21dfb8) 'label' => ARRAY(0x1a2650b8) 0 'example' 1 'com' 'name' => 'example.com' 'refresh' => 14400 'retry' => 3600 'rname' => Net::DNS::Mailbox1035=HASH(0x1a2555d0) 'label' => ARRAY(0x1a223760) 0 'sn' 1 'example' 2 'org' 'serial' => 1439185151 'ttl' => 86400 'type' => 6 In Net::DNS 1.06, $rr->rdstring is wrapped with brackets: DB<10> print $rr->rdstring ( ns.example.org. sn.example.org. 1439185151 ;serial 14400 ;refresh 3600 ;retry 604800 ;expire 10800 ;minimum ) In Net::DNS 1.07 (a change introduced, I believe, in 1.06_01), there are no brackets: DB<4> print $rr->rdstring ns.example.org. sn.example.org. 1439185151 ;serial 14400 ;refresh 3600 ;retry 604800 ;expire 10800 ;minimum I believe this change was introduced in the refactor of Net::DNS::RR::rdstring to use the wrap method. Was this deliberate?

Sun Feb 05 06:26:58 2017 rwfranks [...] acm.org - Correspondence added

From:

rwfranks [...] acm.org

On Fri Feb 03 13:35:22 2017, SKINGTON wrote: Show quoted text

> Consider the following record, generated in our tests: > > DB<9> x $rr > 0 Net::DNS::RR::SOA=HASH(0x19da0e28) > 'expire' => 604800 > 'minimum' => 10800 > 'mname' => Net::DNS::DomainName1035=HASH(0x1a253618) > 'label' => ARRAY(0x1a27e1b0) > 0 'ns' > 1 'example' > 2 'org' > 'owner' => Net::DNS::DomainName1035=HASH(0x1a21dfb8) > 'label' => ARRAY(0x1a2650b8) > 0 'example' > 1 'com' > 'name' => 'example.com' > 'refresh' => 14400 > 'retry' => 3600 > 'rname' => Net::DNS::Mailbox1035=HASH(0x1a2555d0) > 'label' => ARRAY(0x1a223760) > 0 'sn' > 1 'example' > 2 'org' > 'serial' => 1439185151 > 'ttl' => 86400 > 'type' => 6 > > In Net::DNS 1.06, $rr->rdstring is wrapped with brackets: > > DB<10> print $rr->rdstring > ( ns.example.org. sn.example.org. > 1439185151 ;serial > 14400 ;refresh > 3600 ;retry > 604800 ;expire > 10800 ;minimum > ) > > In Net::DNS 1.07 (a change introduced, I believe, in 1.06_01), there > are no brackets: > DB<4> print $rr->rdstring > ns.example.org. sn.example.org. > 1439185151 ;serial > 14400 ;refresh > 3600 ;retry > 604800 ;expire > 10800 ;minimum > > I believe this change was introduced in the refactor of > Net::DNS::RR::rdstring to use the wrap method. Was this deliberate?

Sun Feb 05 06:27:00 2017 The RT System itself - Status changed from 'new' to 'open'

Sun Feb 05 08:21:05 2017 rwfranks [...] acm.org - Correspondence added

From:

rwfranks [...] acm.org

The RT server seems to have fallen over and trashed my reply! On Fri Feb 03 13:35:22 2017, SKINGTON wrote: Show quoted text

> In Net::DNS 1.06, $rr->rdstring is wrapped with brackets:

8< Show quoted text

> In Net::DNS 1.07 (a change introduced, I believe, in 1.06_01), there > are no brackets:

8< Show quoted text

> I believe this change was introduced in the refactor of > Net::DNS::RR::rdstring to use the wrap method. Was this deliberate?

Yes. RFC1035 (5.1) says: ( ) Parentheses are used to group data that crosses a line boundary. In effect, line terminations are not recognized within parentheses. Note the word used is "data" rather than the more specific "RDATA". Zone (master) file format is specified as a sequence of directives and complete RRs. Nowhere is it specified where brackets may, or may not, appear. There is no requirement for the result of $rr->rdstring(), in isolation, to be parsable by a zone file parser. Neither is there any justification for the view that the brackets are an integral part of the RDATA.

Sun Feb 05 09:09:36 2017 rt [...] illuminated.co.uk - Correspondence added

On Sun Feb 05 08:21:05 2017, rwfranks@acm.org wrote: [...] Show quoted text

> There is no requirement for the result of $rr->rdstring(), in > isolation, to be parsable by a zone file parser. > > Neither is there any justification for the view that the brackets are > an integral part of the RDATA.

OK, that sounds plausible (I'm not a DNS expert). But can we have that change documented please? We have tests at work that check the results of $rr->rdstring and they broke with the latest version of Net::DNS; if there had been something in the Changes that said "we do things this way now, it's fine" I wouldn't have had to delve into the code to work out what was happening.

Sun Feb 05 13:10:32 2017 rwfranks [...] acm.org - Correspondence added

From:

rwfranks [...] acm.org

On Sun Feb 05 09:09:36 2017, SKINGTON wrote: Show quoted text

> On Sun Feb 05 08:21:05 2017, rwfranks@acm.org wrote: > [...]

> > There is no requirement for the result of $rr->rdstring(), in > > isolation, to be parsable by a zone file parser. > > > > Neither is there any justification for the view that the brackets are > > an integral part of the RDATA.

> > OK, that sounds plausible (I'm not a DNS expert). But can we have that > change documented please? We have tests at work that check the results > of $rr->rdstring and they broke with the latest version of Net::DNS; > if there had been something in the Changes that said "we do things > this way now, it's fine" I wouldn't have had to delve into the code to > work out what was happening.

If your test is to add any value at all, it needs to test that requirements as documented in the (many) relevant RFCs are met. The documentation for $rr->rdstring(), which first appeared in 0.69, has always claimed: Returns a string representation of the RR-specific data. This is a different proposition from the format described for $rr->string(): Returns a string representation of the RR using the zone file format described in RFC1035. Your test is based on the unwarranted assumption that the two formats would be identical, which claim is not supported by the documentation. In this instance, the fault lies in your test. It also demonstrates the folly of getting too cosy with the implementation when constructing the test. I admit to making the same assumption in the code introduced at 0.69, when $rr->string() was implemented by calling $rr->rdstring(), which seemed convenient at the time, but had the unintended consequence of inserting brackets where not required. This began to unravel at 1.01 when the boundary between Net::DNS and Net::DNS::SEC was redrawn. Line-wrapping becomes a significant issue when dealing with zone files containing DNSKEY and RRSIG records.

Sun Feb 05 13:35:08 2017 rt [...] illuminated.co.uk - Correspondence added

On Sun Feb 05 13:10:32 2017, rwfranks@acm.org wrote: Show quoted text

> If your test is to add any value at all, it needs to test that > requirements as documented in the (many) relevant RFCs are met.

As it happens, the test in question checks that an internal API call of ours, which I think was provided so our front-end Javascript code can do DNS lookups, returns data in something like a reasonable format. It doesn't pretend to be part of an exhaustive DNS test suite, and I don't think I ever suggested it did? When the other devs are back at work tomorrow I'll check what this route is used for, and whether we'd be happy using $rr->string rather than $rr->rdstring. (Or, if we prefer the shorter format we get from $rr->rdstring, whether we're happy mandating Net::DNS 1.07 or above.) All I'm asking for - literally, this is *all* I'm asking for - is a mention in the Changes file that the behaviour of $rr->rdstring changed. As it is, when I narrowed the test failure down to Net::DNS 1.07 vs Net::DNS 1.06, and looked at the Changes file of Net::DNS, I saw a mention of three RT tickets and nothing else. I checked the tickets and didn't see anything that matched the behaviour I was seeing. If there had been literally *any* mention of "some internal refactoring" or something, I'd have double-checked that against the documentation, confirmed that $rr->rdstring wasn't guaranteed to be in any particular format, and not bothered you with any of this.

Sun Feb 05 17:40:20 2017 rwfranks [...] acm.org - Correspondence added

From:

rwfranks [...] acm.org

On Sun Feb 05 13:35:08 2017, SKINGTON wrote: Show quoted text

> On Sun Feb 05 13:10:32 2017, rwfranks@acm.org wrote:

> > If your test is to add any value at all, it needs to test that > > requirements as documented in the (many) relevant RFCs are met.

> > As it happens, the test in question checks that an internal API call > of ours, which I think was provided so our front-end Javascript code > can do DNS lookups, returns data in something like a reasonable > format.

If "reasonable format" is the full extent of the requirement, the presence or absence of brackets seems irrelevant. Show quoted text

> All I'm asking for - literally, this is *all* I'm asking for - is a > mention in the Changes file that the behaviour of $rr->rdstring > changed.

The Changes file notes changes visible to users who are presumed to be using Net::DNS documented behaviour either because the code and documentation are inconsistent, or new functionality has been added. Users relying upon undocumented behaviour, or some misinterpretation of the documentation, can not reasonably expect us to anticipate every possibility. Show quoted text

> As it is, when I narrowed the test failure down to Net::DNS > 1.07 vs Net::DNS 1.06, and looked at the Changes file of Net::DNS, I > saw a mention of three RT tickets and nothing else. I checked the > tickets and didn't see anything that matched the behaviour I was > seeing. If there had been literally *any* mention of "some internal > refactoring" or something, I'd have double-checked that against the > documentation, confirmed that $rr->rdstring wasn't guaranteed to be in > any particular format, and not bothered you with any of this.

This could be viewed another way. If instead you had read the documentation first, you might have complained about the superfluous brackets, and we would both have arrived at this point a few versions sooner.

Sun Feb 05 18:44:11 2017 rt [...] illuminated.co.uk - Correspondence added

I don't want to belabour this point, so feel free to close this ticket as a WONTFIX. I'll just want to say one last thing. Show quoted text

> > All I'm asking for - literally, this is *all* I'm asking for - is a > > mention in the Changes file that the behaviour of $rr->rdstring > > changed.

> > The Changes file notes changes visible to users who are presumed to be > using Net::DNS documented behaviour either because the code and > documentation are inconsistent, or new functionality has been added. > > Users relying upon undocumented behaviour, or some misinterpretation > of the documentation, can not reasonably expect us to anticipate every > possibility.

But $rr->rdstring *is* documented, as "Returns a string representation of the RR-specific data". In the latest version you still get a string, so nobody could reasonably argue that the change broke the public Net::DNS API; but nonetheless a change did happen, and apparently for a good reason. Why not say that? (A brief entry like "Changes to the internal format used by Net::DNS::RR->rdstring for future-proofing reasons" would be good enough, IMO.) Why insist that you didn't make the change that you did?

Tue Feb 28 17:00:01 2017 NLNETLABS [...] cpan.org - Status changed from 'open' to 'rejected'