pkg/oauth2: consider session cancellation #182
Conversation
Currently, if telemeter assumes a refresh token is still valid, it will treat errors from fetching an access token as fatal. In reality, the authorization server might have reset our session and invalidate the refresh token server-side despite still being valid. In that case it returns a 400 status code. This fixes it. Fixes MON-690
openshift-ci-robot
added
approved
pkg/oauth2/token_source.go
Outdated
| return c.passwordCredentialsToken() | ||
| } | ||
|
|
||
| func (c *passwordCredentialsTokenSource) passwordCredentialsToken() (*oauth2.Token, error) { |
There was a problem hiding this comment.
This shouldn’t happen too often, but it would be good to know when it does. Let’s add a counter metric to understand how often this happens.
There was a problem hiding this comment.
agreed 👍 added a metric, PTAL
pkg/oauth2/token_source.go
Outdated
| var ( | ||
| grantsTotal = prometheus.NewCounter( | ||
| prometheus.CounterOpts{ | ||
| Name: "password_credentials_grants_total", |
There was a problem hiding this comment.
this should have a telemeter_ prefix
pkg/oauth2/token_source.go
Outdated
| ) | ||
|
|
||
| func init() { | ||
| prometheus.MustRegister(grantsTotal) |
There was a problem hiding this comment.
Could we not use the global registry? I'd like us to generally move away from this pattern, and we should be a role model for the rest of the teams.
There was a problem hiding this comment.
good idea. we can do a head-start here 👍 I suggest to dependency-inject the registry here, then gradually factor all the other places, wdyt?
pkg/oauth2/token_source.go
Outdated
| } | ||
|
|
||
| func (c *passwordCredentialsTokenSource) passwordCredentialsToken() (*oauth2.Token, error) { | ||
| c.grantsCounter() |
There was a problem hiding this comment.
it seems to me we may want to differentiate in a successful and errored token retrieval no?
There was a problem hiding this comment.
We have that indication (status code returned by tollbooth) already in the oauth client metric, i.e. client_api_requests_total{client="oauth",code="..."}. This metric is also used to set up alerting.
Would it be idiomatic to add a err="false|true" here?
There was a problem hiding this comment.
but it's differentiated in terms of refresh token use and credentials flow? if so lgtm
There was a problem hiding this comment.
As discussed oob:
- The existing
client_api_requests_totalmetric does not differentiate the cause of the refresh. I added thecauselabel for that reason. - We'll add a
statuslabel here to have more insight on the result of the password grant operation.
ptal
| username: username, | ||
| password: password, | ||
| cfg: cfg, | ||
| grantsCounter: grantsCounter, |
There was a problem hiding this comment.
typically the actual metric is passed in here, or even grouped in a metrics struct
There was a problem hiding this comment.
That was just to ease testing, but I can pass a metrics struct here (or the counter interface) too.
This gives us more insights on a) why the refresh token was re-issued (cause label) b) if the operation failed (status label)
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brancz, s-urbaniak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Currently, if telemeter assumes a refresh token is still valid, it will
treat errors from fetching an access token as fatal.
In reality, the authorization server might have reset our session and
invalidate the refresh token server-side despite still being valid.
In that case it returns a 400 status code.
This fixes it.
Fixes MON-690
/cc @brancz @squat @paulfantom