Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#7408 release blocker: regression closed fixed (fixed)

twistd dns --secondary has been completely broken since twisted 13.2.0

Reported by: Glyph Owned by: Jean-Paul Calderone
Priority: highest Milestone:
Component: names Keywords:
Cc: hawkowl Branch: branches/secondary-dns-lookup-error-7408
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

twistd -n dns --secondary 66.35.39.66/twistedmatrix.com --port 53 on every version of Twisted since Twisted 13.2.0 has resulted in simply this traceback in a loop:

2014-05-24 06:36:05+0000 [DNSDatagramProtocol (UDP)] Unhandled Error
	Traceback (most recent call last):
	  File "/srv/names/.virtualenvs/names/site-packages/twisted/internet/defer.py", line 139, in maybeDeferred
	    result = f(*args, **kw)
	  File "/srv/names/.virtualenvs/names/site-packages/twisted/names/common.py", line 81, in lookupAddress
	    return self._lookup(name, dns.IN, dns.A, timeout)
	  File "/srv/names/.virtualenvs/names/site-packages/twisted/names/resolve.py", line 79, in _lookup
	    d = self.resolvers[0].query(q, timeout)
	  File "/srv/names/.virtualenvs/names/site-packages/twisted/names/common.py", line 73, in query
	    return defer.maybeDeferred(method, query.name.name, timeout)
	--- <exception caught here> ---
	  File "/srv/names/.virtualenvs/names/site-packages/twisted/internet/defer.py", line 139, in maybeDeferred
	    result = f(*args, **kw)
	  File "/srv/names/.virtualenvs/names/site-packages/twisted/names/common.py", line 81, in lookupAddress
	    return self._lookup(name, dns.IN, dns.A, timeout)
	  File "/srv/names/.virtualenvs/names/site-packages/twisted/names/secondary.py", line 151, in _lookup
	    return FileAuthority.__dict__['_lookup'](self, name, cls, type, timeout)
	  File "/srv/names/.virtualenvs/names/site-packages/twisted/names/authority.py", line 181, in _lookup
	    additionalInformation = self._additionalRecords(
	exceptions.AttributeError: SecondaryAuthority instance has no attribute '_additionalRecords'

The server runs and listens on a port but no names are resolvable.

I've rolled back Twisted's own secondary DNS to 13.1.0 so that it's functioning again, but it would be great to fix this ASAP.

Change History (11)

comment:1 Changed 5 years ago by Glyph

Component: corenames

comment:2 Changed 5 years ago by Richard Wall

Status: newassigned

A few notes:

  • I think this bug was introduced in r39893 which was when SecondaryAuthority._lookup started calling out to FileAuthority._additionalRecords.
  • It wasn't detected because SecondaryAuthority has very few tests. See #6454.
  • The basic problem seems to be that SecondaryAuthority doesn't subclass FileAuthority as the comment says here source:trunk/twisted/names/secondary.py#L151
  • ..but that's because it can't. FileAuthority is too tightly coupled to the loading of records from a file. We need an in-memory authority described in #4686. Such a thing has been hacked together in source:trunk/twisted/names/test/test_names.py#L28
  • But even then, SecondaryAuthority shouldn't subclass but instead should *have* a MemoryAuthority attribute and call its _lookup method. Although it would be better if we could find a way to call a public API instead.

So perhaps the proper way forward is:

  • #4686 Implement a MemoryAuthority
  • #6454 Add tests for the current SecondaryAuthority
  • #7408 Finally refactor SecondaryAuthority to use MemoryAuthority.

comment:3 Changed 5 years ago by Glyph

That seems like a reasonable plan, except won't the tests in #6454 necessarily be failing without the fix in #7408? It seems like those may need to land together.

comment:4 Changed 5 years ago by hawkowl

Cc: hawkowl added
Milestone: Twisted 14.1.0

Marking this as a 14.1 blocker, since I think it'd be nice to get a release out which didn't break Twisted's own infra :)

ref: #7335

comment:5 Changed 5 years ago by Jean-Paul Calderone

Owner: changed from Richard Wall to Jean-Paul Calderone
Status: assignednew

comment:6 Changed 5 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/secondary-dns-lookup-error-7408

(In [43044]) Branching to 'secondary-dns-lookup-error-7408'

comment:7 Changed 5 years ago by Jean-Paul Calderone

Keywords: review added

So perhaps the proper way forward is: #4686 Implement a MemoryAuthority #6454 Add tests for the current SecondaryAuthority #7408 Finally refactor SecondaryAuthority to use MemoryAuthority.

I agree those steps make sense. However since the release manager marked this as a blocker for 14.1 I went ahead and implemented a more limited fix. I made SecondaryAuthority just subclass FileAuthority.

comment:8 in reply to:  7 Changed 5 years ago by Glyph

Replying to exarkun:

So perhaps the proper way forward is: #4686 Implement a MemoryAuthority #6454 Add tests for the current SecondaryAuthority #7408 Finally refactor SecondaryAuthority to use MemoryAuthority.

I agree those steps make sense. However since the release manager marked this as a blocker for 14.1 I went ahead and implemented a more limited fix. I made SecondaryAuthority just subclass FileAuthority.

Those tickets can feel free to stay open, in the meanwhile. Thanks for tackling this.

  1. I really don't understand what twistedchecker's deal is, but it looks like one of the "new" errors it produced is actually valid; the one about blank lines.
  2. As far as I can tell, this subclassing actually doesn't inadvertently expose any new public API attributes. Great.

I guess I don't have much more to add; I manually tested and it seems to address the issue - land away!

comment:9 Changed 5 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [43058]) Merge secondary-dns-lookup-error-7408

Author: exarkun (sponsored by Hybrid Logic, Ltd.) Reviewer: glyph Fixes: #7408

Fix a regression in SecondaryAuthority which prevented it from working in most basic uses.

comment:10 Changed 4 years ago by hawkowl

Milestone: Twisted 14.1.0

Ticket retargeted after milestone deleted

comment:11 Changed 3 years ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.