Allow health check endpoints to bypass authentication #6931
Conversation
📝 WalkthroughWalkthroughThe Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Note ⚡️ Unit Test Generation - BetaCodeRabbit's unit test generation is now available in Beta! Automatically generate comprehensive unit tests for your code changes, ensuring better test coverage and catching edge cases you might miss. Our AI analyzes your code structure and creates tests that follow best practices and your project's testing patterns. Learn more here, or just try it under ✨ Finishing Touches. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/tonic/auth.rs (2)
22-33: Authentication bypass implementation looks secure and correct.The implementation properly restricts the bypass to specific health check endpoints and follows the same extension pattern as authenticated requests. The full access grant is appropriate for health monitoring operations.
Consider adding audit logging for bypassed requests to maintain security visibility:
async fn check(auth_keys: Arc<AuthKeys>, mut req: Request) -> Result<Request, Status> { // Allow health check endpoints to bypass authentication let path = req.uri().path(); if path == "/qdrant.Qdrant/HealthCheck" || path == "/grpc.health.v1.Health/Check" { + log::debug!("Bypassing authentication for health check endpoint: {}", path); // Set default full access for health check endpoints let access = Access::full("Health check endpoints have full access without authentication");
24-24: Consider making health check paths configurable.The hardcoded endpoint paths work well for the current use case. For better maintainability, consider making these paths configurable through environment variables or configuration files if endpoint paths might change in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/tonic/auth.rs(1 hunks)
🔇 Additional comments (1)
src/tonic/auth.rs (1)
26-27: Health check endpoints bypass authentication as designedVerified that in
src/tonic/auth.rsthe two gRPC paths (/qdrant.Qdrant/HealthCheckand/grpc.health.v1.Health/Check) are explicitly grantedAccess::full, and the corresponding handlers insrc/tonic/mod.rsonly return version info or serving status without touching protected resources. No RBAC restrictions are needed for these endpoints.
|
Thanks, @generall. Any idea when this will be merged / released? |
1.15.1 |
Summary
This PR removes the authentication requirement for gRPC health check endpoints, allowing them to be accessed without API keys or JWT tokens. This change enables proper health monitoring and load balancer checks in production environments where authentication credentials may not be available or appropriate (we're using ALB's in AWS and can't really add keys)
Changes Made
Modified Files
src/tonic/auth.rs: Updated the authentication middleware to bypass validation for health check endpointsImplementation Details
check()function to identify health check requestsauth_keys.validate_request()flowAccessandInferenceTokenextensionsAffected Endpoints
The following gRPC endpoints now work without authentication:
/qdrant.Qdrant/HealthCheck/grpc.health.v1.Health/CheckTesting
To test these changes: