Support https for es client #3150
Conversation
bright-starry-sky
requested review from
yixinglu,
critical27,
jievince and
panda-sheep
Codecov Report
@@ Coverage Diff @@
## master #3150 +/- ##
==========================================
+ Coverage 84.30% 85.01% +0.71%
==========================================
Files 1287 1289 +2
Lines 115636 115920 +284
==========================================
+ Hits 97483 98549 +1066
+ Misses 18153 17371 -782
Continue to review full report at Codecov.
|
| { | ||
| std::string query = | ||
| "SIGN IN TEXT SERVICE (127.0.0.1:9200, HTTP, \"user\", \"password\"), " | ||
| "(127.0.0.1:9200, HTTPS, \"user\", \"password\")"; |
There was a problem hiding this comment.
why not use following format:
SIGN IN TEXT SERVICE
(http://127.0.0.1:9200, "user", "password"),
(https://127.0.0.1:9200, "user", "password")
There was a problem hiding this comment.
why not use following format:
SIGN IN TEXT SERVICE (http://127.0.0.1:9200, "user", "password"), (https://127.0.0.1:9200, "user", "password")
Good idea, I agree with you. If we change it now, there will be compatibility issues. I suggest improvements in the next big release, WDYT ?
There was a problem hiding this comment.
Why exist incompatible issue? We can use http by default if user don't specify it.
There was a problem hiding this comment.
Why exist incompatible issue? We can use http by default if user don't specify it.
I want to change the meta data storage by thrift structure , current as below :
struct FTClient {
1: required common.HostAddr host,
2: optional binary user,
3: optional binary pwd,
4: optional binary conn_type,
}
I think below is better :
struct FTClient {
1: required binary conn_prefix, <----- "http://127.0.0.1:9200"
2: optional binary user,
3: optional binary pwd,
}
This change needs to be handled by the upgrade tool.
src/common/plugin/fulltext/FTUtils.h
Outdated
|
|
||
| explicit HttpClient(const HostAddr& h) noexcept : host(h) {} | ||
| explicit HttpClient(const HostAddr& h) noexcept : host(h) { connType = "http"; } |
There was a problem hiding this comment.
In line 42, directly set the default value of connType to "http"， wdyt?
There was a problem hiding this comment.
In line 42, directly set the default value of connType to
"http"， wdyt?
Good point.
| @@ -98,7 +98,7 @@ TEST(FulltextPluginTest, ESBulkTest) { | |||
| auto header = ESStorageAdapter().bulkHeader(hc); | |||
| std::string expected = | |||
| "/usr/bin/curl -H \"Content-Type: application/x-ndjson; " | |||
| "charset=utf-8\" -XPOST \"http://127.0.0.1:9200/_bulk\""; | |||
| "charset=utf-8\" -XPOST -k \"http://127.0.0.1:9200/_bulk\""; | |||
There was a problem hiding this comment.
-k allow curl to use non-secure ssl connection and transfer data ?
src/common/plugin/fulltext/FTUtils.h
Outdated
| HttpClient(const HostAddr& h, | ||
| const std::string& u, | ||
| const std::string& p, | ||
| const std::string&& c) noexcept |
There was a problem hiding this comment.
const std::string&& doesn't seem to work std::string&& is ok ?
There was a problem hiding this comment.
const std::string&&doesn't seem to workstd::string&&is ok ?
Good point, negligent.
bright-starry-sky
force-pushed
the
support_https_for_es_client
branch
from
a4daec0
to
b4a3ac3
Compare
|
addressed comments |
|
close this PR， further discussion is needed. |
Support https for elasticsearch client, such as :
curl -u elastic:123456 -k https://127.0.0.1:9200/_cat/indices?vsyntax:
requirement from :
https://discuss.nebula-graph.com.cn/t/topic/4787