Fixed len computation when size just goes beyond 14 bits #668

pierresouchay · 2018-04-10T01:00:06Z

Fixes #663

Since offset of compression has to start within the first 14 bits, we don't want to take it into account before trying to compress but once the length computation has been performed

miekg · 2018-04-10T06:44:45Z

Fun times trying to make 2 diff. algorithms do the same thing: This now fails in travis:

-- FAIL: TestMsgCompressLengthLargeRecords (0.03s)
	length_test.go:172: predicted compressed length is wrong: predicted my.service.acme. (len=250) 22833, actual 22850

You PR looks sane btw.

pierresouchay · 2018-04-10T17:08:13Z

Ok, I undestand fully the problem and have a fix:

Basically, the function compressionLenHelper should take as parameter the current len of message to guess whether compression is possible or not:

When you arrive around 16384, lets say 16380, if you plan to add those entries:

let's say you already added those labels:
dom8.example.com

x1.dom9.example.com => should be compressed fully and re-usable, meaning that dom9.example.com can be re-used later
long_name.dom9.example.com => will only re-use .example.com later, not dom9.example.com because long_name overflows 16384, thus dom9.example.com won't be accessible to next entries (since it would be outside the 16384 bytes range)

I made a patch, but I am a bit lost regarding the overhead of DNS structure to compute really the overhead of labels...

You probably know the answer better than I do and how to fix my offsets :-)

I you can have a look

Regards

pierresouchay · 2018-04-10T17:18:39Z

@miekg I added a new test that does padding at begin of DNS query to ensure we properly handle all cases, it should fail for now

miekg · 2018-04-11T06:45:56Z

Ah, I see, because compressionLenHelper adds the labels that can be used for compression and this can also step over the boundary.

I wonder if all that extra complexity is worth it though, right now Len() will over estimate, so this boils down to X missing RRs (compared to Pack())

miekg · 2018-04-11T06:47:36Z

Of course in the range 16K <-> 64K this adds up. hmm

pierresouchay · 2018-04-11T06:52:41Z

@miekg yes... :(

miekg · 2018-04-11T06:57:15Z

Ok lbs gives you the index of the dots in the name, so if you have the current len in l, then
l + lbs < compressionOffset can be used for compression and not the rest.

I similar fix may or may not be needed in compressionLenHelperType.

andrewtj · 2018-04-11T06:58:45Z

On 11 Apr 2018, at 4:45 pm, Miek Gieben ***@***.***> wrote: I wonder if all that extra complexity is worth it though, right now Len() will over estimate, so this boils down to X missing RRs (compared to Pack())

This use case would probably be better handled by an API that allowed incrementally building a wire format message. Maybe something to consider for v2?

miekg · 2018-04-11T08:37:06Z

[ Quoting <notifications@github.com> in "Re: [miekg/dns] Fixed len computati..." ]

> On 11 Apr 2018, at 4:45 pm, Miek Gieben ***@***.***> wrote: > > I wonder if all that extra complexity is worth it though, right now Len() will over estimate, so this boils down to X missing RRs (compared to Pack()) This use case would probably be better handled by an API that allowed incrementally building a wire format message. Maybe something to consider for v2?

It would, but one of the reason I like the current API is that you can just do `ns.Extra = append(ns.Extra, xx)`. Having a lib that surfaces the wireformat is entirely different (and probably harder to use). Now because DNS is silly protocol it is hard to not seep through some of those underlaying details.

codecov-io · 2018-04-16T08:36:14Z

Codecov Report

Merging #668 into master will increase coverage by 0.01%.
The diff coverage is 51.94%.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
+ Coverage   58.05%   58.07%   +0.01%     
==========================================
  Files          37       37              
  Lines        9999    10072      +73     
==========================================
+ Hits         5805     5849      +44     
- Misses       3144     3174      +30     
+ Partials     1050     1049       -1

Impacted Files	Coverage Δ
msg.go	`78.19% <100%> (+0.66%)`	⬆️
zcompress.go	`25.69% <26%> (+0.69%)`	⬆️
dns.go	`62.5% <0%> (-7.5%)`	⬇️
dnssec.go	`59.1% <0%> (ø)`	⬆️
privaterr.go	`70.83% <0%> (+0.97%)`	⬆️
server.go	`61.78% <0%> (+1.78%)`	⬆️

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 01d5935...33f7d9f. Read the comment docs.

miekg · 2018-04-16T10:10:39Z

msg.go

-func compressionLenHelper(c map[string]int, s string) {
+// Put the parts of the name in the compression map, return the size added in payload
+func compressionLenHelper(c map[string]int, s string, currentLen int) int {
+	addedSize := 0


I wonder if we can optimize this more; if we have seen s here, there shouldn't be a need a spit it at all. WDYT?

Yes, probably, split might be reused... but since the review is already big (I mean for the bug we consider), I did prefer to limit as much as possible the changes

Done in next patch version

miekg · 2018-04-16T10:10:49Z

msg.go

-func compressionLenHelper(c map[string]int, s string) {
+// Put the parts of the name in the compression map, return the size added in payload
+func compressionLenHelper(c map[string]int, s string, currentLen int) int {
+	addedSize := 0


Well, my comment is probably not clear: compressionLenHelper return the size added to global DNS response (thus the addedSize name).

Considering adding "host1.example.com" with a compression map having already:
"host0.example.com" (and thus "example.com" as well as "com")

it is supposed to return 2+ len("host0") + 2 (pointer to "example.com" label

Do you have a suggestion to have clearer name?

miekg · 2018-04-16T10:12:24Z

msg.go

-			c[pref] = len(pref)
+			lenAdded := len(pref)
+			numLabelsBefore := len(lbs) - j - 1
+			offsetOfLabel := currentLen + len(s) - len(pref) + numLabelsBefore*2


don't fully understand this yet... why the *2 ?

numLabelsBefore*2 => size of label pointers (since their size is 2 bytes => 2 times)

miekg · 2018-04-16T17:13:13Z

[ Quoting <notifications@github.com> in "Re: [miekg/dns] Fixed len computati..." ]

pierresouchay commented on this pull request. > } -// Put the parts of the name in the compression map. -func compressionLenHelper(c map[string]int, s string) { +// Put the parts of the name in the compression map, return the size added in payload +func compressionLenHelper(c map[string]int, s string, currentLen int) int { + addedSize := 0 Well, my comment is probably not clear: compressionLenHelper return the size added to global DNS response (thus the addedSize name). Considering adding "host1.example.com" with a compression map having already: "host0.example.com" (and thus "example.com" as well as "com") it is supposed to return 2+ len("host0") + 2 (pointer to "example.com" label

Ok, I see where you're heading with this, you're not adding when we shouldn't but then the callers does now how much saving there where. But `addedSizeBytes += 2 + lenAdded` in a loop can't be right (I think). I need to take a step back here, because I find the logic hard to follow. There is also bunch of constants that we should probably name (2 and 6 in the current PR). Let's see: the current code works, unless, precisely on the border of the 14 bits we have name that can be compressed, but the end of the name overflows the 14 bits; i.e. we should use any labels before that to compress as well (if we haven't seen them). I.e if "len < 2<<13" and "len+x > 2<<13", we could just check for that case and split the name our selves. Or did you find more issues with the current code? /Miek

…

-- Miek Gieben

pierresouchay · 2018-04-16T17:37:59Z

@miekg Ok for the constants...

About the change, I tried several approaches (including adding another function within compressionLenSlice that did check whether currentLen + r.len() did overflow 14bits, but the code was even more unreadable.

The idea I took is that when we add compression, we also add some data in the global payload, so, all function adding compression (compressionLenHelper / compressionLenSearch should return how much data size they added themselves.

The algorithm is weird, because there are actually 2 parts:

one that does decrease the size of x (with the += (1 - k) ) Note that the decrease is weird as well (why += 1 - k ? => took me a while to understand why)
the other that increase it (sound like a more common approach since the size is in bytes)

I first tried to decompose it, but it was not easy, hence the addedSize stuff => but I agree having both ways is crap. (But method for length computation were not present - hence the +6 from nowhere)

Of course, you are the boss, so decide what you really want, but for now, among all my tries, this sounds like the easier to understand

miekg · 2018-04-16T17:48:47Z

[ Quoting <notifications@github.com> in "Re: [miekg/dns] Fixed len computati..." ]

@miekg Ok for the constants... About the change, I tried several approaches (including adding another function within compressionLenSlice that did check whether currentLen + r.len() did overflow 14bits, but the code was even more unreadable. The idea I took is that when we add compression, we also add some data in the global payload, so, all function adding compression (compressionLenHelper / compressionLenSearch should return how much data size they added themselves. The algorithm is weird, because there are actually 2 parts: * one that does decrease the size of x (with the += (1 - k) ) Note that the decrease is weird as well (why += 1 - k ? => took me a while to understand why) * the other that increase it (sound like a more common approach since the size is in bytes) I first tried to decompose it, but it was not easy, hence the addedSize stuff => but I agree having both ways is crap. (But method for length computation were not present - hence the +6 from nowhere) Of course, you are the boss, so decide what you really want, but for now, among all my tries, this sounds like the easier to understand

Thanks for your mail. yeah, while thinking about a simple change is occurred that this must also be expanded to the compressionLenTypeHelper. Let me whip up some (internal) branches to understand this code better, so I can give a better code review.

miekg · 2018-04-23T14:02:20Z

I don't think I can come up with something better - this seems to work :)

I give a quick glance over one final time and then we should be good to go.

pierresouchay · 2018-04-23T14:55:53Z

@miekg Great, I'll update Consul dependency with it as soon as a release is done to re-enable compression in it! Thank you

miekg · 2018-04-23T15:29:51Z

msg.go

 	for _, r := range rs {
 		if r == nil {
 			continue
 		}
-		// track this length, and the global length in len, while taking compression into account for both.
+		// TmpLen is to track len of record at 14bits boudaries, 6 bytes is the raw overhead
+		tmpLen := lenp + 6


I still have trouble understanding where this +6 comes from; what's it this? What bytes of the message are we skipping over here?

Don't have access to the code easily from my phone, but I would say the signaling of a new record? (I am not at all an expert in DNS, I just read RFC part related to compression for this patch)

pierresouchay · 2018-04-25T22:31:18Z

@miekg I had not the time to look further, but I think I know why we have to add 6 and not 10 or something similar.

Actually 6 is a constant regarding the start of message, erased at each iteration since we take x (we discard 6 and all added len at each iteration)

What it could mean is just that we already added some len of the DNS message (lets say 4 bytes if you expected the constant to be 10) before entering this section of the code and those bytes are actually supposed to be AFTER this computation. I'll try to have a look tomorrow

pierresouchay · 2018-04-30T10:56:04Z

@miekg I finally go this right I think

I changed the compression map used by Len() to have the same semantics (aka the offset of label) as the one used in Pack() to be able to add unit tests properly.

With this one, I change a few things, but everything is well tested now and "Fake" (aka used by Len()) has the same values as Pack() -> so I am confident compression will work the same way around 14bits

miekg · 2018-04-30T12:03:12Z

[ Quoting <notifications@github.com> in "Re: [miekg/dns] Fixed len computati..." ]

@miekg I finally go this right I think I changed the compression map used by Len() to have the same semantics (aka the offset of label) as the one used in Pack() to be able to add unit tests properly. With this one, I change a few things, but everything is well tested now and "Fake" (aka used by Len()) has the same values as Pack() -> so I am confident compression will work the same way around 14bits

Ok, thanks. I will take a look after kubecon, so next weekend probably.

pierresouchay · 2018-05-02T21:26:52Z

@miekg have a nice kubecon!

pierresouchay · 2018-05-11T09:16:19Z

@miekg does the patch is OK for you ?

Since now compression map contains the same exact information as the one used for real packing, I am quite confident the result will always be the same since I test many permutations with it in test TestCompareCompressionMapsForSRV(), we now are for sure that any change in computation is equivalent in both Pack() and Len() since offset are all equal. In order to test this, I had to create a new packBufferWithCompressionMap() function that takes into parameter the compression map so we can compare it easily both maps easily.

The patch now does not take any magic offset anymore and is tested with a very large combination of lengths and offset, so I am pretty confident it works well.

miekg

This looks very sane. Actual code changes (apart from tests) are not too massive. Some nits that i'll brush up.

Thanks for pushing this through!

Some post #668 comments nits.

pierresouchay · 2018-05-16T06:30:00Z

@miekg Thank you, I'll be able to fully re-enable compression in Consul then once a new release is created :)

Some post #668 comments nits.

miekg · 2018-05-16T08:01:07Z

[ Quoting <notifications@github.com> in "Re: [miekg/dns] Fixed len computati..." ]

@miekg Thank you, I'll be able to fully re-enable compression in Consul then once a new release is created :)

Pushed 1.0.7

Fixed len computation when size just goes beyond 14 bits

302b9c0

Added bouds checks around 14bits

6c75d82

Len() always right including when around 14bits boudaries

9452f40

miekg reviewed Apr 16, 2018

View reviewed changes

Avoid splitting into labels when not applicable

80580bd

Fixed comments

a4e1a80

pierresouchay added 2 commits April 19, 2018 00:15

Added comments in code

3a2f2f6

Added new test cases

7edff1b

miekg reviewed Apr 23, 2018

View reviewed changes

pierresouchay added 2 commits April 30, 2018 12:39

Fixed computation of Len() for SRV and all kind of records

d3b59f8

Fixed Sign that was relying on non-copy for Unit tests

51c5108

Removed unused padding

0d606ad

pierresouchay mentioned this pull request May 1, 2018

TODO: Re-enable full compression on DNS requests once https://github.com/miekg/dns/pull/668 is merged hashicorp/consul#4071

Closed

Fixed typo in PackBuffer() function

e471e7f

Added comment about packBufferWithCompressionMap() for testing purposes

33f7d9f

miekg approved these changes May 16, 2018

View reviewed changes

miekg merged commit 0964911 into miekg:master May 16, 2018

miekg added a commit that referenced this pull request May 16, 2018

Small comment tweaks

baaac9a

Some post #668 comments nits.

miekg mentioned this pull request May 16, 2018

Small comment tweaks #678

Merged

miekg added a commit that referenced this pull request May 16, 2018

Small comment tweaks (#678)

3745b97

Some post #668 comments nits.

pierresouchay mentioned this pull request May 24, 2018

Enable full dns compression hashicorp/consul#4131

Merged

This was referenced Nov 27, 2018

Properly calculate compressed message lengths #833

Merged

Pretty print test compression map differences #853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed len computation when size just goes beyond 14 bits #668

Fixed len computation when size just goes beyond 14 bits #668

pierresouchay commented Apr 10, 2018

miekg commented Apr 10, 2018

pierresouchay commented Apr 10, 2018

pierresouchay commented Apr 10, 2018

miekg commented Apr 11, 2018

miekg commented Apr 11, 2018

pierresouchay commented Apr 11, 2018

miekg commented Apr 11, 2018

andrewtj commented Apr 11, 2018 via email

miekg commented Apr 11, 2018 via email

codecov-io commented Apr 16, 2018 •

edited

Loading

miekg Apr 16, 2018

pierresouchay Apr 16, 2018

pierresouchay Apr 16, 2018

miekg Apr 16, 2018

pierresouchay Apr 16, 2018

miekg Apr 16, 2018

pierresouchay Apr 16, 2018

miekg commented Apr 16, 2018 via email

pierresouchay commented Apr 16, 2018

miekg commented Apr 16, 2018 via email

miekg commented Apr 23, 2018

pierresouchay commented Apr 23, 2018

miekg Apr 23, 2018

pierresouchay Apr 23, 2018

pierresouchay commented Apr 25, 2018

pierresouchay commented Apr 30, 2018

miekg commented Apr 30, 2018 via email

pierresouchay commented May 2, 2018

pierresouchay commented May 11, 2018

miekg left a comment

pierresouchay commented May 16, 2018

miekg commented May 16, 2018 via email

Fixed len computation when size just goes beyond 14 bits #668

Fixed len computation when size just goes beyond 14 bits #668

Conversation

pierresouchay commented Apr 10, 2018

miekg commented Apr 10, 2018

pierresouchay commented Apr 10, 2018

pierresouchay commented Apr 10, 2018

miekg commented Apr 11, 2018

miekg commented Apr 11, 2018

pierresouchay commented Apr 11, 2018

miekg commented Apr 11, 2018

andrewtj commented Apr 11, 2018 via email

miekg commented Apr 11, 2018 via email

codecov-io commented Apr 16, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miekg commented Apr 16, 2018 via email

pierresouchay commented Apr 16, 2018

miekg commented Apr 16, 2018 via email

miekg commented Apr 23, 2018

pierresouchay commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierresouchay commented Apr 25, 2018

pierresouchay commented Apr 30, 2018

miekg commented Apr 30, 2018 via email

pierresouchay commented May 2, 2018

pierresouchay commented May 11, 2018

miekg left a comment

Choose a reason for hiding this comment

pierresouchay commented May 16, 2018

miekg commented May 16, 2018 via email

codecov-io commented Apr 16, 2018 •

edited

Loading