Allow custom chunksizes in dns.rdata.to_text

[peterthomassen]

This implements #612 not only for (C)DNSKEY (as discussed there), but also for all other occurrences of _base64ify and _hexify inside any of the dns.rdata.to_text() methods.

I added a comprehensive test for the DNSKEY case, but did not explicitly add tests for other record types. If more tests should be added, I'd require some guidance how to avoid the bloat. (I noticed that you have examples in e.g. tests/example1.good, but as the new tests apply various chunk sizes, I wasn't sure if writing out all variations into a static file would be the best approach.)

Fixes #612.

[codecov-io]

Codecov Report

Merging #617 (731d418) into master (692e915) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
+ Coverage   97.81%   97.89%   +0.07%     
==========================================
  Files         121      121              
  Lines       10081    10083       +2     
==========================================
+ Hits         9861     9871      +10     
+ Misses        220      212       -8     

Impacted Files	Coverage Δ
dns/rdtypes/ANY/CERT.py	100.00% <ø> (ø)
dns/rdtypes/ANY/RRSIG.py	100.00% <ø> (ø)
dns/rdtypes/IN/IPSECKEY.py	100.00% <ø> (ø)
dns/rdtypes/dnskeybase.py	100.00% <ø> (ø)
dns/rdata.py	100.00% <100.00%> (ø)
dns/rdtypes/ANY/OPENPGPKEY.py	100.00% <100.00%> (ø)
dns/rdtypes/ANY/SSHFP.py	100.00% <100.00%> (ø)
dns/rdtypes/IN/DHCID.py	100.00% <100.00%> (ø)
dns/rdtypes/dsbase.py	100.00% <100.00%> (ø)
dns/rdtypes/euibase.py	100.00% <100.00%> (ø)
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 692e915...731d418. Read the comment docs.

[rthalley]

This doesn't quite work, as all keywords passed to rdata's to_text are passed to _hexify or _base64ify, so if you passed a parameter 'foo' as well as 'chunksize' you'd get:

TypeError: _hexify() got an unexpected keyword argument 'foo'

Probably the easiest thing is to make _hexify and _base64ify have a **kw parameter to soak up any unknown rdata keywords that are not relevant to them.

[peterthomassen]

I added a fixup commit. Once the PR has settled on the way to go, I'll squash it.

Edit: done.

[rthalley]

Almost ready to merge this, but after having a second set of eyes review we noticed that the changes in a927fa3 are doing

_hexify(..., chunksize=kw.get('chunksize', 128))

Instead of just passing **kw like the invocations of _hexify() and _base64ify() in 18feb95.

Fix that and squash it and we're good to merge.

/Bob

[peterthomassen]

Done.

The calls you pointed out previously had hardcoded chunksize=128 arguments. I had turned those into chunksize=kw.get('chunksize', 128) as to avoid a got multiple values for argument 'chunksize' error in case kw contains chunksize.

The new approach is to pop chunksize from kw if present, otherwise fall back to the default. **kw is then passed with chunksize removed, so there can be no collision.

I also added **kw to OPENPGPKEY.to_text, but without the collision logic. This is because I saw in the history that chunking had been turned off deliberately in the past, so my impression is that passing another value is an error. The same holds true for EUIBase.to_text, where the RFC prescribes the format explicitly.

There are 4 calls of dns.rdata._base64ify left in TKEY.py and TSIG.py that do not pass **kw, as I believe that the format is also explicitly specified and should not be modifiable. Please let me know, Bob, if you prefer a change here.

[bwelling]

I believe that this change introduces a new problem, as it's modifying kwargs. With code like:

kwargs = {"chunksize" : 100}
print(r1.to_text(**kwargs))
print(r2.to_text(**kwargs))

the first record would be converted with chunksize=100, and the second one would not, as chunksize has been removed from kwargs. This would be unexpected, to say the least.

As far as I can tell, there are 3 separate cases - types that have a required chunk size, types that use the default, and types that override the default, but should still be configurable. In the first two, a user-provided value should take precedence, and in the last one, a user-provided value should be ignored.

One solution to this would be to have (potentially) 4 different chunk size parameters that _hexifyand _base64ify could deal with - the builtin default, a type-specific default, and a type-specific requirement, and a user-provided value. This is pretty complicated, though.

I'm wondering if this shouldn't be punted to post-2.1.0, to allow more time to come up with a better solution.

[peterthomassen]

I now added kw = kw.copy() before calling pop to avoid that problem.

This PR, which is a simple extension of the current behavior, does not introduce any incompatibility or forestall any future changes regarding the treatment of default chunksize values. However, the current chunking behavior, e.g. of TLSA records, is a real problem in some situations (for details, see #612). It would help a great deal if the parameter could be configured already in the next release.

My impression is that the benefit from the usability improvement in itself justifies making the change.

[rthalley]

Merged, thanks!