Fix server cannot start without the configuration file (#1804)

Co-authored-by: Twice <[email protected]>

src/config/config.cc

@@ -844,7 +844,7 @@ Status Config::Set(Server *svr, std::string key, const std::string &value) {
 }
 
 Status Config::Rewrite(const std::map<std::string, std::string> &tokens) {
-  if (path_.empty()) {
+  if (!HasConfigFile()) {
     return {Status::NotOK, "the server is running without a config file"};
   }
 

src/config/config.h

@@ -222,6 +222,7 @@ struct Config {
   void SetMaster(const std::string &host, uint32_t port);
   void ClearMaster();
   bool IsSlave() const { return !master_host.empty(); }
+  bool HasConfigFile() const { return !path_.empty(); }
 
  private:
   std::string path_;

src/server/namespace.cc

@@ -31,6 +31,8 @@ constexpr const char* kErrClusterModeEnabled = "forbidden to add namespace when
 constexpr const char* kErrDeleteDefaultNamespace = "forbidden to delete the default namespace";
 constexpr const char* kErrAddDefaultNamespace = "forbidden to add the default namespace";
 constexpr const char* kErrInvalidToken = "the token is duplicated with requirepass or masterauth";
+constexpr const char* kErrCantModifyNamespace =
+    "modify namespace requires the server is running with a configuration file or enabled namespace replication";
 
 Status IsNamespaceLegal(const std::string& ns) {
   if (ns.size() > UINT8_MAX) {
@@ -43,6 +45,12 @@ Status IsNamespaceLegal(const std::string& ns) {
   return Status::OK();
 }
 
+bool Namespace::IsAllowModify() const {
+  auto config = storage_->GetConfig();
+
+  return config->HasConfigFile() || config->repl_namespace_enabled;
+}
+
 Status Namespace::LoadAndRewrite() {
   auto config = storage_->GetConfig();
   // Load from the configuration file first
@@ -100,6 +108,9 @@ Status Namespace::Set(const std::string& ns, const std::string& token) {
   if (config->cluster_enabled) {
     return {Status::NotOK, kErrClusterModeEnabled};
   }
+  if (!IsAllowModify()) {
+    return {Status::NotOK, kErrCantModifyNamespace};
+  }
   if (ns == kDefaultNamespace) {
     return {Status::NotOK, kErrAddDefaultNamespace};
   }
@@ -142,6 +153,9 @@ Status Namespace::Del(const std::string& ns) {
   if (ns == kDefaultNamespace) {
     return {Status::NotOK, kErrDeleteDefaultNamespace};
   }
+  if (!IsAllowModify()) {
+    return {Status::NotOK, kErrCantModifyNamespace};
+  }
 
   for (const auto& iter : tokens_) {
     if (iter.second == ns) {
@@ -159,9 +173,12 @@ Status Namespace::Del(const std::string& ns) {
 
 Status Namespace::Rewrite() {
   auto config = storage_->GetConfig();
-  auto s = config->Rewrite(tokens_);
-  if (!s.IsOK()) {
-    return s;
+  // Rewrite the configuration file only if it's running with the configuration file
+  if (config->HasConfigFile()) {
+    auto s = config->Rewrite(tokens_);
+    if (!s.IsOK()) {
+      return s;
+    }
   }
 
   // Don't propagate write to DB if its role is slave to prevent from

src/server/namespace.h

@@ -42,6 +42,7 @@ class Namespace {
   Status Del(const std::string &ns);
   const std::map<std::string, std::string> &List() const { return tokens_; }
   Status Rewrite();
+  bool IsAllowModify() const;
 
  private:
   engine::Storage *storage_;

tests/gocase/integration/cli_options/cli_options_test.go

@@ -28,7 +28,7 @@ import (
 )
 
 func TestCLIOptions(t *testing.T) {
-	srv := util.StartServerWithCLIOptions(t, map[string]string{}, []string{"--maxclients", "23333"})
+	srv := util.StartServerWithCLIOptions(t, true, map[string]string{}, []string{"--maxclients", "23333"})
 	defer srv.Close()
 
 	ctx := context.Background()

tests/gocase/unit/config/config_test.go

@@ -129,3 +129,15 @@ func TestConfigSetCompression(t *testing.T) {
 	}
 	require.ErrorContains(t, rdb.ConfigSet(ctx, configKey, "unsupported").Err(), "invalid enum option")
 }
+
+func TestStartWithoutConfigurationFile(t *testing.T) {
+	srv := util.StartServerWithCLIOptions(t, false, map[string]string{}, []string{})
+	defer srv.Close()
+
+	ctx := context.Background()
+	rdb := srv.NewClient()
+	defer func() { require.NoError(t, rdb.Close()) }()
+
+	require.NoError(t, rdb.Do(ctx, "SET", "foo", "bar").Err())
+	require.Equal(t, "bar", rdb.Do(ctx, "GET", "foo").Val())
+}

tests/gocase/unit/namespace/namespace_test.go

@@ -241,3 +241,33 @@ func TestNamespaceReplicate(t *testing.T) {
 		util.ErrorRegexp(t, masterRdb.ConfigSet(ctx, "repl-namespace-enabled", "no").Err(), ".*cannot switch off repl_namespace_enabled when namespaces exist in db.*")
 	})
 }
+
+func TestNamespaceRewrite(t *testing.T) {
+	password := "pwd"
+	srv := util.StartServerWithCLIOptions(t, false, map[string]string{
+		"requirepass": password,
+	}, []string{})
+	defer srv.Close()
+
+	rdb := srv.NewClientWithOption(&redis.Options{
+		Password: password,
+	})
+	defer func() { require.NoError(t, rdb.Close()) }()
+	ctx := context.Background()
+
+	t.Run("Cannot modify namespace if running without the configuration", func(t *testing.T) {
+		errMsg := ".*ERR modify namespace requires the server is running with a configuration file or enabled namespace replication.*"
+		r := rdb.Do(ctx, "NAMESPACE", "ADD", "ns1", "token1")
+		util.ErrorRegexp(t, r.Err(), errMsg)
+		r = rdb.Do(ctx, "NAMESPACE", "SET", "ns1", "token1")
+		util.ErrorRegexp(t, r.Err(), errMsg)
+		r = rdb.Do(ctx, "NAMESPACE", "DEL", "ns1")
+		util.ErrorRegexp(t, r.Err(), errMsg)
+
+		// Good to go after enabling namespace replication
+		require.NoError(t, rdb.ConfigSet(ctx, "repl-namespace-enabled", "yes").Err())
+		require.NoError(t, rdb.Do(ctx, "NAMESPACE", "ADD", "ns1", "token1").Err())
+		require.NoError(t, rdb.Do(ctx, "NAMESPACE", "SET", "ns1", "token2").Err())
+		require.NoError(t, rdb.Do(ctx, "NAMESPACE", "DEL", "ns1").Err())
+	})
+}

tests/gocase/util/server.go

@@ -194,10 +194,16 @@ func StartTLSServer(t testing.TB, configs map[string]string) *KvrocksServer {
 }
 
 func StartServer(t testing.TB, configs map[string]string) *KvrocksServer {
-	return StartServerWithCLIOptions(t, configs, []string{})
+	return StartServerWithCLIOptions(t, true, configs, []string{})
 }
 
-func StartServerWithCLIOptions(t testing.TB, configs map[string]string, options []string) *KvrocksServer {
+func StartServerWithCLIOptions(
+	t testing.TB,
+	withConfigFile bool,
+	configs map[string]string,
+	options []string,
+) *KvrocksServer {
+
 	b := *binPath
 	require.NotEmpty(t, b, "please set the binary path by `-binPath`")
 	cmd := exec.Command(b)
@@ -215,16 +221,21 @@ func StartServerWithCLIOptions(t testing.TB, configs map[string]string, options
 	require.NoError(t, err)
 	configs["dir"] = dir
 
-	f, err := os.Create(filepath.Join(dir, "kvrocks.conf"))
-	require.NoError(t, err)
-	defer func() { require.NoError(t, f.Close()) }()
-
-	for k := range configs {
-		_, err := f.WriteString(fmt.Sprintf("%s %s\n", k, configs[k]))
+	if withConfigFile {
+		f, err := os.Create(filepath.Join(dir, "kvrocks.conf"))
 		require.NoError(t, err)
-	}
+		defer func() { require.NoError(t, f.Close()) }()
 
-	cmd.Args = append(cmd.Args, "-c", f.Name())
+		for k := range configs {
+			_, err := f.WriteString(fmt.Sprintf("%s %s\n", k, configs[k]))
+			require.NoError(t, err)
+		}
+		cmd.Args = append(cmd.Args, "-c", f.Name())
+	} else {
+		for k := range configs {
+			cmd.Args = append(cmd.Args, fmt.Sprintf("--%s", k), configs[k])
+		}
+	}
 	cmd.Args = append(cmd.Args, options...)
 
 	stdout, err := os.Create(filepath.Join(dir, "stdout"))