Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REST] Default value when registering server by curl #2497

Open
a24-yamaguchi opened this issue Jan 12, 2017 · 4 comments
Open

[REST] Default value when registering server by curl #2497

a24-yamaguchi opened this issue Jan 12, 2017 · 4 comments
Labels

Comments

@a24-yamaguchi
Copy link
Contributor

a24-yamaguchi commented Jan 12, 2017

次のURLに掲載されている手順でcurlを使って監視サーバーの登録を行った場合、
明示的にTLSの使用をFALSEにしないとTRUEで登録される。
https://www.miraclelinux.com/tech-blog/bwqr2p
ただし、証明書のパスが空であるためか、監視そのものは正しく行われる。
DBを確認すると、TBL:arm_pluginsのtls_enable_verify列に1が入っている。

TLSの使用をFALSEにする場合、curlでPOSTする際明示的に次の設定値を追加する必要がある。

-d "tlsEnableVerify=false"

■発生環境、ver/rev
16.04

%%260

@a24-yamaguchi
Copy link
Contributor Author

$ git diff server/common/ArmPluginInfo.cc
diff --git a/server/common/ArmPluginInfo.cc b/server/common/ArmPluginInfo.cc
index 0172477..1eaa181 100644
--- a/server/common/ArmPluginInfo.cc
+++ b/server/common/ArmPluginInfo.cc
@@ -24,7 +24,7 @@ void ArmPluginInfo::initialize(ArmPluginInfo &armPluginInfo)
        armPluginInfo.id = INVALID_ARM_PLUGIN_INFO_ID;
        armPluginInfo.type = MONITORING_SYSTEM_UNKNOWN;
        armPluginInfo.serverId = INVALID_SERVER_ID;
-       armPluginInfo.tlsEnableVerify = 1;
+       armPluginInfo.tlsEnableVerify = 0;
 }
 
 bool ArmPluginInfo::isTLSVerifyEnabled(void) const

デフォルト値0の方が良いような。

@cosmo0920
Copy link
Contributor

提案のパッチで良さそうにも思います。
冗長になりますが、以下のようにしておいた方がfalseと一目で分かるのではないでしょうか。

diff --git a/server/common/ArmPluginInfo.cc b/server/common/ArmPluginInfo.cc
index 0172477..1eaa181 100644
--- a/server/common/ArmPluginInfo.cc
+++ b/server/common/ArmPluginInfo.cc
@@ -24,7 +24,7 @@ void ArmPluginInfo::initialize(ArmPluginInfo &armPluginInfo)
        armPluginInfo.id = INVALID_ARM_PLUGIN_INFO_ID;
        armPluginInfo.type = MONITORING_SYSTEM_UNKNOWN;
        armPluginInfo.serverId = INVALID_SERVER_ID;
-       armPluginInfo.tlsEnableVerify = 1;
+       armPluginInfo.tlsEnableVerify = static_cast<int>(false);
 }
 
 bool ArmPluginInfo::isTLSVerifyEnabled(void) const

@kz0817
Copy link
Member

kz0817 commented Jan 13, 2017

ここの変数初期化では、0を代入するのが自然な気もしますが、
問題に対する解決方法としては、RestResourceServer.ccの中で
tlsEnableVerifyパラメータがない場合に明示的にfalseを
設定する処理を入れるほうがいいのなと思いました。

armPluginInfo.tlsEnableVerify = static_cast(false);

C++のキャストは、長ったらしいので、むしろ見難くなる印象です。
1が真で、0が偽という取り扱いは、わりとコモンセンスだと思うので、
それほど違和感はないです。こだわるなら、TLS_ENABLE_VERITYや
TLS_ENABLE_NO_VERITYみたいな定数を定義するほうがいいと
よいと思います。

@kz0817
Copy link
Member

kz0817 commented Jan 13, 2017

tlsEnableVerifyパラメータがない場合に明示的にfalseを
設定する処理を入れるほうがいいのなと思いました。

とはいえ、未指定の場合、armPluginInfoの初期値に従うのは
それほど、へんな実装ではない気がしますので、提案のパッチで
いい思います。
ただ、RestAPIの仕様としては、未指定の場合、falseの明示する
のが妥当で、それをユニットテストすべきと思います。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants