- Notifications
You must be signed in to change notification settings - Fork1k
chore: instrument external oauth2 requests#11519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
22cc18e to721fb0aCompareEmyrk commentedJan 9, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
In this stack:
This stack of pull requests is managed by Graphite.Learn more about stacking. |
1993093 to5222f66CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| } | ||
| resp,err:=do(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Noticed we never check HTTP status code forresp, should we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am not sure, I did not write this part. Have not checked the payloads myself here.
| } | ||
| // If the underlying transport is the default, we need to clone it. | ||
| // We should also clone it if it supports cloning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How about testing forunder.(interface{ Clone() ... }) instead? That would cover the "supports cloning" case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wish I could but the method signature is:
func (t*Transport)Clone()*Transport
So the interface ofClone() http.RoundTripper does not match, and the default transport would not implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, of course. That's unfortunate :(.
coderd/promoauth/oauth2_test.go Outdated
| // Verify the default client was not broken. This check is added because we | ||
| // extend the http.DefaultTransport. If a `.Clone()` is not done, this can be | ||
| // mis-used. It is cheap to run this quick check. | ||
| req,err:=http.NewRequest(http.MethodGet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Minor suggestion: Could usehttptest.NewRequest to avoid checking error. Alternativelyhttp.NewRequestWithContext to skip that step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
httptest just uses panics instead of an error 🤷
https://github.com/golang/go/blob/master/src/net/http/httptest/httptest.go#L46
I can useNewRequestWithContext though
Uh oh!
There was an error while loading.Please reload this page.
e48a44f to85e2d91Compare
Uh oh!
There was an error while loading.Please reload this page.
Supports#10853
What this does
This adds an instrumentation layer to all of our oidc/oauth2 external requests.
Debugging this has been very challenging because I cannot reproduce the issue. So I figured adding some instrumentation could at best help debug, at worst it is just some nice extra metrics 🤷♂️.
What the metrics track for each oauth2 config.
external_auth.idor justlogin-githubfor primary authentication.[TokenSource|Exchange]. I think this could be extended in debug logging if we need for debugging purposes. For now, this doubles the number of tracked metrics, and I don't think we should make this list any larger.domain: The domain the request is sent to.Is this redundant??REMOVEDExample metrics
What this does not do
This only instruments oauth and some related methods. Verify OIDC and userinfo OIDC are not instrumented. ThereforeOIDC is not instrumented at this time