Refactor routes - finish authguards #16
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the routes implementation by extracting the Server struct and handlers from internal/api into a new internal/server package to resolve circular import issues. The refactor introduces a contract-based architecture where handlers are organized by tiers (public, admin, read, write, apply, system, static) and the server package exports all handler methods for clean interface compliance.
Key changes:
- Extracted Server struct and all handlers from
internal/apitointernal/serverpackage - Renamed all private handler methods (
handleXxx) to exported methods (HandleXxx) - Added HPA (HorizontalPodAutoscaler) support with timeseries integration and WebSocket broadcasting
- Implemented interface-based routing contracts to eliminate circular imports
Reviewed Changes
Copilot reviewed 141 out of 145 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/server/ |
New package containing extracted Server struct and all handlers with exported method names |
internal/timeseries/keys.go |
Added HPA-level metric base keys for tracking replica counts and scale events |
internal/k8s/informers/ |
Added HPA informer support with event handling and timeseries integration |
internal/metrics/prometheus.go |
Added HPA-specific Prometheus metrics for monitoring |
proposed_reorganization.md |
Removed comprehensive API reorganization proposal documentation |
internal/authz/phase4_e2e_test.go |
Removed end-to-end authorization tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| var responses []map[string]interface{} | ||
| for _, endpoint := range filteredEndpoints { | ||
| responses = append(responses, s.endpointsToResponse(endpoint)) | ||
| responses = append(responses, s.endpointsToResponse(endpoint)) // TODO: This was calling response formatter from api package. |
There was a problem hiding this comment.
[nitpick] Remove the TODO comment as it doesn't provide actionable information. The comment acknowledges a previous state but doesn't indicate what needs to be done or if any action is required.
| responses = append(responses, s.endpointsToResponse(endpoint)) // TODO: This was calling response formatter from api package. | |
| responses = append(responses, s.endpointsToResponse(endpoint)) |
Copilot uses AI. Check for mistakes.
| package api | ||
| package server | ||
|
|
||
| import ( |
There was a problem hiding this comment.
The removed import of context on line 4 appears to be used in the dryRunRBACConfiguration and applyRBACConfiguration methods which now take an *http.Request parameter instead of context.Context. Verify that the context is properly extracted from the request in these methods.
| import ( | |
| import ( | |
| "context" |
Copilot uses AI. Check for mistakes.
| // Check user bindings only when authz.mode == "user_bindings" | ||
| if s.authMiddleware != nil && s.config.Authz.Mode == "user_bindings" { |
There was a problem hiding this comment.
The modification from always checking user bindings to only checking when authz.mode == \"user_bindings\" changes the behavior. Ensure this change is intentional and doesn't break existing functionality for other authorization modes.
Copilot uses AI. Check for mistakes.
|
|
||
| // Get CSP nonce from context | ||
| nonce := "" | ||
| if nonceValue := r.Context().Value(auth.CSPNonceKey{}); nonceValue != nil { |
There was a problem hiding this comment.
Using an empty struct auth.CSPNonceKey{} as a context key creates a new instance each time. This should typically be a package-level variable or constant to ensure the same key is used consistently across the application.
| if nonceValue := r.Context().Value(auth.CSPNonceKey{}); nonceValue != nil { | |
| if nonceValue := r.Context().Value(auth.CSPNonceKey); nonceValue != nil { |
Copilot uses AI. Check for mistakes.
| // Initialize capability service with TTL from config (fallback to 30s) | ||
| capTTL := 30 * time.Second | ||
| if ttlStr := s.config.Caching.SummaryTTL; ttlStr != "" { | ||
| if parsed, err := time.ParseDuration(ttlStr); err == nil { | ||
| capTTL = parsed | ||
| } else { | ||
| s.logger.Warn("Invalid capability cache TTL, using default", zap.String("ttl", ttlStr), zap.Error(err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The capability service TTL configuration is sourced from s.config.Caching.SummaryTTL but this is conceptually about capability caching, not summary caching. Consider using a more specific configuration field like s.config.Caching.CapabilityTTL or documenting why SummaryTTL is being reused.
Copilot uses AI. Check for mistakes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
1 participant
No description provided.