diff --git a/mooncake-common/include/environment_variables.h b/mooncake-common/include/environment_variables.h index c1fea161c7..cbc3907e4c 100644 --- a/mooncake-common/include/environment_variables.h +++ b/mooncake-common/include/environment_variables.h @@ -191,6 +191,14 @@ struct NvmeKvConnectorEnvironmentVariables { MC_DEFINE_ENV_VAR(std::string, MOONCAKE_NVME_KV_TRANSPORT); }; +struct RedisConnectionEnvironmentVariables { + // Keep the DB index as a string so an explicitly empty value remains + // distinguishable from a nonempty malformed value. + MC_DEFINE_ENV_VAR(std::string, MC_REDIS_DB_INDEX); + MC_DEFINE_ENV_VAR(std::string, MC_REDIS_USERNAME); + MC_DEFINE_ENV_VAR(std::string, MC_REDIS_PASSWORD); +}; + struct ClientAutoDiscoveryEnvironmentVariables { // Keep the raw strings to preserve std::stoi prefix acceptance and the // distinction between unset and explicitly empty filter values. diff --git a/mooncake-store/src/CMakeLists.txt b/mooncake-store/src/CMakeLists.txt index f57e4040ac..be0b555110 100644 --- a/mooncake-store/src/CMakeLists.txt +++ b/mooncake-store/src/CMakeLists.txt @@ -17,6 +17,7 @@ set(MOONCAKE_STORE_MASTER_SOURCES allocation_strategy.cpp allocator.cpp config/local_file_snapshot_config.cpp + config/redis_connection_config.cpp segment_allocator_registration.cpp client_offboarding.cpp master_service.cpp diff --git a/mooncake-store/src/config/redis_connection_config.cpp b/mooncake-store/src/config/redis_connection_config.cpp new file mode 100644 index 0000000000..245bd9a382 --- /dev/null +++ b/mooncake-store/src/config/redis_connection_config.cpp @@ -0,0 +1,31 @@ +#include "redis_connection_config.h" + +#include "environ.h" +#include "environment_value_parser.h" +#include "environment_variables.h" + +namespace mooncake { + +tl::expected +RedisConnectionConfig::FromEnvironment() { + RedisConnectionConfig config; + const auto raw_db_index = + Environ::Read(RedisConnectionEnvironmentVariables::MC_REDIS_DB_INDEX) + .value_or(""); + if (!raw_db_index.empty()) { + const auto db_index = TryParseEnvironmentValue(raw_db_index); + if (!db_index.has_value() || *db_index < 0 || *db_index > 255) { + return tl::make_unexpected(ErrorCode::INVALID_PARAMS); + } + config.db_index = *db_index; + } + config.username = + Environ::Read(RedisConnectionEnvironmentVariables::MC_REDIS_USERNAME) + .value_or(""); + config.password = + Environ::Read(RedisConnectionEnvironmentVariables::MC_REDIS_PASSWORD) + .value_or(""); + return config; +} + +} // namespace mooncake diff --git a/mooncake-store/src/config/redis_connection_config.h b/mooncake-store/src/config/redis_connection_config.h new file mode 100644 index 0000000000..ec38d7faf0 --- /dev/null +++ b/mooncake-store/src/config/redis_connection_config.h @@ -0,0 +1,19 @@ +#pragma once + +#include + +#include + +#include "types.h" + +namespace mooncake { + +struct RedisConnectionConfig { + int db_index = 0; + std::string username; + std::string password; + + static tl::expected FromEnvironment(); +}; + +} // namespace mooncake diff --git a/mooncake-store/src/ha/common/redis/redis_connection.cpp b/mooncake-store/src/ha/common/redis/redis_connection.cpp index d694cb40df..3edc5c2861 100644 --- a/mooncake-store/src/ha/common/redis/redis_connection.cpp +++ b/mooncake-store/src/ha/common/redis/redis_connection.cpp @@ -2,10 +2,10 @@ #include #include -#include -#include #include +#include "config/redis_connection_config.h" + #ifdef STORE_USE_REDIS #include #endif @@ -74,11 +74,11 @@ tl::expected ParsePositiveInt(std::string_view text, } tl::expected ResolveRedisDbIndex() { - const char* raw_db_index = std::getenv("MC_REDIS_DB_INDEX"); - if (raw_db_index == nullptr || std::strlen(raw_db_index) == 0) { - return 0; + const auto config = RedisConnectionConfig::FromEnvironment(); + if (!config.has_value()) { + return tl::make_unexpected(config.error()); } - return ParsePositiveInt(raw_db_index, 0, 255); + return config->db_index; } tl::expected ParseRedisEndpoint( @@ -174,9 +174,9 @@ tl::expected ConnectRedis( return tl::make_unexpected(endpoint.error()); } - auto db_index = ResolveRedisDbIndex(); - if (!db_index) { - return tl::make_unexpected(db_index.error()); + const auto config = RedisConnectionConfig::FromEnvironment(); + if (!config.has_value()) { + return tl::make_unexpected(config.error()); } timeval connect_timeout{}; @@ -195,26 +195,26 @@ tl::expected ConnectRedis( return tl::make_unexpected(connection_error); } - const char* username = std::getenv("MC_REDIS_USERNAME"); - const char* password = std::getenv("MC_REDIS_PASSWORD"); - if (password != nullptr && std::strlen(password) > 0) { + if (!config->password.empty()) { RedisReplyPtr reply; - if (username != nullptr && std::strlen(username) > 0) { + if (!config->username.empty()) { reply.reset(static_cast(redisCommand( - context.get(), "AUTH %b %b", username, std::strlen(username), - password, std::strlen(password)))); + context.get(), "AUTH %b %b", config->username.data(), + config->username.size(), config->password.data(), + config->password.size()))); } else { - reply.reset(static_cast(redisCommand( - context.get(), "AUTH %b", password, std::strlen(password)))); + reply.reset(static_cast( + redisCommand(context.get(), "AUTH %b", config->password.data(), + config->password.size()))); } if (reply == nullptr || reply->type == REDIS_REPLY_ERROR) { return tl::make_unexpected(connection_error); } } - if (db_index.value() != 0) { + if (config->db_index != 0) { RedisReplyPtr reply(static_cast( - redisCommand(context.get(), "SELECT %d", db_index.value()))); + redisCommand(context.get(), "SELECT %d", config->db_index))); if (reply == nullptr || reply->type == REDIS_REPLY_ERROR) { return tl::make_unexpected(connection_error); } diff --git a/mooncake-store/tests/CMakeLists.txt b/mooncake-store/tests/CMakeLists.txt index 5f667e7e0b..1208f31d55 100644 --- a/mooncake-store/tests/CMakeLists.txt +++ b/mooncake-store/tests/CMakeLists.txt @@ -263,6 +263,7 @@ add_store_test(master_service_test_for_snapshot ha/snapshot/master_service_test_for_snapshot.cpp) add_store_test(non_ha_reconnect_test non_ha_reconnect_test.cpp) add_store_test(nvme_kv_connector_config_test nvme_kv_connector_config_test.cpp) +add_store_test(redis_connection_config_test redis_connection_config_test.cpp) add_store_test(nvme_kv_io_concurrency_config_test nvme_kv_io_concurrency_config_test.cpp) add_store_test(nvme_kv_storage_backend_test nvme_kv_storage_backend_test.cpp) diff --git a/mooncake-store/tests/redis_connection_config_test.cpp b/mooncake-store/tests/redis_connection_config_test.cpp new file mode 100644 index 0000000000..4180044197 --- /dev/null +++ b/mooncake-store/tests/redis_connection_config_test.cpp @@ -0,0 +1,211 @@ +#include "ha/common/redis/redis_connection.h" + +#include "../src/config/redis_connection_config.h" + +#include + +#include +#include +#include +#include +#include + +namespace mooncake::test { +namespace { + +std::mutex environment_mutex; + +class RedisConnectionConfigTest : public ::testing::Test { + protected: + void SetUp() override { + environment_lock_ = std::unique_lock(environment_mutex); + for (size_t i = 0; i < kVariables.size(); ++i) { + if (const char* value = std::getenv(kVariables[i])) { + original_[i] = value; + } + ASSERT_EQ(unsetenv(kVariables[i]), 0); + } + } + + void TearDown() override { + for (size_t i = 0; i < kVariables.size(); ++i) { + if (original_[i].has_value()) { + EXPECT_EQ(setenv(kVariables[i], original_[i]->c_str(), 1), 0); + } else { + EXPECT_EQ(unsetenv(kVariables[i]), 0); + } + } + } + + void SetDbIndex(const char* value) { + ASSERT_EQ(setenv("MC_REDIS_DB_INDEX", value, 1), 0); + } + + private: + inline static constexpr std::array kVariables = { + "MC_REDIS_DB_INDEX", "MC_REDIS_USERNAME", "MC_REDIS_PASSWORD"}; + std::array, kVariables.size()> original_; + std::unique_lock environment_lock_; +}; + +TEST_F(RedisConnectionConfigTest, UnsetAndEmptyDbIndexUseZero) { + auto result = ha::common::redis::ResolveRedisDbIndex(); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, 0); + + SetDbIndex(""); + result = ha::common::redis::ResolveRedisDbIndex(); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, 0); +} + +TEST_F(RedisConnectionConfigTest, AcceptsSupportedDbIndexSyntax) { + struct Case { + const char* value; + int expected; + }; + const Case cases[] = {{"0", 0}, {"255", 255}, {" \t42\r\n", 42}, + {"+17", 17}, {"010", 10}, {"-0", 0}}; + for (const auto& entry : cases) { + SCOPED_TRACE(entry.value); + SetDbIndex(entry.value); + const auto result = ha::common::redis::ResolveRedisDbIndex(); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, entry.expected); + } +} + +TEST_F(RedisConnectionConfigTest, RejectsInvalidDbIndexSilently) { + const char* values[] = {"-1", "256", "999999999999999999999999", "abc"}; + for (const char* value : values) { + SCOPED_TRACE(value); + SetDbIndex(value); + testing::internal::CaptureStderr(); + const auto result = ha::common::redis::ResolveRedisDbIndex(); + const auto diagnostics = testing::internal::GetCapturedStderr(); + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error(), ErrorCode::INVALID_PARAMS); + EXPECT_TRUE(diagnostics.empty()) << diagnostics; + } +} + +TEST_F(RedisConnectionConfigTest, RejectsDbIndexWithNonIntegerSuffix) { + for (const char* value : {"1junk", "1e2", "0x1"}) { + SCOPED_TRACE(value); + SetDbIndex(value); + const auto result = ha::common::redis::ResolveRedisDbIndex(); + EXPECT_FALSE(result.has_value()); + if (result.has_value()) { + continue; + } + EXPECT_EQ(result.error(), ErrorCode::INVALID_PARAMS); + } +} + +TEST_F(RedisConnectionConfigTest, UnsetValuesUseConnectionDefaults) { + const auto config = RedisConnectionConfig::FromEnvironment(); + + ASSERT_TRUE(config.has_value()); + EXPECT_EQ(config->db_index, 0); + EXPECT_TRUE(config->username.empty()); + EXPECT_TRUE(config->password.empty()); +} + +TEST_F(RedisConnectionConfigTest, EmptyCredentialsRemainEmpty) { + ASSERT_EQ(setenv("MC_REDIS_USERNAME", "", 1), 0); + ASSERT_EQ(setenv("MC_REDIS_PASSWORD", "", 1), 0); + + const auto config = RedisConnectionConfig::FromEnvironment(); + + ASSERT_TRUE(config.has_value()); + EXPECT_TRUE(config->username.empty()); + EXPECT_TRUE(config->password.empty()); +} + +TEST_F(RedisConnectionConfigTest, UsernameWithoutPasswordRemainsValid) { + ASSERT_EQ(setenv("MC_REDIS_USERNAME", "unused-user", 1), 0); + + const auto config = RedisConnectionConfig::FromEnvironment(); + + ASSERT_TRUE(config.has_value()); + EXPECT_EQ(config->username, "unused-user"); + EXPECT_TRUE(config->password.empty()); +} + +TEST_F(RedisConnectionConfigTest, PasswordWithoutUsernameRemainsValid) { + ASSERT_EQ(setenv("MC_REDIS_PASSWORD", "secret", 1), 0); + + const auto config = RedisConnectionConfig::FromEnvironment(); + + ASSERT_TRUE(config.has_value()); + EXPECT_TRUE(config->username.empty()); + EXPECT_EQ(config->password, "secret"); +} + +TEST_F(RedisConnectionConfigTest, PreservesCredentialTextExactly) { + ASSERT_EQ(setenv("MC_REDIS_USERNAME", "user name", 1), 0); + ASSERT_EQ(setenv("MC_REDIS_PASSWORD", "p@ss word", 1), 0); + + const auto config = RedisConnectionConfig::FromEnvironment(); + + ASSERT_TRUE(config.has_value()); + EXPECT_EQ(config->username, "user name"); + EXPECT_EQ(config->password, "p@ss word"); +} + +TEST_F(RedisConnectionConfigTest, LoadsIndependentConnectionSettings) { + SetDbIndex("7"); + ASSERT_EQ(setenv("MC_REDIS_USERNAME", "alice", 1), 0); + ASSERT_EQ(setenv("MC_REDIS_PASSWORD", "secret", 1), 0); + + const auto config = RedisConnectionConfig::FromEnvironment(); + + ASSERT_TRUE(config.has_value()); + EXPECT_EQ(config->db_index, 7); + EXPECT_EQ(config->username, "alice"); + EXPECT_EQ(config->password, "secret"); +} + +TEST_F(RedisConnectionConfigTest, NewConfigsReadCurrentEnvironment) { + SetDbIndex("1"); + const auto first = RedisConnectionConfig::FromEnvironment(); + SetDbIndex("2"); + const auto second = RedisConnectionConfig::FromEnvironment(); + + ASSERT_TRUE(first.has_value()); + ASSERT_TRUE(second.has_value()); + EXPECT_EQ(first->db_index, 1); + EXPECT_EQ(second->db_index, 2); +} + +TEST_F(RedisConnectionConfigTest, PublicDbResolverMatchesOwnerConfig) { + const char* values[] = {"", "0", "255", " \t42\r\n", "+17", "010", + "-0", "-1", "256", "abc", "1junk", "0x1"}; + for (const char* value : values) { + SCOPED_TRACE(value); + SetDbIndex(value); + const auto config = RedisConnectionConfig::FromEnvironment(); + const auto resolved = ha::common::redis::ResolveRedisDbIndex(); + EXPECT_EQ(config.has_value(), resolved.has_value()); + if (config.has_value() && resolved.has_value()) { + EXPECT_EQ(config->db_index, *resolved); + } else if (!config.has_value() && !resolved.has_value()) { + EXPECT_EQ(config.error(), resolved.error()); + } + } +} + +#ifdef STORE_USE_REDIS +TEST_F(RedisConnectionConfigTest, + ConnectRedisRejectsMalformedDbBeforeOpeningConnection) { + SetDbIndex("1junk"); + const auto result = ha::common::redis::ConnectRedis( + "redis://127.0.0.1:1", ErrorCode::PERSISTENT_FAIL); + + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error(), ErrorCode::INVALID_PARAMS); +} +#endif + +} // namespace +} // namespace mooncake::test