Ticket #6169 defect closed fixed

Opened 18 months ago

Last modified 3 months ago

replace twisted.pair.tuntap dependency on Eunuchs (and make it work again)

Reported by: glyph Owned by: exarkun
Priority: normal Milestone:
Component: pair Keywords:
Cc: Branch: branches/tuntap-pytun-6169-5
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

We release Twisted Pair with each new release of Twisted, but it doesn't really function at all. The  Eunuchs package which it depends upon for tuntap functionality,  effectively does not exist any more, and has been abandoned for many years.

There is an MIT licensed alternative,  python-pytun, which we could use instead (and maybe we could have, like, some unit tests, or something).

Change History

1

  Changed 18 months ago by glyph

See also #1813.

2

  Changed 18 months ago by exarkun

  • branch set to branches/tuntap-pytun-6169
  • branch_author set to exarkun

(In [36341]) Branching to 'tuntap-pytun-6169'

3

  Changed 17 months ago by exarkun

  • owner set to exarkun

4

  Changed 17 months ago by exarkun

  • branch changed from branches/tuntap-pytun-6169 to branches/tuntap-pytun-6169-2

(In [36507]) Branching to 'tuntap-pytun-6169-2'

5

  Changed 11 months ago by exarkun

  • branch changed from branches/tuntap-pytun-6169-2 to branches/tuntap-pytun-6169-3

(In [38621]) Branching to 'tuntap-pytun-6169-3'

6

  Changed 6 months ago by exarkun

  • branch changed from branches/tuntap-pytun-6169-3 to branches/tuntap-pytun-6169-4

(In [40407]) Branching to 'tuntap-pytun-6169-4'

7

  Changed 5 months ago by exarkun

  • branch changed from branches/tuntap-pytun-6169-4 to branches/tuntap-pytun-6169-5

(In [40936]) Branching to 'tuntap-pytun-6169-5'

8

  Changed 5 months ago by exarkun

  • owner exarkun deleted
  • keywords review added

Well I did some things. I hope they're great.

9

  Changed 5 months ago by tom.prince

Some initial thoughts:

  1. react!
  2. Please open a ticket for ARP support.
  3. Please file a ticket to allow the addresses used by the tests to be configured.
  4. Perhaps ReactorFDSet should be FakeReactorFDSet?
  5. File a ticket for making testing._ip/testing._udp3 and polishing them. Or alternatively, using the protocol implementation instead.
  6. TunHelper.parser should have a docstring.
  7. IInputOutputSystem should be private. It doesn't seem like a well-thought out abstraction, so shouldn't be fixed in stone too early.
  8. _RealSystem doesn't implement sendUDP/receiveUDP. From in-person discussion, it seems like the only exist in the test implementation, so shouldn't be part of the interface.

10

  Changed 4 months ago by glyph

A couple of new pydoctor errors:

twisted.pair.testing.MemoryIOSystem.ioctl: invalid ref to fcntl.ioctl
twisted.pair.tuntap.IInputOutputSystem.ioctl: invalid ref to fcntl.ioctl

11

  Changed 4 months ago by glyph

Also  new pyflakes errors:

twisted/pair/test/test_tuntap.py:23: redefinition of unused '_ioctl' from line 20
twisted/pair/test/test_tuntap.py:54: redefinition of unused '_RealSystem' from line 48

12

  Changed 4 months ago by glyph

  • owner set to exarkun
  • keywords review removed

This branch is pretty epic. Thank you for making this somewhat major feature actually work again. I especially enjoyed the documentation.

However, it looks like it's not really fully baked yet. You'll forgive me for not calling these all out individually, but it would be pretty repetitive:

  1. There are several XXX comments throughout the code which should be transformed into links to bugs or removed.
  2. Several methods and test methods are missing docstrings (test_receive, test_send, RealDeviceTestsMixin.system has a comment instead of a docstring, _H, _udp, _ip, _ethernet, etc.; MemoryIOSystem's docstring is only halfway done)
  3. What are those magical four bytes? At least make the "4" into a constant of some kind, even if it's just a guess at this point. Similarly, header = "\x00\x00\x00\x00" should probably explain itself a bit better.
  4. "privileged" should be private, I think, since it's only an implementation helper for MemoryIOSystem?
  5. What's with the nested looping in test_receive? Loops in tests are bad, and for i in range(100): for j in range(100): seems like it would be doubly or triply bad.
  6. TunnelTestsMixin.setUp should not need to access a private _devices attribute, since that class exists pretty much only to serve those tests.

Tom's feedback also seems good, although I'd go with Memory rather than Fake.

One idea that I've been playing with lately that might be a good strategy would be to separate methods like getTunnel, which appears to be a feature of the fixture, from methods like sendUDP and receiveUDP which are actually part of the "real" implementation as well. Right now there's nothing to stop the system under test from calling getTunnel.

I don't know enough about tuntap to fill in the gaps here, so I really want to see the finished versions of many of these docstrings before giving a passing review, but aside from perhaps some naming issues and some making things less public, I'm favorably inclined towards most of this branch right now.

13

  Changed 4 months ago by exarkun

Tom's review:

1. react!

r40980

2. Please open a ticket for ARP support.

#6875

3. Please file a ticket to allow the addresses used by the tests to be configured.

Hmm. But trial has no mechanism for supplying configuration to tests. That is probably a pre-requisite for actually supplying configuration to any particular tests.

4. Perhaps ReactorFDSet should be FakeReactorFDSet?

Could be, maybe. Though it is a pretty complete, realistic implementation. I'd be more inclined to turn it into the canonical implementation and make real reactors share it somehow. Since it's in a test module, perhaps this can wait until later though?

5. File a ticket for making testing._ip/testing._udp3 and polishing them. Or alternatively, using the protocol implementation instead.

#6876, #6877

6. TunHelper.parser should have a docstring.

r40981

7. IInputOutputSystem should be private. It doesn't seem like a well-thought out abstraction, so shouldn't be fixed in stone too early.

r40982

8. _RealSystem doesn't implement sendUDP/receiveUDP. From in-person discussion, it seems like the only exist in the test implementation, so shouldn't be part of the interface.

Hmm. I don't remember exactly what I said at that in-person discussion (but I do remember I was sleep deprived at the time :). These methods are only *used* by the tests but there are both in-memory and "real" implementations of them. The observation that _RealSystem is not a complete implementation of _IInputOutputSystem is a good one. I suppose what might make sense is to split _IInputOutputSystem into two pieces, one with sendUDP/receiveUDP and one with the rest. The tests could extend (not necessarily by subclassing) _RealSystem to add these two methods. On the other hand, all these things are private. I wonder if it is worth it?

Glyph's review:

Several methods and test methods are missing docstrings (test_receive, test_send, RealDeviceTestsMixin.system has a comment instead of a docstring, _H, _udp, _ip, _ethernet, etc.; MemoryIOSystem's docstring is only halfway done)

r40984

What are those magical four bytes? At least make the "4" into a constant of some kind, even if it's just a guess at this point. Similarly, header = "\x00\x00\x00\x00" should probably explain itself a bit better.

r40988, r40989

"privileged" should be private, I think, since it's only an implementation helper for MemoryIOSystem?

r40985

What's with the nested looping in test_receive? Loops in tests are bad, and for i in range(100): for j in range(100): seems like it would be doubly or triply bad.

Pretty awful, huh? Unfortunately this test is trying to pass UDP traffic - ie, a system without delivery guarantees. Trying a lot of times is the most obvious way to me to avoid making the test *incredibly* unreliable. Unfortunately it's also the only solution I can really see... Suggestions welcome. Some totally ad hoc testing suggests that without these loops the test fails (EAGAIN) about 1 time in 100. With these loops I have yet to see it fail (>30000 iterations on my laptop). r40986 has a couple comments explaining the loops to future maintainers, at least.

TunnelTestsMixin.setUp should not need to access a private _devices attribute, since that class exists pretty much only to serve those tests.

r40987

There are several XXX comments throughout the code which should be transformed into links to bugs or removed.

These I have yet to address.

14

  Changed 4 months ago by exarkun

There are several XXX comments throughout the code which should be transformed into links to bugs or removed.

Partially addressed in r40990.

twisted/pair/test/test_tuntap.py:23: redefinition of unused '_ioctl' from line 20

A pyflakes bug but apparently one I don't *need* to trip over in this code anymore. r40991.

twisted/pair/test/test_tuntap.py:54: redefinition of unused '_RealSystem' from line 48

Well.. I just discovered that pyflakes doesn't complaint if you do this:

x = y
try:
    import z as x
except ImportError:
    pass

which seems like a bug to me (I don't know why the above shouldn't be an error if the more obvious idiom *is* - and I don't know why these don't share a code path). But... oh well? Guess I'll take advantage of it? r40992

A couple of new pydoctor errors: {{{ twisted.pair.testing.MemoryIOSystem.ioctl: invalid ref to fcntl.ioctl twisted.pair.tuntap.IInputOutputSystem.ioctl: invalid ref to fcntl.ioctl }}}

Indeed. fcntl.ioctl certainly exists, though. Do we want to avoid making links to them because pydoctor doesn't like them? Or perhaps I should file a bug report against pydoctor to cause it to like them?

Just one XXX left to address, I think.

15

  Changed 4 months ago by exarkun

  • keywords review added
  • owner exarkun deleted

The parser XXX is fixed by r40993. The PI XXX is fixed by r40994.

16

in reply to: ↑ description   Changed 4 months ago by thijs

Replying to glyph:

There is an MIT licensed alternative,  python-pytun, which we could use instead (and maybe we could have, like, some unit tests, or something).

I was wondering why this alternative wasn't chosen?

17

  Changed 4 months ago by exarkun

I was wondering why this alternative wasn't chosen?

The tuntap library would only replace one or two lines of implementation at most (it's actually very little work to initialize a tun/tap device - open a file, do an ioctl). In exchange, it would make it more difficult to unit test the code. The trade-off didn't seem worth it to me.

18

  Changed 4 months ago by rwall

  • owner set to rwall
  • status changed from new to assigned

19

  Changed 4 months ago by rwall

  • status changed from assigned to new
  • owner rwall deleted

Thanks exarkun,

This branch is fascinating and looks extremely useful. I wanted to do a really good review for you (I owe you), but I've run out of steam...sorry. Maybe I can do some more tomorrow, unless someone else steps in in the meantime.

Here's my partial review with some (mostly cosmetic) points.

  1. Broken API documentation links
    1.  https://buildbot.twistedmatrix.com/builders/documentation/builds/4215/steps/api-documentation/logs/new%20pydoctor%20errors
  2. Twisted Checker warnings
    1.  https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1586/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
  3. source:branches/tuntap-pytun-6169-5/twisted/pair/tuntap.py
    1. TunnelFlags
      1. Maybe add a link to some online documentation of these flags.
    2. TunnelDescription
      1. Maybe just add @type name: L{bytes} instead of "A L{bytes} instance"
    3. _IInputOutputSystem
      1. Not sure whether pydoctor will resolve links to module and class variables -- only modules, functions, classes and methods I think.
      2. There are other examples of this:  https://buildbot.twistedmatrix.com/builders/documentation/builds/4215/steps/api-documentation/logs/new%20pydoctor%20errors
    4. _RealSystem
      1. Implements IInputOutputSystem but doesn't provide sendUDP or receiveUDP
      2. I see there was some discussion about this in previous comments, but I can't really think of anything to add to the discussion.
    5. TuntapPort
      1. "assert raw.IRawPacketProtocol.providedBy(proto)" looks like it should be replaced with a more specific exception....although I see there was previously an assertion there marked FIXME. Not sure what that means.
      2. doRead
        1. This looks unfinished eg
          1. "# TODO pkt.isPartial()? "
          2. " log.err(None, "monkeys") "
      3. write, writeSequence, stopListening, connectionLost + others
        1. Missing @param, @return, @raises epydocs
        2.  https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1586/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
      4. loseConnection
        1. Typo "This is Use"
  4. source:branches/tuntap-pytun-6169-5/doc/pair/examples/pairudp.py
    1. I set up a tuntap interface as described and ran the example. It worked!
    2. It's not clear how this ties in with the narrative documentation. Perhaps you can add a link to it.
  5. source:branches/tuntap-pytun-6169-5/doc/pair/howto/tunnels.xhtml
    1. Incomplete sentence "Twisted Pair supports the special <em>tun</em> and <em>tap</em>." (devices? maybe)
    2. "familiarize yourself with Linux tuntap" maybe add a link to a good online reference.
    3. Add a note to say that the example code must be run as root (I think it does)
    4. The example doesn't have a "reactor.run" or a "react" - so does it actually do anything. I didn't try as root.
    5. Does this help document need to be linked from somewhere?
  6. source:branches/tuntap-pytun-6169-5/doc/pair/howto/configuration.xhtml
    1. Does this help document need to be linked from somewhere?
    2. The setup script worked.
    3. But then the tests hang on RealDeviceWithProtocolInformationTests.test_send
    4. (On Fedora 19 x86_64)
    5. And RealDeviceWithProtocolInformationTests.test_receive seems to fail every time.
             [FAIL]
             Traceback (most recent call last):
               File "/home/richard/projects/Twisted/branches/tuntap-pytun-6169-5/twisted/pair/test/test_tuntap.py", line 438, in test_receive
                 self.fail("Never saw probe UDP packet on tunnel")
             twisted.trial.unittest.FailTest: Never saw probe UDP packet on tunnel
      
             twisted.pair.test.test_tuntap.RealDeviceWithoutProtocolInformationTests.test_receive
      

20

  Changed 4 months ago by exarkun

Thanks rwall!

Broken API documentation links

Several of these fixed in r41117. I hope that updating buildbot to a newer version of pydoctor will fix the remaining two errors.

Twisted Checker warnings

Many fixed in r41118. Some others are spurious and I'm not going to fix them.

TunnelFlags - Maybe add a link to some online documentation of these flags.

r41120

TunnelDescription - Maybe just add @type name: L{bytes} instead of "A L{bytes} instance"

r41119

_IInputOutputSystem - Not sure whether pydoctor will resolve links to module and class variables -- only modules, functions, classes and methods I think

I added @cvar to TunnelType (r41117) and the pydoctor errors seem to have gone away

_RealSystem - Implements IInputOutputSystem but doesn't provide sendUDP or receiveUDP. I see there was some discussion about this in previous comments, but I can't really think of anything to add to the discussion.

Indeed. I guess I'll just move the declaration into the test module. r41121.

TuntapPort - "assert raw.IRawPacketProtocol.providedBy(proto)" looks like it should be replaced with a more specific exception....although I see there was previously an assertion there marked FIXME. Not sure what that means

Yea, definitely not great. This is pre-existing behavior that I don't want to touch. #6890.

doRead This looks unfinished eg "# TODO pkt.isPartial()? "

Indeed. More pre-existing stuff. I don't think pair ever implemented anything special for truncated datagrams/frames. #6891.

log.err(None, "monkeys")

Oops, forgot about that one. :) r41122.

write, writeSequence, stopListening, connectionLost + others

r41123.

loseConnection - Typo "This is Use"

r41124.

I set up a tuntap interface as described and ran the example. It worked!

Yaay. :) Quite far-fetched, really...

It's not clear how this ties in with the narrative documentation. Perhaps you can add a link to it.

r41125.

Incomplete sentence "Twisted Pair supports the special <em>tun</em> and <em>tap</em>." (devices? maybe)

r41126.

"familiarize yourself with Linux tuntap" maybe add a link to a good online reference.

Pssh I wish there were one. :/ I added the google search results for "linux tuntap tutorial" in r41126. Not sure if that's really a great idea...

Add a note to say that the example code must be run as root (I think it does) The example doesn't have a "reactor.run" or a "react" - so does it actually do anything. I didn't try as root.

Indeed, it doesn't generate much interesting output (nor even if you start the reactor). r41127.

Does this help document need to be linked from somewhere?

Oh yea. Verily. r41128.

configuration.xhtml - Does this help document need to be linked from somewhere?

r41129.

The setup script worked. But then the tests hang on RealDeviceWithProtocolInformationTests.test_send (On Fedora 19 x86_64) And RealDeviceWithProtocolInformationTests.test_receive seems to fail every time.

Oh boy. Well, that'll be some super fun debugging... probably not for tonight though.

Thanks again for a great review!

21

  Changed 4 months ago by exarkun

(In [41131]) slight cleanup of that test refactoring

refs #6169

22

  Changed 4 months ago by exarkun

(In [41132]) a few extra twistedchecker-reported errors

refs #6169

23

  Changed 4 months ago by rwall

  • keywords review removed
  • owner set to exarkun

Hi exarkun,

Ok, The changes since the last review all look correct and I think I've reviewed all the remaining new and changed files now.

Not sure I've understood it all, but I certainly understand more about the various networking layers having read this branch.

Here are my notes:

  1. source:branches/tuntap-pytun-6169-5/twisted/pair/test/test_tuntap.py
    1. Tests using real tuntap devices were still getting stuck.
      1. It was a firewall problem. Inserting this rule allows the test to pass. "sudo iptables -I INPUT -p udp --dport 12345 -j ACCEPT"
      2. Maybe add a note about that to the tests, or to configuration.xhtml
      3. Maybe add some sort of timeout to the tests to prevent them hanging indefinitely when the firewall blocks the test messages.
    2. Looks like the test module has been written with Python3 in mind
      1. So I tried with Python3, but it failed :(
                  File "/home/richard/projects/Twisted/branches/tuntap-pytun-6169-5/twisted/python/reflect.py", line 281
                    print 'unknown type', type(start), start
        
      2. I guess that's for the future
    3. There are a few multi-assertion tests...which I thought you disliked :) Maybe break those up where possible.
  1. source:branches/tuntap-pytun-6169-5/twisted/pair/testing.py
    1. There are some uncovered branches even when running the real tests with a real device. Maybe these should be covered.
  1. source:branches/tuntap-pytun-6169-5/twisted/pair/tuntap.py
    1. Missing coverage of L:226 even when running the real tuntap. Maybe this should be covered.
    2. receiveUDP
      1. I don't quite see the point of the fileno argument
      2. It seems to be ignored by the _RealSystem implementation
      3. So only used to satisfy the _FakePort?
      4. Maybe I'm misunderstanding something.

If you're satisfied with it, I'm satisfied with it. So please merge after answering or addressing the numbered points above and fixing any remaining twistedchecker warnings that you think are valid.

Thanks again.

24

  Changed 3 months ago by exarkun

(In [41191]) Split up test methods which make multiple assertions into multiple tests

Or collapse multiple assertions into one where it is feasible.

This involves adding an equality implementation to TunnelType which in turn suggested a nice __repr__ would probably be a good addition.

It also revealed a minor bug in closeOnExec which previously returned 4 instead of True.

refs #6169

25

  Changed 3 months ago by exarkun

(In [41192]) Replace assertIdentical with assertIs

refs #6169

26

  Changed 3 months ago by exarkun

(In [41195]) Test the error behavior of the _IInputOutputSystem implementations

refs #6169

27

  Changed 3 months ago by exarkun

It was a firewall problem. Inserting this rule allows the test to pass. "sudo iptables -I INPUT -p udp --dport 12345 -j ACCEPT"

I wonder if this rule was required to override some other rule on the system? Otherwise it seems like it should be the default... Why wouldn't a system accept UDP traffic on some arbitrary unprivileged port?

Looks like the test module has been written with Python3 in mind

Definitely in mind... but I didn't do any real testing on Python 3, so not too surprising it doesn't actually work.

There are a few multi-assertion tests...which I thought you disliked :) Maybe break those up where possible.

Quite so! Thanks for pointing that out. Some of these tests are probably a couple years old at this point and I've gotten to the point where I'm a bit too tired to apply all the rules to myself. :)

Fixed in r41191. In doing this I also noticed use of the to-be-deprecated assertIdentical and fixed that in r41192.

There are some uncovered branches even when running the real tests with a real device. Maybe these should be covered.

Oops. Yea. Thanks. That revealed an important issue with one of the testing helpers.

Missing coverage of L:226 even when running the real tuntap. Maybe this should be covered.

Covered in r41193.

The interesting uncovered branches in testing.py are covered in r41194 (which includes the bug fix - a double PI header being added in certain cases).

A couple other misc testing.py, boring branches in testing.py are covered by r41195 and r41196.

I think this brings coverage to 100% modulo a bug in coverage.py that causes some non-executable lines to be marked as unexecuted.

receiveUDP

Yea... It's not the best abstraction ever. For the time being I think I'm satisfied with it, though. If someone wants to invent a better minimal abstraction over UDP/IP I think they'll easily be able to replace receiveUDP with it. Meanwhile receiveUDP will get the job done. :)

28

  Changed 3 months ago by exarkun

(In [41197]) Change the network range used by the tests from the larger, somewhat more common 10/24 to the smaller, slightly more obscure 172.16/12.

Hopefully this makes the test suite runnable more easily on more hosts (including some of our buildslaves).

refs #6169

29

  Changed 3 months ago by tom.prince

twisted.pair.test.test_tuntap.RealDeviceWithProtocolInformationTests.test_send seems to hang if the packet is swallowed by a firewall.

The following makes the test pass

iptables -I INPUT --dest 172.16.1.1 -j ACCEPT
iptables -I INPUT --dest 172.16.2.1 -j ACCEPT

but the test should timeout, or something.

30

  Changed 3 months ago by rwall

Also just noticed there's a real test failure on Windows.

[ERROR]
Traceback (most recent call last):
  File "C:\Users\Build Slave\bot-glyph-6\windows7-64-py2.7-select\Twisted\twisted\trial\runner.py", line 498, in loadPackage
    module = modinfo.load()
  File "C:\Users\Build Slave\bot-glyph-6\windows7-64-py2.7-select\Twisted\twisted\python\modules.py", line 383, in load
    return self.pathEntry.pythonPath.moduleLoader(self.name)
  File "C:\Users\Build Slave\bot-glyph-6\windows7-64-py2.7-select\Twisted\twisted\python\_reflectpy3.py", line 266, in namedAny
    topLevelPackage = _importAndCheckStack(trialname)
  File "C:\Users\Build Slave\bot-glyph-6\windows7-64-py2.7-select\Twisted\twisted\python\_reflectpy3.py", line 205, in _importAndCheckStack
    return __import__(importName)
  File "C:\Users\Build Slave\bot-glyph-6\windows7-64-py2.7-select\Twisted\twisted\pair\test\test_tuntap.py", line 554, in <module>
    @implementer(_IInputOutputSystem)
exceptions.NameError: name '_IInputOutputSystem' is not defined

twisted.pair.test.test_tuntap

31

  Changed 3 months ago by exarkun

(In [41198]) Help out other platforms by not having any hard dependencies on classes we might not be able to import.

refs #6169

32

  Changed 3 months ago by exarkun

(In [41199]) Set a timeout on the blocking socket receive operation so the test run does eventually complete even if something is terribly broken.

refs #6169

33

  Changed 3 months ago by exarkun

(In [41200]) mention the iptables rules for anyone who might get bitten by this

refs #6169

34

  Changed 3 months ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

(In [41202]) Merge tuntap-pytun-6169-5

Author: exarkun, therve Reviewer: tom.prince, glyph, rwall Fixes: #6169

Bring Twisted Pair's TUN/TAP support back to life by dropping the "Eunuchs" library dependency and adding a comprehensive test suite and some basic howto documentation.

Note: See TracTickets for help on using tickets.