support tls

[liunyl]

Summary by CodeRabbit

• New Features
  • Added TLS/SSL support with configurable certificate and private key file paths for secure service connections
  • Includes validation to ensure proper TLS configuration during server initialization

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR adds TLS configuration support to the Redis service by introducing CLI flags for enablement and certificate/key paths, implementing TLS accessors in the service header, validating TLS configurations during initialization, and configuring TLS in the brpc server startup sequence.

Changes

Cohort / File(s)	Summary
Submodule Update data_substrate	Submodule pointer updated from 09d3b4d4 to ca396ca3; no functional code changes to public APIs.
TLS Configuration Interface include/redis_service.h	Added three public accessor methods (IsTlsEnabled(), GetTlsCertFile(), GetTlsKeyFile()) and three corresponding private member variables (enable_tls_, tls_cert_file_, tls_key_file_) to expose TLS configuration state.
TLS Configuration & Validation src/redis_service.cpp	Introduced three new CLI flags (enable_tls, tls_cert_file, tls_key_file); added initialization logic to read and validate TLS configuration; included filesystem validation to verify certificate and key file existence and type; aborts initialization if TLS is enabled but files are invalid or missing.
TLS Server Setup src/redis_server.cpp	Integrated TLS initialization into brpc server startup; when TLS is enabled, configures SSL options with certificate and key paths from the service; logs TLS enablement details before server start.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• update submodule #342 — Updates the same data_substrate submodule pointer reference.

Suggested reviewers

• lzxddz
• thweetkomputer

Poem

🐰 A rabbit hops through encrypted fields,
TLS now shields what Redis yields,
Certificates validated, keys secured tight,
Server starts wrapped in cryptographic light!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'support tls' directly and clearly summarizes the main change: adding TLS configuration support across the Redis service implementation, including new accessor methods, member variables, server-side setup, and CLI configuration flags.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch tls

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/redis_server.cpp (1)

24-24: TLS wiring into brpc::ServerOptions looks good; consider hardening logging of key path

The TLS hookup here is clean: you gate on IsTlsEnabled(), use mutable_ssl_options() once, and feed it the validated cert/key paths from RedisServiceImpl. That’s a good separation of concerns and keeps TLS decisions in the service layer.

One thing to reconsider is the INFO log that prints both the certificate and private key paths. While it doesn’t leak key contents, logging the key path can still be undesirable from a security/posture standpoint (logs are often broadly accessible, shipped to external systems, etc.).

If you want an easy win:

• Keep logging that TLS is enabled.
• Optionally log only the certificate path, or neither path, and avoid logging the private key path.

Example tweak

-        LOG(INFO) << "TLS enabled for brpc server. Certificate: "
-                  << redis_service_ptr->GetTlsCertFile()
-                  << ", Key: " << redis_service_ptr->GetTlsKeyFile();
+        LOG(INFO) << "TLS enabled for brpc server. Certificate: "
+                  << redis_service_ptr->GetTlsCertFile();
+        // Avoid logging the private key path for better security posture.

Also applies to: 509-525

src/redis_service.cpp (1)

41-41: TLS configuration parsing and validation are solid; a couple of minor polish opportunities

The overall TLS config flow here is well-structured:

• Flags enable_tls, tls_cert_file, tls_key_file follow the existing “CLI overrides config” pattern via CheckCommandLineFlagIsDefault.
• enable_tls_ is latched once during Init(); no later mutation, which is good for thread-safety.
• When TLS is enabled you correctly require both cert and key paths, check existence, and ensure they are regular files before allowing startup to proceed.

A few optional refinements you might consider:

1. Factor repeated filesystem checks into a helper

   The exists + is_regular_file pattern is repeated for cert and key; a tiny helper would reduce duplication and keep error-handling consistent:

   • Takes a path and a human-readable label ("certificate", "key").
   • Runs exists and is_regular_file with its own std::error_code.
   • Logs both the high-level error and, when non-zero, the error_code and message().

2. Log filesystem errors for is_regular_file as well

   Right now you log error_code only for the exists failures. If is_regular_file fails because of permissions or some other filesystem issue, error_code is silently ignored; logging it there as well would simplify debugging misconfigurations.

3. Be deliberate about what you expose via logs

   The error logs include full paths to the cert and key. That’s often acceptable, but if you decide in redis_server.cpp to stop logging the private key path on success, you may want to mirror that thinking here (e.g., still log full path on hard failure, or only log the key’s basename).

None of these are correctness blockers—the current implementation should behave as intended in normal configurations.

Also applies to: 138-140, 449-518

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b615e13 and 2ee76c9.

📒 Files selected for processing (4)

• data_substrate
• include/redis_service.h
• src/redis_server.cpp
• src/redis_service.cpp

🧰 Additional context used

🧬 Code graph analysis (1)

src/redis_service.cpp (2)

src/vector/vector_type.cpp (2)

• CheckCommandLineFlagIsDefault (37-49)
• CheckCommandLineFlagIsDefault (37-37)

src/redis_server.cpp (1)

• config_reader (442-442)

🔇 Additional comments (2)

data_substrate (1)

1-1: Submodule pointer provided; actual TLS implementation code not visible for review.

Only the submodule commit reference has been provided. The enriched summary references TLS-related changes in RedisServiceImpl, Redis server initialization, and service initialization (CLI flags, accessors, validation), but those source files are not included in this review.

To perform a comprehensive code review of the TLS support (including configuration accessors, TLS validation logic, brpc SSL options setup, and CLI flag handling), please provide the actual modified source files from the data_substrate submodule or the parent repository that contain:

• RedisServiceImpl (TLS accessors and member variables)
• Redis server initialization (TLS/SSL configuration in brpc startup)
• Redis service initialization (CLI flag definitions and TLS validation)

include/redis_service.h (1)

221-225: TLS accessor surface and state layout look appropriate

The TLS additions here integrate cleanly with the rest of RedisServiceImpl:

• enable_tls_ and the two path strings are private and only exposed via narrow, const accessors.
• The accessors are trivially inline and side-effect free, which is ideal for use from the server startup path.
• Given that these members are initialized once during Init() and not mutated afterward, exposing them as simple const std::string& getters is safe from a concurrency standpoint in this codebase’s initialization model.

No changes needed from my side on this header; it’s a good, minimal interface for TLS configuration.

Also applies to: 568-572