Support ARP in ExactMatch.new.

yasuhito commented 9 years ago

@rumb #240 をリファクタリングしました。レビューおねがいします。

rumb commented 9 years ago

@yasuhito お疲れ様です。

以下の２点が気になりました。

ExactMatchの処理を各formatでのto_exactmatchで行う理由
openflow10/match.rb を openflow10/match.rb を openflow10/match10.rb に分けた理由初心者考えなので適当に流してもらって構いません。

yasuhito commented 9 years ago

@rumb 隊長、

ExactMatchの処理を各formatでのto_exactmatchで行う理由

これはオブジェト指向な言語の定番リファクタリングで、switch case があったらこういうふうに各クラスのインスタンスメソッドにする、というやつです。よくポリモルフィズムの例で教科書なんかに出てきます。

openflow10/match.rb を openflow10/match.rb を openflow10/match10.rb に分けた理由

require 'pio/exact_match' したときに、ファイルを分けないと require が循環参照になっていたのでついでに直してみました。

rumb commented 9 years ago

@yasuhito 司令

回答ありがとうございます。勉強になりました。

少し逸れますが、Match.new()でether_typeが指定されていない時にはエラーを出した方がいいと思います。

ether_typeが指定されていない状況でsend_flow_mod_add()をすると、例え他のip_protocol等を指定していても全て無視され、matchの条件がない状態で登録されてしまいます。

つまり、以下のコードを実行した場合、

options = {
　　ether_type: 2048,
     ip_protocol: 17,
     ip_destination_address: ipaddr,
     transport_destination_port: 5002
     }
send_flow_mod_add(datapath_id,
      match: Match.new( options ),
      actions: [SetEtherDestinationAddress.new(hwaddr),
      　　SendOutPort.new(entry.port)]
      )

$ovs-ofctl dump-flows ovs
cookie=0x0, duration=2.848s, table=0, n_packets=0, n_bytes=0, idle_age=2, priority=0,udp,nw_dst=192.168.122.1,tp_dst=5002 actions=mod_dl_dst:66:55:44:33:22:11,output:3

このように正常に登録されますが、ether_typeの部分をコメントアウトすると、

$ovs-ofctl dump-flows ovs
cookie=0x0, duration=1.410s, table=0, n_packets=0, n_bytes=0, idle_age=1, priority=0 actions=mod_dl_dst:66:55:44:33:22:11,output:3

このように、マッチ条件部分が全てなくなってしまいます。（自分がここではまりました）

また、以上を踏まえて、ExactMatchに限らずMatchも同じように各クラスごとにメソッドを作り、ether_type部分を意識せずに”カジュアルに”マッチを作成できるといいかなと思います。

どんな実装がいいかはわかりませんが、

Arp::Request.match(source_mac: "11:11:11:11:11:11")
UdpHeader.match(ip_destination: "192.168.0.1")

みたいな感じで作成し、ether_typeとその他ip_protocolなどの必要な条件をクラスに応じて自動的に補完してくれるとありがたいです。

というユーザ視点での意見でした。

yasuhito commented 9 years ago

@rumb ether_type をマッチで指定しないとほかの条件が無視されるのは、ovs 特有のことでしょうか? それとも、そもそもOpenFlowの仕様がそうなってたりしますか?

Arp とかから .match でマッチを作れるのはたしかに便利そうですね! いいアイデアだと思います。デザインについて相談したいので、別個にイシューを切ってもらってよいですか?

rumb commented 9 years ago

@yasuhito

ether_type をマッチで指定しないとほかの条件が無視されるのは、ovs 特有のことでしょうか? それとも、そもそもOpenFlowの仕様がそうなってたりしますか?

確かに、openvswitch固有の問題？であることも考えられますね。 openvswitchのマニュアルには、ether_type (openvswitch的にはdl_type)がない場合には、いくつかのオプションが無視されますということが書いてありました。それがOpenFlowの仕様かどうかはspecificationを読んでみないと分からないです。ざっと目を通した限りでは、そのような細かい仕様を見つけることはできませんでした。

openvswitchマニュアル http://openvswitch.org/support/dist-docs/ovs-ofctl.8.txt

Arp とかから .match でマッチを作れるのはたしかに便利そうですね! いいアイデアだと思います。デザインについて相談したいので、別個にイシューを切ってもらってよいですか?

イシューつくりました。#245

chibacchie commented 9 years ago

それがOpenFlowの仕様かどうかはspecificationを読んでみないと分からないです。ざっと目を通した限りでは、そのような細かい仕様を見つけることはできませんでした。

See 7.2.3.6 Flow Match Field Prerequisite and Table 12 on https://www.opennetworking.org/images/stories/downloads/sdn-resources/onf-specifications/openflow/openflow-switch-v1.3.4.pdf, or 3.4 Ignoring fields in the match on https://www.opennetworking.org/images/stories/downloads/sdn-resources/onf-specifications/openflow/openflow-spec-v1.0.2.pdf.

yasuhito commented 9 years ago

@chibacchie ありがとうございます。とりあえずそのあたりの議論は #245 に移動します。

trema / pio

Support ARP in ExactMatch.new. #244