Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly proxy truncated responses #467

Merged
merged 3 commits into from
Sep 5, 2016

Conversation

timcharper
Copy link
Contributor

@timcharper timcharper commented Aug 26, 2016

Fixes #457

@mesosphere-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@timcharper
Copy link
Contributor Author

Truncation state is being reset here:

dns/resolver/resolver.go

 459 | func truncate(m *dns.Msg, udp bool) *dns.Msg {
 460 |  max := dns.MinMsgSize
 461 |  if !udp {
 462 |      max = dns.MaxMsgSize
 463 |  } else if opt := m.IsEdns0(); opt != nil {
 464 |      max = int(opt.UDPSize())
 465 |  }
 466 | 
 467 |  m.Truncated = m.Len() > max
 468 |  if !m.Truncated {
 469 |      return m
 470 |  }
 471 | 
 472 |  m.Extra = nil // Drop all extra records first
 473 |  if m.Len() < max {
 474 |      return m
 475 |  }
 476 |  answers := m.Answer[:]
 477 |  left, right := 0, len(m.Answer)
 478 |  for {
 479 |      if left == right {
 480 |          break
 481 |      }
 482 |      mid := (left + right) / 2
 483 |      m.Answer = answers[:mid]
 484 |      if m.Len() < max {
 485 |          left = mid + 1
 486 |          continue
 487 |      }
 488 |      right = mid
 489 |  }
 490 |  return m
 491 | }
 492 | 

@timcharper timcharper force-pushed the fix-457 branch 2 times, most recently from 9feac1e to 21f08ec Compare August 26, 2016 05:26
@timcharper
Copy link
Contributor Author

I've confirmed, using tcpdump / wireshark, that the last patch causes mesos-dns to appropriately forward the truncated state.

@timcharper
Copy link
Contributor Author

Looks like the failure may be unrelated to my change

@timcharper timcharper changed the title Stop fully truncating partially truncated responses Properly proxy truncated responses Aug 26, 2016
@drewkerrigan
Copy link
Contributor

@timcharper rebasing should help - some changes to circle.yml and other fixes have been committed since you forked

@drewkerrigan drewkerrigan mentioned this pull request Aug 26, 2016
Before updating the DNS client, no answers were sent when a truncated
response was received from downstream. After updating the DNS client, it
would work properly, but it log the error and then (needlessly) continue
to attempt the other DNS servers listed.

This change causes mesos-DNS to ignore the truncated response error from
the mesos-DNS client, and send whatever response it received.

TODO - the truncated bit state should be forwarded, and not cleared.

Fixes mesosphere#457
@timcharper
Copy link
Contributor Author

@drewkerrigan thanks; I have rebased

@sargun
Copy link
Contributor

sargun commented Aug 26, 2016

Thank you for splitting commits!

@sargun
Copy link
Contributor

sargun commented Aug 26, 2016

Do you have an idea on how to simply get truncated replies out of dnsmasq?

@timcharper
Copy link
Contributor Author

@sargun the way I replicate the issue is using weave DNS. I run 14 containers or so with the same weave host name and it serves them truncated

@timcharper
Copy link
Contributor Author

(Weave DNS does not compress responses so it is easier to replicate)

@timcharper
Copy link
Contributor Author

@sargun
Copy link
Contributor

sargun commented Aug 27, 2016

@timcharper I'll recreate this issue over the coming week. Would anyone be interested if we had an integration test that propped up a dns server?

@sargun sargun merged commit 7f9fe18 into mesosphere:master Sep 5, 2016
@timcharper
Copy link
Contributor Author

🎉

@timcharper timcharper deleted the fix-457 branch September 11, 2016 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants