This repository was archived by the owner on Sep 9, 2026. It is now read-only.
NO-ISSUE: Retry once on Unauthenticated in the gRPC client - #581
Merged
Merged
Conversation
|
@jhernand: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand 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 |
Summary by CodeRabbit
WalkthroughThis pull request adds a unary gRPC client interceptor that detects gRPC codes.Unauthenticated, calls tokenSource.Invalidate(), and retries the unary RPC once. The interceptor and necessary imports are implemented, the client builder prepends the unary interceptor when a token source exists, and three tests were added to validate success-after-retry, persistent Unauthenticated, and no-retry-for-other-errors scenarios. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🧹 Nitpick comments (1)
internal/network/grpc_client_test.go (1)
341-527: ⚡ Quick winAdd one streaming retry test.
All new cases cover unary
Check, so the new stream interceptor still has no coverage. One focused streaming case would prevent unary/stream parity bugs from slipping through.🤖 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 `@internal/network/grpc_client_test.go` around lines 341 - 527, Add a new It test that mirrors the "Retries once when the server returns the unauthenticated status code" unary case but uses a streaming RPC (health.Watch) to exercise the stream interceptor: create the TLS listener and a server whose unary/stream interceptor returns codes.Unauthenticated on the first stream call and succeeds on the second (track with the same atomic.Int32 calls), set up the mock auth.TokenSource to Expect Token(...) returning a token Twice and Invalidate(...) once, build the client with SetTokenSource(tokenSource), open a stream via healthpb.NewHealthClient(client).Watch(ctx, &healthpb.HealthCheckRequest{Service:""}), receive from the stream and assert you get the SERVING status and that calls.Load() == 2; include proper server startup/teardown and client.Close() cleanup like the other tests.
🤖 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 `@internal/network/grpc_client.go`:
- Around line 371-380: The current code unconditionally prepends
unauthRetryInterceptor (constructed when b.tokenSource != nil) to all RPCs via
unaryInterceptors/streamInterceptors; change this to be opt-in or allowlisted:
add a builder flag (e.g., b.enableUnauthRetry) or an allowlist of safe method
names on the builder and only append retry.unary/retry.stream when that flag is
true or the allowlist is non-empty; also extend unauthRetryInterceptor to accept
the allowlist and check the RPC method name before performing a retry so
non-idempotent RPCs are not retried automatically (update references to
tokenSource, unauthRetryInterceptor, retry.unary, retry.stream,
unaryInterceptors, streamInterceptors accordingly).
- Around line 455-463: The stream-path error handler should log
tokenSource.Invalidate failures but preserve and return the original gRPC
Unauthenticated error instead of replacing it with invalidateErr: when
tokenSource.Invalidate(ctx) returns invalidateErr, call
i.logger.ErrorContext(...) as you do today but set err back to the original gRPC
error (the Unauthenticated error received from the stream) rather than err =
invalidateErr; i.e., keep the original error variable from the stream path (the
Unauthenticated error) and return that after logging the Invalidate failure,
referencing tokenSource.Invalidate, logger.ErrorContext, and the err/original
stream error variable.
---
Nitpick comments:
In `@internal/network/grpc_client_test.go`:
- Around line 341-527: Add a new It test that mirrors the "Retries once when the
server returns the unauthenticated status code" unary case but uses a streaming
RPC (health.Watch) to exercise the stream interceptor: create the TLS listener
and a server whose unary/stream interceptor returns codes.Unauthenticated on the
first stream call and succeeds on the second (track with the same atomic.Int32
calls), set up the mock auth.TokenSource to Expect Token(...) returning a token
Twice and Invalidate(...) once, build the client with
SetTokenSource(tokenSource), open a stream via
healthpb.NewHealthClient(client).Watch(ctx,
&healthpb.HealthCheckRequest{Service:""}), receive from the stream and assert
you get the SERVING status and that calls.Load() == 2; include proper server
startup/teardown and client.Close() cleanup like the other tests.
🪄 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: 17e3a97c-6398-4b30-b183-205cb3300273
📒 Files selected for processing (2)
internal/network/grpc_client.gointernal/network/grpc_client_test.go
jhernand
force-pushed
the
retry_on_unauthenticated
branch
2 times, most recently
from
May 25, 2026 10:54
33a131d to
3b00129
Compare
When a `TokenSource` is configured, the gRPC client now installs unary a interceptor that handlers `Unauthenticated` responses from the server. On the first such response the interceptor calls `TokenSource.Invalidate` to discard the cached token and retries the request. If the retry also returns `Unauthenticated` the error is passed through to the caller, so the client never loops indefinitely. The interceptor is placed at the outermost position in the chain so that each attempt (including the retry) flows through all inner interceptors such as logging and metrics, giving proper visibility. Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
jhernand
force-pushed
the
retry_on_unauthenticated
branch
from
May 25, 2026 10:56
3b00129 to
ce9558d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/network/grpc_client_test.go (1)
341-527: ⚡ Quick winAdd coverage for
TokenSource.Invalidatefailure path.The new tests don’t cover the branch where
Invalidatefails (defined ininternal/network/grpc_client.goaround Line 443-Line 451). That branch is part of the retry contract and should be locked with a regression test.As per coding guidelines `**`: Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.Proposed test addition
import ( "context" "crypto/tls" + "errors" "net" "sync/atomic" "time" @@ It("Returns the unauthenticated status code when the retry also fails", func() { @@ Expect(calls.Load()).To(BeNumerically("==", 2)) }) + It("Returns the original unauthenticated status code when token invalidation fails", func() { + tcpListener, err := net.Listen("tcp", "127.0.0.1:0") + Expect(err).ToNot(HaveOccurred()) + tlsListener := tls.NewListener(tcpListener, &tls.Config{ + Certificates: []tls.Certificate{ + testing.LocalhostCertificate(), + }, + }) + + var calls atomic.Int32 + interceptor := func(ctx context.Context, request any, info *grpc.UnaryServerInfo, + handler grpc.UnaryHandler) (response any, err error) { + calls.Add(1) + return nil, status.Error(codes.Unauthenticated, "token expired") + } + + server := grpc.NewServer(grpc.UnaryInterceptor(interceptor)) + healthServer := health.NewServer() + healthpb.RegisterHealthServer(server, healthServer) + healthServer.SetServingStatus("", healthpb.HealthCheckResponse_SERVING) + go func() { + defer GinkgoRecover() + _ = server.Serve(tlsListener) + }() + defer server.Stop() + + token := &auth.Token{ + Access: "test-token", + Expiry: time.Now().Add(time.Hour), + } + tokenSource := auth.NewMockTokenSource(ctrl) + tokenSource.EXPECT().Token(gomock.Any()).Return(token, nil).Times(1) + tokenSource.EXPECT().Invalidate(gomock.Any()).Return(errors.New("invalidate failed")).Times(1) + + client, err := NewGrpcClient(). + SetLogger(logger). + SetAddress(tcpListener.Addr().String()). + SetInsecure(true). + SetTokenSource(tokenSource). + Build() + Expect(err).ToNot(HaveOccurred()) + defer func() { + err := client.Close() + Expect(err).ToNot(HaveOccurred()) + }() + + healthClient := healthpb.NewHealthClient(client) + _, err = healthClient.Check(ctx, &healthpb.HealthCheckRequest{Service: ""}) + Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.Unauthenticated)) + Expect(calls.Load()).To(Equal(int32(1))) + }) + It("Does not retry when the server returns a status code other than unauthenticated", func() {🤖 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 `@internal/network/grpc_client_test.go` around lines 341 - 527, Add a new unit test in grpc_client_test.go that exercises the TokenSource.Invalidate failure branch: create a server that returns codes.Unauthenticated on the first call and succeeds on retry (or always unauthenticated depending on desired contract), mock auth.NewMockTokenSource to Expect Token(...) to be called twice and Invalidate(...) to Return(errors.New("invalidate failed")) once, then perform a health.Check call and assert the client behavior (e.g., that an error is returned or the retry occurred) and that calls.Load() and the mock expectations match; target the TokenSource.Invalidate symbol and the retry interceptor behavior in grpc_client.go so this covers the invalidate-failure branch.
🤖 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.
Nitpick comments:
In `@internal/network/grpc_client_test.go`:
- Around line 341-527: Add a new unit test in grpc_client_test.go that exercises
the TokenSource.Invalidate failure branch: create a server that returns
codes.Unauthenticated on the first call and succeeds on retry (or always
unauthenticated depending on desired contract), mock auth.NewMockTokenSource to
Expect Token(...) to be called twice and Invalidate(...) to
Return(errors.New("invalidate failed")) once, then perform a health.Check call
and assert the client behavior (e.g., that an error is returned or the retry
occurred) and that calls.Load() and the mock expectations match; target the
TokenSource.Invalidate symbol and the retry interceptor behavior in
grpc_client.go so this covers the invalidate-failure branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 368c5eef-2faf-441f-ae27-b21fd7712070
📒 Files selected for processing (2)
internal/network/grpc_client.gointernal/network/grpc_client_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a
TokenSourceis configured, the gRPC client now retries a request once if the serverreturns
Unauthenticated. On the first such response the client callsTokenSource.Invalidateto discard the cached token and retries, letting the
PerRPCCredentialsfetch a fresh one.If the retry also returns
Unauthenticatedthe error is passed through to the caller so theclient never loops indefinitely.
The retry logic lives in a new
unauthRetryInterceptorplaced at the outermost position inthe interceptor chain, giving logging and metrics visibility into each attempt.
Test plan
Unauthenticatedon the first call then succeeds on the second,verifying the client recovers transparently,
Tokenis called twice, andInvalidateonce.Unauthenticated, verifying the error reaches the callerafter exactly two server calls and one
Invalidate.PermissionDenied, verifying no retry and noInvalidate.