WIP: Pio::Dhcp修正 about pio HOT 41 CLOSED

@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で指定した値適用してます)
とかもろもろの理由から、ほかでやっているようなテストができないのです。

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

from pio.

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

宜しくお願いします。

from pio.

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

• RFC3442: http://webcache.googleusercontent.com/search?q=cache:eV_BcvyUo3MJ:www.ietf.org/rfc/rfc3442.txt+&cd=2&hl=ja&ct=clnk&gl=jp&client=firefox-a

axsh/openvnet @akry さんより
axsh/openvnet#233 (comment)

from pio.

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

from pio.

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

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

よろしくお願いします。

from pio.

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

from pio.

@yasuhito

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

from pio.

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

from pio.

@yasuhito

wiki有難うございます。

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

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

from pio.

オプションとかデフォルト値周りのテストを以下のとおりに考えています。
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 の書き方を復習するのも近道と思います。

from pio.

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

from pio.

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

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

from pio.

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

from pio.

@yasuhito

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

from pio.

@yasuhito

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

了解です。

from pio.

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

from pio.

@yasuhito

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

from pio.

@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させるのはいかがでしょうか？

よろしくです。

from pio.

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

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

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

• セッターで Pio::IPv4Address.new したときに、問題はどう解決しますか?
• クラス名が変わってもエディタで一括置換するだけなので、個人的にはあまり手間はかからないと思っています。

from pio.

@yasuhito

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

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

如何でしょうか。

from pio.

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

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

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

from pio.

はい、いま意図的に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

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

from pio.

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

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 オブジェクトが返ってきます。

from pio.

失礼しました…

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

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

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

from pio.

@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'

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

from pio.

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

from pio.

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

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

from pio.

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

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

理由は、

• ここだけ読めば #ip_source_address の型が IPv4Address と確信できる。またこのために IPv4Address.new は引数にそれ自体を取れるようにしてある。
• 後で IPv4Address の名前が変わっても、変更自体は簡単。

from pio.

ここだけ読めば #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のコード量が大きくなりつつ
あるので、如何にしてそのコード量を小さくするか、ということを
追求したいので、議論させて頂いていますので、何卒ご承知いただければ
幸いです。

from pio.

ここだけ読めば #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 してる、というごくストレートな話です。意図もよく分かります。

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

from pio.

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

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

from pio.

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

from pio.

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

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

from pio.

メッセージの種別（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：内部でパースさせた方が、コントローラの行数減ると考えています。

from pio.

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

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

from pio.

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

話を端折ってしまいました。申し訳ありません。
これは、当初（勝手な私の脳内で）、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、となんとか一つにまとまらないかと
考えていたところです。

如何でしょうか？

from pio.

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

from pio.

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

from pio.

タイトルかえました

#78 を取り消して、改めて出し直して良いでしょうか？
※houndからのコメントが1k超えてて閲覧できないので。

from pio.

はい、どうぞ!

from pio.

#78　閉じました。

from pio.