Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 20 commits into from
Jan 10, 2024
Merged

chore: instrument external oauth2 requests #11519

merged 20 commits into from
Jan 10, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Jan 9, 2024 •
edited
Loading

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.

  • name: A human readable name to identify the provider. This is the external_auth.id or just login-github for primary authentication.
  • source: Indicates which method was called to trigger the request. [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?? REMOVED
  • status_code: Returned status code. For alerting purposes, you could alert when 429s occur.

Example metrics

# HELP coderd_oauth2_external_requests_total The total number of api calls made to external oauth2 providers. 'status_code' will be 0 if the request failed with no response.
# TYPE coderd_oauth2_external_requests_total counter
coderd_oauth2_external_requests_total{name="github-login",source="Exchange",status_code="200"} 2
coderd_oauth2_external_requests_total{name="primary-github",source="TokenSource",status_code="200"} 1
coderd_oauth2_external_requests_total{name="github-login",source="Exchange",status_code="200"} 2
coderd_oauth2_external_requests_total{name="primary-github",source="TokenSource",status_code="200"} 1
coderd_oauth2_external_requests_total{name="github-login",source="Exchange",status_code="200"} 1
coderd_oauth2_external_requests_total{name="primary-github",source="AppInstallations",status_code="200"} 5
coderd_oauth2_external_requests_total{name="primary-github",source="ValidateToken",status_code="200"} 5

What this does not do

This only instruments oauth and some related methods. Verify OIDC and userinfo OIDC are not instrumented. Therefore OIDC is not instrumented at this time

@Emyrk Emyrk marked this pull request as ready for review January 9, 2024 18:44
@Emyrk Emyrk force-pushed the stevenmasley/oauth_instrument branch from 22cc18e to 721fb0a Compare January 9, 2024 20:08
@Emyrk Emyrk requested a review from mafredri January 9, 2024 21:04
@Emyrk Emyrk mentioned this pull request Jan 9, 2024
Merged
@Emyrk Graphite App
Copy link
Member Author

Emyrk commented Jan 9, 2024 •
edited
Loading

In this stack: 📊 Improve observability and metrics for OAuth2 integration

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Emyrk and the rest of your teammates on Graphite Graphite

@Emyrk Emyrk force-pushed the stevenmasley/oauth_instrument branch from 1993093 to 5222f66 Compare January 9, 2024 22:31
@Emyrk Emyrk mentioned this pull request Jan 9, 2024
Merged
mafredri
mafredri approved these changes Jan 10, 2024
View reviewed changes
coderd/externalauth/externalauth.go
}
}

resp, err := do(req)
Copy link
Member

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 for resp, should we?

Copy link
Member Author

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.

coderd/promoauth/oauth2.go
}

// If the underlying transport is the default, we need to clone it.
// We should also clone it if it supports cloning.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about testing for under.(interface{ Clone() ... }) instead? That would cover the "supports cloning" case.

Copy link
Member Author

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 of Clone() http.RoundTripper does not match, and the default transport would not implement it.

Copy link
Member

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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion: Could use httptest.NewRequest to avoid checking error. Alternatively http.NewRequestWithContext to skip that step.

Copy link
Member Author

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 use NewRequestWithContext though

@Emyrk Emyrk force-pushed the stevenmasley/oauth_instrument branch from e48a44f to 85e2d91 Compare January 10, 2024 14:50
@Emyrk Emyrk merged commit 50b78e3 into main Jan 10, 2024
@Emyrk Emyrk deleted the stevenmasley/oauth_instrument branch January 10, 2024 15:13
@github-actions github-actions bot locked and limited conversation to collaborators Jan 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mafredri mafredri mafredri approved these changes

Assignees

@Emyrk Emyrk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Emyrk @mafredri