🌱 add a client to support token request - #189
Conversation
WalkthroughAdds a CloudEvents-driven Kubernetes ServiceAccount client and TokenRequest codec, with unit and integration tests for token issuance over gRPC, and updates SAR authorization handling and server logging configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@pkg/cloudevents/clients/serviceaccount/codec.go`:
- Around line 50-56: In TokenRequestCodec.Decode, add a nil check for the
incoming evt and return a clear error if evt == nil, then on DataAs failure do
not include raw event bytes (evt.Data()) in the error; instead return a safe
error message that references TokenRequestCodec.Decode and
authenticationv1.TokenRequest and optionally include non-sensitive metadata
(e.g. data length or error type) but never the raw token data; ensure the
function still returns nil, error on failure and the parsed
*authenticationv1.TokenRequest on success.
- Around line 31-46: Guard against a nil tokenRequest at the start of
TokenRequestCodec.Encode by returning a descriptive error if tokenRequest ==
nil, and move any dereferences (like tokenRequest.UID and
tokenRequest.Namespace) after that guard; also avoid potentially incorrect %s
formatting for eventType.CloudEventsDataType by using %v (or including the value
via fmt.Sprintf) in the fmt.Errorf call so the error message formats the data
type correctly. Ensure you reference TokenRequestCodec.Encode,
TokenRequestDataType, types.NewEventBuilder, and evt.SetData when making the
changes.
In `@pkg/cloudevents/clients/serviceaccount/integration_test.go`:
- Around line 54-75: The HandleStatusUpdate method may panic if s.handler is
nil; update tokenRequestService.HandleStatusUpdate to check s.handler before
calling s.handler.OnCreate: after storing s.tokenRequests[string(tokenReq.UID)]
= tokenReq verify if s.handler != nil and only call s.handler.OnCreate when
non-nil (otherwise return a non-nil error or nil per existing behavior),
ensuring no nil pointer dereference occurs when RegisterHandler has not been
called.
- Around line 301-314: The test has a data race on the receivedUIDs map: protect
concurrent access by adding a sync.Mutex (or sync.RWMutex) alongside
receivedUIDs in the test scope, then lock before writing to receivedUIDs in
tokenRequestService.handleStatusUpdateFn and lock/ RLock before
reading/asserting receivedUIDs in the test goroutine; ensure you unlock
appropriately around the map access used by the assertion so reads see the
updated entries.
🧹 Nitpick comments (4)
pkg/cloudevents/clients/serviceaccount/integration_test.go (4)
83-103: Redundant cleanup:defer grpcServer.Stop()andt.CleanupwithGracefulStop.The
defer grpcServer.Stop()at line 84 andgrpcServer.GracefulStop()int.Cleanupat line 101 are redundant. Sincet.Cleanupruns after the test function returns, both will execute, potentially causing double-stop issues. Keep only thet.Cleanupapproach for cleaner resource management.Proposed fix
// Set up gRPC server grpcServer := grpc.NewServer() - defer grpcServer.Stop() grpcEventServer := grpcserver.NewGRPCBroker(grpcserver.NewBrokerOptions())
163-164: Same redundant cleanup issue.Same as
TestCreateToken_Integration- removedefer grpcServer.Stop()sincet.Cleanuphandles shutdown.
197-199: Mutating package-levelTokenRequestTimeoutis not parallel-safe.If tests run with
t.Parallel(), concurrent modification ofTokenRequestTimeoutcan cause race conditions and flaky tests. Consider accepting the timeout as a client option or using a test-specific client configuration instead.
257-290: Consider strengthening test assertions.The test allows failures with "may be timing issue" and only requires 1 out of 2 successes. This is a weak assertion that could mask real bugs. If timing issues are expected, consider:
- Investigating root cause of connection reuse issues
- Increasing the delay or using exponential backoff
- At minimum, requiring all requests to succeed since
numRequestsis only 2Proposed stricter assertion
- // At least one request should succeed - if successCount < 1 { - t.Errorf("Expected at least 1 successful request, got %d", successCount) + // All requests should succeed + if successCount != numRequests { + t.Errorf("Expected all %d requests to succeed, got %d", numRequests, successCount) }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/cloudevents/clients/serviceaccount/client.go`:
- Around line 81-135: The CreateToken method currently dereferences tokenRequest
via tokenRequest.DeepCopy() and will panic if tokenRequest is nil; add a nil
guard at the top of CreateToken (before tokenRequestCtx or use the incoming ctx)
that checks if tokenRequest == nil and returns a clear error (e.g., fmt.Errorf
or errors.New) so the function fails fast; update callers/tests if needed and
reference the tokenRequest parameter, the CreateToken method, and the
newTokenRequest assignment where DeepCopy() is called.
In `@pkg/cloudevents/server/grpc/authz/kube/sar.go`:
- Around line 207-212: In the serviceaccount.TokenRequestDataType branch, change
the SAR mapping to set sar.Spec.ResourceAttributes.Resource = "serviceaccounts"
and sar.Spec.ResourceAttributes.Subresource = "token" (keep Group = "" and Verb
= "create"), and when the service account name is available populate
sar.Spec.ResourceAttributes.Name with that name so the subresource token
authorization checks align correctly; update the case handling around
serviceaccount.TokenRequestDataType to apply these fields instead of using
"serviceaccounts/token".
♻️ Duplicate comments (4)
pkg/cloudevents/clients/serviceaccount/integration_test.go (2)
54-75: Guard against nil handler in HandleStatusUpdate.🛠️ Suggested fix
s.tokenRequests[string(tokenReq.UID)] = tokenReq // Notify the handler that we have a response ready + if s.handler == nil { + return nil + } return s.handler.OnCreate(ctx, TokenRequestDataType, string(tokenReq.UID))
300-372: Protect receivedUIDs with a mutex to avoid data races.🛠️ Suggested fix
+ var mu sync.Mutex // Create a service that tracks UIDs receivedUIDs := make(map[types.UID]bool) @@ svc.handleStatusUpdateFn = func(ctx context.Context, evt *cloudevents.Event) error { codec := NewTokenRequestCodec() tokenReq, err := codec.Decode(evt) if err != nil { return err } + mu.Lock() receivedUIDs[tokenReq.UID] = true + mu.Unlock() @@ - if !receivedUIDs[result.UID] { + mu.Lock() + received := receivedUIDs[result.UID] + mu.Unlock() + if !received { t.Errorf("Service did not receive request with UID %s", result.UID) }pkg/cloudevents/clients/serviceaccount/codec.go (2)
32-40: Guard against nil TokenRequest and improve error formatting.🛠️ Suggested fix
func (c *TokenRequestCodec) Encode(source string, eventType types.CloudEventsType, tokenRequest *authenticationv1.TokenRequest) (*cloudevents.Event, error) { + if tokenRequest == nil { + return nil, fmt.Errorf("tokenRequest is nil") + } if eventType.CloudEventsDataType != TokenRequestDataType { - return nil, fmt.Errorf("unsupported cloudevents data type %s", eventType.CloudEventsDataType) + return nil, fmt.Errorf("unsupported cloudevents data type %v", eventType.CloudEventsDataType) }
50-54: Avoid nil-event panic and leaking raw event data in errors.🛠️ Suggested fix
func (c *TokenRequestCodec) Decode(evt *cloudevents.Event) (*authenticationv1.TokenRequest, error) { + if evt == nil { + return nil, fmt.Errorf("nil cloudevent") + } tokenRequest := &authenticationv1.TokenRequest{} if err := evt.DataAs(tokenRequest); err != nil { - return nil, fmt.Errorf("failed to unmarshal event data %s, %v", string(evt.Data()), err) + return nil, fmt.Errorf("failed to unmarshal event data: %w", err) }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test_stability.sh`:
- Around line 8-17: The test loop currently detects failures by grepping
"$result" for "FAIL:", which can miss build or package errors; change the logic
in the loop that runs `go test -count=1 ./pkg/cloudevents/clients/serviceaccount
-run TestCreateToken` so you capture both the command output and its exit status
(keep the `result` variable for output and check the command's exit code `$?` or
use `if ! go test ...; then`), then increment `failed` when the exit code is
non-zero and `passed` when it is zero; keep printing the captured `result` for
diagnostics and continue using RUNS, TestCreateToken, failed, and passed
variables as before.
♻️ Duplicate comments (1)
pkg/cloudevents/clients/serviceaccount/integration_test.go (1)
297-368: GuardreceivedUIDswith a mutex to avoid a data race.
handleStatusUpdateFnruns in a gRPC handler goroutine while the assertion reads in the test goroutine. Unsynchronized map access can race or panic. This matches a previously flagged issue.🔧 Suggested fix
import ( "context" "net" + "sync" "testing" "time" @@ grpcEventServer := grpcserver.NewGRPCBroker(grpcserver.NewBrokerOptions()) pbv1.RegisterCloudEventServiceServer(grpcServer, grpcEventServer) // Create a service that tracks UIDs + var mu sync.Mutex receivedUIDs := make(map[types.UID]bool) @@ svc.handleStatusUpdateFn = func(ctx context.Context, evt *cloudevents.Event) error { codec := NewTokenRequestCodec() tokenReq, err := codec.Decode(evt) if err != nil { return err } + mu.Lock() receivedUIDs[tokenReq.UID] = true + mu.Unlock() @@ // Verify the service received a request with this UID - if !receivedUIDs[result.UID] { + mu.Lock() + received := receivedUIDs[result.UID] + mu.Unlock() + if !received { t.Errorf("Service did not receive request with UID %s", result.UID) }
🧹 Nitpick comments (2)
pkg/cloudevents/clients/serviceaccount/client.go (1)
114-123: Avoid potential blocking on duplicate responses.If multiple responses for the same UID arrive, the buffered channel can fill and block the handler goroutine. A non-blocking send (or ctx-aware send) avoids this edge case.
♻️ Proposed tweak
cloudEventsClient.Subscribe(tokenRequestCtx, func(handlerCtx context.Context, resp *authenticationv1.TokenRequest) error { if resp.UID != requestID { return nil } logger := klog.FromContext(handlerCtx) logger.V(4).Info("response token", "requestID", resp.UID, "serviceAccountName", resp.Name) - responseChan <- resp + select { + case responseChan <- resp: + case <-handlerCtx.Done(): + } return nil })pkg/cloudevents/clients/serviceaccount/integration_test.go (1)
222-288: Avoid masking failures in the multi-request test.Allowing request failures can hide regressions. Consider asserting all requests succeed and use readiness/backoff to improve determinism instead of tolerating errors.
4590693 to
8880031
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/cloudevents/clients/serviceaccount/client_test.go`:
- Around line 120-122: The inline comment above TestCreateToken is stale; update
or remove it so it no longer claims CreateToken lacks integration coverage. Edit
the comment block near TestCreateToken in
pkg/cloudevents/clients/serviceaccount/client_test.go to either delete the note
or replace it with a brief accurate statement referencing the new integration
tests (e.g., mention that integration coverage exists), ensuring the
TestCreateToken comment reflects the current test coverage.
In `@pkg/cloudevents/clients/serviceaccount/integration_test.go`:
- Around line 53-77: HandleStatusUpdate writes to the tokenRequests map without
synchronization which races with concurrent readers; add a mutex field (e.g.,
sync.RWMutex named mu) to the tokenRequestService struct and use
mu.Lock()/mu.Unlock() around the write in HandleStatusUpdate when assigning
s.tokenRequests[string(tokenReq.UID)] = tokenReq, and update any reader method
(e.g., Get) to use mu.RLock()/mu.RUnlock() to prevent data races while allowing
concurrent reads.
🧹 Nitpick comments (2)
pkg/cloudevents/clients/serviceaccount/integration_test.go (2)
83-157: Consider replacing the fixed sleep with a readiness probe.
A dial-with-timeout or retry loop will reduce flakiness in slow CI environments.
255-286: Tolerating failures hides real regressions.
The test allows requests to fail due to timing issues; consider making it deterministic (e.g., fresh client per request or explicit connection reset) so all requests must succeed.
9bab3c9 to
160cfd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cloudevents/clients/serviceaccount/codec.go`:
- Around line 54-61: TokenRequestCodec.Decode is missing a nil check for the evt
parameter which will cause a panic when calling evt.DataAs; add a guard at the
start of the Decode method to return a descriptive error if evt == nil (e.g.,
"nil event passed to TokenRequestCodec.Decode") before calling evt.DataAs on the
event to safely handle callers that pass nil.
🧹 Nitpick comments (4)
pkg/cloudevents/clients/serviceaccount/integration_test.go (3)
119-120: Consider replacingtime.Sleepwith a ready-signal mechanism.Using
time.Sleep(100 * time.Millisecond)to wait for server startup is fragile and can cause flaky tests on slow CI environments. Consider using a channel or polling mechanism to confirm the server is ready.Example approach
// Instead of time.Sleep, verify connectivity: conn, err := grpc.DialContext(ctx, lis.Addr().String(), grpc.WithInsecure(), grpc.WithBlock(), grpc.WithTimeout(time.Second)) if err != nil { t.Fatalf("failed to connect: %v", err) } conn.Close()
200-203: Mutating package-level variable creates test isolation risk.Modifying
TokenRequestTimeoutdirectly affects other tests running in parallel. Whiledeferrestores the value, consider usingt.Parallel()avoidance or injecting timeout via constructor/options for better isolation.
262-292: Lenient assertion may mask reliability issues.Requiring only 1 of 2 requests to succeed and logging failures as "timing issues" could hide genuine bugs. Consider making the test more deterministic or documenting why intermittent failures are expected.
pkg/cloudevents/clients/serviceaccount/client.go (1)
24-27: Exported mutable variable creates testing/concurrency concerns.
TokenRequestTimeoutis exported and mutable, which can lead to race conditions if modified during concurrent requests. Consider using a functional option pattern or making it a field onServiceAccountClient.Alternative approach
-var ( - // TokenRequestTimeout is the timeout for CreateToken requests - TokenRequestTimeout = 10 * time.Second -) +const defaultTokenRequestTimeout = 10 * time.Second type ServiceAccountClient struct { grpcOptions *grpc.GRPCOptions clusterName string + timeout time.Duration }
| func (c *TokenRequestCodec) Decode(evt *cloudevents.Event) (*authenticationv1.TokenRequest, error) { | ||
| tokenRequest := &authenticationv1.TokenRequest{} | ||
| if err := evt.DataAs(tokenRequest); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal event data: %w", err) | ||
| } | ||
|
|
||
| return tokenRequest, nil | ||
| } |
There was a problem hiding this comment.
Add nil guard for evt parameter.
A past review flagged this concern and it was marked as addressed, but the current code shows no nil check. If evt is nil, evt.DataAs() will panic.
Suggested fix
func (c *TokenRequestCodec) Decode(evt *cloudevents.Event) (*authenticationv1.TokenRequest, error) {
+ if evt == nil {
+ return nil, fmt.Errorf("event is nil")
+ }
tokenRequest := &authenticationv1.TokenRequest{}
if err := evt.DataAs(tokenRequest); err != nil {
return nil, fmt.Errorf("failed to unmarshal event data: %w", err)
}
return tokenRequest, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *TokenRequestCodec) Decode(evt *cloudevents.Event) (*authenticationv1.TokenRequest, error) { | |
| tokenRequest := &authenticationv1.TokenRequest{} | |
| if err := evt.DataAs(tokenRequest); err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal event data: %w", err) | |
| } | |
| return tokenRequest, nil | |
| } | |
| func (c *TokenRequestCodec) Decode(evt *cloudevents.Event) (*authenticationv1.TokenRequest, error) { | |
| if evt == nil { | |
| return nil, fmt.Errorf("event is nil") | |
| } | |
| tokenRequest := &authenticationv1.TokenRequest{} | |
| if err := evt.DataAs(tokenRequest); err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal event data: %w", err) | |
| } | |
| return tokenRequest, nil | |
| } |
🤖 Prompt for AI Agents
In `@pkg/cloudevents/clients/serviceaccount/codec.go` around lines 54 - 61,
TokenRequestCodec.Decode is missing a nil check for the evt parameter which will
cause a panic when calling evt.DataAs; add a guard at the start of the Decode
method to return a descriptive error if evt == nil (e.g., "nil event passed to
TokenRequestCodec.Decode") before calling evt.DataAs on the event to safely
handle callers that pass nil.
Signed-off-by: Wei Liu <liuweixa@redhat.com>
|
/assign @qiujian16 @elgnay |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/cloudevents/clients/serviceaccount/integration_test.go`:
- Around line 200-203: The test mutates the package-level TokenRequestTimeout
causing races; instead add an injectable timeout on ServiceAccountClient (e.g.,
a timeout field on ServiceAccountClient and an optional parameter to
NewServiceAccountClient that defaults to TokenRequestTimeout) and update the
code path that uses TokenRequestTimeout to read the client's timeout field; then
change the test to construct a ServiceAccountClient with the short timeout
rather than modifying TokenRequestTimeout. Ensure symbols updated:
TokenRequestTimeout (default), ServiceAccountClient.timeout (new), and
NewServiceAccountClient (new optional timeout parameter) and update tests to use
that constructor.
In `@pkg/server/grpc/server.go`:
- Around line 84-85: The code calls log.SetLogger(textlogger.NewLogger(...))
inside Run which mutates the global controller-runtime logger; remove that
global call and instead accept a configured logr.Logger (or an options struct)
into the server constructor/Run entrypoint and use that logger instance for
local logging; replace usages that rely on the global log with the injected
logger, and keep references to textlogger.NewLogger and textlogger.NewConfig
only where the application (main) constructs the logger before creating the
server.
♻️ Duplicate comments (1)
pkg/cloudevents/clients/serviceaccount/codec.go (1)
54-61: Missing nil guard forevtparameter.The past review correctly identified that
evt.DataAs()will panic ifevtis nil. The code still lacks this guard.Suggested fix
func (c *TokenRequestCodec) Decode(evt *cloudevents.Event) (*authenticationv1.TokenRequest, error) { + if evt == nil { + return nil, fmt.Errorf("event is nil") + } tokenRequest := &authenticationv1.TokenRequest{} if err := evt.DataAs(tokenRequest); err != nil { return nil, fmt.Errorf("failed to unmarshal event data: %w", err) } return tokenRequest, nil }
🧹 Nitpick comments (3)
pkg/cloudevents/clients/serviceaccount/client.go (1)
81-110: Consider consistent error wrapping.Errors from
BuildCloudEventsAgentOptions(line 98) andNewCloudEventAgentClient(line 109) are returned as-is, while other errors in this method are wrapped witherrors.NewInternalError. Consider wrapping these for consistency.Suggested fix
options, err := builder.BuildCloudEventsAgentOptions( sa.grpcOptions, sa.clusterName, sa.clusterName, TokenRequestDataType, ) if err != nil { - return nil, err + return nil, errors.NewInternalError(err) } cloudEventsClient, err := clients.NewCloudEventAgentClient( tokenRequestCtx, options, nil, // resync is disabled, so lister is not required nil, // resync is disabled, so statusHashGetter is not required &TokenRequestCodec{}, ) if err != nil { - return nil, err + return nil, errors.NewInternalError(err) }pkg/cloudevents/clients/serviceaccount/integration_test.go (2)
119-121: Consider replacing sleep with a ready signal.Using
time.Sleepto wait for server startup is a flaky pattern. Consider using a synchronization mechanism like a channel or polling for server readiness. This pattern repeats in other tests (lines 198, 254).Example approach using a ready channel
ready := make(chan struct{}) go func() { close(ready) // Signal that goroutine started if err := grpcServer.Serve(lis); err != nil { t.Logf("server stopped: %v", err) } }() <-ready // Optionally add a small dial check to confirm server is accepting connections
261-292: Weak assertion may mask flakiness.Asserting "at least 1 successful request" out of 2 (line 290) is lenient and may hide intermittent failures. The comment at line 266 mentioning "connection reuse issues" suggests there might be underlying problems worth investigating. Consider either strengthening the assertion or documenting the expected behavior more explicitly.
| // Override timeout for this test | ||
| originalTimeout := TokenRequestTimeout | ||
| TokenRequestTimeout = 200 * time.Millisecond | ||
| defer func() { TokenRequestTimeout = originalTimeout }() |
There was a problem hiding this comment.
Mutating package-level variable is not parallel-safe.
Modifying TokenRequestTimeout during the test will cause race conditions if tests run in parallel with t.Parallel(). While the defer restores the original value, concurrent tests could observe the modified value.
Alternative: Inject timeout via struct field or test option
Consider adding an optional timeout parameter to ServiceAccountClient or NewServiceAccountClient to allow test-specific overrides without mutating package state:
type ServiceAccountClient struct {
grpcOptions *grpc.GRPCOptions
clusterName string
timeout time.Duration // optional, defaults to TokenRequestTimeout
}🤖 Prompt for AI Agents
In `@pkg/cloudevents/clients/serviceaccount/integration_test.go` around lines 200
- 203, The test mutates the package-level TokenRequestTimeout causing races;
instead add an injectable timeout on ServiceAccountClient (e.g., a timeout field
on ServiceAccountClient and an optional parameter to NewServiceAccountClient
that defaults to TokenRequestTimeout) and update the code path that uses
TokenRequestTimeout to read the client's timeout field; then change the test to
construct a ServiceAccountClient with the short timeout rather than modifying
TokenRequestTimeout. Ensure symbols updated: TokenRequestTimeout (default),
ServiceAccountClient.timeout (new), and NewServiceAccountClient (new optional
timeout parameter) and update tests to use that constructor.
| // Set textlogger with verbosity level 4 for controller-runtime logging | ||
| log.SetLogger(textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(4)))) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
controller-runtime log.SetLogger best practices library code main
💡 Result:
Recommended practices for controller-runtime logging (log.SetLogger)
Key rules
- Call log.SetLogger exactly once in your program’s main (not in library code or init()). Libraries should not call SetLogger. [2][3]
- Use controller-runtime’s zap wrapper (pkg/log/zap) and bind its flags so runtime configuration (level/encoder) is possible. [1]
- Pass scoped logr.Logger values into libraries (or use logf.Log.WithName/WithValues inside packages) rather than setting global logger from a library. This keeps libraries testable and avoids surprising global state. [1][2]
- Prefer structured key/value logging (WithValues) and avoid formatting messages with Sprintf. Don’t log an error and also return it from a reconciler (controller-runtime will handle logging in many cases). [1][4]
Minimal main.go pattern
- bind flags, create zap logger with flag options, call log.SetLogger in main, then construct manager/controllers.
Example (concise):
package main
import (
"flag"
"os"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
)
func main() {
opts := zap.Options{}
opts.BindFlags(flag.CommandLine) // allow runtime config via flags
flag.Parse()
logger := zap.New(zap.UseFlagOptions(&opts))
logf.SetLogger(logger) // set global logger once, in main
// ... create manager, controllers, pass loggers to components ...
_ = os.Exit(0)
}
Practices for libraries/packages
- Do NOT call log.SetLogger in init() or package-level code. Instead:
- Use logf.Log.WithName("pkg") or accept a logr.Logger parameter: func NewController(l logr.Logger, ...) {...}.
- Create scoped loggers via l.WithValues(...). [2][3]
Further reading / references
- Operator SDK logging examples (zap binding & SetLogger in main). [1]
- controller-runtime manager/docs: warning not to call SetLogger in init(). [2]
- Logging message/key guidelines and avoiding double-logging. [4]
Sources
[1] Operator SDK — Logging (examples and zap flags).
[2] controller-runtime manager docs / pkg examples (don’t call SetLogger in init()).
[3] controller-runtime pkg documentation / usage patterns.
[4] Gardener logging guidelines (structured logging, WithValues, avoid double-logging).
🏁 Script executed:
# Find the Run method and its context in the server.go file
fd pkg/server/grpc/server.go -x cat -n {}Repository: open-cluster-management-io/sdk-go
Length of output: 59
🏁 Script executed:
# Check if there's an options structure and what logging options exist
rg -B 5 -A 5 "type.*options" pkg/server/grpc/server.goRepository: open-cluster-management-io/sdk-go
Length of output: 59
🏁 Script executed:
# Search for other SetLogger calls in the codebase to see patterns
rg "SetLogger" --type goRepository: open-cluster-management-io/sdk-go
Length of output: 4545
Avoid setting the global controller-runtime logger inside Run.
Line 84-85 calls log.SetLogger in library code, which violates controller-runtime best practices. SetLogger is intended to be called exactly once in main only—not in library or package code. This globally overrides the host application's logging configuration and creates races if multiple tests or servers initialize logging. Libraries should instead accept a configured logr.Logger as a parameter or gate logging setup behind an explicit option.
Suggested adjustment
-// Set textlogger with verbosity level 4 for controller-runtime logging
-log.SetLogger(textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(4))))
+// Expect caller to configure controller-runtime logging once (e.g., in main),
+// or gate it behind an explicit option.
+if b.options.ControllerRuntimeLogger != nil {
+ log.SetLogger(b.options.ControllerRuntimeLogger)
+}🤖 Prompt for AI Agents
In `@pkg/server/grpc/server.go` around lines 84 - 85, The code calls
log.SetLogger(textlogger.NewLogger(...)) inside Run which mutates the global
controller-runtime logger; remove that global call and instead accept a
configured logr.Logger (or an options struct) into the server constructor/Run
entrypoint and use that logger instance for local logging; replace usages that
rely on the global log with the injected logger, and keep references to
textlogger.NewLogger and textlogger.NewConfig only where the application (main)
constructs the logger before creating the server.
There was a problem hiding this comment.
this this fine to us, we don't use the controller-runtime to start the broker, only leverage its certwatcher
There was a problem hiding this comment.
@skeeey, understood! Since you're only leveraging the certwatcher component and not using the full controller-runtime manager setup, the global logger configuration in this context makes sense for your use case. Thank you for the clarification.
✏️ Learnings added
Learnt from: skeeey
Repo: open-cluster-management-io/sdk-go PR: 189
File: pkg/server/grpc/server.go:84-85
Timestamp: 2026-01-19T07:13:02.082Z
Learning: In the open-cluster-management-io/sdk-go repository, setting log.SetLogger in pkg/server/grpc/server.go's Run method is acceptable because the codebase only uses controller-runtime's certwatcher component, not the full controller-runtime manager/controller setup.
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.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, skeeey The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
6d087c9
into
open-cluster-management-io:main
3 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.