feat(server/v2): add swagger server component #23486
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive enhancement to the Cosmos SDK's server/v2 component by adding default Swagger support. The changes include creating a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
Documentation and Community
|
github-actions
bot
added
C:Confix
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
server/v2/api/swagger/handler.go (1)
8-10: Consider exporting the SwaggerHandler structThe handler struct is unexported but needs to be instantiated by the Server. Consider exporting it if it's meant to be used by other packages.
-type swaggerHandler struct { +type SwaggerHandler struct { swaggerFS fs.FS }server/v2/api/swagger/config.go (1)
29-34: Fix incorrect comment for Disable functionThe comment mentions GRPC server but this is for the Swagger UI server.
-// Disable the grpc server by default (default enabled). +// Disable disables the Swagger UI server. func Disable() CfgOption {server/v2/api/swagger/server.go (1)
88-100: Improve error handling and context usage in Start methodThe Start method:
- Doesn't use the provided context for cancellation
- Could improve error wrapping
func (s *Server[T]) Start(ctx context.Context) error { if !s.config.Enable { s.logger.Info(fmt.Sprintf("%s server is disabled via config", s.Name())) return nil } + errCh := make(chan error, 1) s.logger.Info("starting swagger server...", "address", s.config.Address) - if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed { - return fmt.Errorf("failed to start swagger server: %w", err) - } + go func() { + if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed { + errCh <- fmt.Errorf("failed to start swagger server: %w", err) + } + close(errCh) + }() + + select { + case err := <-errCh: + return err + case <-ctx.Done(): + return s.Stop(ctx) + }client/docs/swagger-ui/index.html (1)
9-10: Consider making the server URL configurable.The server URL is hardcoded to
http://localhost:1317. This might not be suitable for production environments or different deployment scenarios.Consider making it configurable through environment variables or a configuration file:
- <rapi-doc spec-url="swagger.yaml" server-url="http://localhost:1317"> + <rapi-doc spec-url="swagger.yaml" server-url="${API_SERVER_URL:-http://localhost:1317}">
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
client/docs/config.json(1 hunks)client/docs/embed.go(1 hunks)client/docs/swagger-ui/index.html(1 hunks)server/v2/CHANGELOG.md(1 hunks)server/v2/api/grpcgateway/handler.go(2 hunks)server/v2/api/grpcgateway/server.go(1 hunks)server/v2/api/swagger/config.go(1 hunks)server/v2/api/swagger/doc.go(1 hunks)server/v2/api/swagger/handler.go(1 hunks)server/v2/api/swagger/server.go(1 hunks)server/v2/api/telemetry/server.go(1 hunks)simapp/v2/simdv2/cmd/commands.go(5 hunks)tools/confix/data/v2-app.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/v2/api/swagger/doc.go
🧰 Additional context used
📓 Path-based instructions (9)
server/v2/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
client/docs/embed.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpcgateway/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/swagger/handler.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/swagger/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/telemetry/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/swagger/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpcgateway/handler.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 checkov (3.2.334)
client/docs/config.json
[HIGH] 1-180: Ensure that security requirement defined in securityDefinitions - version 2.0 files
(CKV_OPENAPI_6)
[HIGH] 1-180: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[HIGH] 1-180: Ensure that securityDefinitions is defined and not empty - version 2.0 files
(CKV_OPENAPI_1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (14)
server/v2/api/swagger/handler.go (1)
14-15: Review CORS configuration for securityThe current CORS configuration is very permissive. Consider:
- Restricting allowed origins to specific domains
- Limiting allowed methods to only those needed
- Adding additional security headers (Content-Security-Policy, X-Frame-Options)
server/v2/api/telemetry/server.go (1)
32-32: LGTM! Parameter reordering improves consistency.The reordering of parameters to place
loggerandenableTelemetrybeforecfgfollows a good practice where context-like parameters precede configuration parameters.server/v2/api/grpcgateway/server.go (1)
79-79: LGTM! Simplified function call by removing redundant logger parameter.The removal of the logger parameter from
mountHTTPRoutesis appropriate as logging is handled at the server level.simapp/v2/simdv2/cmd/commands.go (3)
128-131: LGTM! Updated telemetry server initialization.The parameter reordering matches the updated function signature in the telemetry package.
133-136: LGTM! Well-structured swagger server initialization.The swagger server is properly initialized with appropriate error handling and necessary dependencies (logger, filesystem, and config).
90-90: LGTM! Proper integration of swagger server component.The swagger server is correctly added to both the CLI skeleton and full app configurations.
Also applies to: 179-179
server/v2/api/grpcgateway/handler.go (2)
Line range hint
49-58: LGTM! Simplified HTTP route mounting.The removal of the logger parameter streamlines the function while maintaining its core functionality and error handling.
63-63: LGTM! Consistent parameter cleanup.The removal of the logger parameter from
registerMethodsaligns with the changes inmountHTTPRoutes.client/docs/swagger-ui/index.html (1)
4-5: LGTM!The meta tag update follows HTML5 standards and the UTF-8 comment is helpful for maintainers.
server/v2/CHANGELOG.md (1)
25-26: LGTM!The changelog entry follows the guidelines and properly references the PR.
client/docs/config.json (2)
171-177: LGTM!The operation ID renames for the protocolpool service follow the established pattern and maintain consistency with other services.
Line range hint
1-180: Add security definitions to the OpenAPI specification.The OpenAPI specification is missing security definitions and requirements. This could expose your API to security risks.
Please add the following sections to ensure proper security:
{ "swagger": "2.0", "info": { "title": "Cosmos SDK - gRPC Gateway docs", "description": "A REST interface for state queries.", "version": "1.0.0" }, + "securityDefinitions": { + "ApiKeyAuth": { + "type": "apiKey", + "in": "header", + "name": "X-API-Key" + } + }, + "security": [ + { + "ApiKeyAuth": [] + } + ], "apis": [🧰 Tools
🪛 checkov (3.2.334)
[HIGH] 1-180: Ensure that security requirement defined in securityDefinitions - version 2.0 files
(CKV_OPENAPI_6)
[HIGH] 1-180: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[HIGH] 1-180: Ensure that securityDefinitions is defined and not empty - version 2.0 files
(CKV_OPENAPI_1)
tools/confix/data/v2-app.toml (2)
2-176: LGTM!The configuration parameters are well-documented with clear descriptions and sensible default values.
135-141: Review security implications of enabled Swagger UI in production.While enabling Swagger UI by default is helpful for development, consider the security implications for production deployments.
Consider adding a warning in the configuration comments about production usage:
[swagger] -# Enable enables/disables the Swagger UI server +# Enable enables/disables the Swagger UI server. +# WARNING: It's recommended to disable this in production environments unless properly secured. enable = true # Address defines the server address to bind to address = 'localhost:8090'✅ Verification successful
Add security warning for Swagger UI configuration
The suggestion to add a warning comment about production usage is appropriate. While the default localhost binding provides some security, an explicit warning helps developers make informed decisions about production deployments.
- The current configuration binds to localhost:8090, which already limits access to local connections
- Adding the warning comment reinforces security consciousness without affecting functionality
- This follows security best practices of providing clear guidance for production environments
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for similar configuration files and Swagger-related content echo "=== Searching for similar configuration files ===" fd -e toml -x cat {} echo -e "\n=== Searching for Swagger-related documentation ===" fd -e md -e txt README -x cat {} echo -e "\n=== Searching for Swagger implementation ===" rg -i "swagger" --type go -A 5 -B 5Length of output: 67187
| func (s *Server[T]) Config() any { | ||
| if s.config == nil || s.config.Address == "" { | ||
| cfg := DefaultConfig() | ||
| // overwrite the default config with the provided options | ||
| for _, opt := range s.cfgOptions { | ||
| opt(cfg) | ||
| } | ||
|
|
||
| return cfg | ||
| } | ||
|
|
||
| return s.config | ||
| } |
There was a problem hiding this comment.
Fix potential race condition in Config method
The Config method has a potential race condition when checking and initializing the config. Consider using sync.Once or initializing config in New.
| s.server = &http.Server{ | ||
| Addr: s.config.Address, | ||
| Handler: mux, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout configurations for HTTP server
The HTTP server should have reasonable timeouts configured to prevent potential DoS attacks.
s.server = &http.Server{
Addr: s.config.Address,
Handler: mux,
+ ReadTimeout: 15 * time.Second,
+ WriteTimeout: 15 * time.Second,
+ IdleTimeout: 60 * time.Second,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s.server = &http.Server{ | |
| Addr: s.config.Address, | |
| Handler: mux, | |
| } | |
| s.server = &http.Server{ | |
| Addr: s.config.Address, | |
| Handler: mux, | |
| ReadTimeout: 15 * time.Second, | |
| WriteTimeout: 15 * time.Second, | |
| IdleTimeout: 60 * time.Second, | |
| } |
julienrbrt
added
the
backport/v0.52.x
|
Adding the backport label for the changes in confix and in client. |
(cherry picked from commit 0cc73ba) # Conflicts: # server/v2/CHANGELOG.md # server/v2/api/grpcgateway/handler.go # server/v2/api/grpcgateway/server.go # server/v2/api/telemetry/server.go # simapp/v2/simdv2/cmd/commands.go
Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #23020
Replaces #23092
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates