SHARED-2644: Downgrade node auth context-cancel errors#2086
Conversation
|
👋 dvashchuk, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
📊 API Diff Results
|
There was a problem hiding this comment.
Pull request overview
Adjusts NodeJWTAuthenticator.AuthenticateJWT logging to avoid emitting ERROR logs for expected shutdown / request-cancellation scenarios when IsNodePubKeyTrusted returns context.Canceled or context.DeadlineExceeded, while preserving ERROR logs for real provider failures.
Changes:
- Downgrade log level from
ERRORtoWARNwhenIsNodePubKeyTrustedfails due to context cancellation/deadline. - Add tests asserting log level behavior for provider errors vs context cancellation.
- Introduce a minimal
slog.Handlerto capture log records in tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/nodeauth/jwt/node_jwt_authenticator.go | Downgrades logging to WARN for context-canceled/deadline errors from the provider. |
| pkg/nodeauth/jwt/node_jwt_authenticator_test.go | Adds log-capture handler and new tests verifying log levels for provider error vs context cancellation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "error", err, | ||
| ) | ||
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { | ||
| v.logger.Warn("Node validation skipped: context cancelled", |
There was a problem hiding this comment.
Addressed: message now reads "Node validation skipped: context canceled or deadline exceeded" (covers both).
| func (h *captureHandler) WithAttrs([]slog.Attr) slog.Handler { return h } | ||
| func (h *captureHandler) WithGroup(string) slog.Handler { return h } |
There was a problem hiding this comment.
Addressed: WithAttrs and WithGroup now return a new captureHandler with merged attrs/groups (not the same handler). See lines in node_jwt_authenticator_test.go.
dvashchuk
changed the title
dvashchuk
force-pushed
the
fix/node-auth-context-cancel-log
branch
from
f95c05b to
f35e344
Compare
dvashchuk
force-pushed
the
fix/node-auth-context-cancel-log
branch
2 times, most recently
from
11eadf3 to
7b7656a
Compare
product-security-plaid-production
Bot
requested review from
4of9,
engnke,
fouadkada,
hendoxc,
kirqz23,
pkcll and
thomaska
dvashchuk
force-pushed
the
fix/node-auth-context-cancel-log
branch
4 times, most recently
from
5a7b58c to
c225417
Compare
…thenticator When IsNodePubKeyTrusted returns context.Canceled or context.DeadlineExceeded, log at WARN instead of ERROR. These are expected transient conditions (caller-initiated cancellation or slow upstream) and should not fire alerts. Adds tests for both Canceled and DeadlineExceeded paths, and a well-behaved captureHandler for slog assertions. Ticket: SHARED-2644
dvashchuk
force-pushed
the
fix/node-auth-context-cancel-log
branch
from
e466681 to
0314cd8
Compare
Why is this change necessary?
AuthenticateJWTlogged ERROR forNode validation failedwhenIsNodePubKeyTrustedreturned context cancellation/deadline. These are expected during shutdown/timeout paths and should not page as high-severity failures.What is changing?
pkg/nodeauth/jwt/node_jwt_authenticator.go, log WARN (not ERROR) when provider error is context cancellation/deadlineHow was this tested?
go test -timeout 30s -run TestNodeJWTAuthenticator ./pkg/nodeauth/jwt/...