diff --git a/src/config/config.cc b/src/config/config.cc index ad922dd6232..d3549e59636 100644 --- a/src/config/config.cc +++ b/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 &tokens) { - if (path_.empty()) { + if (!HasConfigFile()) { return {Status::NotOK, "the server is running without a config file"}; } diff --git a/src/config/config.h b/src/config/config.h index d79d1527d61..d540906ff9d 100644 --- a/src/config/config.h +++ b/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_; diff --git a/src/server/namespace.cc b/src/server/namespace.cc index a0e8d05abf1..01a44dcfe40 100644 --- a/src/server/namespace.cc +++ b/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 diff --git a/src/server/namespace.h b/src/server/namespace.h index cccf46e024f..943a3ecead3 100644 --- a/src/server/namespace.h +++ b/src/server/namespace.h @@ -42,6 +42,7 @@ class Namespace { Status Del(const std::string &ns); const std::map &List() const { return tokens_; } Status Rewrite(); + bool IsAllowModify() const; private: engine::Storage *storage_; diff --git a/tests/gocase/integration/cli_options/cli_options_test.go b/tests/gocase/integration/cli_options/cli_options_test.go index 767679dd6a1..43f3cf770fa 100644 --- a/tests/gocase/integration/cli_options/cli_options_test.go +++ b/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() diff --git a/tests/gocase/unit/config/config_test.go b/tests/gocase/unit/config/config_test.go index 2bca988af60..c6c9b682bfb 100644 --- a/tests/gocase/unit/config/config_test.go +++ b/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()) +} diff --git a/tests/gocase/unit/namespace/namespace_test.go b/tests/gocase/unit/namespace/namespace_test.go index bc04a7c3092..1b80e5b94b0 100644 --- a/tests/gocase/unit/namespace/namespace_test.go +++ b/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()) + }) +} diff --git a/tests/gocase/util/server.go b/tests/gocase/util/server.go index 5d190750b7b..a3f4314e342 100644 --- a/tests/gocase/util/server.go +++ b/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"))