Skip to content

server: Use warning log level for client errors#2147

Merged
bobcallaway merged 1 commit into
sigstore:mainfrom
jku:lower-log-level-for-client-errors
Sep 10, 2025
Merged

server: Use warning log level for client errors#2147
bobcallaway merged 1 commit into
sigstore:mainfrom
jku:lower-log-level-for-client-errors

Conversation

@jku

@jku jku commented Sep 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Decide the log level of errors based on the grpc code: client mistakes should not be logged at "error" level.

Fulcio itself currently uses only Internal and InvalidArgument, grpc framework does not use most of the codes listed either: this code still tries to categorize all valid codes in order to be future proof.

Fixes #908.

test

this client call:

$ curl -X POST -d '{}' http://localhost:5555/api/v2/signingCert
{"code":3,"message":"There was an error processing the identity token","details":[]}

results in:

fulcio-server-1        | 2025-09-10T09:39:58.799Z       WARN    server/error.go:57      oidc: malformed jwt, expected 3 parts got 1     {"requestID": "-03xA5aM", "code": "InvalidArgument", "clientMessage": "There was an error processing the identity token", "error": "oidc: malformed jwt, expected 3 parts got 1"}
fulcio-server-1        | github.com/sigstore/fulcio/pkg/server.handleFulcioGRPCError
fulcio-server-1        |        /opt/app-root/src/pkg/server/error.go:57
fulcio-server-1        | github.com/sigstore/fulcio/pkg/server.(*grpcaCAServer).CreateSigningCertificate

<snipped rest of stack trace>

@jku
server: Use warning log level for client errors
c1fd429 
Decide the log level of errors based on the grpc code: client mistakes
should not be logged at "error" level.

Fulcio itself currently uses only Internal and InvalidArgument, grpc
framework does not use most codes either: this code still tries to
categorize all valid codes.

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku jku requested a review from a team as a code owner September 10, 2025 09:45
@jku jku mentioned this pull request Sep 10, 2025
Closed
@codecov

codecov Bot commented Sep 10, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 43.64%. Comparing base (cf238ac) to head (c1fd429).
⚠️ Report is 458 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #2147       +/-   ##
===========================================
- Coverage   57.93%   43.64%   -14.29%     
===========================================
  Files          50       71       +21     
  Lines        3119     5666     +2547     
===========================================
+ Hits         1807     2473      +666     
- Misses       1154     2968     +1814     
- Partials      158      225       +67

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jku

jku commented Sep 10, 2025

Copy link
Copy Markdown
Member Author

The issue has this suggestion:

for JWT errors, I would remove logging entirely since there's nothing actionable to do.

We could still do that... It requires adding some custom errors (since they are not used currently) and then handling them specifically in handleFulcioGRPCError.

@bobcallaway bobcallaway merged commit bbde1a4 into sigstore:main Sep 10, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bobcallaway bobcallaway bobcallaway approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Every failed request is logged as an error

2 participants

@jku @bobcallaway