You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/09/03 18:17:10 UTC
[GitHub] [incubator-kvrocks] torwig opened a new pull request, #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
torwig opened a new pull request, #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809
This allows Kvrocks to listen to only the Unix socket
Close: #805
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962900152
##########
src/config.cc:
##########
@@ -539,6 +538,16 @@ 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() && 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 "
+ "wasn't specified via configuration file.");
+ }
+ if (master_port != 0 && binds.size() == 0) {
+ return Status(Status::NotOK, "replication doesn't supports unix socket.");
Review Comment:
```suggestion
return Status(Status::NotOK, "replication doesn't supports unix socket");
```
##########
src/config.cc:
##########
@@ -539,6 +538,16 @@ 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() && 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 "
+ "wasn't specified via configuration file.");
Review Comment:
```suggestion
"wasn't specified via configuration file");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962553989
##########
src/config.cc:
##########
@@ -88,14 +88,13 @@ Config::Config() {
bool readonly;
std::unique_ptr<ConfigField> 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[] = {
{"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, kDefaultPort, 1, 65535)},
Review Comment:
```suggestion
{"port", true, new IntField(&port, kDefaultPort, 0, 65535)},
```
typo?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962634492
##########
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";
Review Comment:
thanks @torwig i think old comment is wrong since default bind address is 127.0.0.1, also ping @git-hulk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962500028
##########
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";
Review Comment:
i think default bind address should be "127.0.0.1" which is safer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962558615
##########
src/config.cc:
##########
@@ -88,14 +88,13 @@ Config::Config() {
bool readonly;
std::unique_ptr<ConfigField> 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[] = {
{"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, kDefaultPort, 1, 65535)},
Review Comment:
Oh I got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962515756
##########
src/config.cc:
##########
@@ -539,6 +538,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;
+ }
Review Comment:
Let me see..
How about
```
{"port", true, new IntField(&port, 0, 0, 65535)},
```
rather than
```
{"port", true, new IntField(&port, 6666, 0, 65535)},
```
?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962893068
##########
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";
Review Comment:
Yes, the default bind address will be better since it can prevent users from starting with external IPs and getting security attacks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962513221
##########
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";
Review Comment:
@ShooterIT Ok. However, the comment in config file states that:
`By default kvrocks listens for connections from all the network interfaces available on the server.`
Should I change the related comment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962522985
##########
src/config.cc:
##########
@@ -539,6 +538,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;
+ }
Review Comment:
Yes, it may work. I'll check and add changes if everything is OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962552190
##########
src/config.cc:
##########
@@ -539,6 +538,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;
+ }
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT merged pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
ShooterIT merged PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962515756
##########
src/config.cc:
##########
@@ -539,6 +538,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;
+ }
Review Comment:
Let me see..
How about
```
{"port", true, new IntField(&port, 6666, 0, 65535)},
```
rather than
```
{"port", true, new IntField(&port, 0, 0, 65535)},
```
?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962513038
##########
src/config.cc:
##########
@@ -539,6 +538,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;
+ }
Review Comment:
I think if in current config, `port == 0` and `unixsocket` is empty, kvrocks would be better to report an error and then terminate, rather than assigning a default port.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962514726
##########
src/config.cc:
##########
@@ -539,6 +538,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;
+ }
Review Comment:
Yes, that was my first intention. But currently, if the config is empty (I mean absolutely empty), kvrocks will start with defaults showing just the warning. If I report an error if port is 0 and unixsocket is empty, the behavior will be changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962601407
##########
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";
Review Comment:
@ShooterIT I changed the default bind address from `0.0.0.0` to `127.0.0.1` and edited the comment in `kvrocks.conf`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#discussion_r962556848
##########
src/config.cc:
##########
@@ -88,14 +88,13 @@ Config::Config() {
bool readonly;
std::unique_ptr<ConfigField> 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[] = {
{"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, kDefaultPort, 1, 65535)},
Review Comment:
Now `min` parameter should be `1`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] aleksraiden commented on pull request #809: If unix socket is specified, don't listen default TCP if addr:port wasn't set
Posted by GitBox <gi...@apache.org>.
aleksraiden commented on PR #809:
URL: https://github.com/apache/incubator-kvrocks/pull/809#issuecomment-1236307850
Awesome, @torwig, lot of thanks! Please, review @PragmaTwice @ShooterIT @git-hulk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org