Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add User-Agent header to oauth2 requests #386

Merged

Conversation

cig2312
Copy link
Contributor

@cig2312 cig2312 commented Jun 28, 2022

This PR adds the ability to specify the User-Agent header for oauth2 requests.

The default User-Agent: go-http-client/1.1 is ratelimited by many sites which leads to a lot of oauth2 requests failing.

Fixes: 384

@cig2312 cig2312 force-pushed the add-user-agent-header-oauth2-request branch from ef6984c to ebd7f04 Compare June 28, 2022 10:53
Signed-off-by: clayton-gonsalves <gonsalves.clayton@yahoo.co.in>
@cig2312 cig2312 force-pushed the add-user-agent-header-oauth2-request branch from ebd7f04 to 99a1aca Compare June 28, 2022 10:57
@roidelapluie
Copy link
Member

Can you please be more specific with the issue you try to solve?

@cig2312
Copy link
Contributor Author

cig2312 commented Jun 28, 2022

I have updated the PR description
but the problem is that the default User-Agent: go-http-client/1.1 is ratelimited by many sites which leads to a lot of oauth2 requests failing.

My usecase:
Probing some oauth2 authenticated endpoints via the blackbox prober. Because of the default client in the UA my probes are failing.

@roidelapluie
Copy link
Member

I don't think we should expose this to users. I agree we could pass on the Blackbox Exporter and Prometheus user agents.

Can you move to code to an http option like WithIdleConnTimeout ?

@cig2312
Copy link
Contributor Author

cig2312 commented Jun 28, 2022

I did think about this but there are a couple of reasons i wanted to expose this to users of this pkg:

  • The reason the default http UA is limited is because of the sheer number of requests. While this wouldn't be the case with Blackbox Exporter/Prometheus UA's, i worry it may still trigger limits on sites that have set them very aggressively.
  • Having something custom also lets us group/filter out requests based on UA on our metrics etc.
  • While i know this is a common library for all of prometheus, the blackbox prober does allow the user the specify the UA. I tried to follow the same convention.

Maybe we can keep a default UA(prometheus/blackbox) and allow the user the customise if needed?

@roidelapluie
Copy link
Member

In BBE's case, it would be exposed to the user, but not directly with the http config.

@cig2312
Copy link
Contributor Author

cig2312 commented Jun 28, 2022

Isn't the proxy_url in the oauth2 config also similar? That is exposed to the users with the http config.

@roidelapluie
Copy link
Member

Isn't the proxy_url in the oauth2 config also similar? That is exposed to the users with the http config.

Yes, but it is valid for users to change it.

User agent is already set in many cases, I think we should just make sure to move it here with an http option so it's also applied to oauth 2.

@cig2312
Copy link
Contributor Author

cig2312 commented Jun 29, 2022

Can you move to code to an http option like WithIdleConnTimeout ?

Do you mean adding it as an optional func to the http client?
I dont think that would work as the client generated by NewClientFromConfig isnt reused by the oauth roundtripper.

@roidelapluie
Copy link
Member

What about #387

@cig2312
Copy link
Contributor Author

cig2312 commented Jul 4, 2022

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Ability to set custom User-Agent on oauth2
2 participants