From aaffc1e88e1f1050d57e2d6bf3426d941f128e8c Mon Sep 17 00:00:00 2001 From: Yaroslav Stepanchuk Date: Sat, 3 Sep 2022 17:42:48 +0300 Subject: [PATCH 1/5] If unix socket is specified, don't listen default TCP if addr:port wasn't explicitly set --- src/config.cc | 21 +++++++++++++++++++-- src/config.h | 4 +++- src/main.cc | 4 ++-- src/server.cc | 3 ++- src/worker.cc | 3 ++- 5 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/config.cc b/src/config.cc index 2dae8aa8c7b..867130b9476 100644 --- a/src/config.cc +++ b/src/config.cc @@ -44,6 +44,8 @@ const char *errNotEnableBlobDB = "Must set rocksdb.enable_blob_files to yes firs const char *errNotSetLevelCompactionDynamicLevelBytes = "Must set rocksdb.level_compaction_dynamic_level_bytes yes first."; +const char *kDefaultBindAddress = "0.0.0.0"; + configEnum compression_type_enum[] = { {"no", rocksdb::CompressionType::kNoCompression}, {"snappy", rocksdb::CompressionType::kSnappyCompression}, @@ -94,8 +96,8 @@ Config::Config() { }; FieldWrapper fields[] = { {"daemonize", true, new YesNoField(&daemonize, false)}, - {"bind", true, new StringField(&binds_, "127.0.0.1")}, - {"port", true, new IntField(&port, 6666, 1, 65535)}, + {"bind", true, new StringField(&binds_, "")}, + {"port", true, new IntField(&port, 0, 0, 65535)}, {"workers", true, new IntField(&workers, 8, 1, 256)}, {"timeout", false, new IntField(&timeout, 0, 0, INT_MAX)}, {"tcp-backlog", true, new IntField(&backlog, 511, 0, INT_MAX)}, @@ -539,6 +541,21 @@ Status Config::finish() { if ((cluster_enabled) && !tokens.empty()) { return Status(Status::NotOK, "enabled cluster mode wasn't allowed while the namespace exists"); } + if (unixsocket.empty()) { + if (binds.size() == 0) { + binds.emplace_back(kDefaultBindAddress); + } + if (port == 0) { + port = kDefaultPort; + } + } + if (cluster_enabled && binds.size() == 0) { + return Status(Status::NotOK, "node is in cluster mode, but TCP listen address " + "wasn't specified via configuration file."); + } + if (master_port != 0 && binds.size() == 0) { + return Status(Status::NotOK, "replication doesn't supports unix socket."); + } if (db_dir.empty()) db_dir = dir + "/db"; if (backup_dir.empty()) backup_dir = dir + "/backup"; if (log_dir.empty()) log_dir = dir; diff --git a/src/config.h b/src/config.h index 793d2ce08e7..8d6e2bd14ed 100644 --- a/src/config.h +++ b/src/config.h @@ -47,6 +47,7 @@ class Storage; const size_t KiB = 1024L; const size_t MiB = 1024L * KiB; const size_t GiB = 1024L * MiB; +const int kDefaultPort = 6666; extern const char *kDefaultNamespace; @@ -64,7 +65,8 @@ struct Config{ public: Config(); ~Config() = default; - int port = 6666; + + int port = 0; int workers = 0; int timeout = 0; int loglevel = 0; diff --git a/src/main.cc b/src/main.cc index a61e2fa8394..543d5b2e414 100644 --- a/src/main.cc +++ b/src/main.cc @@ -285,11 +285,11 @@ int main(int argc, char* argv[]) { exit(1); } initGoogleLog(&config); - LOG(INFO)<< "Version: " << VERSION << " @" << GIT_COMMIT << std::endl; + LOG(INFO) << "Version: " << VERSION << " @" << GIT_COMMIT << std::endl; // Tricky: We don't expect that different instances running on the same port, // but the server use REUSE_PORT to support the multi listeners. So we connect // the listen port to check if the port has already listened or not. - if (Util::IsPortInUse(config.port)) { + if (config.port != 0 && Util::IsPortInUse(config.port)) { LOG(ERROR)<< "Could not create server TCP since the specified port[" << config.port << "] is already in use" << std::endl; exit(1); diff --git a/src/server.cc b/src/server.cc index a90bbb2e53a..5f5a73a14be 100644 --- a/src/server.cc +++ b/src/server.cc @@ -60,10 +60,11 @@ Server::Server(Engine::Storage *storage, Config *config) : if (!config->unixsocket.empty() && i == 0) { Status s = worker->ListenUnixSocket(config->unixsocket, config->unixsocketperm, config->backlog); if (!s.IsOK()) { - LOG(ERROR) << "[server] Failed to listen on unix socket: "<< config->unixsocket + LOG(ERROR) << "[server] Failed to listen on unix socket: " << config->unixsocket << ", encounter error: " << s.Msg(); exit(1); } + LOG(INFO) << "[server] Listening on unix socket: " << config->unixsocket; } worker_threads_.emplace_back(Util::MakeUnique(std::move(worker))); } diff --git a/src/worker.cc b/src/worker.cc index 5a64167f47f..8fafcd44428 100644 --- a/src/worker.cc +++ b/src/worker.cc @@ -50,10 +50,11 @@ Worker::Worker(Server *svr, Config *config, bool repl) : svr_(svr) { for (const auto &bind : binds) { s = listenTCP(bind, port, config->backlog); if (!s.IsOK()) { - LOG(ERROR) << "[worker] Failed to listen on: "<< bind << ":" << port + LOG(ERROR) << "[worker] Failed to listen on: " << bind << ":" << port << ", encounter error: " << s.Msg(); exit(1); } + LOG(INFO) << "[worker] Listening on: " << bind << ":" << port; } } From 33ef60781794f5a732738686d06218e67e2ea65e Mon Sep 17 00:00:00 2001 From: Yaroslav Stepanchuk Date: Sun, 4 Sep 2022 12:10:08 +0300 Subject: [PATCH 2/5] Minor refactoring --- src/config.cc | 5 +---- src/config_type.h | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/config.cc b/src/config.cc index 867130b9476..dbc5a875ca5 100644 --- a/src/config.cc +++ b/src/config.cc @@ -61,8 +61,6 @@ configEnum supervised_mode_enum[] = { {nullptr, 0} }; -ConfigField::~ConfigField() = default; - std::string trimRocksDBPrefix(std::string s) { if (strncasecmp(s.data(), "rocksdb.", 8)) return s; return s.substr(8, s.size()-8); @@ -90,8 +88,7 @@ Config::Config() { bool readonly; std::unique_ptr field; - FieldWrapper(std::string name, bool readonly, - ConfigField* field) + FieldWrapper(std::string name, bool readonly, ConfigField *field) : name(std::move(name)), readonly(readonly), field(field) {} }; FieldWrapper fields[] = { diff --git a/src/config_type.h b/src/config_type.h index 6cf811e537d..6fa070b6fc1 100644 --- a/src/config_type.h +++ b/src/config_type.h @@ -42,7 +42,7 @@ const char *configEnumGetName(configEnum *ce, int val); class ConfigField { public: ConfigField() = default; - virtual ~ConfigField() = 0; + virtual ~ConfigField() = default; virtual std::string ToString() = 0; virtual Status Set(const std::string &v) = 0; virtual Status ToNumber(int64_t *n) { return Status(Status::NotOK, "not supported"); } From 4f69488e9543ccebca4d6a6d384af16f5a6d6ac8 Mon Sep 17 00:00:00 2001 From: Yaroslav Stepanchuk Date: Mon, 5 Sep 2022 10:00:35 +0300 Subject: [PATCH 3/5] Set default TCP port on parsing config file --- src/config.cc | 5 +---- src/main.cc | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/config.cc b/src/config.cc index dbc5a875ca5..b508f4ad681 100644 --- a/src/config.cc +++ b/src/config.cc @@ -94,7 +94,7 @@ Config::Config() { FieldWrapper fields[] = { {"daemonize", true, new YesNoField(&daemonize, false)}, {"bind", true, new StringField(&binds_, "")}, - {"port", true, new IntField(&port, 0, 0, 65535)}, + {"port", true, new IntField(&port, kDefaultPort, 1, 65535)}, {"workers", true, new IntField(&workers, 8, 1, 256)}, {"timeout", false, new IntField(&timeout, 0, 0, INT_MAX)}, {"tcp-backlog", true, new IntField(&backlog, 511, 0, INT_MAX)}, @@ -542,9 +542,6 @@ Status Config::finish() { if (binds.size() == 0) { binds.emplace_back(kDefaultBindAddress); } - if (port == 0) { - port = kDefaultPort; - } } if (cluster_enabled && binds.size() == 0) { return Status(Status::NotOK, "node is in cluster mode, but TCP listen address " diff --git a/src/main.cc b/src/main.cc index 543d5b2e414..348036c0c08 100644 --- a/src/main.cc +++ b/src/main.cc @@ -289,7 +289,7 @@ int main(int argc, char* argv[]) { // Tricky: We don't expect that different instances running on the same port, // but the server use REUSE_PORT to support the multi listeners. So we connect // the listen port to check if the port has already listened or not. - if (config.port != 0 && Util::IsPortInUse(config.port)) { + if (config.binds.size() != 0 && Util::IsPortInUse(config.port)) { LOG(ERROR)<< "Could not create server TCP since the specified port[" << config.port << "] is already in use" << std::endl; exit(1); From 9579fb999c6f778f816528f0fdf2c469e5c625b8 Mon Sep 17 00:00:00 2001 From: Yaroslav Stepanchuk Date: Mon, 5 Sep 2022 10:57:29 +0300 Subject: [PATCH 4/5] Change default bind address from 0.0.0.0 to 127.0.0.1 --- kvrocks.conf | 7 +++---- src/config.cc | 8 +++----- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/kvrocks.conf b/kvrocks.conf index d472ee44d8b..2f5c7752f5e 100644 --- a/kvrocks.conf +++ b/kvrocks.conf @@ -1,9 +1,8 @@ ################################ GENERAL ##################################### -# By default kvrocks listens for connections from all the network interfaces -# available on the server. It is possible to listen to just one or multiple -# interfaces using the "bind" configuration directive, followed by one or -# more IP addresses. +# By default kvrocks listens for connections from localhost interface. +# It is possible to listen to just one or multiple interfaces using +# the "bind" configuration directive, followed by one or more IP addresses. # # Examples: # diff --git a/src/config.cc b/src/config.cc index b508f4ad681..55a31d66bf6 100644 --- a/src/config.cc +++ b/src/config.cc @@ -44,7 +44,7 @@ const char *errNotEnableBlobDB = "Must set rocksdb.enable_blob_files to yes firs const char *errNotSetLevelCompactionDynamicLevelBytes = "Must set rocksdb.level_compaction_dynamic_level_bytes yes first."; -const char *kDefaultBindAddress = "0.0.0.0"; +const char *kDefaultBindAddress = "127.0.0.1"; configEnum compression_type_enum[] = { {"no", rocksdb::CompressionType::kNoCompression}, @@ -538,10 +538,8 @@ Status Config::finish() { if ((cluster_enabled) && !tokens.empty()) { return Status(Status::NotOK, "enabled cluster mode wasn't allowed while the namespace exists"); } - if (unixsocket.empty()) { - if (binds.size() == 0) { - binds.emplace_back(kDefaultBindAddress); - } + if (unixsocket.empty() && binds.size() == 0) { + binds.emplace_back(kDefaultBindAddress); } if (cluster_enabled && binds.size() == 0) { return Status(Status::NotOK, "node is in cluster mode, but TCP listen address " From 989586f5ce736cf98497d46c1c35c9ffba91a417 Mon Sep 17 00:00:00 2001 From: Yaroslav Date: Mon, 5 Sep 2022 16:20:36 +0300 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: hulk --- src/config.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config.cc b/src/config.cc index 55a31d66bf6..efa57793e46 100644 --- a/src/config.cc +++ b/src/config.cc @@ -543,10 +543,10 @@ Status Config::finish() { } if (cluster_enabled && binds.size() == 0) { return Status(Status::NotOK, "node is in cluster mode, but TCP listen address " - "wasn't specified via configuration file."); + "wasn't specified via configuration file"); } if (master_port != 0 && binds.size() == 0) { - return Status(Status::NotOK, "replication doesn't supports unix socket."); + return Status(Status::NotOK, "replication doesn't supports unix socket"); } if (db_dir.empty()) db_dir = dir + "/db"; if (backup_dir.empty()) backup_dir = dir + "/backup";