-
Notifications
You must be signed in to change notification settings - Fork 316
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
Adding 'Headers' field on HTTPClientConfig #326
Conversation
Signed-off-by: Alan Protasio <approtas@amazon.com>
69d2d46
to
72c32f5
Compare
thank you. let me talk to some people and get back to you. this has an impact in the complete project, and has been rejected before. it was rejected because the risk that users would use that to put secrets, or in ways that would break their requests. I also don't want them to override the user agent. what we have done for remote write (and that we should do here if we accept this), is to block some http headers. Please see here for a list of blocked headers: I expect that we would need at least the sane block list here. for consistency, the field should be called |
Oh @roidelapluie .. Thanks a lot for looking into that. I can definitely use the same list of blocked headers (maybe bringing this list to here and reusing on prometheus?) and change the field name. |
yeah if we do this here we would just delete the code in prometheus, the http client is used everywhere. can I know what is your usecase? that'd help the decision. |
Yeah, for sure... We are using Cortex Alertmanager (which basically a multi tenant implementation of Prometheus AlertManager) and we want to add some extra headers when calling our notifiers (SNS in our case). Those extra headers are needed in our case as we have an internal defensive system that rely on those extra headers. https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html |
db00d11
to
f343adb
Compare
Signed-off-by: Alan Protasio <approtas@amazon.com>
f343adb
to
d80718e
Compare
I can follow up with the change on Prometheus to update commons there and remove the existing code. I think if we don't do it we can end up adding the headers 2 times, right? |
Signed-off-by: Alan Protasio <approtas@amazon.com>
38287d8
to
11d3b87
Compare
I just added the blocked headers validation! :D |
It seems that you only need to add 2 AWS-specific headers to the SNS receiver. I think it is probably safer to simply "hardcode" the headers. It does not seem that this requires a generic way to add http headers. |
Yeah.. Our use case is to add headers to the SNS receiver. But i was thinking it may be useful in any of the other receives (specially the webhook one) where the user want add extra information to be used by the integration - Eg: which receiver, what cluster (imagine that cluster is a label and you can route to a specific receiver) etc. |
All those informations are available in the payload. Can you please open a PR on the SNS receiver with the two new config parameters (aws:SourceAccount and aws:SourceArn) whoch would set the headers accordingly? Thanks! |
Based on previous discussions, I will close this PR. Thanks! |
Pity it was not generic, I want to add a custom header "myCustomHeaderName" : "myCustomHeaderValue" :( |
Allowing configure extra headers to be added in the HTTP requests.
Use case
We want to add extra headers on the requests send by one of Our alert Manager Integrations.
Ex: https://github.com/prometheus/alertmanager/blob/main/notify/webhook/webhook.go#L48
https://github.com/prometheus/alertmanager/blob/main/notify/wechat/wechat.go#L75
https://github.com/prometheus/alertmanager/blob/main/notify/sns/sns.go#L51
Etc.