From tateishi.katsuyuki ＠ oss.ntt.co.jp Tue Jun 29 15:30:02 2010 From: tateishi.katsuyuki ＠ oss.ntt.co.jp (TATEISHI Katsuyuki) Date: Tue, 29 Jun 2010 15:30:02 +0900 (JST) Subject: [Ultramonkey-l7-develop 622] Re: [SCM] ultramonkey-l7-v2 (ultramonkey-l7) branch, tproxy, created. v2.1.3-0-4-g11d8515 In-Reply-To: <1277119636.943984.28856.nullmailer@users.sourceforge.jp> References: <1277119636.943984.28856.nullmailer@users.sourceforge.jp> Message-ID: <20100629.153002.1018962831995038949.tateishi.katsuyuki@oss.ntt.co.jp> 田沼さん、 立石です。お疲れさまです。 -san wrote: > This is an automated email from the git hooks/post-receive script. It was > generated because a ref change was pushed to "ultramonkey-l7-v2" repository > containing the "ultramonkey-l7" project. > > The branch, tproxy has been created > at 11d85152c6ebf8a1f6bd45111b5a0280b32bbf0a (commit) tproxyブランチの内容をパッチから分かる範囲でレビューしました。 以下コメントです。 1. 新たに stdio.h をインクルードしているヘッダがいくつかあり ますが、コンパイル時にsnprintfまわりでエラーがでることに対 する修正ですよね？ tproxy 関係でないのでブランチ（すくなく ともコミット）をわけたほうがいいと思います。(gccの新版対応 でしょうか) # 万が一 tporxy 関連の変更を取り消したくなったときとかを考 # えると分けておいた方がいいです。 あと私の環境(Fedora12)だと include/l7vs.h は stdio.h のイ ンクルードを消してもコンパイル時のエラー等は出ません。また make distcheck の際にエラーにならないよう以下のパッチが必 要でした。 ============================================================ diff --git a/snmpagent/l7snmpagent.cpp b/snmpagent/l7snmpagent.cpp index 5303170..dd672e6 100644 --- a/snmpagent/l7snmpagent.cpp +++ b/snmpagent/l7snmpagent.cpp @@ -1,3 +1,4 @@ +#include #include #include "logger_wrapper.h" ============================================================ 2. configure.ac の書き方はいいと思います。 3. include/l7vs_conn.h で IP_TRANSPARENT を定義してますが、 linux/in.h 全体を読み込みたくないからですよね？ /usr/include 配下のファイルもほとんどこのファイルを読み込 んでない上、普通インクルードしそうなファイルから暗黙にイン クルードされることもなさそうなので、この処理でいいと思いま す。 4. include/l7vsadm.h のコメントはtypoでしょうか。code -> mode? ============================================================ @@ -128,6 +130,7 @@ struct l7vsadm_option_data { int category_all_flag; //!< All Log-Category flag int replication_start_flag; //!< Start Replication flag enum PARAMETER_COMPONENT_TAG reload_param; //!< Parameter Reload Componet + enum l7vs_dest_forward_type forward; //!< Forward code }; //! L7vsadm command code list. ============================================================ 5. コメントでは Forward mode(「モード」), enum のタグ名では l7vs_dest_forward_type(「タイプ」)となっているので mode か type のどちらかに統一したほうがメンテナンス性は上がると思 います。また各構造体における新しいメンバ変数 forward は fwdmode あるいは fwdtype とかの名前の方がわかりやすい気が します。 6. src/conn.c の下記コードですが・・・ ============================================================ diff --git a/src/conn.c b/src/conn.c index fa184f2..835f5d4 100644 --- a/src/conn.c +++ b/src/conn.c @@ -3139,6 +3139,20 @@ l7vs_conn_connect_rs(struct l7vs_conn *conn, struct l7vs_dest *dest) /*------ DEBUG LOG END ------*/ } +#ifdef IP_TRANSPARENT + if (dest->forward == L7VS_DEST_FORWARD_TPROXY) { + int value = 1; + ret = setsockopt(s, SOL_IP, IP_TRANSPARENT, &value, sizeof(int)); + if (ret != 0) { + LOGGER_PUT_LOG_WARN(LOG_CAT_L7VSD_SYSTEM_SOCKET,/*XXX*/999, "setsockopt(IP_TRANSPARENT) not supported on this platform."); + } else { + ret = bind(s, (struct sockaddr*) &conn->caddr, sizeof(struct sockaddr)); + if (ret != 0) { + LOGGER_PUT_LOG_WARN(LOG_CAT_L7VSD_SYSTEM_SOCKET,/*XXX*/999, "Cannot bind client address."); + } + } + } +#endif /*-------- DEBUG LOG --------*/ if (LOG_LV_DEBUG == logger_get_log_level(LOG_CAT_L7VSD_NETWORK)) { char addr_str[DEBUG_STR_LEN] = {0}; ============================================================ * setsockopt のリターンコードが 0 でない場合に IP_TRANSPARENT が実装されていないものと決め打ちにするのは 良くないと思います。エラーには EBADF や EFAULT とかもある ので。 * IP_TRANSPARENT が実装されていない処理系ではそもそも include/l7vs_conn.hで IP_TRANSPARENT が定義されないので、 #ifdef 内部のコードを通らないと思います。 * ここに到達する以前に l7vsadm で指定できないようガードされ てると思いますが、実際に setsockopt する箇所でもチェック するのは賛成です。 以上を踏まえて、 * (dest->forward == L7VS_DEST_FORWARD_TPROXY) が真でかつ、 IP_TRANSPARENT が定義されていない場合に LOGGER_PUT_LOG_WARN(LOG_CAT_L7VSD_SYSTEM_SOCKET,/*XXX*/999, "setsockopt(IP_TRANSPARENT) not supported on this platform."); を呼ぶようにして、IP_TRANSPARENT が定義されている場合にの み setsockopt を呼び、setsockopt() がエラーでリターンした 場合は errno を見て本当のエラーをログ出力するほうが良いと 思います(これはbind()がエラーになった場合も同様です)。 7. src/l7vsadm_main.c の -m オプションって必要ですかね？(-t だけじゃダメ？) 必要な場合は・・・ * src/l7vsadm_main.c でリアルサーバオプションに -m も -t 指 定しなかった場合のデフォルト値の設定処理を入れませんか？ ファイル内 static なl7vsadm_option_dataが変数なので0、つ まりL7VS_DEST_FORWARD_MASQに初期化されるとは思います が・・・ * 同 l7vsadm_main.c で -m と -t を同時に指定したときはエラー で弾いた方がいいと思います(または後から指定した方が有効で あると文書に明記するか)。 9. マニュアル(man/l7vsadm.8, man/l7directord.8)も修正が必要だ と思います。特に、tproxy は l7vsd だけでは完結できず、 iptablesのルール投入も必要だと思いますので、そのことにつ いても触れておく必要があると思います。 最悪「Linux の Documentation/networking/tproxy.txt 見ろ」 で。 10. sorry サーバも tproxy を選択できるとうれしいです。(今は masq 決め打ちですよね？) 11. fallback サーバは l7vsd からすると単なるリアルサーバなの で、今回 l7vsd/l7vsadm側に特別な修正は不要だと理解しまし た。(fallbackサーバというものを知ってるのは l7directord だけですよね) 以上です。 -- TATEISHI Katsuyuki From tanuma.kouhei ＠ nttcom.co.jp Tue Jun 29 17:19:41 2010 From: tanuma.kouhei ＠ nttcom.co.jp (Kohei TANUMA) Date: Tue, 29 Jun 2010 17:19:41 +0900 Subject: [Ultramonkey-l7-develop 623] Re: [SCM] ultramonkey-l7-v2 (ultramonkey-l7) branch, tproxy, created. v2.1.3-0-4-g11d8515 In-Reply-To: <20100629.153002.1018962831995038949.tateishi.katsuyuki@oss.ntt.co.jp> References: <1277119636.943984.28856.nullmailer@users.sourceforge.jp> <20100629.153002.1018962831995038949.tateishi.katsuyuki@oss.ntt.co.jp> Message-ID: <4C29AC9D.80301@nttcom.co.jp> 立石さま 田沼です。 ご確認ありがとうございます。 > 1. 新たに stdio.h をインクルードしているヘッダがいくつかあり > ますが、コンパイル時にsnprintfまわりでエラーがでることに対 > する修正ですよね？ tproxy 関係でないのでブランチ（すくなく > ともコミット）をわけたほうがいいと思います。(gccの新版対応 > でしょうか) stdio.h は上記理由であってます。 これ、いまから分ける際にスマートな git 操作はどんな感じに なるんでしょうか。 前に以下のような流れでやって大変だった記憶があるので…。 git diff OLD_COMMIT diff 修正 git checkout OLD_COMMIT patch git commit patch git commit git push そもそもローカルで細かくコミットしておかないとダメですかね？ > あと私の環境(Fedora12)だと include/l7vs.h は stdio.h のイ > ンクルードを消してもコンパイル時のエラー等は出ません。また > make distcheck の際にエラーにならないよう以下のパッチが必 > 要でした。 > ============================================================ > diff --git a/snmpagent/l7snmpagent.cpp b/snmpagent/l7snmpagent.cpp > index 5303170..dd672e6 100644 > --- a/snmpagent/l7snmpagent.cpp > +++ b/snmpagent/l7snmpagent.cpp > @@ -1,3 +1,4 @@ > +#include > #include > > #include "logger_wrapper.h" > ============================================================ 了解です。 > 4. include/l7vsadm.h のコメントはtypoでしょうか。code -> mode? これは typo ですね。修正します。 > 5. コメントでは Forward mode(「モード」), enum のタグ名では > l7vs_dest_forward_type(「タイプ」)となっているので mode か > type のどちらかに統一したほうがメンテナンス性は上がると思 > います。また各構造体における新しいメンバ変数 forward は > fwdmode あるいは fwdtype とかの名前の方がわかりやすい気が > します。 これは他の enum が _type で終わっていたので _type に合わせた 記憶があったのですが、他にそんな enum ないですね…。 統一しておきます。 > 6. src/conn.c の下記コードですが・・・ > * (dest->forward == L7VS_DEST_FORWARD_TPROXY) が真でかつ、 > IP_TRANSPARENT が定義されていない場合に > LOGGER_PUT_LOG_WARN(LOG_CAT_L7VSD_SYSTEM_SOCKET,/*XXX*/999, "setsockopt(IP_TRANSPARENT) not supported on this platform."); > を呼ぶようにして、IP_TRANSPARENT が定義されている場合にの > み setsockopt を呼び、setsockopt() がエラーでリターンした > 場合は errno を見て本当のエラーをログ出力するほうが良いと > 思います(これはbind()がエラーになった場合も同様です)。 了解です。 > 7. src/l7vsadm_main.c の -m オプションって必要ですかね？(-t > だけじゃダメ？) 必要な場合は・・・ これは、いまさら感があるのですが LVS にあわせるために -m も 用意しました。 -m はモジュール指定のオプションで使われているので かなり微妙とか思いつつも、既にいろいろなオプション文字が 重複してるので、もういいかと実装に至りました。 やっぱり、いらないですかね…？ > 9. マニュアル(man/l7vsadm.8, man/l7directord.8)も修正が必要だ > と思います。特に、tproxy は l7vsd だけでは完結できず、 > iptablesのルール投入も必要だと思いますので、そのことにつ > いても触れておく必要があると思います。 > 最悪「Linux の Documentation/networking/tproxy.txt 見ろ」 > で。 そうですね、了解です。 > 10. sorry サーバも tproxy を選択できるとうれしいです。(今は > masq 決め打ちですよね？) これはそのとおりなんですよね…。 指定の仕方が思いつかなくて諦めてしまいました。 sorry サーバ用の tproxy 専用のオプションを用意するか、 -b の sorry サーバ指定オプションを小細工するか、 l7vsadm -A|E の場合の -t オプションは sorry サーバに 対するものとして処理するか、などどれもいまいちで…。 とにかくオプションについては、重複とか自前処理とか ちょっと厳しいと思います。 ipvsadm -Ln の変わりに l7vsadm -Ln って打つと エラーになるし…。（-L がそもそもないですが…） モジュールのオプションもヘルプ表示で出てこないから 何が指定できるかわからないし…。 この辺のコマンドは iptables に似せて作ってるんだから iptables -j REJECT -h みたいに l7vsadm -m sessionless -h とかが出来たらいいんだけどなぁ…。 > 11. fallback サーバは l7vsd からすると単なるリアルサーバなの > で、今回 l7vsd/l7vsadm側に特別な修正は不要だと理解しまし > た。(fallbackサーバというものを知ってるのは l7directord > だけですよね) そのとおりです。