search

trema / pio

Packet parser and generator in Ruby
27 stars 11 forks source link

WIP: Pio::Dhcp修正 #77

Closed shun159 closed 8 years ago

shun159 commented 10 years ago

https://github.com/trema/pio/pull/65#issue-30931747 での新仕様検討をここで行いますので、こちらに転記します。

更新ごとにつらつらと書いていきます。 @yasuhito 何かありましたら追記をお願いします。

TODO

仕様の検討

http://www.zytrax.com/books/dhcp/apc/

方針：

http://www.cisco.com/cisco/web/support/JP/100/1007/1007781_100-j.html#dhcprfcref を参考にする。

Pio::Dhcp 新仕様

クライアントメッセージ別オプション

サーバメッセージ別オプション

質問事項

      def message_type
        get_tlv_field(Dhcp::MESSAGE_TYPE_TLV)
      end

      def server_identifier
        get_tlv_field(Dhcp::SERVER_IDENTIFIER_TLV)
      end

      def client_identifier
        get_tlv_field(Dhcp::CLIENT_IDENTIFIER_TLV)
      end

これが今後要望などあると、追加するためのメソッドとともに増えてくるので、 行数と修正にかかる時間、デグレーションを減らしたい考えです。いい方法などあれば教えてください。

2014/06/24 以下のようにして自動生成するようにした

      def method_missing(method)
        tlv_methods = Dhcp::Constants.constants(true).map do |const|
          const.downcase
        end
        if dhcp.methods.include?(method)
          dhcp.send(method)
        elsif tlv_methods.include?(method)
          send :get_tlv_field, Dhcp::Constants.const_get(method.upcase)
        end
      end

      def get_tlv(tlv_type)
        tlv = dhcp.optional_tlvs.find do | each |
          each['tlv_type'] == tlv_type
        end
        tlv['tlv_value'] if tlv
      end

      def get_tlv_field(tlv_type)
        tlv = get_tlv(tlv_type)
        tlv.snapshot if tlv
      end
shun159 commented 10 years ago

@yasuhito

オプションとかデフォルト値周りのテストを以下のとおりに考えています。

https://github.com/shun159/pio/blob/new_dhcp/spec/pio/dhcp/discover/options_spec.rb

書き方が仕様書チックになってないなど、 いろいろ突っ込みどころあるとは思いますが、 基本的な方針としては、Pio::Dhcp::Discover.newの結果えられたデフォルトの値の取得と、 全オプションを指定した場合のPio::Dhcp::Discover.newの結果得られた値の取得、 また、Ａｒｐでやってるオプションのテストのやり方組み合わせて行いたいと考えています。

なぜかというと、今の実装がDhcp::FieldやらDhcp::Formatやらで デフォルト値を与えているから(オプションがnilだったらinitial_valueで指定した値適用してます) とかもろもろの理由から、ほかでやっているようなテストができないのです。

意見・ご指摘をお願いします。

shun159 commented 10 years ago

@yasuhito すみませんが、TODOリストにある、 yardドキュメント追加　タスクの中の ドキュメント英訳のヘルプをお願いできませんか。 ※ドキュメント追加は仕上げにやりますので、後々に。

宜しくお願いします。

shun159 commented 10 years ago

サーバメッセージに Classless Static Route Option を追加します。

axsh/openvnet @akry さんより https://github.com/axsh/openvnet/issues/233#issuecomment-47184751

shun159 commented 10 years ago

Pio::Dhcp::Contantsで指定している 定数の類は、RFC別にモジュールを分けることにします。

shun159 commented 10 years ago

dhcp_specのテスト方法の変更について。 これまでは、直接__spec.rbにバイナリデータを書いていましたが、 dhcp_specの場合は、dhcp_.pcapと同じデータで.rawデータを 作成し、そのパース結果でdhcp*.pcapと同じ値が各フィールドで 取得できることを確認できるようにします。

今、可能かわかりませんが、 openvnetで生成するDHCPパケットを お願いしているので、一応それらも読めるとか同じものを生成できる とかのテストをすばやく書いて保証できるようにしたいなどと思っています。

よろしくお願いします。

yasuhito commented 10 years ago

@shun159 まとめ作業ありがとうございます。 長くなりそうなので Wiki 使いますか? 必要あれば有効にしておきます。

shun159 commented 10 years ago

@yasuhito

はい、是非使いましょう。 今回、かなりの量になると思いますので。。。

yasuhito commented 10 years ago

@shun159 有効にしておきました。 あと、仕様ドラフトについてはなるべく早めに _spec.rb として実装してみてください。項目だけ挙げて pending としておけば OK です。ドキュメントはあまり増やさずに、コード化してしまうことが重要です。

shun159 commented 10 years ago

@yasuhito

wiki有難うございます。

あと、仕様ドラフトについてはなるべく早めに _spec.rb として実装してみてください。項目だけ挙げて pending としておけば OK です。ドキュメントはあまり増やさずに、コード化してしまうことが重要です。

承知しました。 このあたりについては、お手透きなときに相談させてください。

yasuhito commented 10 years ago

オプションとかデフォルト値周りのテストを以下のとおりに考えています。 https://github.com/shun159/pio/blob/new_dhcp/spec/pio/dhcp/discover/options_spec.rb

まだ実行しないでざっと見たかんじですが、Given, When, Then がちぐはぐな気がします。

describe Pio::Dhcp::Discover, '.new' do
  Given(:mandatory_option) do
    {
      # Discover のインスタンスが来るはず。
      # オプションなどは When で指定。
      source_mac: Pio::Mac.new('74:e5:0b:2a:18:f8')
    }
  end

  # ...
  context 'with a mandatory option' do
    describe '.new' do  # また .new の仕様?

一行目では Pio::Dhcp::Discover.new の仕様を示します、となっているので Given の前提条件にはふつう Pio::Dhcp::Discover のインスタンスが来るはずです。また、下に進んでいくとまた describe '.new' が出てくるのもおかしいです。

rspec -fs で実行しながら書いてみて、きちんと意味が通るか確認してみてください。いったん RSpec の書き方を復習するのも近道と思います。

shun159 commented 10 years ago

ご指摘有難うございます。 修正します。

yasuhito commented 10 years ago

すみませんが、TODOリストにある、 yardドキュメント追加　タスクの中の ドキュメント英訳のヘルプをお願いできませんか。 ※ドキュメント追加は仕上げにやりますので、後々に。

了解です。そのときになったら、また ping をおねがいします。

yasuhito commented 10 years ago

RSpec は、いったん日本語で書くのもいいかもしれません。

shun159 commented 10 years ago

@yasuhito

すみません、一応確認なんですが、 @yasuhito さんの環境で現在のPioでrspec -fs動きますか？

shun159 commented 10 years ago

@yasuhito

RSpec は、いったん日本語で書くのもいいかもしれません。

了解です。

yasuhito commented 10 years ago

rspec 3.0 からオプションが変わってたみたいです。rspec -fd でした。

shun159 commented 10 years ago

@yasuhito

あぁ、なるほどですね。 了解しました。

shun159 commented 10 years ago

@yasuhito DHCPの件とは違いますが、 質問させてください。

https://github.com/trema/pio/blob/develop/lib/pio/type/ip_address.rb#L13 と https://github.com/trema/pio/blob/develop/lib/pio/type/mac_address.rb#L13

のそれぞれのセッタですが、ここでPio::IPv4Address.newとかPio::Mac.new などをしていない理由はどのような意図でしたでしょうか？

この質問の意図は、Pioが将来、IPv6に対応したときに、 Pio::IPv4Address.newのクラス名が変わったりふえたりする可能性があることを気にしています。 もしそのような場合、各パーサで修正が必要になってしまうので、 個人的にはメンテナンス性的にどうか、、、と思っています。

アドレスオブジェクトの生成をip_address.rbでやらせれば、 その際の修正の時間もかからずに済むんじゃないかなどと思っています。 ちなみに、これにあわせてmac_address.rbも同じように ここでPio::Mac.newさせるのはいかがでしょうか？

よろしくです。

yasuhito commented 10 years ago

この質問の意図は、Pioが将来、IPv6に対応したときに、 Pio::IPv4Address.newのクラス名が変わる可能性があることを気にしています。 もし変わった場合、各パーサで修正が必要になってしまうので、 個人的にはメンテナンス性的にどうか、、、と思っています。

アドレスオブジェクトの生成をip_address.rbでやらせれば、 その際の修正の時間もかからずに済むんじゃないかなどと思っています。

すみません、よく分かっていませんが、

shun159 commented 10 years ago

@yasuhito

ご指摘ありがとうございます。 確かに、仰る通りです。

これのもう一つの意図としては、 オブジェクト生成のための文字数や行数を減らすことです。 それが、セッターでオブジェクト生成させることで 解決できることだとおもっています。

如何でしょうか。

yasuhito commented 10 years ago

これのもう一つの意図としては、 オブジェクト生成のための文字数や行数を減らすことです。 それが、セッターでオブジェクト生成させることで 解決できることだとおもっています。

こういうときは具体的なサンプルコードを貼ってみてください。

セッターで Pio::IPv4Address.new しない/するで具体的にコードがどう変わるのか、一目瞭然にしてもらえればこちらでも判断できます。

shun159 commented 10 years ago

はい、いま意図的にpr中の ブランチで変更を加えたものです。

https://github.com/shun159/pio/blob/new_dhcp/lib/pio/type/ip_address.rb https://github.com/shun159/pio/blob/new_dhcp/lib/pio/type/mac_address.rb

https://github.com/shun159/pio/blob/new_dhcp/lib/pio/dhcp/options_methods.rb#L59 https://github.com/shun159/pio/blob/new_dhcp/lib/pio/dhcp/options_methods.rb#L41

以下が従前の場合です。 https://github.com/shun159/pio/blob/develop/lib/pio/dhcp/common_options.rb#L26 https://github.com/shun159/pio/blob/develop/lib/pio/dhcp/common_options.rb#L30

作成中のブランチで例にするのも どうかと思っていますが、例としては上記の通りです。

yasuhito commented 10 years ago

(リンクではなくて、該当箇所のコードを貼って分かりやすくしてください)

def ip_source_address
  @options[:ip_source_address] || QUAD_ZERO_IP_ADDRESS
end

こうしちゃうと、#ip_source_address の型が文字列だったり数値だったり IPv4Address になったりしませんか? プログラマとしては #ip_source_address は名前から IPv4Address を返すと期待すると思います。

たとえば Pio::Dhcp.new(ip_source_address: '1.2.3.4', ...) みたいに文字列でオプションを渡されるとたぶん、文字列が返ってきます (?)。いっぽうで ip_source_address の指定なしだとデフォルトの QUAD_ZERO_IP_ADDRESS で IPv4Address オブジェクトが返ってきます。

shun159 commented 10 years ago

失礼しました…

あぁ、なるほどですね。 セッターでやっていない意図、理解しました。 そこまでは考え切れてなかったです。

ご回答、ありがとうございます。 了解しました。

※ optional_tlvどうしよう…メソッド一々定義するの面倒だったので自動生成してたけれど…

shun159 commented 10 years ago

@yasuhito

たとえば Pio::Dhcp.new(ip_source_address: '1.2.3.4', ...) みたいに文字列でオプションを渡されるとたぶん、文字列が返ってきます (?)。いっぽうで ip_source_address の指定なしだとデフォルトの QUAD_ZERO_IP_ADDRESS で IPv4Address オブジェクトが返ってきます。

すみません。 しつこいようで申し訳ないんですが、 確認させてください。

# ip_source_addressに文字列を与えている
ack = Pio::Dhcp.new(ip_source_address: '1.2.3.4')
ack.ip_source_address.class.name.to_s  == 'String'

# QUAD_ZERO_IP_ADDRESSは'Pio::IPv4Addres'なので、
ack = Pio::Dhcp.new()
ack.ip_source_address.class.name.to_s  == 'Pio::IPv4Addres'

ということを指摘しているわけではないですよね。。。

yasuhito commented 10 years ago

@shun159 たぶんそうなると思ったのですが、違いました? (試してみました?)。

shun159 commented 10 years ago

違うはずです。一応、試してもいます（が、今回のprでは、その観点でテスト書いたはちょっと覚えてないです）

上の場合はFormatクラスに定義したフィールド見に行きます。 後ほど、簡単なサンプルを載せますので、宜しくお願いします。

yasuhito commented 10 years ago

どっちみち、次のように IPv4Address.new したほうがいいと思います。

def ip_source_address
  IPv4Address.new(@options[:ip_source_address] || QUAD_ZERO_IP_ADDRESS)
end

理由は、

shun159 commented 10 years ago

ここだけ読めば #ip_source_address の型が IPv4Address と確信できる。またこのために IPv4Address.new は引数にそれ自体を取れるようにしてある。

仰る通りです。同感です。 視認性、という意味では、その通りかと思います。 ただし、重箱の隅をつつくようで申し訳ないんですが、

ここだけ読めば #ip_source_address の型が IPv4Address と確信できる。

というのは、厳密にいうと、異なると思います。 たとえば、現状のip_addressのgetは以下の通りと なっています。

      def get
        IPv4Address.new octets.map { | each | format('%d', each) }.join('.')
      end

これで２パターンで値を入力してみます。

require 'pio'

ack = Pio::Dhcp::Ack.new(
        source_mac: Pio::Mac.new('00:00:00:00:00:01'),
        destination_mac: Pio::Mac.new('00:00:00:00:00:02'),
        ip_source_address: Pio::IPv4Address.new('192.168.0.1'),
        ip_destination_address: Pio::IPv4Address.new('192.168.0.10'),
        transaction_id: 0xdeadbeef,
        your_ip_address: Pio::IPv4Address.new('192.168.0.10'),
        bootp_flags: 0x0000,
        relay_agent_ip_address: Pio::IPv4Address.new(0x0),
        client_mac_address: Pio::Mac.new('00:00:00:00:00:02'),
        server_identifier: Pio::IPv4Address.new('192.168.0.1')
)

p ack.ip_source_address
# => #<Pio::IPv4Address:0x00000002305b68 @value=#<IPAddr: IPv4:192.168.0.1/255.255.255.255>>

ack = Pio::Dhcp::Ack.new(
        source_mac: '00:00:00:00:00:01',
        destination_mac: '00:00:00:00:00:02',
        ip_source_address: '192.168.0.1',
        ip_destination_address: '192.168.0.10',
        transaction_id: 0xdeadbeef,
        your_ip_address: '192.168.0.10',
        bootp_flags: 0x0000,
        relay_agent_ip_address: '0.0.0.0',
        client_mac_address: '00:00:00:00:00:02',
        server_identifier: '192.168.0.1'
)

p ack.ip_source_address

# => #<Pio::IPv4Address:0x00000001cf75f0 @value=#<IPAddr: IPv4:192.168.0.1/255.255.255.255>>

上記のようになります。

今度は、以下の通りgetの実装を変えます。

      # IPv4Addressオブジェクトを生成しなくした。
      def get
        octets.map { | each | format('%d', each) }.join('.')
      end

もう一度、２パターンで値を入力してみます。

require 'pio'

ack = Pio::Dhcp::Ack.new(
        source_mac: Pio::Mac.new('00:00:00:00:00:01'),
        destination_mac: Pio::Mac.new('00:00:00:00:00:02'),
        ip_source_address: Pio::IPv4Address.new('192.168.0.1'),
        ip_destination_address: Pio::IPv4Address.new('192.168.0.10'),
        transaction_id: 0xdeadbeef,
        your_ip_address: Pio::IPv4Address.new('192.168.0.10'),
        bootp_flags: 0x0000,
        relay_agent_ip_address: Pio::IPv4Address.new(0x0),
        client_mac_address: Pio::Mac.new('00:00:00:00:00:02'),
        server_identifier: Pio::IPv4Address.new('192.168.0.1')
)

p ack.ip_source_address
# => "192.168.0.1"

ack = Pio::Dhcp::Ack.new(
        source_mac: '00:00:00:00:00:01',
        destination_mac: '00:00:00:00:00:02',
        ip_source_address: '192.168.0.1',
        ip_destination_address: '192.168.0.10',
        transaction_id: 0xdeadbeef,
        your_ip_address: '192.168.0.10',
        bootp_flags: 0x0000,
        relay_agent_ip_address: '0.0.0.0',
        client_mac_address: '00:00:00:00:00:02',
        server_identifier: '192.168.0.1'
)

p ack.ip_source_address
# => "192.168.0.1"

こんなかんじととなります。 つまり、

def ip_source_address
  IPv4Address.new(@options[:ip_source_address] || QUAD_ZERO_IP_ADDRESS)
end

だけ読んだだけでは、#ip_source_addressの型がIPv4Addressである、 と確信できず、この当たりは、Formatなどで定義している型の、ゲッターの実装 を見なければ、確信はできません。

※ちなみに、これは、Pio::Dhcpのコード量が大きくなりつつ あるので、如何にしてそのコード量を小さくするか、ということを 追求したいので、議論させて頂いていますので、何卒ご承知いただければ 幸いです。

yasuhito commented 10 years ago

ここだけ読めば #ip_source_address の型が IPv4Address と確信できる、というのは Dhcp::CommonOptions#ip_source_address の話であります。

もちろん、

# IPv4 アドレスをかえすっぽい
def get
   octets.map { |each| format('%d', each) }.join('.')
end

こういう、そこだけ見て IP アドレスを返すんだなとはっきり分かるメソッドなら、IPv4Address.new しとくのがごく自然な気がします。

def get
   IPv4Address.new octets.map { |each| format('%d', each) }.join('.')
end

このメソッドは IP アドレスオブジェクトを返すから new してる、というごくストレートな話です。意図もよく分かります。

これを変なふうに省くと、バグったときに余計に大変です。

shun159 commented 10 years ago

あぁ、なるほど。 セッタへの入力のクラスを固定してしまえば、 バグった時の型の実装回りの心配とかをせずにすむ、などですよね。

上記、承知しました。 いったん、この話題はクローズとさせてください。 議論させていただいて有難う御座います。また次何かあればお願いします。

yasuhito commented 10 years ago

はい、コードの中で IP アドレスが '192.168.0.1' だったり #<IPv4Address '192.168.0.1'> だったりとするとまずバグるんで、.new できるならしたほうがいいです。コードも読みやすくなりますし。

shun159 commented 10 years ago

DHCP リレーエージェントが用いる機能について、 仕様を決めていきたいと思います。

少しずつ書き出していきますので、 何かあったらつっこんでください＞@yasuhito

shun159 commented 10 years ago

メッセージの種別（request/reply）にかかわらず、 クラスは一つにまとめる方針にしたいと考えています。

Pio::Dhcp::Relay.new(
  destination_mac: '00:00:00:00:00:01',
  source_mac: '00:00:00:00:00:02',
  interface_address: '192.168.0.254',
  server_address: '192.168.1.1',
  data: packet_in.data
).to_binary

生成されるバイナリデータは、メッセージ種別ごとに、サーバーへユニキャストをするため、 上記で指定されたオプションを使用したり、または、ブロードキャストとして クライアントに返答するため、無視する場合がありますが、 この方が、シンプルさがあって使いやすいと思います。

#client_mac_address、#boot_reply?や#boot_request?を利用することで、 生成したバイナリデータを適切なインターフェイスに対して、PacketOutさせる ことが可能だと考えています。

オプションについて： :destination_mac：隣接機器がサーバとは限らないため、destination_mac:とします。 :source_mac：出力インターフェイスのMACアドレスとなるため、source_mac:とします。 :interface_address：DHCPサーバがクライアントのサブネットの識別に利用されるアドレス。クライアントから入力されたインターフェイスのIPアドレスを指定する必要があります。 server_address：リレーするサーバーのIPアドレスです。 data：内部でパースさせた方が、コントローラの行数減ると考えています。

yasuhito commented 10 years ago

生成されるバイナリデータは、メッセージ種別ごとに、サーバーへユニキャストをするため、 上記で指定されたオプションを使用したり、または、ブロードキャストとして クライアントに返答するため、無視する場合がありますが、 この方が、シンプルさがあって使いやすいと思います。

すみません、これとクラスを一つにまとめるの関係が良く分からなかったのですが、具体的にどういうことでしょうか?

shun159 commented 10 years ago

すみません、これとクラスを一つにまとめるの関係が良く分からなかったのですが、具体的にどういうことでしょうか?

話を端折ってしまいました。申し訳ありません。 これは、当初（勝手な私の脳内で）、bootp_requestとbootp_replyの リレーするためのクラスをまず、どのような形にするか考える際、 それぞれ、二つに分けるようなことを考えていました。

しかし、ざっくりと説明をすると、 DHCPリレーを通過するDHCPパケットは、 destination_mac source_mac source_ip_address destination_ip_address ip_checksum udp_checksum

の書き換えを行うと認識しています。 （もちろん、機器の実装によってはそのほかのフィールドを書き換えたりする機器もあります。） また、boot_requestのリレーの際には、以下のフィールドの書き換えをします。 relay_agent_ip_address(リレーエージェントのIPを示すBOOTPのフィールド)

リレーエージェントは、boot_requestとboot_replyを ブロードキャスト⇔ユニキャストに変換することが目的で、 フィールドの上書きをする、という動作はメッセージ種別にかかわず、 同じです。（relay_agent_ip_addressの書き換えはboot_requestのみです）

結論は、動作ほとんどが同じリレーについては、 Dhcp::Relay、となんとか一つにまとまらないかと 考えていたところです。

如何でしょうか？

yasuhito commented 10 years ago

メッセージの種類が違うものは、クラスを分けるのが自然だと思います。実装がだいぶ重なるということであれば、共通部分をモジュールにして Mix-In するのが Ruby っぽいと思いますです。

shun159 commented 10 years ago

@yasuhito 了解しました！有難う御座います。

shun159 commented 9 years ago

タイトルかえました

78 を取り消して、改めて出し直して良いでしょうか？

※houndからのコメントが1k超えてて閲覧できないので。

yasuhito commented 9 years ago

はい、どうぞ!

shun159 commented 9 years ago

78　閉じました。