first attempt for knuu API #601
Conversation
WalkthroughThis pull request introduces a comprehensive API implementation for the Knuu project, establishing a robust backend service with user authentication, test management, and instance handling capabilities. The changes include setting up a Gin-based HTTP server, implementing user registration and login, creating test and instance management services, and configuring database interactions using GORM with PostgreSQL. The implementation provides a structured approach to API development with middleware for authentication and authorization. Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIServer
participant AuthMiddleware
participant UserService
participant DatabaseRepo
Client->>APIServer: Login Request
APIServer->>AuthMiddleware: Validate Credentials
AuthMiddleware->>UserService: Authenticate User
UserService->>DatabaseRepo: Find User
DatabaseRepo-->>UserService: User Details
UserService-->>AuthMiddleware: Authentication Result
AuthMiddleware-->>APIServer: Generate Token
APIServer-->>Client: Return JWT Token
Assessment against linked issues
Poem
✨ 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
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mojtaba-esk
requested review from
tty47,
smuu and
MSevey
and removed request for
tty47 and
smuu
mojtaba-esk
requested review from
a team
and removed request for
aWN4Y25pa2EK
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
🧹 Nitpick comments (18)
internal/api/v1/middleware/auth.go (2)
3-13: Address goimports formatting issue.The pipeline reports that the file is not properly formatted around the import statements. Please run an automatic formatter (like
goimports) to fix any import ordering or spacing issues.
38-42: Avoid re-parsing JWT token for performance and consistency.Currently, the token is parsed here to validate it and then parsed again in
getUserFromToken(). Consider refactoring to parse the token once, share the parsed claims, and avoid the extra overhead.func (a *Auth) AuthMiddleware() gin.HandlerFunc { return func(c *gin.Context) { token := a.getAuthToken(c) - if token == "" || !a.isValidToken(token) { + user, claims, err := a.parseAndValidateToken(token) + if err != nil { c.JSON(http.StatusUnauthorized, gin.H{"error": "Unauthorized"}) c.Abort() return } - user, err := a.getUserFromToken(token) - ... + // user can be built directly from claims. c.Set(UserContextKey, user) c.Next() } }internal/api/v1/api.go (2)
60-63: Consider adding descriptive endpoint paths.While
public.POST(pathsUserLogin, uh.Login)is fine, more descriptive paths (e.g.,/auth/login) may improve API clarity and reduce confusion for consumers.
113-122: Graceful shutdown might be preferable to immediate close.
server.Close()cancels active connections abruptly. Consider usingserver.Shutdown(ctx)to allow in-flight requests to finish cleanly before termination.-func (a *API) Stop(ctx context.Context) error { - return a.server.Close() +func (a *API) Stop(ctx context.Context) error { + return a.server.Shutdown(ctx) }internal/api/v1/services/test.go (2)
12-13: Fix import organization.The lint pipeline indicates a formatting issue. Please run a tool like
goimportsto correctly order and format the imports.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 13-13:
File is not properly formatted (goimports)🪛 GitHub Actions: Lint
[error] 13-13: File is not properly formatted (goimports)
107-108: Make the blocking knuu initialization asynchronous.Currently, the
prepareKnuucall in theCreatemethod blocks while knuu starts up. Consider making this process asynchronous (e.g., via a queue or go routine) to avoid long-running requests.cmd/root/root.go (1)
9-13: Consider enhancing the command description.While the current description is clear, it could be more informative by including details about available subcommands and their purposes.
Consider updating the long description:
- Long: "Knuu CLI provides commands to manage the Knuu API server and its operations.", + Long: `Knuu CLI provides commands to manage the Knuu API server and its operations. + +Available Commands: + api Start and manage the Knuu API server + help Help about any command`,internal/api/v1/paths.go (1)
10-19: Consider adding CRUD endpoints for test management.The test-related paths cover instance management and logs but lack standard CRUD operations for test resources. Consider adding endpoints for:
- Creating a new test (POST
/api/v1/tests)- Updating a test (PUT/PATCH
/api/v1/tests/:scope)- Deleting a test (DELETE
/api/v1/tests/:scope)internal/api/v1/services/errors.go (1)
7-16: Consider adding more error cases for test operations.The error definitions cover basic scenarios but could be expanded to include:
ErrTestDeadlineExceededfor when a test's deadline is reachedErrInvalidLogLevelfor invalid log level valuesErrInvalidTestScopefor malformed scope valuesinternal/database/models/user.go (2)
5-10: Consider using string constants for roles.Using integers for roles makes the database values less readable. Consider using string constants:
-type UserRole int +type UserRole string const ( - RoleUser UserRole = iota - RoleAdmin + RoleUser UserRole = "user" + RoleAdmin UserRole = "admin" )
27-40: Consider using bit flags for access levels.Using sequential integers for access levels makes it harder to combine permissions. Consider using bit flags:
type AccessLevel int const ( - AccessLevelRead AccessLevel = iota + 1 - AccessLevelWrite - AccessLevelAdmin + AccessLevelRead AccessLevel = 1 << iota // 1 + AccessLevelWrite // 2 + AccessLevelAdmin // 4 )This allows for combining permissions using bitwise operations:
// Grant both read and write access permission.AccessLevel = AccessLevelRead | AccessLevelWritepkg/builder/git.go (1)
15-15: Add validation for repository URL.Consider adding validation for the repository URL to ensure it's a valid Git repository URL before processing.
- Repo string `json:"repo"` + Repo string `json:"repo" validate:"required,url"`internal/api/v1/index.go (1)
26-41: Move CSS to a separate file.Inline CSS makes the code harder to maintain. Consider:
- Moving CSS to a separate file.
- Using Go's
embedpackage to include static assets.internal/database/repos/test.go (1)
35-37: Add optimistic locking for concurrent updates.The
Updatemethod should handle concurrent updates using GORM's optimistic locking to prevent lost updates.func (r *TestRepository) Update(ctx context.Context, test *models.Test) error { - return r.db.WithContext(ctx).Model(&models.Test{}).Where(&models.Test{Scope: test.Scope, UserID: test.UserID}).Updates(test).Error + result := r.db.WithContext(ctx).Model(&models.Test{}). + Where(&models.Test{Scope: test.Scope, UserID: test.UserID, Version: test.Version}). + Updates(map[string]interface{}{ + "finished": test.Finished, + "version": test.Version + 1, + }) + if result.Error != nil { + return fmt.Errorf("failed to update test: %w", result.Error) + } + if result.RowsAffected == 0 { + return ErrConcurrentUpdate + } + return nil }internal/api/v1/services/instance.go (1)
10-25: Add validation for critical fields.Consider adding validation for:
- TCP/UDP ports range and uniqueness
- Environment variable names
- Git context fields when repo is specified
Also, consider:
- Adding field descriptions in struct tags or godoc
- Implementing validation for the readonly Hostname field
cmd/api/api.go (1)
52-76: Add environment variable support and validation.Consider the following improvements:
- Add environment variable support for all flags
- Add validation for port range, log level values
- Add support for loading configuration from file
Example implementation:
func NewAPICmd() *cobra.Command { + // Add support for environment variables + viper.AutomaticEnv() + viper.SetEnvPrefix("KNUU") + viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) + apiCmd := &cobra.Command{ Use: apiCmdName, Short: "Start the Knuu API server", Long: "Start the API server to manage tests, tokens, and users.", + PreRunE: func(cmd *cobra.Command, args []string) error { + // Validate port range + port, _ := cmd.Flags().GetInt(flagPort) + if port < 1 || port > 65535 { + return fmt.Errorf("invalid port number: %d", port) + } + return nil + }, RunE: runAPIServer, }Makefile (1)
5-14: Improve build configuration and versioning.Consider the following improvements:
- Add version information during build
- Make log level configurable
- Add cross-compilation support
Example improvements:
BINARY_NAME := knuu +VERSION := $(shell git describe --tags --always --dirty) +LDFLAGS := -X main.version=$(VERSION) build: - go build -o bin/$(BINARY_NAME) -v ./cmd + go build -ldflags "$(LDFLAGS)" -o bin/$(BINARY_NAME) -v ./cmd run: build - ./bin/$(BINARY_NAME) api -l debug + ./bin/$(BINARY_NAME) api -l $(LOG_LEVEL)go.mod (1)
10-10: Remove redundant lib/pq dependency.The
lib/pqpackage is indirectly included viagorm.io/driver/postgresand is officially deprecated in favor ofpgx. Consider removing this direct dependency.- github.com/lib/pq v1.10.9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
.gitignore(1 hunks)Makefile(1 hunks)cmd/api/api.go(1 hunks)cmd/main.go(1 hunks)cmd/root/root.go(1 hunks)go.mod(3 hunks)internal/api/v1/api.go(1 hunks)internal/api/v1/handlers/errors.go(1 hunks)internal/api/v1/handlers/instance.go(1 hunks)internal/api/v1/handlers/test.go(1 hunks)internal/api/v1/handlers/token.go(1 hunks)internal/api/v1/handlers/user.go(1 hunks)internal/api/v1/handlers/utils.go(1 hunks)internal/api/v1/index.go(1 hunks)internal/api/v1/middleware/auth.go(1 hunks)internal/api/v1/paths.go(1 hunks)internal/api/v1/services/errors.go(1 hunks)internal/api/v1/services/instance.go(1 hunks)internal/api/v1/services/test.go(1 hunks)internal/api/v1/services/user.go(1 hunks)internal/database/db.go(1 hunks)internal/database/models/test.go(1 hunks)internal/database/models/token.go(1 hunks)internal/database/models/user.go(1 hunks)internal/database/repos/test.go(1 hunks)internal/database/repos/token.go(1 hunks)internal/database/repos/user.go(1 hunks)pkg/builder/git.go(1 hunks)pkg/k8s/namespace.go(1 hunks)pkg/k8s/namespace_test.go(1 hunks)pkg/k8s/pod.go(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- internal/database/repos/token.go
- internal/database/models/token.go
- internal/api/v1/handlers/token.go
- .gitignore
- pkg/k8s/pod.go
🧰 Additional context used
🪛 GitHub Check: golangci-lint
internal/api/v1/services/user.go
[failure] 5-5:
File is not properly formatted (goimports)
internal/api/v1/handlers/instance.go
[failure] 6-6:
File is not properly formatted (goimports)
internal/api/v1/services/test.go
[failure] 13-13:
File is not properly formatted (goimports)
🪛 GitHub Check: CodeQL
internal/api/v1/handlers/test.go
[failure] 99-99: Uncontrolled data used in path expression
This path depends on a user-provided value.
internal/api/v1/services/test.go
[failure] 200-200: Uncontrolled data used in path expression
This path depends on a user-provided value.
[failure] 294-294: Uncontrolled data used in path expression
This path depends on a user-provided value.
🪛 GitHub Actions: Lint
internal/api/v1/services/test.go
[error] 13-13: File is not properly formatted (goimports)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (./e2e/netshaper, 60m)
- GitHub Check: test (./e2e/basic, 15m)
🔇 Additional comments (18)
pkg/k8s/namespace.go (1)
40-40: LGTM! The error handling change aligns with Kubernetes best practices.The change to ignore "not found" errors during namespace deletion is correct and follows the idempotency principle of DELETE operations in Kubernetes. This is a common pattern in Kubernetes controllers where attempting to delete a non-existent resource is considered successful.
pkg/k8s/namespace_test.go (1)
90-93: LGTM! Test case correctly verifies the updated error handling.The test case has been properly updated to align with the new behavior in
DeleteNamespacewhere deleting a non-existent namespace is considered successful. The test maintains consistency with other test cases and provides good coverage of the error handling change.internal/api/v1/middleware/auth.go (4)
55-70: Role enforcement looks good.The role-based authorization logic here is clear and straightforward. Good job applying a 403 response for insufficient permissions!
72-82: JWT token generation is straightforward.The approach to creating a signed token with user claims and an expiration is generally sound. Good use of a 24-hour duration for validity.
84-107: Validate the numeric claims carefully.You’re relying on casting float64 to uint and models.UserRole. If an unexpected or out-of-range value is supplied, it might introduce subtle errors. Verify that these casts behave as intended for all possible user/role IDs.
122-128: Header-based token extraction is well-implemented.Using the "Bearer" prefix in your Authorization header is standard practice and is clearly handled here. Nice work!
internal/api/v1/api.go (2)
49-53: Defaulting logic is clean and intuitive.Setting defaults for log mode and ensuring
gin.SetModeis a good way to handle different environments. This looks good.
73-86: Protected routes configuration looks correct.Using
auth.AuthMiddleware()andRequireRole()for admin-only routes is a solid approach to secure endpoints for test creation and user registration.internal/api/v1/services/test.go (1)
294-301: Same path traversal concern as above.🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 294-294: Uncontrolled data used in path expression
This path depends on a user-provided value.internal/api/v1/handlers/errors.go (1)
1-9: LGTM! Clean error handling implementation.The error handling implementation is well-structured with:
- Clear separation of error types using type alias
- Consistent error message format
- User-friendly error descriptions
cmd/main.go (1)
1-12: LGTM! Well-structured main entry point.The implementation is clean with proper error handling and logging.
internal/api/v1/paths.go (1)
3-9: LGTM! User-related paths follow RESTful conventions.The user-related paths are well-structured and follow RESTful conventions for authentication endpoints.
internal/api/v1/services/errors.go (1)
5-5: LGTM! Error type alias provides good abstraction.The type alias allows for easy switching of the underlying error implementation if needed.
internal/database/models/test.go (2)
7-10: LGTM! Field name constants are well-defined.The constants for field names help maintain consistency when used in queries.
12-23: 🛠️ Refactor suggestionAdd validation for LogLevel and consider soft deletes.
A few suggestions for improvement:
- Add validation for LogLevel to ensure only valid log levels are stored:
- LogLevel string `json:"log_level" gorm:""` // logrus level as string + LogLevel string `json:"log_level" gorm:"check:log_level IN ('debug', 'info', 'warn', 'error', 'fatal', 'panic')"`
- Consider adding soft delete support for better data management:
+ DeletedAt gorm.DeletedAt `json:"-" gorm:"index"`Also, verify that using a string as the primary key (Scope) won't cause performance issues at scale.
pkg/builder/git.go (1)
15-19:⚠️ Potential issueConsider security implications of exposing sensitive data in JSON.
The
Passwordfield is exposed through JSON serialization, which could lead to security risks if the JSON is logged or transmitted. Consider:
- Omitting the password from JSON output using
json:"-".- Using a secure credential store instead of plain text.
- Password string `json:"password"` + Password string `json:"-"`✅ Verification successful
Password exposure in JSON is a valid security concern, but requires a different solution.
The password field is required for accessing private git repositories through the API, so simply omitting it from JSON using
json:"-"would break functionality. Instead:
- Consider encrypting the password field before storage/transmission
- Implement a secure credential management system (e.g., vault) for storing git credentials
- Support token-based authentication as a more secure alternative to passwords
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find GitContext struct definition and its usages echo "=== GitContext struct definition ===" ast-grep --pattern 'type GitContext struct { $$$ }' echo -e "\n=== GitContext usage patterns ===" rg "GitContext" -A 5 -B 5 echo -e "\n=== JSON marshal/unmarshal patterns ===" rg "json\.(Marshal|Unmarshal).*GitContext" -A 3 -B 3 echo -e "\n=== Password handling patterns ===" rg "Password.*json:\".*\"" --type goLength of output: 9415
internal/api/v1/index.go (1)
16-24: Filter sensitive build information.The code exposes all build settings without filtering. Some settings might contain sensitive information like paths or environment variables.
Run this script to check for sensitive information in build settings:
go.mod (1)
Line range hint
1-124: Verify dependencies for security vulnerabilities.Let's check for any known security vulnerabilities in the dependencies.
✅ Verification successful
Dependencies are secure but updates are available
Several dependencies have newer versions available, but current versions don't indicate security concerns. Core dependencies are properly pinned via replace directives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for known vulnerabilities in Go dependencies # Install and run govulncheck if available if command -v govulncheck >/dev/null 2>&1; then govulncheck ./... else # Fallback to nancy for vulnerability scanning go list -json -m all | nancy sleuth fi # Check for deprecated or unmaintained packages go list -u -m -json all | jq -r '. | select(.Update != null) | "Package: \(.Path)\nCurrent: \(.Version)\nLatest: \(.Update.Version)\n"'Length of output: 7568
| func getUserFromContext(c *gin.Context) (*models.User, error) { | ||
| user, ok := c.Get(middleware.UserContextKey) | ||
| if !ok { | ||
| return nil, errors.New("user not found in context") | ||
| } | ||
| authUser, ok := user.(*models.User) | ||
| if !ok { | ||
| return nil, errors.New("invalid user data in context") | ||
| } | ||
| return authUser, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add documentation.
The function implementation could be improved in several ways:
- Use the custom
Errortype defined in this package instead of standard errors - Follow the error pattern established in errors.go
- Add function documentation
Consider applying these changes:
+// getUserFromContext extracts and validates the authenticated user from the gin context.
+// Returns the user if present and valid, or an error if the user is missing or invalid.
func getUserFromContext(c *gin.Context) (*models.User, error) {
user, ok := c.Get(middleware.UserContextKey)
if !ok {
- return nil, errors.New("user not found in context")
+ return nil, Error{
+ Code: "UserNotFound",
+ Description: "user not found in context",
+ }
}
authUser, ok := user.(*models.User)
if !ok {
- return nil, errors.New("invalid user data in context")
+ return nil, Error{
+ Code: "InvalidUserData",
+ Description: "invalid user data in context",
+ }
}
return authUser, nil
}Committable suggestion skipped: line range outside the PR's diff.
| type Token struct { | ||
| ID uint `json:"-" gorm:"primaryKey"` | ||
| UserID uint `json:"-" gorm:"index;not null"` | ||
| Token string `json:"token" gorm:"unique;not null"` | ||
| ExpiresAt time.Time `json:"expires_at" gorm:"not null"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add token type and revocation support.
The Token model could be improved by:
- Adding a type field to differentiate between access and refresh tokens
- Adding support for token revocation
type Token struct {
ID uint `json:"-" gorm:"primaryKey"`
UserID uint `json:"-" gorm:"index;not null"`
Token string `json:"token" gorm:"unique;not null"`
+ Type string `json:"type" gorm:"not null;check:type IN ('access', 'refresh')"`
ExpiresAt time.Time `json:"expires_at" gorm:"not null"`
+ RevokedAt *time.Time `json:"-"`
}📝 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.
| type Token struct { | |
| ID uint `json:"-" gorm:"primaryKey"` | |
| UserID uint `json:"-" gorm:"index;not null"` | |
| Token string `json:"token" gorm:"unique;not null"` | |
| ExpiresAt time.Time `json:"expires_at" gorm:"not null"` | |
| } | |
| type Token struct { | |
| ID uint `json:"-" gorm:"primaryKey"` | |
| UserID uint `json:"-" gorm:"index;not null"` | |
| Token string `json:"token" gorm:"unique;not null"` | |
| Type string `json:"type" gorm:"not null;check:type IN ('access', 'refresh')"` | |
| ExpiresAt time.Time `json:"expires_at" gorm:"not null"` | |
| RevokedAt *time.Time `json:"-"` | |
| } |
| const ( | ||
| DefaultHost = "localhost" | ||
| DefaultUser = "postgres" | ||
| DefaultPassword = "postgres" | ||
| DefaultDBName = "postgres" | ||
| DefaultPort = 5432 | ||
| DefaultSSLEnabled = false | ||
| ) |
There was a problem hiding this comment.
Security: Improve default database configuration.
Current defaults are insecure:
- Default credentials should not be hardcoded
- SSL should be enabled by default
- Connection parameters should be required through environment variables
Example improvement:
const (
- DefaultHost = "localhost"
- DefaultUser = "postgres"
- DefaultPassword = "postgres"
- DefaultDBName = "postgres"
- DefaultPort = 5432
- DefaultSSLEnabled = false
+ DefaultHost = "" // Required via env
+ DefaultUser = "" // Required via env
+ DefaultPassword = "" // Required via env
+ DefaultDBName = "" // Required via env
+ DefaultPort = 5432 // Can keep default port
+ DefaultSSLEnabled = true // Enable SSL by default
)📝 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.
| const ( | |
| DefaultHost = "localhost" | |
| DefaultUser = "postgres" | |
| DefaultPassword = "postgres" | |
| DefaultDBName = "postgres" | |
| DefaultPort = 5432 | |
| DefaultSSLEnabled = false | |
| ) | |
| const ( | |
| DefaultHost = "" // Required via env | |
| DefaultUser = "" // Required via env | |
| DefaultPassword = "" // Required via env | |
| DefaultDBName = "" // Required via env | |
| DefaultPort = 5432 // Can keep default port | |
| DefaultSSLEnabled = true // Enable SSL by default | |
| ) |
| func New(opts Options) (*gorm.DB, error) { | ||
| opts = setDefaults(opts) | ||
| sslMode := "disable" | ||
| if opts.SSLEnabled != nil && *opts.SSLEnabled { | ||
| sslMode = "enable" | ||
| } | ||
| dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%d sslmode=%s", | ||
| opts.Host, opts.User, opts.Password, opts.DBName, opts.Port, sslMode) | ||
| db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := migrate(db); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| db.Logger = db.Logger.LogMode(opts.LogLevel) | ||
| return db, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add connection pooling and retry mechanism.
Important database features are missing:
- Connection pool configuration
- Retry mechanism for initial connection
- Connection timeouts
Example implementation:
func New(opts Options) (*gorm.DB, error) {
opts = setDefaults(opts)
+
+ // Add connection pooling configuration
+ poolConfig := &gorm.Config{
+ PrepareStmt: true,
+ ConnPool: &gorm.ConnPool{
+ MaxIdleConns: 10,
+ MaxOpenConns: 100,
+ ConnMaxLifetime: time.Hour,
+ },
+ }
+
+ // Add retry mechanism
+ var db *gorm.DB
+ var err error
+ for retries := 3; retries > 0; retries-- {
+ db, err = gorm.Open(postgres.Open(dsn), poolConfig)
+ if err == nil {
+ break
+ }
+ time.Sleep(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.
| func New(opts Options) (*gorm.DB, error) { | |
| opts = setDefaults(opts) | |
| sslMode := "disable" | |
| if opts.SSLEnabled != nil && *opts.SSLEnabled { | |
| sslMode = "enable" | |
| } | |
| dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%d sslmode=%s", | |
| opts.Host, opts.User, opts.Password, opts.DBName, opts.Port, sslMode) | |
| db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{}) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if err := migrate(db); err != nil { | |
| return nil, err | |
| } | |
| db.Logger = db.Logger.LogMode(opts.LogLevel) | |
| return db, nil | |
| } | |
| func New(opts Options) (*gorm.DB, error) { | |
| opts = setDefaults(opts) | |
| sslMode := "disable" | |
| if opts.SSLEnabled != nil && *opts.SSLEnabled { | |
| sslMode = "enable" | |
| } | |
| dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%d sslmode=%s", | |
| opts.Host, opts.User, opts.Password, opts.DBName, opts.Port, sslMode) | |
| var db *gorm.DB | |
| var err error | |
| config := &gorm.Config{ | |
| PrepareStmt: true, | |
| } | |
| // Add retry mechanism | |
| for retries := 3; retries > 0; retries-- { | |
| db, err = gorm.Open(postgres.Open(dsn), config) | |
| if err == nil { | |
| break | |
| } | |
| time.Sleep(time.Second) | |
| } | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Configure connection pool | |
| sqlDB, err := db.DB() | |
| if err != nil { | |
| return nil, err | |
| } | |
| sqlDB.SetMaxIdleConns(10) | |
| sqlDB.SetMaxOpenConns(100) | |
| sqlDB.SetConnMaxLifetime(time.Hour) | |
| if err := migrate(db); err != nil { | |
| return nil, err | |
| } | |
| db.Logger = db.Logger.LogMode(opts.LogLevel) | |
| return db, nil | |
| } |
| @@ -4,39 +4,48 @@ go 1.22.5 | |||
|
|
|||
| require ( | |||
| github.com/celestiaorg/bittwister v0.0.0-20231213180407-65cdbaf5b8c7 | |||
| github.com/docker/docker v26.1.4+incompatible | |||
| github.com/gin-gonic/gin v1.10.0 | |||
| github.com/golang-jwt/jwt v3.2.2+incompatible | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider upgrading to golang-jwt/jwt/v5.
The current JWT library version (v3.2.2+incompatible) is outdated. Version 5 provides better security features and type safety.
- github.com/golang-jwt/jwt v3.2.2+incompatible
+ github.com/golang-jwt/jwt/v5 v5.2.0📝 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.
| github.com/golang-jwt/jwt v3.2.2+incompatible | |
| github.com/golang-jwt/jwt/v5 v5.2.0 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/api/api.go(1 hunks)cmd/main.go(1 hunks)internal/api/v1/api.go(1 hunks)internal/api/v1/handlers/instance.go(1 hunks)internal/api/v1/handlers/test.go(1 hunks)internal/api/v1/handlers/user.go(1 hunks)internal/api/v1/handlers/utils.go(1 hunks)internal/api/v1/services/test.go(1 hunks)internal/api/v1/services/user.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/main.go
- internal/api/v1/handlers/utils.go
- internal/api/v1/handlers/instance.go
- cmd/api/api.go
- internal/api/v1/handlers/user.go
- internal/api/v1/services/user.go
- internal/api/v1/api.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test (./e2e/netshaper, 60m)
- GitHub Check: test (./e2e/basic, 15m)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
internal/api/v1/handlers/test.go (4)
15-28: LGTM! Well-structured constructor with proper nil checks.The struct definition and constructor are well-implemented with proper dependency injection and nil checks for the logger.
30-62: LGTM! Well-implemented handler with proper logging, auth, and error handling.The handler follows best practices:
- Structured logging with relevant fields
- User authentication check
- Input validation
- Proper error handling with specific status codes
64-74: Complete the implementation of GetTestDetails.The handler is incomplete:
- Logger is created but not used
- Returns an empty test object without any actual implementation
91-100: Fix potential path traversal vulnerability.The handler needs security improvements:
- Validate and sanitize the scope parameter
- Use path.Clean to prevent directory traversal
- Verify the file exists within allowed boundaries
internal/api/v1/services/test.go (4)
23-50: LGTM! Well-structured constants and thread-safe struct design.The implementation shows good practices:
- Clear constant definitions with meaningful names
- Thread-safe design with proper mutex for concurrent access
- Well-organized struct fields
52-93: LGTM! Well-implemented constructor with proper initialization sequence.The implementation follows best practices:
- Directory creation with proper permissions
- K8s client initialization
- Database state recovery
- Background cleanup routine
190-207: Prevent potential path traversal in log file access.The
scopestring, which may come from user input, is directly concatenated into a file path. This poses a risk of directory traversal if malicious input is provided.
348-381: LGTM! Well-implemented periodic cleanup with proper error handling.The implementation follows best practices:
- Clean ticker usage
- Proper shutdown handling
- Comprehensive error logging
| // TODO: currently this process is blocking the request until the knuu is ready | ||
| // we need to make it non-blocking | ||
| err = s.prepareKnuu(ctx, test) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the Knuu preparation non-blocking.
The TODO comment indicates that the Knuu preparation is blocking the request. Consider implementing an async pattern:
- Return early with a "pending" status
- Use a background worker to prepare the Knuu instance
- Update the test status when preparation is complete
| func (a *API) Stop(ctx context.Context) error { | ||
| fmt.Println("Shutting down API server") | ||
| if a.cleanup.testService != nil { | ||
| err := a.cleanup.testService.Shutdown(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return a.server.Close() | ||
| } |
There was a problem hiding this comment.
will it close the db connections as well?
There was a problem hiding this comment.
Good point!
Closing DB is missing. Since the API server receives it as a param, it is not its responsibility to close it, so we can close it where we pass it.
Thanks for pointing out, will fix it.
| ) | ||
|
|
||
| type Auth struct { | ||
| secretKey string |
There was a problem hiding this comment.
is the secret coming from the .env or where is it specified?, (cannot figure out :S)
There was a problem hiding this comment.
the secret is coming from the cli which ofc can be coming from an ENV var if we want during the deployment.
We need to specify it in a Dockerfile when we want to create one
| func (s *TestService) CreateInstance(ctx context.Context, userID uint, instance *Instance) error { | ||
| if userID == 0 { |
There was a problem hiding this comment.
correct me if I'm wrong (I'd like to understand it), this function basically uses the knuu pkgs as library to create the instance with the data that we have in the struct, is it correct?
There was a problem hiding this comment.
Exactly, but it still has work to do. For example I think we need to keep a reference to the instances to be able to interact with them properly. to be decided on how.
| "gorm.io/driver/postgres" | ||
| "gorm.io/gorm" | ||
| "gorm.io/gorm/logger" |
There was a problem hiding this comment.
out of curiosity, what are the benefits of using gorm? (I know it but never used it before)
There was a problem hiding this comment.
it enables us to be almost database agnostic, not fully, but almost! if our queries are not complex.
Also it keeps the DB schema logic in the code and updates when we change the code, so it is easier to manage it for us. We could use something else instead of postgres if we want with probably small changes in the code.
| func migrate(db *gorm.DB) error { | ||
| return db.AutoMigrate( | ||
| &models.User{}, | ||
| &models.Token{}, | ||
| &models.Permission{}, | ||
| &models.Test{}, | ||
| ) | ||
| } |
There was a problem hiding this comment.
are the migrations applied/generated automatically by gorm?
There was a problem hiding this comment.
yes it takes care of it.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/api/v1/services/test.go (1)
107-109: 🛠️ Refactor suggestionConsider implementing async Knuu preparation.
The blocking nature of Knuu preparation could impact API responsiveness. Consider implementing an async pattern:
- Return early with a "pending" status
- Use a background worker to prepare the Knuu instance
- Update the test status when preparation is complete
🧹 Nitpick comments (2)
internal/api/v1/services/test.go (2)
26-26: Consider using a more appropriate logs directory for production.The default logs directory
/tmp/knuu-logsmight be cleared on system reboot. Consider using a more permanent location like/var/log/knuufor production environments.
352-365: Consider making cleanup interval configurable.The cleanup interval is hardcoded to 10 minutes. Consider making it configurable through the
TestServiceOptionsstruct to allow for environment-specific tuning.type TestServiceOptions struct { TestsLogsPath string Logger *logrus.Logger + CleanupInterval time.Duration } func setServiceOptsDefaults(opts TestServiceOptions) TestServiceOptions { if opts.Logger == nil { opts.Logger = logrus.New() } if opts.TestsLogsPath == "" { opts.TestsLogsPath = DefaultTestLogsPath } + if opts.CleanupInterval == 0 { + opts.CleanupInterval = PeriodicCleanupInterval + } return opts }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/api/v1/services/test.go(1 hunks)pkg/k8s/namespace.go(2 hunks)pkg/k8s/validate.go(3 hunks)pkg/k8s/validate_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/k8s/namespace.go
🧰 Additional context used
📓 Learnings (1)
internal/api/v1/services/test.go (2)
Learnt from: mojtaba-esk
PR: celestiaorg/knuu#601
File: internal/api/v1/services/test.go:190-205
Timestamp: 2025-01-31T13:05:32.410Z
Learning: The `TestService.TestLogsPath()` method has two layers of validation for the scope parameter:
1. Database-level validation via `repo.Get()` which ensures the scope exists and belongs to the user
2. Filesystem-level validation which prevents path traversal attacks
Learnt from: mojtaba-esk
PR: celestiaorg/knuu#601
File: internal/api/v1/services/test.go:190-205
Timestamp: 2025-01-31T13:05:32.410Z
Learning: The scope parameter in `TestService.TestLogsPath()` is sufficiently validated through the database layer where:
1. It's constrained as varchar(255)
2. It's a primary key
3. The `repo.Get()` ensures it exists and belongs to the user
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test (./e2e/netshaper, 60m)
- GitHub Check: test (./e2e/system, 15m)
- GitHub Check: test (./e2e/basic, 15m)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
pkg/k8s/validate.go (3)
27-29: LGTM! Function visibility change is appropriate.Making
ValidateNamespacepublic allows external packages to validate namespace names, which is a common requirement when working with Kubernetes resources.
172-174: LGTM! Function call updated correctly.The call to
ValidateNamespaceinvalidatePodConfighas been updated to match the new exported name.
215-217: LGTM! Function call updated correctly.The call to
ValidateNamespaceinvalidateReplicaSetConfighas been updated to match the new exported name.pkg/k8s/validate_test.go (1)
60-60: LGTM! Test updated to use exported function name.The test has been correctly updated to use the exported
ValidateNamespacefunction.internal/api/v1/services/test.go (2)
149-154: LGTM! Good security practice.The explicit setting of
userIDandscopein theUpdatemethod prevents unauthorized modifications.
190-207: LGTM! Proper validation of scope parameter.The implementation includes two layers of validation:
- Database-level validation via
repo.Get()which ensures the scope exists and belongs to the user- Filesystem-level validation which prevents path traversal attacks
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
internal/api/v1/services/test.go (1)
26-26: Consider using a more appropriate path for production logs.Using
/tmpfor storing test logs is not recommended for production environments due to:
- Security:
/tmpis typically world-readable- Persistence: Contents may be cleared on system reboot
- Space limitations:
/tmpoften has limited spaceConsider using a dedicated log directory (e.g.,
/var/log/knuu) or making it configurable through environment variables.internal/database/models/user.go (3)
10-15: Add String method for UserRole type.Consider adding a String method to provide a human-readable representation of the role for logging and debugging purposes.
+func (r UserRole) String() string { + switch r { + case RoleUser: + return "user" + case RoleAdmin: + return "admin" + default: + return "unknown" + } +}
32-38: Add String method for AccessLevel type.Consider adding a String method to provide a human-readable representation of the access level for logging and debugging purposes.
+func (l AccessLevel) String() string { + switch l { + case AccessLevelRead: + return "read" + case AccessLevelWrite: + return "write" + case AccessLevelAdmin: + return "admin" + default: + return "unknown" + } +}
40-45: Add validation for Resource field.Consider adding validation to ensure the Resource field follows a consistent format and contains valid values.
+func (p *Permission) BeforeCreate(tx *gorm.DB) error { + if p.Resource == "" { + return fmt.Errorf("resource cannot be empty") + } + // Add more validation as needed + return nil +}cmd/api/api.go (2)
216-238: Add timeout for graceful shutdown.Consider adding a timeout context for the API server shutdown to prevent hanging indefinitely.
- if err := apiServer.Stop(context.Background()); err != nil { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if err := apiServer.Stop(ctx); err != nil {
193-200: Refactor log level mapping for better readability.Consider extracting the log level mapping logic into a separate function.
+func getLogrusLevel(apiLogLevel string) logrus.Level { + switch apiLogLevel { + case gin.DebugMode: + return logrus.DebugLevel + case gin.ReleaseMode: + return logrus.ErrorLevel + case gin.TestMode: + return logrus.InfoLevel + default: + return logrus.ErrorLevel + } +} - switch apiLogLevel { - case gin.DebugMode: - logger.SetLevel(logrus.DebugLevel) - case gin.ReleaseMode: - logger.SetLevel(logrus.ErrorLevel) - case gin.TestMode: - logger.SetLevel(logrus.InfoLevel) - } + logger.SetLevel(getLogrusLevel(apiLogLevel))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)cmd/api/api.go(1 hunks)internal/api/v1/services/test.go(1 hunks)internal/database/models/user.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
📓 Learnings (2)
internal/database/models/user.go (1)
Learnt from: mojtaba-esk
PR: celestiaorg/knuu#601
File: internal/database/models/user.go:12-18
Timestamp: 2025-01-31T13:07:34.845Z
Learning: Password hashing and validation should be implemented in the User model using GORM hooks (BeforeCreate) and validation methods, rather than in the service layer, to ensure better encapsulation.
internal/api/v1/services/test.go (2)
Learnt from: mojtaba-esk
PR: celestiaorg/knuu#601
File: internal/api/v1/services/test.go:190-205
Timestamp: 2025-01-31T13:05:32.410Z
Learning: The `TestService.TestLogsPath()` method has two layers of validation for the scope parameter:
1. Database-level validation via `repo.Get()` which ensures the scope exists and belongs to the user
2. Filesystem-level validation which prevents path traversal attacks
Learnt from: mojtaba-esk
PR: celestiaorg/knuu#601
File: internal/api/v1/services/test.go:190-205
Timestamp: 2025-01-31T13:05:32.410Z
Learning: The scope parameter in `TestService.TestLogsPath()` is sufficiently validated through the database layer where:
1. It's constrained as varchar(255)
2. It's a primary key
3. The `repo.Get()` ensures it exists and belongs to the user
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (./e2e/netshaper, 60m)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
internal/api/v1/services/test.go (1)
344-344: Verify the deadline PR status.The TODO comment indicates waiting for a deadline PR to be merged. This might affect timeout handling in the Knuu options.
Let's check the status of the deadline PR:
✅ Verification successful
TODO comment is valid - deadline PR #599 is still open
The TODO comment is correct as PR #599 "feat: add deadline instead of timeout" is currently open and pending merge. The current implementation using
time.Until(test.Deadline)is appropriate until the PR is merged.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for PRs related to deadline implementation gh pr list --repo celestiaorg/knuu --search "deadline in:title,body" --state allLength of output: 325
internal/database/models/user.go (3)
25-30: Add token type and revocation support.The Token model could be improved by adding token type and revocation support as previously suggested.
47-54: LGTM! Secure password hashing implementation.The implementation correctly uses bcrypt for password hashing and follows the recommendation to handle this in the User model.
56-58: LGTM! Secure password validation implementation.The implementation correctly uses bcrypt for password validation and follows the recommendation to handle this in the User model.
cmd/api/api.go (1)
49-51: Security: Replace insecure default values.The default values for sensitive data are insecure as previously noted.
| for userID, scopes := range userScopes { | ||
| for _, scope := range scopes { | ||
| s.logger.Debugf("TestService: Running periodic cleanup for userID: %d, scope: %s", userID, scope) | ||
| if err := s.cleanupIfFinishedTest(context.Background(), userID, scope); err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Background context with a timeout context.
Using context.Background() in cleanup operations can lead to hanging operations or resource leaks. Consider using a timeout context to ensure cleanup operations complete within a reasonable time.
Apply this diff:
-if err := s.cleanupIfFinishedTest(context.Background(), userID, scope); err != nil {
+ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+defer cancel()
+if err := s.cleanupIfFinishedTest(ctx, userID, scope); err != nil {📝 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.
| if err := s.cleanupIfFinishedTest(context.Background(), userID, scope); err != nil { | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) | |
| defer cancel() | |
| if err := s.cleanupIfFinishedTest(ctx, userID, scope); err != nil { |
| s := &TestService{ | ||
| repo: repo, | ||
| knuuList: make(map[uint]map[string]*knuu.Knuu), | ||
| testsLogsPath: opts.TestsLogsPath, | ||
| logger: opts.Logger, | ||
| stopCleanupChan: make(chan struct{}), | ||
| } |
There was a problem hiding this comment.
Initialize the cleanup field to prevent nil pointer dereference.
The cleanup field is used in prepareKnuu but is never initialized, which could lead to a nil pointer dereference.
Apply this diff to initialize the cleanup field:
s := &TestService{
repo: repo,
knuuList: make(map[uint]map[string]*knuu.Knuu),
testsLogsPath: opts.TestsLogsPath,
logger: opts.Logger,
stopCleanupChan: make(chan struct{}),
+ cleanup: &testServiceCleanup{},
}📝 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 := &TestService{ | |
| repo: repo, | |
| knuuList: make(map[uint]map[string]*knuu.Knuu), | |
| testsLogsPath: opts.TestsLogsPath, | |
| logger: opts.Logger, | |
| stopCleanupChan: make(chan struct{}), | |
| } | |
| s := &TestService{ | |
| repo: repo, | |
| knuuList: make(map[uint]map[string]*knuu.Knuu), | |
| testsLogsPath: opts.TestsLogsPath, | |
| logger: opts.Logger, | |
| stopCleanupChan: make(chan struct{}), | |
| cleanup: &testServiceCleanup{}, | |
| } |
| type User struct { | ||
| ID uint `json:"-" gorm:"primaryKey"` | ||
| Username string `json:"username" gorm:"unique;not null"` | ||
| Password string `json:"password" gorm:"not null"` | ||
| Role UserRole `json:"role" gorm:"not null"` | ||
| CreatedAt time.Time `json:"created_at"` | ||
| } |
There was a problem hiding this comment.
Security: Hide password field from JSON responses.
The Password field is currently exposed in JSON responses. This is a security risk as it could leak sensitive information.
- Password string `json:"password" gorm:"not null"`
+ Password string `json:"-" gorm:"not null"`📝 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.
| type User struct { | |
| ID uint `json:"-" gorm:"primaryKey"` | |
| Username string `json:"username" gorm:"unique;not null"` | |
| Password string `json:"password" gorm:"not null"` | |
| Role UserRole `json:"role" gorm:"not null"` | |
| CreatedAt time.Time `json:"created_at"` | |
| } | |
| type User struct { | |
| ID uint `json:"-" gorm:"primaryKey"` | |
| Username string `json:"username" gorm:"unique;not null"` | |
| Password string `json:"-" gorm:"not null"` | |
| Role UserRole `json:"role" gorm:"not null"` | |
| CreatedAt time.Time `json:"created_at"` | |
| } |
Closes #583
Summary by CodeRabbit
Release Notes
New Features
Improvements
Infrastructure
Security
These release notes provide a high-level overview of the significant changes without revealing internal implementation details.