feat: Add JSON Schema + remove redundant CA file check #137
Conversation
Remove redundant CA cert check
|
Warning Rate limit exceeded@nickdnk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes include modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
schema.json (3)
2-2: Consider using versioned URLs for schema $id.The schema $id points to the master branch, which could lead to breaking changes if the schema evolves. Consider using a tagged/versioned URL (e.g., v1.0.0) to ensure schema stability.
- "$id": "https://raw.githubusercontent.com/roadrunner-server/grpc/refs/heads/master/schema.json", + "$id": "https://raw.githubusercontent.com/roadrunner-server/grpc/v1.0.0/schema.json",
64-64: Remove leading space in description.There's an extra space at the start of the MaxConnectionIdle description.
- "description": " MaxConnectionIdle is a duration for the amount of time after which an idle connection would be closed by sending a GoAway. Idle duration is defined by the most recent time the number of outstanding RPCs became zero or since the connection was established. Defaults to infinite.", + "description": "MaxConnectionIdle is a duration for the amount of time after which an idle connection would be closed by sending a GoAway. Idle duration is defined by the most recent time the number of outstanding RPCs became zero or since the connection was established. Defaults to infinite.",
53-62: Clarify message size unit conversion in descriptions.The descriptions could be more explicit about how the MB value is converted to bytes.
- "description": "Maximum send message size in MB.", + "description": "Maximum send message size in MB (will be converted to bytes, e.g., 50 becomes 50*1024*1024 bytes).", - "description": "Maximum receive message size in MB.", + "description": "Maximum receive message size in MB (will be converted to bytes, e.g., 50 becomes 50*1024*1024 bytes).",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- config.go (1 hunks)
- schema.json (1 hunks)
🔇 Additional comments (4)
schema.json (2)
30-52: TLS configuration structure looks good!The schema correctly:
- Makes key and cert required fields
- Keeps root_ca optional
- References appropriate external SSL definitions
94-98: Schema definitions section is well-structured!Good practice to reference the common duration definition from the main RoadRunner schema.
config.go (2)
Line range hint
42-183: Verify TLS configuration in dependent services.While the TLS enablement logic is simplified, the code maintains robust validation in
InitDefaults:
- Key and Cert files are validated for existence
- RootCA is still validated when provided
- Client authentication types are properly handled
Let's check for any services that might be affected:
✅ Verification successful
Let me try a different search strategy to verify the TLS configuration usage.
Based on the search results, I can now provide a final response:
TLS configuration changes are safely contained within the package
The verification shows that:
EnableTLS()is only used internally within the same package (inconfig.goandserver.go)- The TLS struct is not exposed or used outside this package
- The server initialization properly handles TLS credentials using the validated configuration
The simplified TLS enablement logic is safe as:
- All TLS usage is encapsulated within this package
- The server still properly initializes TLS credentials when enabled
- The robust validation in
InitDefaultsensures configuration correctness🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for imports of this package and EnableTLS usage rg -A 3 "github.com/roadrunner-server/grpc.*EnableTLS"Length of output: 56
Script:
#!/bin/bash # Search for EnableTLS method usage rg "EnableTLS\(\)" # Search for TLS configuration usage in general rg "TLS\s+struct|type\s+TLS\s+struct" # Search for potential gRPC server initialization with TLS rg -A 5 "NewServer|grpc.NewServer"Length of output: 813
181-183: LGTM! Simplified TLS enablement logic.The change correctly simplifies the TLS enablement check to only require Key and Cert, which are the essential components for TLS. This is safe because:
- RootCA remains optional and is still validated in InitDefaults if provided
- The change aligns with standard TLS configuration where RootCA is only needed for client authentication
Let's verify if any documentation needs updates:
✅ Verification successful
Let me try a different approach to find documentation files and TLS-related content.
Let me check the test configuration files mentioned in the output to understand the TLS configuration examples.
Verified: TLS configuration change is correct and well-documented
The change to simplify the
EnableTLSmethod is correct and properly documented across the codebase:
- The schema.json explicitly shows that only
keyandcertare required fields, whileroot_cais optional- Test configurations demonstrate both use cases:
- Basic TLS setup with just cert/key in
.rr-grpc-rq-tls.yaml- Advanced setup with root_ca in
.rr-grpc-rq-tls-rootca.yaml- The change aligns with the actual TLS implementation in
server.gowhich handles root_ca separately and optionallyNo documentation updates are needed as the configuration structure is properly defined in schema.json and demonstrated in test configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any documentation mentioning TLS configuration rg -i "tls|certificate|root.?ca" "*.md" "docs/"Length of output: 138
Script:
#!/bin/bash # First, let's find all documentation files in the repository echo "=== Documentation files ===" fd -e md # Then search for TLS configuration in all files echo -e "\n=== TLS configuration mentions ===" rg -i "tls|certificate|root.?ca" # Look for any configuration examples echo -e "\n=== Configuration examples ===" rg -i "example.*config|config.*example"Length of output: 8421
Script:
#!/bin/bash # Check the test configuration files echo "=== TLS Test Configs ===" cat tests/configs/.rr-grpc-rq-tls.yaml echo -e "\n=== TLS with RootCA Test Configs ===" cat tests/configs/.rr-grpc-rq-tls-rootca.yaml # Check schema.json for TLS configuration documentation echo -e "\n=== Schema TLS Definition ===" cat schema.json | jq '.properties.tls'Length of output: 5111
|
LGTM 👍 |
Remove redundant CA cert check
Reason for This PR
Add JSON schema
Description of Changes
Added JSON schema.
Removed Redundant check for Root CA file, as key + cert was already being checked.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
KeyandCert.listen,proto, andtls.Bug Fixes