SANDBOX-316 | refactor: replace Gin framework with Echo#593
SANDBOX-316 | refactor: replace Gin framework with Echo#593MikelAlejoBR wants to merge 10 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MikelAlejoBR The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced Gin with Echo across the codebase: added Echo-aware context helpers, converted handlers/middleware/services/logging/errors to use ChangesGin → Echo Framework Migration (single cohesive DAG)
Sequence Diagram(s)sequenceDiagram
participant Client as Client (browser/API)
participant Echo as Echo Router
participant Middleware as Middleware (JWT/Metrics/Logging)
participant Controller as Controller/Handler
participant Service as Application Service
participant K8s as Namespaced Client (Kubernetes)
rect rgba(135,206,250,0.5)
Client->>Echo: HTTP request
end
rect rgba(144,238,144,0.5)
Echo->>Middleware: pass request through middleware chain
Middleware->>Echo: set context values / metrics
end
rect rgba(255,228,181,0.5)
Echo->>Controller: invoke handler with echo.Context
Controller->>Service: call service methods (pass echo.Context)
Service->>K8s: use RequestContext(ctx) to call Kubernetes APIs
K8s-->>Service: return resources
Service-->>Controller: return result
Controller-->>Echo: write response (JSON/NoContent)
Echo-->>Client: HTTP response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labelsrefactoring ✨ Finishing Touches🧪 Generate unit tests (beta)
|
coderabbitai
Bot
added
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/proxy/metrics_server.go (1)
16-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale doc comment still references Echo.
Line 17 still reads "Uses echo web framework" after the migration to Gin.
📝 Proposed fix
// StartMetricsServer start server with a single `/metrics` endpoint to server the Prometheus metrics -// Uses echo web framework +// Uses gin web framework func StartMetricsServer(reg *prometheus.Registry, port int) *http.Server {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/proxy/metrics_server.go` around lines 16 - 17, Update the stale package-level doc comment above StartMetricsServer: replace "Uses echo web framework" with an accurate description referencing Gin (e.g., "Uses Gin web framework") so the comment for StartMetricsServer correctly reflects the migration from Echo to Gin; locate the comment immediately above the StartMetricsServer function and adjust wording/capitalization accordingly.
🧹 Nitpick comments (1)
pkg/log/log_test.go (1)
61-125: 💤 Low valueLGTM — Gin context setup and
InfoGinFassertions look correct.One optional readability improvement carried over from the original test: the
namefield in the table-test struct (line 63) is never populated, sot.Run(tc.name, ...)(line 92) generates unnamed subtests (#00,#01, …). Switching tofor name, tc := range tt { t.Run(name, ...) }would give each subtest the descriptive map key, making failures easier to identify.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/log/log_test.go` around lines 61 - 125, The table-driven test uses unnamed subtests because the struct includes a redundant "name" field and the loop does "for _, tc := range tt { t.Run(tc.name, ...)" — change the iteration to "for name, tc := range tt { t.Run(name, func(t *testing.T) { ... }) }" so each subtest uses the map key as its name (and remove or ignore the unused "name" field in the test-case struct); keep all references to the test case as tc and ensure the inner closure captures tc correctly (no other changes to InfoGinF, ctx setup, or assertions are necessary).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/log/log.go`:
- Around line 124-126: The package-level wrapper WithValues calls ensureLogger()
but the underlying method (*Logger).WithValues still dereferences the
package-global logger fields (e.g., logger.name) and can nil-panic if Init()
hasn't run; update (*Logger).WithValues to defensively handle an uninitialized
logger by checking for nil or missing internal fields and falling back to the
default/fallback logger behavior (the same behavior ensureLogger() provides),
and apply the same defensive check/fallback to the other similar methods
referenced around lines 205-208 so callers using the package wrappers before
Init() won't dereference nil internals.
- Around line 151-155: The code currently appends ctx.Request.URL (which
contains raw query strings) to ctxFields and can leak sensitive query params;
update the logging to append a sanitized URL instead—use ctx.Request.URL.Path
(or construct ctx.Request.URL.Scheme/Host/Path without RawQuery) or explicitly
strip/redact sensitive query keys like "code", "token", "access_token" before
appending; change the two lines that append ctx.Request.URL to append the
sanitized string and ensure any helper used (e.g., InfoGinF) receives the
sanitized value.
In `@pkg/proxy/proxy.go`:
- Around line 230-233: Logs currently print full origin/target URLs (e.g., in
Proxy.redirectTo which calls log.InfoGinF and other nearby log.InfoGinF calls),
potentially exposing sensitive query parameters; fix this by sanitizing URLs
before logging—parse the URL (or parse ctx.Request.URL/ the "to" string), strip
or clear RawQuery (and Fragment) to produce a redacted URL, use that sanitized
string in the log.InfoGinF calls, but keep the original (unmodified) URL value
when calling http.Redirect or proxying so behavior is unchanged; apply the same
redaction logic to all other log.InfoGinF usages in this file that log request
or target URLs.
- Around line 586-590: The error log in the p.List ban-list lookup should not
include the raw email (PII); update the log.Errorf call inside the block that
calls p.List(ctx.Request.Context(), bannedUsers,
client.InNamespace(p.Namespace),
client.MatchingLabels{toolchainv1alpha1.BannedUserEmailHashLabelKey:
hashedEmail}) to remove the plain email and instead include non-PII context such
as the hashedEmail and the error (e.g., "error retrieving banned users for hash
%s"), and keep the existing ctx.AbortWithStatusJSON(...) behavior so the HTTP
response remains the same.
- Around line 183-185: The unsecured-path check in unsecured currently uses
ctx.Request.URL.RequestURI(), which includes query strings and trailing slashes;
change it to use the normalized ctx.Request.URL.Path (which removeTrailingSlash
already normalizes) so comparisons against proxyHealthEndpoint,
wellKnownOauthConfigEndpoint, and strings.HasPrefix(uri, authEndpoint) work
correctly for variants like /proxyhealth?x=1 or /proxyhealth/; update the
variable name (e.g., uri := ctx.Request.URL.Path) and keep the rest of the logic
in unsecured unchanged.
- Around line 149-153: The routes defined on ginRouter use unnamed catch-all
wildcards which will panic; update the patterns to use named catch-all
parameters (e.g. "/*path") so Gin accepts them: change the
wellKnownOauthConfigEndpoint/openidAuthEndpoint()/authEndpoint patterns to
append a named catch-all (e.g. fmt.Sprintf("%s/*path", openidAuthEndpoint()) and
fmt.Sprintf("%s/*path", authEndpoint)) and change the main proxy route to
"/*path" so p.openidAuth, p.auth and p.handleRequestAndRedirect are registered
with valid Gin routes; keep wellKnownOauthConfigEndpoint as-is if it’s an exact
path, otherwise use a named parameter there too.
---
Outside diff comments:
In `@pkg/proxy/metrics_server.go`:
- Around line 16-17: Update the stale package-level doc comment above
StartMetricsServer: replace "Uses echo web framework" with an accurate
description referencing Gin (e.g., "Uses Gin web framework") so the comment for
StartMetricsServer correctly reflects the migration from Echo to Gin; locate the
comment immediately above the StartMetricsServer function and adjust
wording/capitalization accordingly.
---
Nitpick comments:
In `@pkg/log/log_test.go`:
- Around line 61-125: The table-driven test uses unnamed subtests because the
struct includes a redundant "name" field and the loop does "for _, tc := range
tt { t.Run(tc.name, ...)" — change the iteration to "for name, tc := range tt {
t.Run(name, func(t *testing.T) { ... }) }" so each subtest uses the map key as
its name (and remove or ignore the unused "name" field in the test-case struct);
keep all references to the test case as tc and ensure the inner closure captures
tc correctly (no other changes to InfoGinF, ctx setup, or assertions are
necessary).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 798a2457-dc6a-4c78-b7e7-468338e33d0a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
go.modpkg/configuration/configuration.gopkg/context/keys.gopkg/context/public_viewer.gopkg/context/public_viewer_test.gopkg/log/log.gopkg/log/log_test.gopkg/proxy/handlers/spacelister.gopkg/proxy/handlers/spacelister_get.gopkg/proxy/handlers/spacelister_get_test.gopkg/proxy/handlers/spacelister_list.gopkg/proxy/handlers/spacelister_list_test.gopkg/proxy/metrics_server.gopkg/proxy/proxy.go
💤 Files with no reviewable changes (1)
- go.mod
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/context/public_viewer_test.gopkg/context/public_viewer.gopkg/context/keys.gopkg/configuration/configuration.gopkg/proxy/handlers/spacelister_list_test.gopkg/log/log_test.gopkg/proxy/handlers/spacelister.gopkg/proxy/metrics_server.gopkg/log/log.gopkg/proxy/handlers/spacelister_list.gopkg/proxy/handlers/spacelister_get.gopkg/proxy/proxy.gopkg/proxy/handlers/spacelister_get_test.go
🪛 GitHub Check: SonarCloud Code Analysis
pkg/proxy/handlers/spacelister_get.go
[failure] 98-98: Define a constant instead of duplicating this literal "unauthorized access" 3 times.
🔇 Additional comments (6)
pkg/context/keys.go (1)
26-27: LGTM — comment correctly updated to referencegin.Context.pkg/context/public_viewer.go (1)
9-12: LGTM —ctx.GetBoolcorrectly handles all edge cases (nil key, nil value, non-bool type) via Gin's internal nil-guard and silent type assertion.pkg/context/public_viewer_test.go (1)
38-53: LGTM —&gin.Context{}is sufficient here since onlySet/GetBoolare exercised; all nil-map and type-assertion edge cases are handled correctly by Gin internals.pkg/configuration/configuration.go (1)
277-302: LGTM — log calls correctly migrated tologf.Log.Error(err, msg), consistent with the rest of the file.pkg/proxy/handlers/spacelister_list_test.go (1)
82-102: LGTM — Gin test context setup and handler invocation follow the standard pattern; response assertions viarec.Code/rec.Bodyare correct.pkg/proxy/handlers/spacelister_get_test.go (1)
563-607: LGTM — Gin test context setup, path param injection viactx.Params, and handler invocation pattern are correct and consistent across all test functions in this file.
alexeykazakov
left a comment
alexeykazakov
left a comment
There was a problem hiding this comment.
Well.. We actually use Echo in other projects (TARSy, GC, etc). So maybe we should pick Echo and not Gin for reg-service :)
MikelAlejoBR
changed the title
MikelAlejoBR
force-pushed
the
SANDBOX-316-refactor-replace-echo-with-gin
branch
from
1f9903a to
984c66c
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/verification/sender/amazon_sns_sender.go (1)
30-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate request cancellation to the SNS publish call.
This method receives an Echo context but discards it with
_, so the outbound SNS call won't be canceled when the HTTP request is canceled or times out. Use the request context with the context-aware AWS SDK method.Suggested fix
-func (s *AmazonSNSSender) SendNotification(_ echo.Context, content, phoneNumber, _ string) error { +func (s *AmazonSNSSender) SendNotification(ctx echo.Context, content, phoneNumber, _ string) error { @@ - _, err = svc.Publish(&sns.PublishInput{ + _, err = svc.PublishWithContext(ctx.Request().Context(), &sns.PublishInput{ Message: &content, PhoneNumber: &phoneNumber, MessageAttributes: map[string]*sns.MessageAttributeValue{ "AWS.SNS.SMS.SenderID": senderID, "AWS.SNS.SMS.SMSType": smsType, }, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/verification/sender/amazon_sns_sender.go` around lines 30 - 65, SendNotification currently discards the Echo context and calls svc.Publish without propagation, so SNS requests won't be canceled on HTTP timeouts; change the signature use of the context parameter to accept and use the incoming echo.Context (instead of `_`), extract the request context via ctx.Request().Context(), and call the context-aware AWS SNS method (e.g., PublishWithContext) on svc when invoking Publish (replace the svc.Publish call with the context-aware variant and pass the extracted context), ensuring MessageAttributes and other inputs remain unchanged.pkg/proxy/handlers/spacelister.go (1)
30-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the active Echo context to
GetSignupFuncinstead ofnil.Line 45 passes
niltos.GetSignupFunc()which now expects anecho.Context. TheGetSignupmethod at line 318 ofpkg/signup/service/signup_service.goforwards this context downstream, and multiple code paths (lines 211, 217) callctx.Request()which will panic whenctxisnil. Since the function already has access to the active context, pass it through.Proposed fix
func (s *SpaceLister) GetProvisionedUserSignup(ctx echo.Context) (*signup.Signup, error) { username, _ := ctx.Get(context.UsernameKey).(string) - userSignup, err := s.GetSignupFunc(nil, username, false) + userSignup, err := s.GetSignupFunc(ctx, username, false) if err != nil { ctx.Logger().Error(errs.Wrap(err, "error retrieving signup")) return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/proxy/handlers/spacelister.go` around lines 30 - 46, The call in GetProvisionedUserSignup uses s.GetSignupFunc with a nil context which causes downstream panics; update the invocation in GetProvisionedUserSignup to pass the active Echo context (ctx) instead of nil so that GetSignupFunc (and the underlying SignupService.GetSignup) receive a valid echo.Context; locate the call site in the GetProvisionedUserSignup method where s.GetSignupFunc(nil, username, false) is used and replace the nil first argument with ctx.
♻️ Duplicate comments (1)
pkg/log/log.go (1)
136-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize the request URL before logging it.
ctx.Request().URLstill carries the raw query string, so this path can log OAuth codes, tokens, or other sensitive params. StripRawQueryor log only the path before appending the URL field.Suggested fix
ctxFields = append(ctxFields, "url") - ctxFields = append(ctxFields, ctx.Request().URL) + if req := ctx.Request(); req != nil && req.URL != nil { + safeURL := *req.URL + safeURL.RawQuery = "" + ctxFields = append(ctxFields, safeURL.String()) + } else { + ctxFields = append(ctxFields, "") + }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/log/log.go` around lines 136 - 149, In Logger.InfoEchof, sanitize the request URL before logging: replace appending ctx.Request().URL (which may include sensitive RawQuery) with a safe value such as ctx.Request().URL.Path or a cloned url where RawQuery and Fragment are cleared, then append that sanitized string to ctxFields under the "url" key; update the code around the "url" append (in InfoEchof) to use the sanitized variable instead of ctx.Request().URL.
🧹 Nitpick comments (2)
pkg/errors/error.go (1)
17-25: ⚡ Quick win
AbortWithErrorbypasses Echo's centralized error handler.Echo advocates for centralized HTTP error handling by returning errors from middleware and handlers, which allows logging errors from a unified location. The current implementation writes the JSON response directly via
ctx.JSON()and returnsnil. When a handler has already sent a response viac.JSON()and there is an error in the middleware chain returning from the handler, the error that the global error handler receives will be ignored because the response is already committed.As a result, any error-observing middleware or a custom
e.HTTPErrorHandlerconfigured later will never see these HTTP error responses.Consider the idiomatic Echo approach: return an
*echo.HTTPErrorand let the error handler write the response, or at minimum keep this pattern documented as intentional.♻️ Idiomatic alternative
-func AbortWithError(ctx echo.Context, code int, err error, details string) error { - return ctx.JSON(code, &Error{ - Status: http.StatusText(code), - Code: code, - Message: err.Error(), - Details: details, - }) -} +// AbortWithError returns an echo.HTTPError that Echo's error handler will process. +// Callers must register a custom HTTPErrorHandler that formats the Error struct. +func AbortWithError(_ echo.Context, code int, err error, details string) error { + return &echo.HTTPError{ + Code: code, + Message: &Error{ + Status: http.StatusText(code), + Code: code, + Message: err.Error(), + Details: details, + }, + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/errors/error.go` around lines 17 - 25, AbortWithError currently writes the JSON response directly via ctx.JSON and therefore bypasses Echo's centralized error handler; change it to construct and return an *echo.HTTPError (e.g. return echo.NewHTTPError(code, &Error{Status: http.StatusText(code), Code: code, Message: err.Error(), Details: details})) instead of calling ctx.JSON so the global e.HTTPErrorHandler can handle logging and response serialization; update any callers to simply return the error from AbortWithError without writing to the context.pkg/verification/captcha/captcha.go (1)
18-20: 🏗️ Heavy liftKeep the captcha API framework-agnostic.
pkg/verification/captchanow depends onecho.Context, even though this code only needs request-scoped cancellation for the reCAPTCHA call. That leaks the HTTP framework into a lower-level package and makes the next transport migration much larger than it needs to be. Prefercontext.Contexthere and convert once at the HTTP boundary withctx.Request().Context(). As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Also applies to: 34-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/verification/captcha/captcha.go` around lines 18 - 20, The Assessor interface and its CompleteAssessment method currently use echo.Context; change the signature to use standard context.Context instead (i.e., CompleteAssessment(ctx context.Context, cfg configuration.RegistrationServiceConfig, token string) (*recaptchapb.Assessment, error)) and update all implementations in pkg/verification/captcha (the methods that implement Assessor, referenced around the other occurrences you saw) to accept context.Context; then convert echo.Context to context.Context at the HTTP/transport boundary by passing ctx.Request().Context() from handlers so the lower-level captcha package stays framework-agnostic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/signup.go`:
- Around line 157-160: The handler in pkg/controller/signup.go currently only
rejects non-string codes but allows empty strings; update the check after code,
ok := body["code"].(string) to also treat empty string as invalid (e.g., if !ok
|| strings.TrimSpace(code) == "") so it returns 400 and logs the missing
activation code, preventing downstream fallback in
pkg/verification/service/verification_service.go (which calls Signup(ctx)) and
the bypass of social-event validation in pkg/signup/service/signup_service.go;
ensure the log message remains descriptive and use the same ctx/log.Error call
site.
In `@pkg/middleware/jwt_middleware.go`:
- Around line 63-73: The code currently extracts the JWT payload (parts[1] into
rawClaims) and includes it in the log message, which leaks sensitive claim data;
remove the parts/splitting and the rawClaims variable and stop logging the raw
payload. Edit the block around the log.Infof call (references: tokenStr, parts,
rawClaims, token.UserID, token.AccountID, token.PreferredUsername,
token.Subject) so the log only indicates that essential claims are missing and
does not include the raw claims segment (or any full claim values); if you need
contextual info, log non-sensitive identifiers or a masked token identifier
instead, but do not parse or output parts[1].
In `@pkg/middleware/promhttp_middleware.go`:
- Around line 27-31: The metric label currently uses the raw request path
(c.Request().URL.Path) which causes high cardinality; update the labels passed
to counter.With(...) to replace c.Request().URL.Path with the Echo route
template c.Path() so the "path" label uses the route pattern instead of dynamic
values while keeping the other labels (code via
strconv.Itoa(c.Response().Status) and method via c.Request().Method) unchanged.
- Around line 26-33: When recording metrics in the middleware around next(c)
(the block that uses counter.With and reads c.Response().Status), determine the
status code from the returned error first: if err != nil and err is an
*echo.HTTPError use he.Code, otherwise if c.Response().Status != 0 use that
value, and if still zero fall back to http.StatusInternalServerError; then pass
that status into counter.With(prometheus.Labels{ "code": strconv.Itoa(status),
... }). Apply the same fix to the analogous block that records the histogram
(the second block around lines 40-49) so both counter.With and histogram.With
use the resolved status code and not raw c.Response().Status when handlers
return errors.
In `@pkg/server/server_test.go`:
- Around line 66-69: The current goroutine asserts that
srv.HTTPServer().ListenAndServe() returns no error, but ListenAndServe returns
http.ErrServerClosed on graceful shutdown; update the goroutine that calls
srv.HTTPServer().ListenAndServe() to treat http.ErrServerClosed as a non-fatal
case by checking if err != nil && err != http.ErrServerClosed before calling
assert.NoError (or assert.Fail) so only real errors fail the test, and add the
net/http import if missing; reference the anonymous goroutine wrapping
srv.HTTPServer().ListenAndServe() in server_test.go and the s.T() test context.
In `@pkg/server/server.go`:
- Around line 35-64: The JSON log currently builds a string with fmt.Printf
inside the LogValuesFunc of RequestLoggerWithConfig and interpolates
request-derived fields (v.URI, v.UserAgent, errMsg) directly; replace that
manual string construction by creating a struct or map with typed fields and
serializing it via encoding/json (json.Marshal or json.Encoder) so those fields
are properly escaped, then write the marshaled JSON (or handle the marshal
error) instead of using fmt.Printf; update the LogValuesFunc implementation
referenced in RequestLoggerConfig/RequestLoggerWithConfig to use json
serialization for the log object.
In `@pkg/signup/service/signup_service.go`:
- Around line 60-65: DoGetSignup/PollUpdateSignup are using context.TODO() which
ignores request cancellation; replace those uses with the incoming Echo request
context by calling requestContext(ctx) (or ctx.Request().Context()) where cl.Get
and s.Update are invoked in PollUpdateSignup and DoGetSignup so all API calls
respect request cancellation; update both the initial cl.Get and the conditional
s.Update (and the similar occurrences around the 318-346 region) to use
requestContext(ctx) instead of context.TODO().
In `@pkg/verification/service/verification_service.go`:
- Around line 66-68: InitVerification currently calls ServiceImpl.Get with
gocontext.TODO(), which fails to propagate the HTTP request context; replace
gocontext.TODO() with the request-scoped context obtained from the incoming
echo.Context (either ctx.Request().Context() or the existing helper added in the
signup service that converts echo.Context to context.Context) so the Kubernetes
client calls (ServiceImpl.Get/Update/etc. using
signupcommon.EncodeUserIdentifier/NamespacedName) are cancelled when the request
is cancelled; apply the same change to the other read/update blocks referenced
(the similar calls around the other two locations).
---
Outside diff comments:
In `@pkg/proxy/handlers/spacelister.go`:
- Around line 30-46: The call in GetProvisionedUserSignup uses s.GetSignupFunc
with a nil context which causes downstream panics; update the invocation in
GetProvisionedUserSignup to pass the active Echo context (ctx) instead of nil so
that GetSignupFunc (and the underlying SignupService.GetSignup) receive a valid
echo.Context; locate the call site in the GetProvisionedUserSignup method where
s.GetSignupFunc(nil, username, false) is used and replace the nil first argument
with ctx.
In `@pkg/verification/sender/amazon_sns_sender.go`:
- Around line 30-65: SendNotification currently discards the Echo context and
calls svc.Publish without propagation, so SNS requests won't be canceled on HTTP
timeouts; change the signature use of the context parameter to accept and use
the incoming echo.Context (instead of `_`), extract the request context via
ctx.Request().Context(), and call the context-aware AWS SNS method (e.g.,
PublishWithContext) on svc when invoking Publish (replace the svc.Publish call
with the context-aware variant and pass the extracted context), ensuring
MessageAttributes and other inputs remain unchanged.
---
Duplicate comments:
In `@pkg/log/log.go`:
- Around line 136-149: In Logger.InfoEchof, sanitize the request URL before
logging: replace appending ctx.Request().URL (which may include sensitive
RawQuery) with a safe value such as ctx.Request().URL.Path or a cloned url where
RawQuery and Fragment are cleared, then append that sanitized string to
ctxFields under the "url" key; update the code around the "url" append (in
InfoEchof) to use the sanitized variable instead of ctx.Request().URL.
---
Nitpick comments:
In `@pkg/errors/error.go`:
- Around line 17-25: AbortWithError currently writes the JSON response directly
via ctx.JSON and therefore bypasses Echo's centralized error handler; change it
to construct and return an *echo.HTTPError (e.g. return echo.NewHTTPError(code,
&Error{Status: http.StatusText(code), Code: code, Message: err.Error(), Details:
details})) instead of calling ctx.JSON so the global e.HTTPErrorHandler can
handle logging and response serialization; update any callers to simply return
the error from AbortWithError without writing to the context.
In `@pkg/verification/captcha/captcha.go`:
- Around line 18-20: The Assessor interface and its CompleteAssessment method
currently use echo.Context; change the signature to use standard context.Context
instead (i.e., CompleteAssessment(ctx context.Context, cfg
configuration.RegistrationServiceConfig, token string) (*recaptchapb.Assessment,
error)) and update all implementations in pkg/verification/captcha (the methods
that implement Assessor, referenced around the other occurrences you saw) to
accept context.Context; then convert echo.Context to context.Context at the
HTTP/transport boundary by passing ctx.Request().Context() from handlers so the
lower-level captcha package stays framework-agnostic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 4172280a-b27f-42d1-bd09-64fab6312e90
📒 Files selected for processing (47)
pkg/application/service/services.gopkg/assets/assets.gopkg/context/keys.gopkg/controller/analytics.gopkg/controller/analytics_test.gopkg/controller/authconfig.gopkg/controller/authconfig_test.gopkg/controller/health_check.gopkg/controller/health_check_test.gopkg/controller/namespaces.gopkg/controller/namespaces_test.gopkg/controller/signup.gopkg/controller/signup_test.gopkg/controller/uiconfig.gopkg/controller/uiconfig_test.gopkg/controller/usernames.gopkg/controller/usernames_test.gopkg/errors/error.gopkg/errors/error_test.gopkg/log/log.gopkg/log/log_test.gopkg/middleware/jwt_middleware.gopkg/middleware/promhttp_middleware.gopkg/namespaces/namespaces_manager.gopkg/namespaces/namespaces_manager_test.gopkg/proxy/handlers/spacelister.gopkg/proxy/handlers/spacelister_get_test.gopkg/proxy/handlers/spacelister_list_test.gopkg/proxy/proxy_test.gopkg/server/metrics_server.gopkg/server/routes.gopkg/server/routes_test.gopkg/server/server.gopkg/server/server_test.gopkg/signup/service/signup_service.gopkg/signup/service/signup_service_test.gopkg/signup/signup.gopkg/signup/socialevent.gopkg/signup/socialevent_test.gopkg/verification/captcha/captcha.gopkg/verification/sender/amazon_sns_sender.gopkg/verification/sender/sender.gopkg/verification/sender/twilio_sender.gopkg/verification/sender/twilio_sender_test.gopkg/verification/service/verification_service.gopkg/verification/service/verification_service_test.gotest/fake/proxy.go
✅ Files skipped from review due to trivial changes (1)
- pkg/proxy/handlers/spacelister_get_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: GolangCI Lint
- GitHub Check: test
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/errors/error_test.gopkg/errors/error.gopkg/namespaces/namespaces_manager.gopkg/signup/signup.gopkg/controller/authconfig.gopkg/controller/namespaces.gopkg/middleware/promhttp_middleware.gopkg/controller/analytics.gopkg/controller/usernames.gopkg/verification/sender/sender.gopkg/verification/sender/twilio_sender.gopkg/application/service/services.gopkg/controller/namespaces_test.gopkg/verification/sender/amazon_sns_sender.gopkg/controller/uiconfig_test.gopkg/context/keys.gopkg/verification/service/verification_service.gopkg/assets/assets.gopkg/server/routes.gopkg/server/server_test.gopkg/controller/uiconfig.gotest/fake/proxy.gopkg/namespaces/namespaces_manager_test.gopkg/verification/sender/twilio_sender_test.gopkg/server/metrics_server.gopkg/verification/captcha/captcha.gopkg/middleware/jwt_middleware.gopkg/controller/analytics_test.gopkg/controller/health_check_test.gopkg/server/routes_test.gopkg/signup/socialevent_test.gopkg/controller/authconfig_test.gopkg/log/log.gopkg/controller/usernames_test.gopkg/log/log_test.gopkg/verification/service/verification_service_test.gopkg/controller/health_check.gopkg/proxy/proxy_test.gopkg/controller/signup.gopkg/signup/service/signup_service_test.gopkg/proxy/handlers/spacelister_list_test.gopkg/proxy/handlers/spacelister.gopkg/controller/signup_test.gopkg/server/server.gopkg/signup/socialevent.gopkg/signup/service/signup_service.go
🪛 GitHub Check: SonarCloud Code Analysis
pkg/controller/signup_test.go
[warning] 567-567: This function has 8 parameters, which is greater than the 7 authorized.
pkg/signup/service/signup_service.go
[failure] 322-322: Refactor this method to reduce its Cognitive Complexity from 33 to the 15 allowed.
🔇 Additional comments (23)
pkg/controller/uiconfig.go (2)
25-28: Good constructor addition for controller consistency.
NewUIConfig()keeps initialization explicit and matches the controller wiring pattern used in routes.
31-38: Echo handler migration is implemented correctly.Using
func(ctx echo.Context) errorandreturn ctx.JSON(...)is the right contract for Echo handlers and preserves proper error propagation.pkg/controller/uiconfig_test.go (2)
49-52: Test invocation update is correct for Echo handlers.Creating
echo.Contextand assertingrequire.NoErroronGetHandlermatches the new controller contract and makes failures explicit.
91-94: Second test migration is consistent and correct.This mirrors the Echo-based handler call pattern and preserves robust error checking.
pkg/server/metrics_server.go (1)
19-44: LGTM — migration is correct.Key points verified:
*echo.Echosatisfieshttp.HandlerviaServeHTTP, so assigningHandler: routerdirectly (line 32) is valid and replaces the oldrouter.Handler()call correctly.echo.WrapHandleris the right adapter for bridgingpromhttp'shttp.Handlerinto Echo's handler chain.HideBanner/HidePortsuppress Echo's default startup noise, appropriate for a production metrics server.- Both callers (
metrics_server_test.goandpromhttp_middleware_test.go) handle the new(*http.Server, *echo.Echo)return type correctly.pkg/signup/socialevent_test.go (1)
24-27: LGTM — Echo context setup is correct.Creating the Echo context once with a real
httptest.NewRequestensuresctx.Request().Context()returns a validcontext.Background()-backed context. Sharingctxacross subtests is safe since none of them mutate the Echo context state.pkg/signup/socialevent.go (1)
32-32: No issue:log.Infofinpkg/logacceptsecho.Context, supporting the migration from Gin to Echo.pkg/middleware/promhttp_middleware.go (1)
7-7: 🏗️ Heavy liftThe review comment's premise about the PR objective is inverted.
The PR's stated goal is to "remove the dependency with Gin and just keep Echo." The file
pkg/middleware/promhttp_middleware.gocorrectly implements Echo middleware, which aligns with this objective. Gin has been completely removed from the codebase—no Gin imports remain. The code in this file is correct and consistent with the actual PR goal.> Likely an incorrect or invalid review comment.pkg/controller/authconfig.go (1)
31-39: LGTM – clean Echo handler migration.The handler correctly returns
ctx.JSON(...)as theerrorvalue, following the Echo handler contract.pkg/controller/authconfig_test.go (1)
44-57: LGTM – Echo test harness correctly wired.The
Containsassertion onContent-Type(Line 57) is the right adaptation since Echo appends the charset suffix.pkg/verification/sender/sender.go (1)
11-13: LGTM – interface and all implementations consistently updated.pkg/signup/signup.go (1)
75-100: LGTM –PollUpdateSignupcorrectly updated toecho.Context.pkg/errors/error_test.go (1)
27-49: LGTM – Echo test harness correctly replaces Gin'sCreateTestContext.pkg/controller/analytics.go (1)
21-32: LGTM – both analytics handlers cleanly migrated.pkg/assets/assets.go (1)
11-13: ⚡ Quick winThe error return from
ServeEmbedContent()is already properly handled by callers. Inroutes.go(line 117-120), the error is checked withif err != niland wrapped before returning. Inroutes_test.go, it's checked withrequire.NoError(t, err). No action needed.> Likely an incorrect or invalid review comment.pkg/log/log_test.go (1)
27-44: LGTM — Echo context setup is correct throughout.Each subtest correctly constructs
echo.New().NewContext(req, rr)and thebuf.Reset()pattern is maintained consistently (omitted only for the first subtest, which is correct sincebufis freshly initialised).pkg/controller/health_check.go (1)
52-58: LGTM — handler and interface correctly migrated to Echo.pkg/controller/health_check_test.go (1)
219-225: LGTM — mock correctly implements the updatedHealthCheckerinterface.pkg/proxy/proxy_test.go (1)
518-518: LGTM —OverrideGetSignupFuncsignature correctly updated toecho.Context.Also applies to: 683-685
pkg/verification/service/verification_service_test.go (1)
43-46: LGTM —newEchoTestContext()is a clean, reusable helper that correctly replaces the removedgin.CreateTestContextcalls.pkg/context/keys.go (1)
7-15: LGTM —GetStringis correctly nil-safe and handles missing/wrong-type values via the two-value type assertion.pkg/namespaces/namespaces_manager_test.go (1)
100-113: LGTM — Echo context setup is correct; the single shared context is safe since no sub-test mutates it.pkg/signup/service/signup_service.go (1)
188-218: ⚡ Quick winThe claim about callers passing nil context cannot be substantiated from the codebase.
IsPhoneVerificationRequiredis only called from theSignupmethod with a valid HTTP context, and the test case passing nil context returns early before reaching thectx.Request()call. While a nil guard would be defensive programming, it addresses a scenario that does not occur in practice.> Likely an incorrect or invalid review comment.
| if token.UserID == "" || token.AccountID == "" { | ||
| rawClaims := "" | ||
|
|
||
| if token.UserID == "" || token.AccountID == "" { | ||
| rawClaims := "" | ||
| parts := strings.Split(tokenStr, ".") | ||
|
|
||
| // Tokens consist of 3 segments; header, claims and signature | ||
| parts := strings.Split(tokenStr, ".") | ||
| if len(parts) >= 2 { | ||
| rawClaims = parts[1] | ||
| } | ||
|
|
||
| // Just capture the claims segment | ||
| if len(parts) >= 2 { | ||
| rawClaims = parts[1] | ||
| log.Infof(c, "Missing essential claims from token - [user_id:%s][account_id:%s] for user [%s], sub [%s]. Raw claims segment: [%s]", | ||
| token.UserID, token.AccountID, token.PreferredUsername, token.Subject, rawClaims) |
There was a problem hiding this comment.
Do not log the JWT claims payload.
parts[1] is the JWT claims segment. Logging it leaks token contents and likely PII into application logs whenever those claims are missing.
Proposed fix
if token.UserID == "" || token.AccountID == "" {
- rawClaims := ""
-
- parts := strings.Split(tokenStr, ".")
-
- if len(parts) >= 2 {
- rawClaims = parts[1]
- }
-
- log.Infof(c, "Missing essential claims from token - [user_id:%s][account_id:%s] for user [%s], sub [%s]. Raw claims segment: [%s]",
- token.UserID, token.AccountID, token.PreferredUsername, token.Subject, rawClaims)
+ log.Infof(c, "Missing essential claims from token - [user_id:%s][account_id:%s] for user [%s], sub [%s]",
+ token.UserID, token.AccountID, token.PreferredUsername, token.Subject)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/middleware/jwt_middleware.go` around lines 63 - 73, The code currently
extracts the JWT payload (parts[1] into rawClaims) and includes it in the log
message, which leaks sensitive claim data; remove the parts/splitting and the
rawClaims variable and stop logging the raw payload. Edit the block around the
log.Infof call (references: tokenStr, parts, rawClaims, token.UserID,
token.AccountID, token.PreferredUsername, token.Subject) so the log only
indicates that essential claims are missing and does not include the raw claims
segment (or any full claim values); if you need contextual info, log
non-sensitive identifiers or a masked token identifier instead, but do not parse
or output parts[1].
There was a problem hiding this comment.
What do you all think about this one? 🤔 I am not sure if I should keep the old behavior or leave the log as is... In theory if we're missing a critical claim, then it should be fine printing the whole JWT to be able to debug the problem?
There was a problem hiding this comment.
@MikelAlejoBR, this is a valid discussion point! Here's the security concern:
JWT parts[1] is base64-encoded, not encrypted. Anyone who can read your application logs can trivially decode it with base64 -d and see the full claims payload — which typically includes email, name, preferred_username, company, and other PII. Centralised logging systems (ELK, Splunk, etc.) are often accessible to a wider audience than production secrets, and this could conflict with GDPR/privacy obligations.
On the debugging argument: the log line already captures the most useful fields for diagnosing the problem — user_id, account_id, preferred_username, and sub. If those four values are in the log, you have everything you need to:
- Identify the user (sub / preferred_username)
- Know which claims are missing (the empty ones)
- Cross-reference with your IdP/OIDC provider using
sub
The raw claims segment doesn't add much beyond what the parsed fields already give you, and the IdP audit trail is a safer place to inspect the full token for a specific user.
If you want a middle ground for deeper diagnostics without logging raw PII, you could log the number of claims present or a SHA-256 prefix of the token for correlation:
if token.UserID == "" || token.AccountID == "" {
// Log which known fields are populated, without exposing the full payload
log.Infof(c, "Missing essential claims from token - [user_id:%s][account_id:%s] for user [%s], sub [%s]",
token.UserID, token.AccountID, token.PreferredUsername, token.Subject)
}My recommendation is to go with the simpler fix (drop rawClaims) — the existing structured fields are sufficient, and it avoids a potential compliance issue.
There was a problem hiding this comment.
I would agree with @CodeRabbit[bot] here. We should not include any potential PII in the logs. When we went through the PIA assessment (or whatever the exact TLA was) in the past (with another project), the reviewers were very strict about this.
There was a problem hiding this comment.
@metlos, thanks for sharing that context — PIA reviewers being strict about PII in logs is exactly the kind of organizational precedent worth recording. The fix is straightforward: drop the rawClaims extraction entirely and keep only the already-parsed, non-sensitive identifiers in the log line.
Tagging @MikelAlejoBR — given the consensus here, the proposed fix from the original comment is the way to go:
if token.UserID == "" || token.AccountID == "" {
log.Infof(c, "Missing essential claims from token - [user_id:%s][account_id:%s] for user [%s], sub [%s]",
token.UserID, token.AccountID, token.PreferredUsername, token.Subject)
}✏️ Learnings added
Learnt from: metlos
Repo: codeready-toolchain/registration-service PR: 593
File: pkg/middleware/jwt_middleware.go:63-73
Timestamp: 2026-05-14T11:40:58.495Z
Learning: In the registration-service codebase (codeready-toolchain/registration-service), do not log raw JWT payload segments (e.g., parts[1] from splitting a JWT) as they are only base64-encoded and expose PII. The project has undergone PIA (Privacy Impact Assessment) reviews where reviewers were strict about PII appearing in logs. Log only already-parsed, non-sensitive identifiers such as user_id, account_id, preferred_username, and sub.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
MikelAlejoBR
force-pushed
the
SANDBOX-316-refactor-replace-echo-with-gin
branch
2 times, most recently
from
af8f041 to
951bdcc
Compare
coderabbitai
Bot
added
test
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
pkg/verification/service/verification_service_test.go (1)
940-989:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse reserved fictional NANP numbers in fixtures.
The repeated literal on Line 940 (also Lines 952, 971, 989) looks like a potentially real number. Use reserved test numbers (e.g.,
+1-555-0100range) to avoid accidental real-world identifiers in test data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/verification/service/verification_service_test.go` around lines 940 - 989, Replace the real-looking phone number literal used repeatedly in the verification tests with a reserved fictional NANP test number (e.g., from the +1-555-0100 to +1-555-0199 range); specifically update all occurrences of "+12268213044" in verification_service_test.go (tests that call verificationservice.PhoneNumberAlreadyInUse) to a reserved test number such as "+1-555-0100" (or its E.164 equivalent) so fixtures do not contain possible real-world identifiers.pkg/controller/health_check.go (1)
78-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to the proxy health probe.
This health endpoint makes a blocking outbound HTTP call with no timeout. If the local proxy stops accepting or stalls,
/healthcan hang indefinitely instead of returning503, breaking liveness/readiness probe reliability.Suggested fix
func (c *healthCheckerImpl) APIProxyAlive(ctx echo.Context) bool { - resp, err := http.Get(fmt.Sprintf("http://localhost:%s/proxyhealth", c.port)) + client := &http.Client{Timeout: 2 * time.Second} + resp, err := client.Get(fmt.Sprintf("http://localhost:%s/proxyhealth", c.port)) if err != nil { log.Error(ctx, err, "API Proxy health check failed") return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/health_check.go` around lines 78 - 89, The APIProxyAlive method on healthCheckerImpl performs http.Get without a timeout; update it to use an http.Client with a short timeout (or create a request with context.WithTimeout) when calling "http://localhost:%s/proxyhealth" using c.port so the call can't hang indefinitely; ensure you replace the direct http.Get with a client.Do (or http.DefaultClient.Do) using the timeout-aware client/context, preserve the existing response body read/close logic and return resp.StatusCode == http.StatusOK while handling and logging timeout/errors as before.pkg/verification/captcha/captcha.go (2)
35-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the request-scoped context for
recaptcha.NewClienttoo.
CreateAssessmentnow correctly usesctx.Request().Context(), butrecaptcha.NewClientis still constructed withgocontext.Background(). Client construction performs auth/credential discovery and gRPC dialing, so this work won't be cancelled if the incoming HTTP request is cancelled. For consistency with the rest of the migration (and with the recommendation already followed inverification_service.go/signup_service.go), prefer the Echo request context here.♻️ Proposed fix
-func (c Helper) CompleteAssessment(ctx echo.Context, cfg configuration.RegistrationServiceConfig, token string) (*recaptchapb.Assessment, error) { - gctx := gocontext.Background() - client, err := recaptcha.NewClient(gctx) +func (c Helper) CompleteAssessment(ctx echo.Context, cfg configuration.RegistrationServiceConfig, token string) (*recaptchapb.Assessment, error) { + reqCtx := ctx.Request().Context() + client, err := recaptcha.NewClient(reqCtx) if err != nil { return nil, fmt.Errorf("error creating reCAPTCHA client: %w", err) } defer client.Close() @@ response, err := client.CreateAssessment( - ctx.Request().Context(), + reqCtx, request)If you go this route, the
gocontextimport alias is no longer needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/verification/captcha/captcha.go` around lines 35 - 40, The recaptcha client is created with gocontext.Background() which prevents request cancellation during auth/GRPC dialing; update recaptcha.NewClient to use the Echo request-scoped context (ctx.Request().Context()) instead of gocontext.Background(), remove the now-unused gocontext import, and keep the existing defer client.Close() and the CreateAssessment usage that already uses ctx.Request().Context().
62-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't drop the underlying
CreateAssessmenterror.
fmt.Errorf("failed to create reCAPTCHA assessment")discardserr, which removes the only diagnostic for transient failures (gRPC code, quota, auth, etc.). This makes captcha-side outages basically un-debuggable from the registration-service logs.♻️ Proposed fix
- if err != nil { - return nil, fmt.Errorf("failed to create reCAPTCHA assessment") - } + if err != nil { + return nil, fmt.Errorf("failed to create reCAPTCHA assessment: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/verification/captcha/captcha.go` around lines 62 - 64, The current error return in the CreateAssessment path drops the underlying err; update the error return in the captcha assessment creation (the CreateAssessment error handling) to include/wrap the original err (for example using fmt.Errorf("failed to create reCAPTCHA assessment: %w", err)) so the gRPC/quota/auth details are preserved in logs and returned.pkg/proxy/handlers/spacelister.go (1)
42-54:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPass
ctxtoGetSignupFuncinstead ofnil.The Echo migration changed
GetSignupFuncto require anecho.Context, but the call here still passesnileven thoughctxis available as a parameter. The downstreamSignupService.GetSignupimplementation reads request‑scoped values from the context (e.g.,EmailKey, theRecaptcha-Tokenheader used byIsPhoneVerificationRequired), so passingnilis a latent NPE / silent data‑loss bug. The same pattern appears atpkg/proxy/proxy.go:414andpkg/proxy/members.go:98— please fix all three call sites as part of this PR.🛠️ Proposed fix
func (s *SpaceLister) GetProvisionedUserSignup(ctx echo.Context) (*signup.Signup, error) { username, _ := ctx.Get(context.UsernameKey).(string) - userSignup, err := s.GetSignupFunc(nil, username, false) + userSignup, err := s.GetSignupFunc(ctx, username, false)And analogously at
pkg/proxy/proxy.go:414andpkg/proxy/members.go:98(the latter likely needsctxplumbed throughgetSignupFromInformerForProvisionedUser).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/proxy/handlers/spacelister.go` around lines 42 - 54, The call sites pass nil for the echo.Context into GetSignupFunc which now requires the request context; update SpaceLister.GetProvisionedUserSignup to call s.GetSignupFunc(ctx, username, false) instead of nil, and make the same change at the other two locations referenced in the review (the call in proxy.go around the proxy handler at the earlier proxy logic and the call in members.go at getSignupFromInformerForProvisionedUser/members flow) so the actual echo.Context is forwarded; if members.go currently doesn't have ctx in that helper, plumb echo.Context through getSignupFromInformerForProvisionedUser (or its caller chain) and use it when invoking GetSignupFunc.pkg/controller/signup.go (1)
24-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
validate:"required"tags are dead code; remove them or wire up validation properly, and align the two 400 error responses.Echo's default
Binddoes not enforce struct tags — validation only runs if you register aValidatoron the Echo instance and explicitly callctx.Validate(&phone). Neither happens here, so thevalidate:"required"tags have no effect. The actual enforcement is the manual check at lines 51–52.Additionally, the two 400 error paths are inconsistent:
- Line 44:
ctx.Binderror returns a JSON body viacrterrors.AbortWithError- Line 51–52: manual check returns
ctx.NoContent(http.StatusBadRequest)with an empty bodyFor the same failure condition (missing required field), clients receive different response formats. Either drop the unused tags and document that validation is manual, or register a Validator, call
ctx.Validate, and remove the manual check to standardize the response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/signup.go` around lines 24 - 72, The Phone.validate tags are unused and the two 400 paths in InitVerificationHandler are inconsistent; fix by either removing the validate:"required" tags and using a single 400 response path, or wire Echo validation and remove the manual field check. Concretely: update Phone (remove or keep tags consistently), register a Validator on the Echo app, call ctx.Validate(&phone) in InitVerificationHandler after ctx.Bind(&phone), and on validation or bind errors return the same error helper (crterrors.AbortWithError) instead of ctx.NoContent so both missing-field and bad-body cases return a consistent JSON error; alternatively, if you choose manual validation, remove validate tags and replace the ctx.NoContent(http.StatusBadRequest) branch with the same crterrors.AbortWithError(path) used for ctx.Bind errors to standardize responses.
♻️ Duplicate comments (3)
pkg/log/log.go (2)
148-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRaw URL with query string still being logged in
InfoEchof.Same concern raised in the previous review:
ctx.Request().URLis appended verbatim, so any sensitive query parameters (OAuthcode,token, etc.) end up in logs whenever this helper runs on an auth/proxy flow. Strip or redactRawQuerybefore logging —addRequestInfo(line 269-274) already shows the masking pattern that should be applied here.♻️ Proposed fix
- ctxFields = append(ctxFields, "url") - ctxFields = append(ctxFields, ctx.Request().URL) + ctxFields = append(ctxFields, "url") + if u := ctx.Request().URL; u != nil { + safeURL := *u + safeURL.RawQuery = "" + ctxFields = append(ctxFields, safeURL.String()) + } else { + ctxFields = append(ctxFields, "") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/log/log.go` around lines 148 - 149, The code currently appends ctx.Request().URL verbatim to ctxFields, which logs the full RawQuery; update the append so the URL is sanitized by removing or redacting RawQuery before logging (follow the masking pattern used in addRequestInfo), e.g., build a copy of ctx.Request().URL with RawQuery cleared or redacted and append that instead; reference functions/variables: ctxFields, ctx.Request().URL, RawQuery and addRequestInfo to replicate its masking behavior used elsewhere (InfoEchof should receive the sanitized URL).
195-202:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
(*Logger).WithValuesstill dereferences package-globallogger.name.Same concern as the previous review: when called on a
*Loggerinstance, this method should usel.namerather thanlogger.name. With the new package-levelWithValueswrapper,WithValues(...)is reachable beforeInit()runs (e.g., in early init paths or chained calls) and will nil-deref via the globallogger.♻️ Proposed fix
func (l *Logger) WithValues(keysAndValues map[string]interface{}) *Logger { if len(keysAndValues) > 0 { - nl := newLogger(logger.name) + nl := newLogger(l.name) nl.logr = nl.logr.WithValues(slice(keysAndValues)...) return nl } return l }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/log/log.go` around lines 195 - 202, The method (*Logger).WithValues wrongly uses the package-global logger.name which can be nil before Init(); change it to reference the receiver's name (use l.name) when creating the new logger via newLogger so the call reads newLogger(l.name), and ensure nl.logr = nl.logr.WithValues(slice(keysAndValues)...) remains the same; update the WithValues function to return a new logger built from the receiver (l) rather than the package-global logger.pkg/middleware/jwt_middleware.go (1)
63-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winJWT claims payload still being logged — same concern as the previous review.
The
rawClaimslogging that was flagged in the previous review is still present here. Loggingparts[1]of a JWT exposes base64-decoded PII (email, names, company, etc.) into application logs whenever essential claims are missing. The structured fields (user_id,account_id,preferred_username,sub) already provide enough to debug the issue, and the IdP audit trail is the safer place to inspect a full token.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/middleware/jwt_middleware.go` around lines 63 - 74, In jwt_middleware.go, remove logging of the JWT payload segment (the rawClaims derived from splitting tokenStr/parts[1]) inside the block that checks token.UserID/token.AccountID; instead only log the structured fields already present (token.UserID, token.AccountID, token.PreferredUsername, token.Subject) or replace the rawClaims output with a non-PII placeholder (e.g. "[redacted]") so the code in the if branch (which currently computes rawClaims from tokenStr and includes it in log.Infof) no longer emits base64-decoded JWT claims to application logs.
🧹 Nitpick comments (5)
pkg/verification/service/verification_service_test.go (1)
907-989: ⚡ Quick winUse
newEchoTestContext()inPhoneNumberAlreadyInUsetests for consistency with the rest of the test file.All other test functions in
verification_service_test.gousenewEchoTestContext(), but this test alone passesnil. While the framework explicitly handles nil contexts (with a documented fallback tocontext.TODO()), following the established pattern improves consistency and maintainability.♻️ Suggested update pattern
- err := verificationservice.PhoneNumberAlreadyInUse(nil, nsdClient, "jsmith", "+987654321") + ctx := newEchoTestContext() + err := verificationservice.PhoneNumberAlreadyInUse(ctx, nsdClient, "jsmith", "+987654321")Apply the same pattern to Lines 927, 940, 952, 971, and 989.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/verification/service/verification_service_test.go` around lines 907 - 989, Several PhoneNumberAlreadyInUse tests pass nil as the context; replace those nil contexts with newEchoTestContext() for consistency; specifically, update each call site of verificationservice.PhoneNumberAlreadyInUse(nil, nsdClient, ...) to verificationservice.PhoneNumberAlreadyInUse(newEchoTestContext(), nsdClient, ...) in the test cases that currently pass nil (the calls that create fakeClient/nsdClient and call PhoneNumberAlreadyInUse in the "when phone is used but not by approved user", "when used by banned user", "when used by another user", and the two failure subtests), ensuring you import or reference the existing newEchoTestContext helper used elsewhere in this test file.pkg/server/routes.go (1)
92-99: 💤 Low value
receivedTimeMwruns after auth — consider reordering.
securedV1.Use(...)runs middlewares in registration order, soRequestReceivedTimeis currently captured after JWT validation (and the prometheus instrumentation) has executed. If JWT parsing or any upstream middleware takes non-trivial time, the annotation value stored onUserSignupwill reflect that delay rather than the actual request arrival. MovingreceivedTimeMwto the front of the chain (or attaching it at the router level along with the prometheus middlewares) gives a more accurate timestamp.♻️ Proposed fix
securedV1.Use( + receivedTimeMw, middleware.InstrumentRoundTripperInFlight(inFlightGauge), middleware.InstrumentRoundTripperCounter(counter), middleware.InstrumentRoundTripperDuration(histVec), - authMiddleware.HandlerFunc(), - receivedTimeMw) + authMiddleware.HandlerFunc())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/routes.go` around lines 92 - 99, The RequestReceivedTime is being set by receivedTimeMw after other middlewares (prometheus instrumentation and authMiddleware) because securedV1.Use() applies handlers in order; move receivedTimeMw to the very front of the middleware chain so it runs immediately when the request arrives (e.g., register receivedTimeMw before the InstrumentRoundTripper* calls and authMiddleware.HandlerFunc() on securedV1, or attach receivedTimeMw at the router level) to ensure RequestReceivedTime reflects actual arrival time rather than post-auth latency.pkg/signup/service/signup_service.go (2)
95-106: 💤 Low value
requestReceivedTimecast can panic if a non-time.Timevalue was set.
ctx.Get(...)returnsinterface{}. If anything other than the middleware sets this key (or sets it with a different type), the type assertion at line 105 (requestReceivedTime.(time.Time)) panics inside the request handler. A safe assertion would degrade totime.Now()instead.♻️ Proposed fix
- requestReceivedTime := ctx.Get(context.RequestReceivedTime) - if requestReceivedTime == nil { - requestReceivedTime = time.Now() - } + receivedTime, ok := ctx.Get(context.RequestReceivedTime).(time.Time) + if !ok { + receivedTime = time.Now() + } @@ - toolchainv1alpha1.UserSignupRequestReceivedTimeAnnotationKey: requestReceivedTime.(time.Time).Format(time.RFC3339), + toolchainv1alpha1.UserSignupRequestReceivedTimeAnnotationKey: receivedTime.Format(time.RFC3339),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/signup/service/signup_service.go` around lines 95 - 106, The code unsafely type-asserts the value returned by ctx.Get into time.Time (requestReceivedTime.(time.Time)) which can panic; modify the handling around ctx.Get(context.RequestReceivedTime) to perform a safe type assertion (value, ok := requestReceivedTime.(time.Time)) and fall back to time.Now() when the assertion fails or the value is nil, then use that safe time value when setting the toolchainv1alpha1.UserSignup Annotations (toolchainv1alpha1.UserSignupRequestReceivedTimeAnnotationKey) so the handler never panics if a non-time.Time was stored in the context.
209-213: 💤 Low valuePrefer
Header.Get()for case-safe header lookup.
ctx.Request().Header["Recaptcha-Token"]works today because Go canonicalises incoming header names and"Recaptcha-Token"happens to already be in canonical MIME form, but direct map access is fragile — a future renaming or any caller settingHeader[...]with a non-canonical key would silently miss.ctx.Request().Header.Get("Recaptcha-Token")is the idiomatic, case-insensitive accessor and removes the need for thelen(captchaToken) != 1slice check.♻️ Proposed fix
- // require verification if captcha token is invalid - captchaToken, exists := ctx.Request().Header["Recaptcha-Token"] - if !exists || len(captchaToken) != 1 { + // require verification if captcha token is invalid + captchaToken := ctx.Request().Header.Get("Recaptcha-Token") + if captchaToken == "" { log.Error(ctx, nil, "no valid captcha token found in request header") return true, -1, "" } @@ - assessment, err := captchaChecker.CompleteAssessment(ctx, cfg, captchaToken[0]) + assessment, err := captchaChecker.CompleteAssessment(ctx, cfg, captchaToken)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/signup/service/signup_service.go` around lines 209 - 213, Replace the direct map access to ctx.Request().Header with the case-insensitive accessor: use ctx.Request().Header.Get("Recaptcha-Token") to read the token into captchaToken (string) and check for empty string instead of testing a slice length; update the existing branch that logs "no valid captcha token found in request header" and returns the same values to trigger when captchaToken == ""; adjust any subsequent uses of captchaToken accordingly (it will be a string, not []string).pkg/signup/service/signup_service_test.go (1)
47-62: 💤 Low valueOptional: reuse the new helpers to cut boilerplate.
You introduced
newEchoTestContext, but many tests still inlineecho.New() + httptest.NewRequest(...) + e.NewContext(...)(e.g., L117‑120, L262‑265, L287‑290, L415‑418, L452‑455, L484‑487, L509‑512, L544‑547, L579‑582, L1035‑1038). Threading these through the helper (or a small variant that lets callers override the request) would shrink the file and keep future framework swaps to one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/signup/service/signup_service_test.go` around lines 47 - 62, Replace inline echo context construction across tests with the provided helpers to reduce boilerplate: use newEchoTestContext() where a default GET "/" request suffices, use echoCtxNilRequest() for nil-request cases, and use echoCtxWithRecaptchaTokens(...) when tests need Recaptcha header tokens; if a test needs a custom request (different method/path/body/headers) add a small variant like newEchoTestContextWithRequest(req *http.Request) that calls e.NewContext(req, httptest.NewRecorder()) and replace the inline patterns (e.g., echo.New() + httptest.NewRequest(...) + e.NewContext(...)) in the listed test locations to call these helpers instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/signup.go`:
- Around line 64-67: The current log in the ctx.Bind error branch uses a
misleading static message; change the log call in the ctx.Bind error path (where
ctx.Bind(&phone) is checked) to a generic "failed to bind request body" (or
similar) and include the actual error details (err) in the log message via
log.Errorf so the real cause (malformed JSON, wrong content-type, type mismatch,
etc.) is recorded; keep the existing crterrors.AbortWithError call but ensure
the log and returned error reflect the binding failure by including err when
calling log.Errorf and in the abort path if appropriate.
In `@pkg/namespaces/namespaces_manager.go`:
- Around line 45-48: Change the Manager.ResetNamespaces signature to accept
standard context and the username (ResetNamespaces(ctx context.Context, username
string) error) to decouple the package from Echo; update the Manager interface
and all concrete implementations (the struct that implements ResetNamespaces in
namespaces_manager.go) to accept those two parameters, modify any callers (HTTP
controller that currently passes echo.Context) to extract req.Context() and the
username (from customCtx.GetString) and pass them in, and update related tests
and mocks to use the new signature; also apply the same change for any other
methods noted (lines 66-70) that currently take echo.Context, and ensure any
dependent calls like SignupService.GetSignup are coordinated or adapted at the
controller boundary.
In `@pkg/server/server.go`:
- Around line 20-31: The requestLog struct currently uses ErrorMessage error
which marshals to {}; change ErrorMessage's type to string in the requestLog
definition and update all assignments that set requestLog.ErrorMessage (e.g.,
where you currently assign an error value at the request handling path) to
assign either err.Error() when err != nil or an empty string when nil so logs
contain the error text; keep the existing json tag and ensure any references to
requestLog.ErrorMessage treat it as a string.
---
Outside diff comments:
In `@pkg/controller/health_check.go`:
- Around line 78-89: The APIProxyAlive method on healthCheckerImpl performs
http.Get without a timeout; update it to use an http.Client with a short timeout
(or create a request with context.WithTimeout) when calling
"http://localhost:%s/proxyhealth" using c.port so the call can't hang
indefinitely; ensure you replace the direct http.Get with a client.Do (or
http.DefaultClient.Do) using the timeout-aware client/context, preserve the
existing response body read/close logic and return resp.StatusCode ==
http.StatusOK while handling and logging timeout/errors as before.
In `@pkg/controller/signup.go`:
- Around line 24-72: The Phone.validate tags are unused and the two 400 paths in
InitVerificationHandler are inconsistent; fix by either removing the
validate:"required" tags and using a single 400 response path, or wire Echo
validation and remove the manual field check. Concretely: update Phone (remove
or keep tags consistently), register a Validator on the Echo app, call
ctx.Validate(&phone) in InitVerificationHandler after ctx.Bind(&phone), and on
validation or bind errors return the same error helper
(crterrors.AbortWithError) instead of ctx.NoContent so both missing-field and
bad-body cases return a consistent JSON error; alternatively, if you choose
manual validation, remove validate tags and replace the
ctx.NoContent(http.StatusBadRequest) branch with the same
crterrors.AbortWithError(path) used for ctx.Bind errors to standardize
responses.
In `@pkg/proxy/handlers/spacelister.go`:
- Around line 42-54: The call sites pass nil for the echo.Context into
GetSignupFunc which now requires the request context; update
SpaceLister.GetProvisionedUserSignup to call s.GetSignupFunc(ctx, username,
false) instead of nil, and make the same change at the other two locations
referenced in the review (the call in proxy.go around the proxy handler at the
earlier proxy logic and the call in members.go at
getSignupFromInformerForProvisionedUser/members flow) so the actual echo.Context
is forwarded; if members.go currently doesn't have ctx in that helper, plumb
echo.Context through getSignupFromInformerForProvisionedUser (or its caller
chain) and use it when invoking GetSignupFunc.
In `@pkg/verification/captcha/captcha.go`:
- Around line 35-40: The recaptcha client is created with gocontext.Background()
which prevents request cancellation during auth/GRPC dialing; update
recaptcha.NewClient to use the Echo request-scoped context
(ctx.Request().Context()) instead of gocontext.Background(), remove the
now-unused gocontext import, and keep the existing defer client.Close() and the
CreateAssessment usage that already uses ctx.Request().Context().
- Around line 62-64: The current error return in the CreateAssessment path drops
the underlying err; update the error return in the captcha assessment creation
(the CreateAssessment error handling) to include/wrap the original err (for
example using fmt.Errorf("failed to create reCAPTCHA assessment: %w", err)) so
the gRPC/quota/auth details are preserved in logs and returned.
In `@pkg/verification/service/verification_service_test.go`:
- Around line 940-989: Replace the real-looking phone number literal used
repeatedly in the verification tests with a reserved fictional NANP test number
(e.g., from the +1-555-0100 to +1-555-0199 range); specifically update all
occurrences of "+12268213044" in verification_service_test.go (tests that call
verificationservice.PhoneNumberAlreadyInUse) to a reserved test number such as
"+1-555-0100" (or its E.164 equivalent) so fixtures do not contain possible
real-world identifiers.
---
Duplicate comments:
In `@pkg/log/log.go`:
- Around line 148-149: The code currently appends ctx.Request().URL verbatim to
ctxFields, which logs the full RawQuery; update the append so the URL is
sanitized by removing or redacting RawQuery before logging (follow the masking
pattern used in addRequestInfo), e.g., build a copy of ctx.Request().URL with
RawQuery cleared or redacted and append that instead; reference
functions/variables: ctxFields, ctx.Request().URL, RawQuery and addRequestInfo
to replicate its masking behavior used elsewhere (InfoEchof should receive the
sanitized URL).
- Around line 195-202: The method (*Logger).WithValues wrongly uses the
package-global logger.name which can be nil before Init(); change it to
reference the receiver's name (use l.name) when creating the new logger via
newLogger so the call reads newLogger(l.name), and ensure nl.logr =
nl.logr.WithValues(slice(keysAndValues)...) remains the same; update the
WithValues function to return a new logger built from the receiver (l) rather
than the package-global logger.
In `@pkg/middleware/jwt_middleware.go`:
- Around line 63-74: In jwt_middleware.go, remove logging of the JWT payload
segment (the rawClaims derived from splitting tokenStr/parts[1]) inside the
block that checks token.UserID/token.AccountID; instead only log the structured
fields already present (token.UserID, token.AccountID, token.PreferredUsername,
token.Subject) or replace the rawClaims output with a non-PII placeholder (e.g.
"[redacted]") so the code in the if branch (which currently computes rawClaims
from tokenStr and includes it in log.Infof) no longer emits base64-decoded JWT
claims to application logs.
---
Nitpick comments:
In `@pkg/server/routes.go`:
- Around line 92-99: The RequestReceivedTime is being set by receivedTimeMw
after other middlewares (prometheus instrumentation and authMiddleware) because
securedV1.Use() applies handlers in order; move receivedTimeMw to the very front
of the middleware chain so it runs immediately when the request arrives (e.g.,
register receivedTimeMw before the InstrumentRoundTripper* calls and
authMiddleware.HandlerFunc() on securedV1, or attach receivedTimeMw at the
router level) to ensure RequestReceivedTime reflects actual arrival time rather
than post-auth latency.
In `@pkg/signup/service/signup_service_test.go`:
- Around line 47-62: Replace inline echo context construction across tests with
the provided helpers to reduce boilerplate: use newEchoTestContext() where a
default GET "/" request suffices, use echoCtxNilRequest() for nil-request cases,
and use echoCtxWithRecaptchaTokens(...) when tests need Recaptcha header tokens;
if a test needs a custom request (different method/path/body/headers) add a
small variant like newEchoTestContextWithRequest(req *http.Request) that calls
e.NewContext(req, httptest.NewRecorder()) and replace the inline patterns (e.g.,
echo.New() + httptest.NewRequest(...) + e.NewContext(...)) in the listed test
locations to call these helpers instead.
In `@pkg/signup/service/signup_service.go`:
- Around line 95-106: The code unsafely type-asserts the value returned by
ctx.Get into time.Time (requestReceivedTime.(time.Time)) which can panic; modify
the handling around ctx.Get(context.RequestReceivedTime) to perform a safe type
assertion (value, ok := requestReceivedTime.(time.Time)) and fall back to
time.Now() when the assertion fails or the value is nil, then use that safe time
value when setting the toolchainv1alpha1.UserSignup Annotations
(toolchainv1alpha1.UserSignupRequestReceivedTimeAnnotationKey) so the handler
never panics if a non-time.Time was stored in the context.
- Around line 209-213: Replace the direct map access to ctx.Request().Header
with the case-insensitive accessor: use
ctx.Request().Header.Get("Recaptcha-Token") to read the token into captchaToken
(string) and check for empty string instead of testing a slice length; update
the existing branch that logs "no valid captcha token found in request header"
and returns the same values to trigger when captchaToken == ""; adjust any
subsequent uses of captchaToken accordingly (it will be a string, not []string).
In `@pkg/verification/service/verification_service_test.go`:
- Around line 907-989: Several PhoneNumberAlreadyInUse tests pass nil as the
context; replace those nil contexts with newEchoTestContext() for consistency;
specifically, update each call site of
verificationservice.PhoneNumberAlreadyInUse(nil, nsdClient, ...) to
verificationservice.PhoneNumberAlreadyInUse(newEchoTestContext(), nsdClient,
...) in the test cases that currently pass nil (the calls that create
fakeClient/nsdClient and call PhoneNumberAlreadyInUse in the "when phone is used
but not by approved user", "when used by banned user", "when used by another
user", and the two failure subtests), ensuring you import or reference the
existing newEchoTestContext helper used elsewhere in this test file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 10d9616e-c759-4f54-be48-a53cff1f9e7b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (49)
go.modpkg/application/service/services.gopkg/assets/assets.gopkg/context/keys.gopkg/controller/analytics.gopkg/controller/analytics_test.gopkg/controller/authconfig.gopkg/controller/authconfig_test.gopkg/controller/health_check.gopkg/controller/health_check_test.gopkg/controller/namespaces.gopkg/controller/namespaces_test.gopkg/controller/signup.gopkg/controller/signup_test.gopkg/controller/uiconfig.gopkg/controller/uiconfig_test.gopkg/controller/usernames.gopkg/controller/usernames_test.gopkg/errors/error.gopkg/errors/error_test.gopkg/log/log.gopkg/log/log_test.gopkg/middleware/jwt_middleware.gopkg/middleware/promhttp_middleware.gopkg/namespaces/namespaces_manager.gopkg/namespaces/namespaces_manager_test.gopkg/proxy/handlers/spacelister.gopkg/proxy/handlers/spacelister_get_test.gopkg/proxy/handlers/spacelister_list_test.gopkg/proxy/proxy.gopkg/proxy/proxy_test.gopkg/server/metrics_server.gopkg/server/routes.gopkg/server/routes_test.gopkg/server/server.gopkg/server/server_test.gopkg/signup/service/signup_service.gopkg/signup/service/signup_service_test.gopkg/signup/signup.gopkg/signup/socialevent.gopkg/signup/socialevent_test.gopkg/verification/captcha/captcha.gopkg/verification/sender/amazon_sns_sender.gopkg/verification/sender/sender.gopkg/verification/sender/twilio_sender.gopkg/verification/sender/twilio_sender_test.gopkg/verification/service/verification_service.gopkg/verification/service/verification_service_test.gotest/fake/proxy.go
💤 Files with no reviewable changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/errors/error_test.go
- pkg/controller/analytics_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/signup/socialevent.gopkg/application/service/services.gopkg/context/keys.gopkg/log/log_test.gopkg/controller/analytics.gopkg/verification/sender/sender.gopkg/controller/uiconfig_test.gopkg/verification/sender/twilio_sender.gopkg/proxy/proxy_test.gopkg/signup/service/signup_service_test.gotest/fake/proxy.gopkg/proxy/handlers/spacelister_list_test.gopkg/controller/namespaces_test.gopkg/middleware/promhttp_middleware.gopkg/proxy/proxy.gopkg/controller/namespaces.gopkg/namespaces/namespaces_manager.gopkg/controller/uiconfig.gopkg/server/metrics_server.gopkg/signup/socialevent_test.gopkg/verification/sender/twilio_sender_test.gopkg/server/routes_test.gopkg/server/routes.gopkg/proxy/handlers/spacelister.gopkg/signup/signup.gopkg/controller/authconfig_test.gopkg/verification/captcha/captcha.gopkg/verification/sender/amazon_sns_sender.gopkg/assets/assets.gopkg/controller/authconfig.gopkg/controller/signup_test.gopkg/errors/error.gopkg/controller/health_check.gopkg/server/server.gopkg/controller/usernames.gopkg/proxy/handlers/spacelister_get_test.gopkg/middleware/jwt_middleware.gopkg/server/server_test.gopkg/namespaces/namespaces_manager_test.gopkg/controller/usernames_test.gopkg/verification/service/verification_service.gopkg/controller/health_check_test.gopkg/verification/service/verification_service_test.gopkg/signup/service/signup_service.gopkg/controller/signup.gopkg/log/log.go
🪛 GitHub Check: SonarCloud Code Analysis
pkg/context/keys.go
[warning] 10-10: Complete the task associated to this TODO comment.
pkg/controller/signup_test.go
[warning] 567-567: This function has 8 parameters, which is greater than the 7 authorized.
pkg/verification/service/verification_service.go
[failure] 65-65: Refactor this method to reduce its Cognitive Complexity from 31 to the 15 allowed.
[failure] 221-221: Refactor this method to reduce its Cognitive Complexity from 42 to the 15 allowed.
[failure] 385-385: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
pkg/signup/service/signup_service.go
[failure] 314-314: Refactor this method to reduce its Cognitive Complexity from 33 to the 15 allowed.
🪛 Microsoft Presidio Analyzer (2.2.362)
pkg/controller/signup_test.go
[medium] 539-539: Detected phone number with confidence 0.75. Remove the number or use a reserved fictional NANP number (e.g. +1-555-0100 in the 555-01XX range).
(PHONE_NUMBER)
[medium] 539-539: Detected phone number with confidence 0.40. Remove the number or use a reserved fictional NANP number (e.g. +1-555-0100 in the 555-01XX range).
(PHONE_NUMBER)
pkg/verification/service/verification_service_test.go
[medium] 940-940: Detected phone number with confidence 0.75. Remove the number or use a reserved fictional NANP number (e.g. +1-555-0100 in the 555-01XX range).
(PHONE_NUMBER)
[medium] 952-952: Detected phone number with confidence 0.75. Remove the number or use a reserved fictional NANP number (e.g. +1-555-0100 in the 555-01XX range).
(PHONE_NUMBER)
[medium] 971-971: Detected phone number with confidence 0.75. Remove the number or use a reserved fictional NANP number (e.g. +1-555-0100 in the 555-01XX range).
(PHONE_NUMBER)
[medium] 989-989: Detected phone number with confidence 0.75. Remove the number or use a reserved fictional NANP number (e.g. +1-555-0100 in the 555-01XX range).
(PHONE_NUMBER)
🔇 Additional comments (21)
pkg/middleware/promhttp_middleware.go (2)
16-24: LGTM — clean Echo port of the in-flight gauge.
defer gauge.Dec()correctly guarantees the counter is decremented on panic and on error returns fromnext(c).
60-69: ⚡ Quick winNo changes needed —
crterrors.StatusCodealready handles*echo.HTTPErrorcorrectly.The
StatusCodefunction inpkg/errors/error.go(lines 91–101) explicitly unwraps*echo.HTTPErrorusingerrors.As()and returns itsCodefield directly. This prevents anycode="0"metrics issue. Test coverage confirms the behavior atpkg/errors/error_test.go:97–99.> Likely an incorrect or invalid review comment.pkg/verification/service/verification_service_test.go (1)
43-46: Echo test context helper looks good.This keeps test setup consistent across migrated cases and reduces repeated boilerplate.
pkg/signup/socialevent_test.go (1)
24-27: Echo test context migration looks correct.The switch to
echo.New().NewContext(req, rr)is clean and keeps test intent unchanged.pkg/signup/signup.go (1)
75-75: Handler context migration is consistent.
PollUpdateSignupnow acceptsecho.Contextwhile preserving existing retry/error behavior.pkg/assets/assets.go (1)
11-12: Nice simplification of embedded static FS serving.Returning the
fs.Sub(...)result directly is cleaner and keeps the assets package framework-agnostic.pkg/controller/authconfig.go (1)
31-39: Echo handler conversion is correct.Returning
ctx.JSON(...)with anerrorreturn type aligns with Echo’s handler model and keeps behavior intact.pkg/controller/authconfig_test.go (1)
48-52: Test migration to Echo is solid.Good update to Echo context creation, explicit
errorassertion from handler invocation, and resilient JSON content-type check.Also applies to: 56-57
pkg/controller/usernames.go (1)
28-33: Echo-based handler flow is consistent and correct.The migration preserves not-found/server-error handling and properly uses
ctx.Request().Context()for client calls.Also applies to: 39-49, 54-56
pkg/controller/uiconfig.go (1)
31-39: UI config handler migration to Echo looks good.The updated signature and
return ctx.JSON(...)usage align with the registered Echo route contract.pkg/controller/uiconfig_test.go (1)
49-53: Echo test updates are correct in both scenarios.Great to see both handler calls updated to capture/validate the returned error from
GetHandler.Also applies to: 91-95
pkg/verification/sender/sender.go (1)
11-12: 🏗️ Heavy liftEcho.Context in the sender interface is justified by the logging layer.
The sender implementations use
echo.Contextforlog.Error()calls, not for cancellation/metadata. The logging API in this codebase requiresecho.Context, notcontext.Context, so changing the sender interface to acceptcontext.Contextand adapting at the service boundary won't work—the sender would still need to convert back toecho.Contextfor logging. The coupling is valid and necessary.If maintainability is a concern, the root issue is that the logging layer is tightly coupled to
echo.Context. That's a separate architectural decision about the logging package itself, not the sender interface.> Likely an incorrect or invalid review comment.pkg/verification/service/verification_service.go (1)
65-203: Migration looks correct; request-context propagation now consistent.All
s.Get/s.Update/cl.Listcalls in this file usecontext.RequestContext(ctx), which addresses the earlier feedback aboutgocontext.TODO()not respecting request cancellation.PhoneNumberAlreadyInUsecorrectly receives the Echo context and threadsreqCtxthrough both list calls. The signature changes match the controller call sites inpkg/controller/signup.go.pkg/server/routes_test.go (1)
16-68: LGTM.Echo
StaticFS+MustSubFSreplacement is idiomatic, and the test now correctly asserts that/servesstatic/index.htmldirectly with 200 (a behavior change from the previous Gin redirect, but explicitly intended).pkg/application/service/services.go (1)
9-18: Interface migration LGTM.Service interfaces now consistently take
echo.Context. Note: several existing call sites still passnilfor the newctxparameter (pkg/proxy/handlers/spacelister.go:45,pkg/proxy/proxy.go:414,pkg/proxy/members.go:98) — flagged at the spacelister.go root‑cause site rather than duplicated here.pkg/controller/signup_test.go (1)
259-589: Test harness migration LGTM.Echo context construction, parameter binding via
SetParamNames/SetParamValues, and theMIMEApplicationJSONcontent‑type handling are correctly applied. The defensive early return at L340‑342 is a reasonable bridge while the validator path stays in place.pkg/server/server_test.go (1)
66-132: LGTM.
http.ErrServerClosedis correctly excluded from both goroutine assertions, matching the previously-raised concern, and the switch tosrv.HTTPServer().ListenAndServe()lines up with the Echo-based server wiring.pkg/proxy/handlers/spacelister_list_test.go (1)
82-219: LGTM.Test contexts are constructed via
echo.New()+e.NewContext, the override hook signature now matches the updatedGetSignupFunc, and request-scoped keys are set consistently with the new contract.pkg/proxy/handlers/spacelister_get_test.go (1)
78-79: Echo context migration is consistent across test overridesThe updated callback signatures and overrides align with the Echo-based handler contracts and keep test wiring coherent.
Also applies to: 272-273, 622-623, 675-676
pkg/server/metrics_server.go (1)
19-37: Metrics server Echo migration looks goodThe endpoint wrapping and server handler transition to Echo are clean and consistent with the refactor intent.
test/fake/proxy.go (1)
44-49: Fake signup service signatures are correctly aligned with EchoThe context-type migration is consistent and preserves existing fake behavior.
| type Manager interface { | ||
| // ResetNamespaces locates the user's namespaces in their corresponding member clusters and deletes them, so that | ||
| // the NSTemplate controller can recreate them. | ||
| ResetNamespaces(ginCtx *gin.Context) error | ||
| ResetNamespaces(ctx echo.Context) error |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the Echo dependency has moved into the namespaces package boundary.
rg -n 'echo\.Context' pkg/namespacesRepository: codeready-toolchain/registration-service
Length of output: 267
🏁 Script executed:
#!/bin/bash
# Read the complete Manager interface and implementation
sed -n '45,100p' pkg/namespaces/namespaces_manager.goRepository: codeready-toolchain/registration-service
Length of output: 2474
🏁 Script executed:
#!/bin/bash
# Check how echo.Context is actually used in the implementation
rg -A 10 'func \(mgr \*manager\) ResetNamespaces' pkg/namespaces/Repository: codeready-toolchain/registration-service
Length of output: 1066
🏁 Script executed:
#!/bin/bash
# Check if there are other interfaces/handlers that extract context without Echo coupling
find . -name '*.go' -type f | head -20 | xargs grep -l 'func.*ctx context.Context' | head -5Repository: codeready-toolchain/registration-service
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Look for patterns where Echo context is extracted/decoupled at boundaries
rg -B 2 -A 3 'customCtx.GetString.*UsernameKey' --type go | head -30Repository: codeready-toolchain/registration-service
Length of output: 1574
🏁 Script executed:
#!/bin/bash
# Find the SignupService interface and its GetSignup method signature
rg -A 5 'type SignupService interface' pkg/Repository: codeready-toolchain/registration-service
Length of output: 537
🏁 Script executed:
#!/bin/bash
# Check other handler patterns in controller to see if Echo is typically decoupled
ls -la pkg/controller/
cat pkg/controller/*.go | head -100Repository: codeready-toolchain/registration-service
Length of output: 4369
🏁 Script executed:
#!/bin/bash
# Check imports in the namespaces package to understand dependencies
head -30 pkg/namespaces/namespaces_manager.goRepository: codeready-toolchain/registration-service
Length of output: 1346
Consider decoupling Manager from Echo at the boundary.
The Manager.ResetNamespaces interface requires echo.Context, which tightly couples the business logic package to the framework. While the implementation extracts what it needs—username via customCtx.GetString() and standard context via ctx.Request().Context()—the interface still forces callers and test mocks to deal with Echo.
A cleaner boundary would be:
ResetNamespaces(ctx context.Context, username string) errorThis decouples the manager from framework details. The controller can extract these values from Echo at the HTTP boundary and pass them down. Note: SignupService.GetSignup also requires echo.Context, so fully addressing this concern may require coordinating that refactor as well.
Also applies to: lines 66-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/namespaces/namespaces_manager.go` around lines 45 - 48, Change the
Manager.ResetNamespaces signature to accept standard context and the username
(ResetNamespaces(ctx context.Context, username string) error) to decouple the
package from Echo; update the Manager interface and all concrete implementations
(the struct that implements ResetNamespaces in namespaces_manager.go) to accept
those two parameters, modify any callers (HTTP controller that currently passes
echo.Context) to extract req.Context() and the username (from
customCtx.GetString) and pass them in, and update related tests and mocks to use
the new signature; also apply the same change for any other methods noted (lines
66-70) that currently take echo.Context, and ensure any dependent calls like
SignupService.GetSignup are coordinated or adapted at the controller boundary.
There was a problem hiding this comment.
Let's do this in a different PR, since replacing the echo.Context with the generic one is a significant refactor in itself
There was a problem hiding this comment.
@MikelAlejoBR, understood! That's a reasonable call given the scope involved — fully decoupling echo.Context from the Manager interface would also require coordinating changes to SignupService.GetSignup and its callers.
Would you like me to open a follow-up GitHub issue to track this refactor?
coderabbitai
Bot
added
refactoring
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/server/server.go (1)
86-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid wildcard origins when credentials are enabled.
The CORS configuration combines
AllowOrigins: "*"withAllowCredentials: true, which violates the CORS specification and Echo's security guidelines. Browsers reject this configuration because it would allow any origin to access resources with user credentials. Echo's middleware documentation explicitly warns against this combination and only provides an unsafe escape hatch (UnsafeWildcardOriginWithAllowCredentials). Replace the wildcard with an explicit allowlist of trusted origins before shipping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/server.go` around lines 86 - 92, The CORS config is using AllowOrigins: []string{"*"} together with AllowCredentials: true in the middleware.CORSWithConfig/middleware.CORSConfig block, which is invalid; replace the wildcard with an explicit allowlist of trusted origins (e.g., a []string of your frontend/origin domains) and ensure AllowCredentials remains true only for that allowlist; if you intentionally need a wildcard for non-production/testing, use Echo's UnsafeWildcardOriginWithAllowCredentials feature deliberately and document it, but do not ship with AllowOrigins set to "*" when AllowCredentials is true.
♻️ Duplicate comments (1)
pkg/controller/signup.go (1)
153-155:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the bind-failure log consistent in activation-code verification.
This branch still collapses every
ctx.Bindfailure into “no activation code provided”, even thoughInitVerificationHandlernow uses a generic bind-failure message. Reusing that pattern here will preserve the real cause in logs instead of hiding malformed-body/type errors behind a missing-code message.Suggested fix
if err := ctx.Bind(&body); err != nil { - log.Error(ctx, nil, "no activation code provided in the request") - return ctx.NoContent(http.StatusBadRequest) + log.Errorf(ctx, err, "unable to bind activation code request body") + return crterrors.AbortWithError(ctx, http.StatusBadRequest, err, "error reading request body") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/signup.go` around lines 153 - 155, The current ctx.Bind error branch in the verification flow logs a misleading "no activation code provided" message; change it to match InitVerificationHandler's generic bind-failure handling by logging the actual bind error (pass the error to log.Error) and use the same generic message used there (e.g., "failed to bind request body" or similar) before returning http.StatusBadRequest; update the branch in pkg/controller/signup.go that calls ctx.Bind(&body) so it calls log.Error(ctx, err, "<generic bind-failure message>") instead of hiding the real error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/server/server.go`:
- Around line 86-92: The CORS config is using AllowOrigins: []string{"*"}
together with AllowCredentials: true in the
middleware.CORSWithConfig/middleware.CORSConfig block, which is invalid; replace
the wildcard with an explicit allowlist of trusted origins (e.g., a []string of
your frontend/origin domains) and ensure AllowCredentials remains true only for
that allowlist; if you intentionally need a wildcard for non-production/testing,
use Echo's UnsafeWildcardOriginWithAllowCredentials feature deliberately and
document it, but do not ship with AllowOrigins set to "*" when AllowCredentials
is true.
---
Duplicate comments:
In `@pkg/controller/signup.go`:
- Around line 153-155: The current ctx.Bind error branch in the verification
flow logs a misleading "no activation code provided" message; change it to match
InitVerificationHandler's generic bind-failure handling by logging the actual
bind error (pass the error to log.Error) and use the same generic message used
there (e.g., "failed to bind request body" or similar) before returning
http.StatusBadRequest; update the branch in pkg/controller/signup.go that calls
ctx.Bind(&body) so it calls log.Error(ctx, err, "<generic bind-failure
message>") instead of hiding the real error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 14cc4d6b-7d89-4988-9ee0-441ad5d29e46
📒 Files selected for processing (2)
pkg/controller/signup.gopkg/server/server.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/signup.gopkg/server/server.go
MikelAlejoBR
force-pushed
the
SANDBOX-316-refactor-replace-echo-with-gin
branch
from
37209f8 to
45a8afa
Compare
The registration service used both the Echo and Gin frameworks to provide its services, and we wanted to remove the dependency with Gin and just keep Echo to improve maintainability. Assited-by: claude-4.6 with Cursor SANDBOX-316
With an empty activation codes were received, the type assertion check did not check for an empty string, and therefore the execution kept going through the "VerifyActivationCode" function, which would set an empty social event and call the "Signup" function. That function, after getting to "newUserSignup", would attempt grabbing the social event code, but since it's an empty string, the signup would fallback to a regular one instead of being associated with an event. SANDBOX-316
The handlers that returned an error without writing a response and a response status code, made the Prometheus middleware to increase counters with a unset "0" status code. This is because there is a centralized catch-all Echo middleware which is supposed to always populate it by default to "500" in case there is no other status code set, but the Prometheus middleware always gets executed before that one. By extracting the logic of the catch-all middleware and applying it to the Prometheus one as well we solve that issue. SANDBOX-316
Using the request's path instead of the template leads to having a high cardinality for the Prometheus metrics, because every ID would generate a different metric. By using the matched template route instead we fix the issue, and it's still readable enough. SANDBOX-316
We weren't skipping the "server closed" errors when asserting for "no error" when launching the Echo server for the tests, which could result in flaky tests. SANDBOX-316
Previously, a field that included a quote or a new line could break the log line due to how it was constructed with the "Sprintf" statement. It could also potentially lead to log injections with bad values. By using a defined structure and a JSON marshaller, we avoid both those issues. SANDBOX-316
The "DoGetSignup" function was making calls with a whole new context instead of taking into account the request context, so in the event that the parent context got cancelled, those Kubernetes calls did not get cancelled. SANDBOX-316
SANDBOX-316
Since binding can fail due to other reasons other than just an unexpected input, we are making the logging message more generic to avoid confusion when debugging this part of the code. SANDBOX-316
Using the error directly might not marshal to its string value, thus rendering that field useless when logging HTTP requests. SANDBOX-316
MikelAlejoBR
force-pushed
the
SANDBOX-316-refactor-replace-echo-with-gin
branch
from
45a8afa to
c675e07
Compare
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
pkg/errors/error.go (1)
19-24:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHandle
nilerror inAbortWithErrorto prevent panic.Line 23 dereferences
errunconditionally (err.Error()), but callers can passnil; that panics on the error path.Proposed fix
func AbortWithError(ctx echo.Context, code int, err error, details string) error { + message := http.StatusText(code) + if err != nil { + message = err.Error() + } return ctx.JSON(code, &Error{ Status: http.StatusText(code), Code: code, - Message: err.Error(), + Message: message, Details: details, }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/errors/error.go` around lines 19 - 24, The AbortWithError function dereferences err unconditionally causing a panic when callers pass nil; update AbortWithError to guard against a nil error by computing the message safely (e.g., if err == nil use an empty string or a default message) before building the Error struct, and use that safeMessage in place of err.Error() so AbortWithError and any callers will not panic when err is nil.pkg/proxy/handlers/spacelister.go (1)
42-54:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPass the request context to
GetSignupFuncinstead ofnil.
DoGetSignupdereferences the context immediately viacontext.RequestContext(ctx)at the start of the polling loop (line 388 of signup_service.go), before the nil check at line 405. Passingnilwill cause a panic. Additionally, the context is passed toPollUpdateSignupandauditUserSignupAgainstClaims, both of which expect a valid context for proper request tracking and cancellation.🔧 Proposed fix
- userSignup, err := s.GetSignupFunc(nil, username, false) + userSignup, err := s.GetSignupFunc(ctx, username, false)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/proxy/handlers/spacelister.go` around lines 42 - 54, In SpaceLister.GetProvisionedUserSignup replace the nil context passed to GetSignupFunc with the actual request context so DoGetSignup and downstream calls (which call context.RequestContext(ctx), PollUpdateSignup, auditUserSignupAgainstClaims) receive a valid context; specifically, call GetSignupFunc with the incoming request context (e.g., derive from the Echo context via ctx.Request().Context() or context.RequestContext(ctx)) instead of nil, preserving the username and flag arguments.pkg/signup/service/signup_service.go (2)
264-273:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the request context into the account-verifier HTTP call.
http.Client.Postignores the Echo request context, so a cancelled signup can still block here until the client timeout expires. Since this runs inline inSignup, it keeps unnecessary work alive on an already-aborted request.Suggested fix
func callAccountVerifier(ctx echo.Context, verifierURL, email string) error { reqBody, err := json.Marshal(accountVerifierRequest{Email: email}) if err != nil { return errs.Wrap(err, "failed to marshal account verifier request") } - resp, err := accountVerifierHTTPClient.Post(verifierURL+"/verify-account", "application/json", bytes.NewReader(reqBody)) + req, err := http.NewRequestWithContext(context.RequestContext(ctx), http.MethodPost, verifierURL+"/verify-account", bytes.NewReader(reqBody)) + if err != nil { + return errs.Wrap(err, "failed to build account verifier request") + } + req.Header.Set("Content-Type", "application/json") + + resp, err := accountVerifierHTTPClient.Do(req) if err != nil { - return errs.Wrapf(err, "failed to call account verifier for email %s", email) + return errs.Wrap(err, "failed to call account verifier") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/signup/service/signup_service.go` around lines 264 - 273, callAccountVerifier currently uses accountVerifierHTTPClient.Post which ignores the Echo request context; update callAccountVerifier to build an *http.Request using the Echo request context (ctx.Request().Context()) — e.g. create a POST request to verifierURL+"/verify-account" with bytes.NewReader(reqBody), set the "Content-Type: application/json" header, then call accountVerifierHTTPClient.Do(req) instead of Post, handle the returned response and error the same way, and ensure resp.Body is closed; reference callAccountVerifier and accountVerifierHTTPClient.Post in your changes.
270-291:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop writing raw email addresses into verifier logs and wrapped errors.
This path includes
verifyAccountlogs those errors verbatim. That turns a monitoring-only integration into a PII sink in application logs.Suggested fix
- resp, err := accountVerifierHTTPClient.Post(verifierURL+"/verify-account", "application/json", bytes.NewReader(reqBody)) + resp, err := accountVerifierHTTPClient.Post(verifierURL+"/verify-account", "application/json", bytes.NewReader(reqBody)) if err != nil { - return errs.Wrapf(err, "failed to call account verifier for email %s", email) + return errs.Wrapf(err, "failed to call account verifier for email_hash %s", hash.EncodeString(email)) } @@ respBody, err := io.ReadAll(resp.Body) if err != nil { - return errs.Wrapf(err, "failed to read account verifier response for email %s", email) + return errs.Wrapf(err, "failed to read account verifier response for email_hash %s", hash.EncodeString(email)) } if resp.StatusCode != http.StatusOK { - return fmt.Errorf("account verifier returned status %d for email %s: %s", resp.StatusCode, email, string(respBody)) + return fmt.Errorf("account verifier returned status %d for email_hash %s: %s", resp.StatusCode, hash.EncodeString(email), string(respBody)) } @@ var verifierResp accountVerifierResponse if err := json.Unmarshal(respBody, &verifierResp); err != nil { - return errs.Wrapf(err, "failed to unmarshal account verifier response for email %s", email) + return errs.Wrapf(err, "failed to unmarshal account verifier response for email_hash %s", hash.EncodeString(email)) } - log.Info(ctx, fmt.Sprintf("account verifier result for %s: result=%s, reasons=%v, error=%s", - email, verifierResp.Result, verifierResp.Reasons, verifierResp.Error)) + log.Info(ctx, fmt.Sprintf("account verifier result for email_hash %s: result=%s, reasons=%v, error=%s", + hash.EncodeString(email), verifierResp.Result, verifierResp.Reasons, verifierResp.Error))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/signup/service/signup_service.go` around lines 270 - 291, The code currently injects the raw email into wrapped errors and logs (see the Post call, error wraps around accountVerifierHTTPClient.Post, io.ReadAll, json.Unmarshal, the fmt.Errorf for non-200, and the log.Info line) — remove the raw email from all those messages and instead compute and use a non-PII identifier (e.g., hashEmail(email) or redactEmail(email) helper) and reference that identifier in wrapped errors and logs; update the fmt.Errorf, errs.Wrapf calls, and the log.Info call to include only the masked/hashed identifier (not the plaintext email) and add a small helper function (maskEmail/hashEmail) used by the function (e.g., verifyAccount) to generate the safe token.pkg/signup/service/signup_service_test.go (1)
1475-1492:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace the lingering Gin test contexts with Echo.
These two subtests at lines 1476 and 1514 still call
gin.CreateTestContext, but the file no longer imports Gin and the service now expectsecho.Context. The test will not compile.Suggested fix
- rr := httptest.NewRecorder() - ctx, _ := gin.CreateTestContext(rr) + e := echo.New() + rr := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/", nil) + ctx := e.NewContext(req, rr)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/signup/service/signup_service_test.go` around lines 1475 - 1492, The test is still creating a Gin test context (gin.CreateTestContext) but SignupService().Signup now expects an echo.Context, so replace the Gin setup with Echo's test context creation: create an echo.New() instance, build an httptest.NewRequest (method and URL can be placeholders), use httptest.NewRecorder for the response, then call e.NewContext(req, rr) to get an echo.Context and call ctx.Set for the same keys (context.UsernameKey, context.SubKey, etc.) before invoking application.SignupService().Signup(ctx); ensure variable names (rr, ctx, application, SignupService().Signup) remain consistent.pkg/controller/signup_test.go (1)
339-353:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest now has dead branches and weakened coverage.
If Echo's binding for this payload reliably returns an empty body with status 400, the early return at Line 340-342 makes the JSON-body assertions at Lines 344-353 unreachable dead code. If the body is sometimes non-empty, the early return silently bypasses assertions that the original test relied on (
Field validation for 'CountryCode' failed..., etc.), which means a regression in validation error reporting could go undetected.Please pick one path: either update the assertions to match Echo's actual response shape for this case (and remove the early return), or drop the now-unreachable block entirely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/signup_test.go` around lines 339 - 353, The test currently contains a dead/unreliable branch: if rr.Body.Bytes() is empty it returns early but still has JSON-body assertions that are never reached; pick one path—here remove the now-unreachable JSON-parsing assertions (the block using bodyParams, json.Unmarshal, messageLines and the require.* checks) and replace them with explicit assertions that reflect Echo's behavior (assert rr.Code == 400 and assert that rr.Body.Bytes() is empty or rr.Body.Len() == 0); this keeps the early-return behavior but makes the test deterministic by only asserting the empty-body 400 response instead of parsing JSON.
♻️ Duplicate comments (3)
pkg/log/log.go (2)
195-202:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
(*Logger).WithValuesstill dereferences the package-globallogger.
nl := newLogger(logger.name)will panic ifInit()hasn't run, even when the receiverlis a valid logger (e.g., the package-levelWithValueswrapper falls back toensureLogger()-style behavior in the global path). Use the receiver's name.Proposed fix
func (l *Logger) WithValues(keysAndValues map[string]interface{}) *Logger { if len(keysAndValues) > 0 { - nl := newLogger(logger.name) + nl := newLogger(l.name) nl.logr = nl.logr.WithValues(slice(keysAndValues)...) return nl } return l }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/log/log.go` around lines 195 - 202, The WithValues method currently constructs a new logger using the package-global logger.name which can be nil if Init() hasn't run; change it to use the receiver's name (use l.name) when creating nl (i.e., call newLogger with l.name) so the method operates on the valid receiver rather than dereferencing the global logger; keep the rest of the logic (nl.logr = nl.logr.WithValues(slice(keysAndValues)...), return nl or l) unchanged.
148-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize the URL before logging it in
InfoEchof.
ctx.Request().URLincludes the raw query string.InfoEchofis used on auth/proxy flows where query params can carry OAuthcode, tokens, and other sensitive values. UnlikeaddRequestInfo, this path does not redacttoken(or other) parameters, so they will land in logs verbatim.Proposed fix
ctxFields = append(ctxFields, "url") - ctxFields = append(ctxFields, ctx.Request().URL) + if u := ctx.Request().URL; u != nil { + safeURL := *u + safeURL.RawQuery = "" + ctxFields = append(ctxFields, safeURL.String()) + } else { + ctxFields = append(ctxFields, "") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/log/log.go` around lines 148 - 149, InfoEchof is currently logging ctx.Request().URL including raw query params which can leak sensitive values; update the code that appends ctx.Request().URL to instead sanitize or redact sensitive query parameters (e.g., token, code, access_token) before logging — either by reusing the sanitization used in addRequestInfo or by building a copy of ctx.Request().URL with RawQuery removed or masked — and then append that sanitized URL to ctxFields (refer to InfoEchof, ctx.Request().URL, and addRequestInfo to locate the relevant logic).pkg/middleware/jwt_middleware.go (1)
63-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not log the raw JWT claims segment.
parts[1]is the base64-encoded JWT claims payload; trivially decodable from logs and typically contains PII (email, name, etc.). The structured fields (user_id,account_id,preferred_username,subject) already provide enough context to diagnose missing claims; droprawClaimsto avoid leaking token contents and creating a GDPR/compliance risk.Proposed fix
if token.UserID == "" || token.AccountID == "" { - rawClaims := "" - - parts := strings.Split(tokenStr, ".") - - if len(parts) >= 2 { - rawClaims = parts[1] - } - - log.Infof(c, "Missing essential claims from token - [user_id:%s][account_id:%s] for user [%s], sub [%s]. Raw claims segment: [%s]", - token.UserID, token.AccountID, token.PreferredUsername, token.Subject, rawClaims) + log.Infof(c, "Missing essential claims from token - [user_id:%s][account_id:%s] for user [%s], sub [%s]", + token.UserID, token.AccountID, token.PreferredUsername, token.Subject) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/middleware/jwt_middleware.go` around lines 63 - 73, The log is exposing the raw JWT payload (rawClaims) by splitting tokenStr and including parts[1]; remove the rawClaims variable and the parts/tokenStr splitting, and update the log call (log.Infof) to only include structured fields token.UserID, token.AccountID, token.PreferredUsername and token.Subject (or their zero values) so no base64 payload is written; delete the unused rawClaims/parts logic and ensure the conditional around token.UserID/token.AccountID remains intact.
🧹 Nitpick comments (3)
pkg/proxy/handlers/spacelister.go (1)
77-82: ⚡ Quick winPrefer Echo-idiomatic response writing in
errorResponse.Writing directly through
ctx.Response().Writerbypasses Echo's response bookkeeping (e.g.,Response.Status,Committedflag, registered after-response hooks). Usectx.JSONso status/size are tracked correctly — this also makes the existingpromhttp_middlewareresponseStatushelper see the right status when this path is hit.♻️ Proposed refactor
func errorResponse(ctx echo.Context, err *apierrors.StatusError) error { ctx.Logger().Error(errs.Wrap(err, "workspace list error")) - ctx.Response().Writer.Header().Set("Content-Type", "application/json") - ctx.Response().Writer.WriteHeader(int(err.ErrStatus.Code)) - return json.NewEncoder(ctx.Response().Writer).Encode(err.ErrStatus) + return ctx.JSON(int(err.ErrStatus.Code), err.ErrStatus) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/proxy/handlers/spacelister.go` around lines 77 - 82, The errorResponse function currently writes directly to ctx.Response().Writer which bypasses Echo's response bookkeeping; change errorResponse(ctx echo.Context, err *apierrors.StatusError) to use Echo's ctx.JSON(int(err.ErrStatus.Code), err.ErrStatus) (after logging via ctx.Logger().Error) so the status, headers and committed flag are set correctly and after-response hooks and promhttp_middleware.responseStatus observe the right status; remove direct Writer.Header/WriteHeader/Encode usage and return the ctx.JSON call result.pkg/verification/service/verification_service_test.go (1)
907-989: ⚡ Quick winPass
newEchoTestContext()instead ofniltoPhoneNumberAlreadyInUse.These call sites pass
nilas theecho.Context. The implementation inverification_service.go(line 453) callscontext.RequestContext(ctx)on it, so the tests rely on that helper's nil-tolerance to avoid a panic. Since you already havenewEchoTestContext()in this file, using it here makes the tests more resilient to future changes in the context helpers and keeps the test pattern consistent with the rest of the suite.Proposed change (apply to all six call sites)
- err := verificationservice.PhoneNumberAlreadyInUse(nil, nsdClient, "jsmith", "+987654321") + err := verificationservice.PhoneNumberAlreadyInUse(newEchoTestContext(), nsdClient, "jsmith", "+987654321")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/verification/service/verification_service_test.go` around lines 907 - 989, Tests call verificationservice.PhoneNumberAlreadyInUse with a nil echo.Context which relies on internal nil-tolerance; replace those nil args with a real test context by passing newEchoTestContext() wherever PhoneNumberAlreadyInUse(nil, nsdClient, ...) is used (all six call sites) so the calls become PhoneNumberAlreadyInUse(newEchoTestContext(), nsdClient, ...); search for PhoneNumberAlreadyInUse invocations in this file and update each nil context to newEchoTestContext() to match the rest of the suite.pkg/signup/service/signup_service.go (1)
385-530: 🏗️ Heavy liftSplit
DoGetSignupbefore the next behavior change.This method now owns polling, claim auditing, state classification, MUR lookup, ToolchainStatus lookup, and response shaping in one path. The Echo migration made an already complex flow harder to reason about, and the current cognitive-complexity warning looks justified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/signup/service/signup_service.go` around lines 385 - 530, DoGetSignup is doing too many responsibilities (polling/auditing, state classification, MUR lookup, ToolchainStatus lookup, and response shaping); refactor by extracting clear helper functions and wiring them from DoGetSignup: implement fetchAndAuditUserSignup(ctx, cl, username) to encapsulate the PollUpdateSignup block and return the UserSignup, implement classifySignupState(userSignup, checkUserSignupCompleted) to encapsulate the approval/complete/deactivated/banned checks and return an early Status/decision, implement fetchMURAndReady(ctx, cl, compliantUsername) to retrieve the MasterUserRecord and its ready condition, and implement enrichResponseWithClusterInfo(ctx, cl, userSignup, mur, signupResponse) to fetch ToolchainStatus, set URLs, Start/End dates and default namespace; then simplify DoGetSignup to call these helpers in sequence and return the shaped signupResponse or early nil/ForbiddenBannedError as appropriate. Ensure you preserve existing behavior and logging and update unit tests to cover the new helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/controller/signup_test.go`:
- Around line 339-353: The test currently contains a dead/unreliable branch: if
rr.Body.Bytes() is empty it returns early but still has JSON-body assertions
that are never reached; pick one path—here remove the now-unreachable
JSON-parsing assertions (the block using bodyParams, json.Unmarshal,
messageLines and the require.* checks) and replace them with explicit assertions
that reflect Echo's behavior (assert rr.Code == 400 and assert that
rr.Body.Bytes() is empty or rr.Body.Len() == 0); this keeps the early-return
behavior but makes the test deterministic by only asserting the empty-body 400
response instead of parsing JSON.
In `@pkg/errors/error.go`:
- Around line 19-24: The AbortWithError function dereferences err
unconditionally causing a panic when callers pass nil; update AbortWithError to
guard against a nil error by computing the message safely (e.g., if err == nil
use an empty string or a default message) before building the Error struct, and
use that safeMessage in place of err.Error() so AbortWithError and any callers
will not panic when err is nil.
In `@pkg/proxy/handlers/spacelister.go`:
- Around line 42-54: In SpaceLister.GetProvisionedUserSignup replace the nil
context passed to GetSignupFunc with the actual request context so DoGetSignup
and downstream calls (which call context.RequestContext(ctx), PollUpdateSignup,
auditUserSignupAgainstClaims) receive a valid context; specifically, call
GetSignupFunc with the incoming request context (e.g., derive from the Echo
context via ctx.Request().Context() or context.RequestContext(ctx)) instead of
nil, preserving the username and flag arguments.
In `@pkg/signup/service/signup_service_test.go`:
- Around line 1475-1492: The test is still creating a Gin test context
(gin.CreateTestContext) but SignupService().Signup now expects an echo.Context,
so replace the Gin setup with Echo's test context creation: create an echo.New()
instance, build an httptest.NewRequest (method and URL can be placeholders), use
httptest.NewRecorder for the response, then call e.NewContext(req, rr) to get an
echo.Context and call ctx.Set for the same keys (context.UsernameKey,
context.SubKey, etc.) before invoking application.SignupService().Signup(ctx);
ensure variable names (rr, ctx, application, SignupService().Signup) remain
consistent.
In `@pkg/signup/service/signup_service.go`:
- Around line 264-273: callAccountVerifier currently uses
accountVerifierHTTPClient.Post which ignores the Echo request context; update
callAccountVerifier to build an *http.Request using the Echo request context
(ctx.Request().Context()) — e.g. create a POST request to
verifierURL+"/verify-account" with bytes.NewReader(reqBody), set the
"Content-Type: application/json" header, then call
accountVerifierHTTPClient.Do(req) instead of Post, handle the returned response
and error the same way, and ensure resp.Body is closed; reference
callAccountVerifier and accountVerifierHTTPClient.Post in your changes.
- Around line 270-291: The code currently injects the raw email into wrapped
errors and logs (see the Post call, error wraps around
accountVerifierHTTPClient.Post, io.ReadAll, json.Unmarshal, the fmt.Errorf for
non-200, and the log.Info line) — remove the raw email from all those messages
and instead compute and use a non-PII identifier (e.g., hashEmail(email) or
redactEmail(email) helper) and reference that identifier in wrapped errors and
logs; update the fmt.Errorf, errs.Wrapf calls, and the log.Info call to include
only the masked/hashed identifier (not the plaintext email) and add a small
helper function (maskEmail/hashEmail) used by the function (e.g., verifyAccount)
to generate the safe token.
---
Duplicate comments:
In `@pkg/log/log.go`:
- Around line 195-202: The WithValues method currently constructs a new logger
using the package-global logger.name which can be nil if Init() hasn't run;
change it to use the receiver's name (use l.name) when creating nl (i.e., call
newLogger with l.name) so the method operates on the valid receiver rather than
dereferencing the global logger; keep the rest of the logic (nl.logr =
nl.logr.WithValues(slice(keysAndValues)...), return nl or l) unchanged.
- Around line 148-149: InfoEchof is currently logging ctx.Request().URL
including raw query params which can leak sensitive values; update the code that
appends ctx.Request().URL to instead sanitize or redact sensitive query
parameters (e.g., token, code, access_token) before logging — either by reusing
the sanitization used in addRequestInfo or by building a copy of
ctx.Request().URL with RawQuery removed or masked — and then append that
sanitized URL to ctxFields (refer to InfoEchof, ctx.Request().URL, and
addRequestInfo to locate the relevant logic).
In `@pkg/middleware/jwt_middleware.go`:
- Around line 63-73: The log is exposing the raw JWT payload (rawClaims) by
splitting tokenStr and including parts[1]; remove the rawClaims variable and the
parts/tokenStr splitting, and update the log call (log.Infof) to only include
structured fields token.UserID, token.AccountID, token.PreferredUsername and
token.Subject (or their zero values) so no base64 payload is written; delete the
unused rawClaims/parts logic and ensure the conditional around
token.UserID/token.AccountID remains intact.
---
Nitpick comments:
In `@pkg/proxy/handlers/spacelister.go`:
- Around line 77-82: The errorResponse function currently writes directly to
ctx.Response().Writer which bypasses Echo's response bookkeeping; change
errorResponse(ctx echo.Context, err *apierrors.StatusError) to use Echo's
ctx.JSON(int(err.ErrStatus.Code), err.ErrStatus) (after logging via
ctx.Logger().Error) so the status, headers and committed flag are set correctly
and after-response hooks and promhttp_middleware.responseStatus observe the
right status; remove direct Writer.Header/WriteHeader/Encode usage and return
the ctx.JSON call result.
In `@pkg/signup/service/signup_service.go`:
- Around line 385-530: DoGetSignup is doing too many responsibilities
(polling/auditing, state classification, MUR lookup, ToolchainStatus lookup, and
response shaping); refactor by extracting clear helper functions and wiring them
from DoGetSignup: implement fetchAndAuditUserSignup(ctx, cl, username) to
encapsulate the PollUpdateSignup block and return the UserSignup, implement
classifySignupState(userSignup, checkUserSignupCompleted) to encapsulate the
approval/complete/deactivated/banned checks and return an early Status/decision,
implement fetchMURAndReady(ctx, cl, compliantUsername) to retrieve the
MasterUserRecord and its ready condition, and implement
enrichResponseWithClusterInfo(ctx, cl, userSignup, mur, signupResponse) to fetch
ToolchainStatus, set URLs, Start/End dates and default namespace; then simplify
DoGetSignup to call these helpers in sequence and return the shaped
signupResponse or early nil/ForbiddenBannedError as appropriate. Ensure you
preserve existing behavior and logging and update unit tests to cover the new
helpers.
In `@pkg/verification/service/verification_service_test.go`:
- Around line 907-989: Tests call verificationservice.PhoneNumberAlreadyInUse
with a nil echo.Context which relies on internal nil-tolerance; replace those
nil args with a real test context by passing newEchoTestContext() wherever
PhoneNumberAlreadyInUse(nil, nsdClient, ...) is used (all six call sites) so the
calls become PhoneNumberAlreadyInUse(newEchoTestContext(), nsdClient, ...);
search for PhoneNumberAlreadyInUse invocations in this file and update each nil
context to newEchoTestContext() to match the rest of the suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 92ed7629-ed06-4136-990b-b641a717799b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (49)
go.modpkg/application/service/services.gopkg/assets/assets.gopkg/context/keys.gopkg/controller/analytics.gopkg/controller/analytics_test.gopkg/controller/authconfig.gopkg/controller/authconfig_test.gopkg/controller/health_check.gopkg/controller/health_check_test.gopkg/controller/namespaces.gopkg/controller/namespaces_test.gopkg/controller/signup.gopkg/controller/signup_test.gopkg/controller/uiconfig.gopkg/controller/uiconfig_test.gopkg/controller/usernames.gopkg/controller/usernames_test.gopkg/errors/error.gopkg/errors/error_test.gopkg/log/log.gopkg/log/log_test.gopkg/middleware/jwt_middleware.gopkg/middleware/promhttp_middleware.gopkg/namespaces/namespaces_manager.gopkg/namespaces/namespaces_manager_test.gopkg/proxy/handlers/spacelister.gopkg/proxy/handlers/spacelister_get_test.gopkg/proxy/handlers/spacelister_list_test.gopkg/proxy/proxy.gopkg/proxy/proxy_test.gopkg/server/metrics_server.gopkg/server/routes.gopkg/server/routes_test.gopkg/server/server.gopkg/server/server_test.gopkg/signup/service/signup_service.gopkg/signup/service/signup_service_test.gopkg/signup/signup.gopkg/signup/socialevent.gopkg/signup/socialevent_test.gopkg/verification/captcha/captcha.gopkg/verification/sender/amazon_sns_sender.gopkg/verification/sender/sender.gopkg/verification/sender/twilio_sender.gopkg/verification/sender/twilio_sender_test.gopkg/verification/service/verification_service.gopkg/verification/service/verification_service_test.gotest/fake/proxy.go
💤 Files with no reviewable changes (1)
- go.mod
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/context/keys.gopkg/application/service/services.gopkg/proxy/handlers/spacelister_list_test.gopkg/proxy/handlers/spacelister_get_test.gopkg/errors/error_test.gopkg/log/log_test.gopkg/server/routes.gopkg/signup/socialevent_test.gopkg/middleware/jwt_middleware.gopkg/server/server_test.gopkg/verification/sender/amazon_sns_sender.gopkg/signup/signup.gopkg/controller/usernames_test.gopkg/controller/authconfig_test.gopkg/verification/sender/sender.gopkg/server/routes_test.gopkg/verification/sender/twilio_sender.gopkg/controller/namespaces_test.gotest/fake/proxy.gopkg/namespaces/namespaces_manager.gopkg/signup/service/signup_service_test.gopkg/controller/health_check_test.gopkg/controller/namespaces.gopkg/controller/analytics_test.gopkg/proxy/proxy_test.gopkg/middleware/promhttp_middleware.gopkg/proxy/handlers/spacelister.gopkg/verification/captcha/captcha.gopkg/verification/sender/twilio_sender_test.gopkg/controller/analytics.gopkg/assets/assets.gopkg/controller/uiconfig_test.gopkg/server/server.gopkg/server/metrics_server.gopkg/controller/uiconfig.gopkg/controller/authconfig.gopkg/verification/service/verification_service_test.gopkg/controller/usernames.gopkg/namespaces/namespaces_manager_test.gopkg/errors/error.gopkg/controller/signup.gopkg/controller/signup_test.gopkg/signup/service/signup_service.gopkg/proxy/proxy.gopkg/controller/health_check.gopkg/signup/socialevent.gopkg/verification/service/verification_service.gopkg/log/log.go
🪛 GitHub Check: SonarCloud Code Analysis
pkg/context/keys.go
[warning] 10-10: Complete the task associated to this TODO comment.
pkg/controller/signup_test.go
[warning] 567-567: This function has 8 parameters, which is greater than the 7 authorized.
pkg/signup/service/signup_service.go
[failure] 385-385: Refactor this method to reduce its Cognitive Complexity from 33 to the 15 allowed.
pkg/verification/service/verification_service.go
[failure] 65-65: Refactor this method to reduce its Cognitive Complexity from 31 to the 15 allowed.
[failure] 221-221: Refactor this method to reduce its Cognitive Complexity from 42 to the 15 allowed.
[failure] 385-385: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
🔇 Additional comments (24)
pkg/signup/socialevent.go (1)
20-23: Echo context migration in social event lookup is consistent and correct.The updated signature and request-context usage preserve behavior while aligning with the new framework direction.
pkg/signup/socialevent_test.go (1)
24-27: Test context setup is correctly migrated to Echo.The new context creation path is compatible with
GetAndValidateSocialEvent(ctx echo.Context, ...)and keeps the test intent unchanged.pkg/verification/captcha/captcha.go (1)
9-60: LGTM — clean Echo migration.The four changed lines are a correct mechanical substitution:
- The
echo.Contextimport and updatedAssessorinterface/Helpersignature are consistent with the rest of the codebase migration and the test mock (context snippet 2).- Passing
ctx.Request().Context()toclient.CreateAssessment(line 60) is actually an improvement over the previous behaviour — it propagates request cancellation/deadline to the gRPC call.- The caller in
signup_service.go(context snippet 1, line 208) already guardsctx.Request() == nilbefore invokingCompleteAssessment, so no nil-deref risk is introduced at line 60.pkg/proxy/proxy_test.go (1)
29-29: LGTM — clean Echo signature update for the test scaffolding.The
OverrideGetSignupFunccallback type and override implementation are consistently migrated toecho.Context, matchinghandlers.SpaceLister.GetSignupFuncand the rest of the migration.Also applies to: 518-518, 683-683
pkg/assets/assets.go (1)
11-13: LGTM — replacinggin-contrib/staticwithfs.Subis a sound simplification.Returning a plain
fs.FSrooted atstatic/is exactly what Echo'sStaticFS/MustSubFSconsumes, and dropping the Gin-specific wrapper removes a dependency without behavior change.pkg/controller/authconfig.go (1)
31-39: LGTM — idiomatic Echo handler.Returning
ctx.JSON(...)and propagating its error matches Echo conventions; payload construction is unchanged.pkg/controller/authconfig_test.go (1)
48-57: LGTM — Echo-based test wiring is correct.Using
echo.New().NewContext(req, rr)andrequire.Containson the Content-Type accommodates Echo'sapplication/json; charset=UTF-8while still validating the media type.pkg/controller/analytics.go (1)
21-32: LGTM — straightforward Echo migration for both write-key endpoints.Behavior and status codes are preserved;
ctx.Stringis the right choice for the plain-text payload.pkg/controller/analytics_test.go (1)
44-54: LGTM — tests correctly assert the new error-returning Echo handlers.
require.NoError(err)before status/body assertions matches the updated controller signatures and prevents masking handler failures.Also applies to: 77-87
pkg/controller/health_check.go (1)
37-63: LGTM — interface and implementations are migrated consistently.
HealthChecker.Alive/APIProxyAlivenow takeecho.Context, andgetHealthInfo/GetHandlerpropagate it correctly. Status mapping (200 vs 503) and the proxy probe logic are preserved.Also applies to: 73-90
pkg/context/keys.go (1)
12-29: LGTM — nil-safe Echo context helpers.
RequestContextcorrectly guards against a nil Echo context and a nil underlyinghttp.Request(useful in tests), andGetStringreturns "" on missing/non-string values via a single type-assertedctx.Get. Good fit as the canonical accessors for the rest of the Echo migration.pkg/proxy/handlers/spacelister_list_test.go (1)
124-124: LGTM — signatures consistently aligned with the Echo-basedGetSignupFunc.Also applies to: 183-183
pkg/proxy/handlers/spacelister_get_test.go (1)
78-78: LGTM — Echo context setup and updatedoverrideSignupFuncsignatures look consistent.Also applies to: 272-272, 622-622, 675-675
pkg/verification/sender/sender.go (1)
8-13: LGTM — interface migration toecho.Contextmatches both Twilio and SNS implementations.pkg/verification/sender/twilio_sender_test.go (1)
88-91: LGTM — Echo context construction is correct for both subtests.Also applies to: 105-108
pkg/verification/sender/amazon_sns_sender.go (1)
9-9: LGTM — signature migration is clean; using_for the unused context is appropriate.Also applies to: 30-30
pkg/verification/sender/twilio_sender.go (1)
10-10: LGTM — Echo context is correctly threaded through tolog.Error.Also applies to: 47-47
pkg/middleware/promhttp_middleware.go (1)
16-69: LGTM — Echo migration withresponseStatushelper andc.Path()cleanly addresses prior cardinality and error-status concerns.The centralized
responseStatuskeeps both counter and histogram middlewares consistent, and falling back throughcrterrors.StatusCode(err)correctly handles the case where Echo's error handler hasn't yet committed the response.pkg/server/metrics_server.go (1)
19-45: LGTM!The migration to Echo is correctly implemented:
*echo.Echosatisfieshttp.HandlerviaServeHTTP,HideBanner/HidePortare appropriate for an embedded metrics server, andecho.WrapHandleris the idiomatic way to bridgepromhttp.InstrumentMetricHandlerinto Echo's routing.pkg/controller/health_check_test.go (1)
47-225: LGTM!Test migration is straightforward:
echo.New().NewContext(req, rr)correctly wires the recorder, the handler now returns anerrorchecked viarequire.NoError, and the mock health checker acceptsecho.Contextconsistently with the updatedHealthCheckerinterface.pkg/controller/usernames_test.go (1)
51-120: LGTM!Echo path parameters via
SetParamNames/SetParamValuesand directGetHandler(ctx)invocation match the new controller signature. The error subtest (lines 119–123) correctly relies ontest.AssertErrorinspecting the response recorder, since the handler writes the error response intorrand returnsnil.pkg/verification/service/verification_service.go (1)
65-481: LGTM!The Echo migration consistently propagates the request-scoped context via
context.RequestContext(ctx)into allGet/Update/Listcalls, including inside thedoUpdateretry closures. The newPhoneNumberAlreadyInUse(ctx echo.Context, ...)signature derivesreqCtxonce and reuses it for both List calls, which is clean.Note: SonarCloud's cognitive-complexity flags on
InitVerificationandVerifyPhoneCodeare pre-existing and unrelated to this refactor; addressing them is out of scope for this PR.pkg/application/service/services.go (1)
6-18: LGTM!Interface signatures cleanly migrated to
echo.Context; no behavioral changes here.pkg/controller/namespaces.go (1)
35-51: ⚡ Quick win
crterrors.AbortWithErrorcorrectly writes the response and returnsnil, preventing any conflict with Echo's error handler.The implementation uses
ctx.JSON(code, ...)which writes the JSON response with the specified status code and implicitly returnsnil. This means the response body is written before control returns to the Echo framework, and no error is propagated to Echo's error handler—the pattern is sound.
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 participants
The registration service used both the Echo and Gin frameworks to provide its services, and we wanted to remove the dependency with Echo and just keep Gin to improve maintainability.
Assited-by: claude-4.6 with Cursor
Jira ticket
[SANDBOX-316]
Summary by CodeRabbit