frenetic-lang / pyretic

The Pyretic language and runtime system
http://frenetic-lang.org/pyretic/
159 stars 99 forks source link

Issue 1 - no 'dstport=80' match entry in flows table #27

Closed danieledc closed 10 years ago

danieledc commented 10 years ago

Hello everyone, I am Daniele De Cicco, master student of Computer Engineering at University of Naples (Italy). Since November I have been working on my master thesis at the Catholic University of Louvain (Belgium). I am developing my project with Pyretic and recently I have run into three different anomalies:

If I try to run the following function

def action():
    q=count_packets(interval=10,group_by=['srcip','dstip']) 
    q.register_callback(count_packets_printer)
    return (match(dstport=80,switch=2) >> q)

no entry that matches on dst port 80 is installed in the flow table at switch 2. I tested it with the mininet command "dpctl dump-flows". This is what the command returns before installing the rule

*** s2 ------------------------------------------------------------------------
NXST_FLOW reply (xid=0x4):
cookie=0x0, duration=8.247s, table=0, n_packets=11, n_bytes=790, priority=59999,vlan_tci=0x0000 actions=drop
cookie=0x0, duration=8.247s, table=0, n_packets=0, n_bytes=0, priority=60000,vlan_tci=0x0000,dl_type=0x88cc actions=CONTROLLER:65535
cookie=0x0, duration=8.247s, table=0, n_packets=0, n_bytes=0, priority=0 actions=CONTROLLER:65535

and this is what it returns after

*** s2 ------------------------------------------------------------------------
NXST_FLOW reply (xid=0x4):
cookie=0x0, duration=2.016s, table=0, n_packets=0, n_bytes=0, priority=59999,vlan_tci=0x0000 actions=CONTROLLER:65535
cookie=0x0, duration=2.016s, table=0, n_packets=0, n_bytes=0, priority=59998,vlan_tci=0x0000 actions=drop
cookie=0x0, duration=2.016s, table=0, n_packets=0, n_bytes=0, priority=60000,vlan_tci=0x0000,dl_type=0x88cc actions=CONTROLLER:65535
cookie=0x0, duration=2.016s, table=0, n_packets=0, n_bytes=0, priority=0 actions=CONTROLLER:65535

It looks very strange to me, mostly because if I use a different field to match, for instance the destination mac address, I find the corresponding match in the entry in the flow table of switch 2. As a result, it seems that the function does not actually filter on the port 80 but it just forwards every incoming packet.

I hope I was clear enough. Thanks in advance, Daniele

joshreich commented 10 years ago

I'm fairly certain this is a valid issue w/ our current implementation. Essentially I believe what is happening is that OpenFlow requires installed rules that match on dstport to also specify a match on ip_proto (either TCP or UDP) and ethtype (IP) and we've left that specialization stage out of the compiler. This should be a quick fix, but I probably won't have time myself until tomorrow or over the weekend. So I'm assigning to @SiGe though any other member of the dev team should feel free to jump in---and if no one manages to get to it before I have cycles, then I'll take care of it.

reitblatt commented 10 years ago

You can't really do that inference "correctly", not with this particular pattern. The user has to specify TCP or UDP, and even then you'd still need to know IPv4 or IPv6. But detecting this "bad" pattern and throwing an exception is easier.

On Thu, Feb 6, 2014 at 6:32 PM, Joshua Reich notifications@github.comwrote:

I'm fairly certain this is a valid issue w/ our current implementation. Essentially I believe what is happening is that OpenFlow requires installed rules that match on dstport to also specify a match on ip_proto (either TCP or UDP) and ethtype (IP) and we've left that specialization stage out of the compiler. This should be a quick fix, but I probably won't have time myself until tomorrow or over the weekend. So I'm assigning to @SiGehttps://github.com/SiGethough any other member of the dev team should feel free to jump in---and if no one manages to get to it before I have cycles, then I'll take care of it.

Reply to this email directly or view it on GitHubhttps://github.com/frenetic-lang/pyretic/issues/27#issuecomment-34387378 .

joshreich commented 10 years ago

I guess it all depends on what you mean by "correctly" ;-) One could say that specifying a dstport or srcport w/o specifying the protocol means 'do this match in any case where it might apply' and thus should produce specialized rules that match both proto=TCP, dstport=foo and proto=UDP, dstport=foo (and similarly for the ethernet protocol field being IPv4 or IPv6, though at the moment we have basically been ignoring IPv6). This is similar in spirit to saying that the policy match(srcmac=foo) in the absence of a specific location means 'do this match in any case (place) where it might apply' resulting in rules on each switch that match(srmac=foo).

The above approach seems more programmer-friendly to me than the alternative, but I'm open to being convinced otherwise.

mcanini commented 10 years ago

Ah, that is quite devious.

I tend to agree with @joshreich that the compiler might feel free to emit rules for both TCP and UDP. Certainly it would still help to see a message in the log about the pattern being underspecified.

jnfoster commented 10 years ago

Another option is to delete srcport from the policy language (which doesn't really make sense without specifying the protocol), and add tcpsrcport, udpsrcport, etc. instead. These expand into the correct lower-level primitives: e.g., tcpsrcport=80 becomes ipproto=6 and tpsrcport=80.

IIRC, NetCore worked this way.

-N

On Thu, Feb 6, 2014 at 7:09 PM, Marco Canini notifications@github.comwrote:

Ah, that is quite devious.

I tend to agree with @joshreich https://github.com/joshreich that the compiler might feel free to emit rules for both TCP and UDP. Certainly it would still help to see a message in the log about the pattern being underspecified.

Reply to this email directly or view it on GitHubhttps://github.com/frenetic-lang/pyretic/issues/27#issuecomment-34391033 .

SiGe commented 10 years ago

@danieledc @joshreich @jnfoster @reitblatt

I just pushed a new branch with fixes to this issue and https://github.com/frenetic-lang/pyretic/issues/27. The new branch is at:

https://github.com/frenetic-lang/pyretic/tree/issues-daniele

For now I am creating new rules if ethtype/proto are missing, as @joshreich and @mcanini suggested.