chore(oidc): Add log client IP address #20901
Conversation
❗ Preview Environment deployment failed on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
omerap12
changed the title
omerap12
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20901 +/- ##
==========================================
+ Coverage 55.04% 55.10% +0.06%
==========================================
Files 324 324
Lines 55421 55429 +8
==========================================
+ Hits 30504 30542 +38
+ Misses 22303 22273 -30
Partials 2614 2614 ☔ View full report in Codecov by Sentry. |
| @@ -130,9 +130,6 @@ func (p *providerImpl) Verify(tokenString string, argoSettings *settings.ArgoCDS | |||
| if err == nil { | |||
| break | |||
| } | |||
| // We store the error for each audience so that we can return a more detailed error message to the user. | |||
| // If this gets merged, we'll be able to detect failures unrelated to audiences and short-circuit this loop | |||
| // to avoid logging irrelevant warnings: https://github.com/coreos/go-oidc/pull/406 | |||
There was a problem hiding this comment.
Why remove this?
There was a problem hiding this comment.
The issue specifies this comment should be removed
util/oidc/oidc.go
Outdated
| @@ -376,6 +376,7 @@ func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) { | |||
| idToken, err := a.provider.Verify(idTokenRAW, a.settings) | |||
| if err != nil { | |||
| log.Warnf("Failed to verify token: %s", err) | |||
| log.Infof("Client IP: %s", r.RemoteAddr) | |||
There was a problem hiding this comment.
Let's have one log statement and/or add client ip as a structured field for the warn message. I find two logs of different kind from the same block confusing. However, we may also log ip address regardless in the outside block together with some other metadata.
There was a problem hiding this comment.
Understood. I recommend including the IP address in all warning messages, not just this one.
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
omerap12
force-pushed
the
issue-20388
branch
from
0f078f6
to
5620378
Compare
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
|
Just an idea, with this we can add/remove fields as we want in one place. |
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
|
What about now? @andrii-korotkov-verkada |
Oh absolutely, go for it. |
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
Adds client IP address and port ("IP:port") when failing to verify token.
Fixes: #20388