[Store] Structure RedisConnectionConfig environment settings

mooncake-common/include/environment_variables.h

@@ -157,6 +157,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);
+};
+
 #undef MC_DEFINE_ENV_VAR
 
 }  // namespace mooncake

mooncake-store/src/CMakeLists.txt

@@ -16,6 +16,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

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, ErrorCode>
+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<int>(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

mooncake-store/src/config/redis_connection_config.h

@@ -0,0 +1,19 @@
+#pragma once
+
+#include <string>
+
+#include <ylt/util/tl/expected.hpp>
+
+#include "types.h"
+
+namespace mooncake {
+
+struct RedisConnectionConfig {
+    int db_index = 0;
+    std::string username;
+    std::string password;
+
+    static tl::expected<RedisConnectionConfig, ErrorCode> FromEnvironment();
+};
+
+}  // namespace mooncake

mooncake-store/src/ha/common/redis/redis_connection.cpp

@@ -2,10 +2,10 @@
 
 #include <algorithm>
 #include <chrono>
-#include <cstdlib>
-#include <cstring>
 #include <exception>
 
+#include "config/redis_connection_config.h"
+
 #ifdef STORE_USE_REDIS
 #include <hiredis/hiredis.h>
 #endif
@@ -74,11 +74,11 @@ tl::expected<int, ErrorCode> ParsePositiveInt(std::string_view text,
 }
 
 tl::expected<int, ErrorCode> 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<RedisEndpoint, ErrorCode> ParseRedisEndpoint(
@@ -174,9 +174,9 @@ tl::expected<RedisContextPtr, ErrorCode> 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<RedisContextPtr, ErrorCode> 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<redisReply*>(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<redisReply*>(redisCommand(
-                context.get(), "AUTH %b", password, std::strlen(password))));
+            reply.reset(static_cast<redisReply*>(
+                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<redisReply*>(
-            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);
         }

mooncake-store/tests/CMakeLists.txt

@@ -258,6 +258,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_storage_backend_test nvme_kv_storage_backend_test.cpp)
 add_store_test(storage_backend_test storage_backend_test.cpp)
 add_store_test(file_per_key_config_test file_per_key_config_test.cpp)

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 <gtest/gtest.h>
+
+#include <array>
+#include <cstdlib>
+#include <mutex>
+#include <optional>
+#include <string>
+
+namespace mooncake::test {
+namespace {
+
+std::mutex environment_mutex;
+
+class RedisConnectionConfigTest : public ::testing::Test {
+   protected:
+    void SetUp() override {
+        environment_lock_ = std::unique_lock<std::mutex>(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<const char*, 3> kVariables = {
+        "MC_REDIS_DB_INDEX", "MC_REDIS_USERNAME", "MC_REDIS_PASSWORD"};
+    std::array<std::optional<std::string>, kVariables.size()> original_;
+    std::unique_lock<std::mutex> 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