remote_write: allow passing along custom HTTP headers

[bonifaido]

This change adds the ability to send custom HTTP headers in remote write requests, which makes the configuration of a proxy in between optional and helps the receiver side recognize the given source better.

For example, when using a Thanos receiver a THANOS-TENANT header can help Thanos to distribute the data of separate tenants, but some examples need to use a reverse-proxy (like NGINX) for this like in this tutorial: https://www.infracloud.io/blogs/multi-tenancy-monitoring-thanos-receiver/ which is somewhat a larger ceremony for adding just an HTTP header to each request.

[roidelapluie]

Thank you for this change. There are many things to consider when allowing users to arbitrary change HTTP headers. I am not opposed with the change but we probably want to somehow find a reasonable list of headers that we would allow to override.

That is also a change from previous policies where this features was not accepted.

[roidelapluie]

cc @csmarchbanks @cstyan @bwplotka WDYT?

[bonifaido]

(I'm happy to extend the PR with header limitation and other changes of course!)

[roidelapluie]

The issue with allowing to pass any header are that some header are changing the way the server reacts.
Think about Connection: close, which would cause the other end to close any connection.

Then, some users could use them as authorization: Authorization. That would conflict with the headers added by the client config and cause confusion to users. Also be a security issue because that would probably be displayed in the UI.

There are headers that simply would break the connection: Content-Type, Content-Size.
Other headers would make debugging harder, and should not be changed by the user: User-Agent.

Then, we could decide to e.g. allow X- prefixed headers. However, this is not recommended anymore: https://tools.ietf.org/html/rfc6648
Based on RFC6648, it seems that to achieve the specification of the tenant, we should craft a new header, maybe Prometheus-Tenant.
But then you have M3, which, instead of accepting a simple tenant name, can take multiple headers: https://m3db.io/docs/m3coordinator/api/remote/

[csmarchbanks]

I am in favor of this feature, I have certainly wanted to be able to specify an arbitrary header like this for working with internal Cortex deployments rather than depending on the Authorization header and a proxy.

I certainly understand that setting some headers could cause issues for users. However, if the alternative is putting a proxy between Prometheus and the remote write destination a user could do that anyway. If any of those concerns are blocking for someone else I won't push though.

I do think the documentation should call out that these are not for secrets and will be visible in plain text on the configs page. Otherwise I can imagine someone using them for custom API tokens and leaking secrets.

[cstyan]

I don't have any opinions about the validity of any use case for this, but as long as the implementation isn't a lot of work to maintain (which this clearly isn't) then I'm good with it.

[roidelapluie]

Thank you all.

Since there is no issues with the remote write maintainers, let's proceed.

For my remarks, it seems that based on my previous comment, that could hurt users, but protecting them against themselves is not really practical. Also multiple remote write endpoints use headers, and in some scenarios, this feature would be super handy for our users.

I have added a few comment. Could you move the documentation next to the per-request docs, like basic authentication?
Please add a comment/warning in the docs that it is aimed at new headers and that not all the headers can be changed.

[Harkishen-Singh]

Should we check internally if the provided headers match the reserved words (like basic_auth and the ones said in related issue) and warn/error the user before it leads to any unintended behaviour?

[csmarchbanks]

Should we check internally if the provided headers match the reserved words (like basic_auth and the ones said in related issue) and warn/error the user before it leads to any unintended behaviour?

Perhaps have the config file validation fail if they try to specify the Authorization header here? With a specific message saying to use basic_auth or bearer_token fields instead.

[Harkishen-Singh]

Perhaps have the config file validation fail if they try to specify the Authorization header here?

Yes, I mean something like that.

[roidelapluie]

The list of headers to reject is non trivial, here is a few.

• Host
• Authorization
• Content-Encoding
• Content-Type
• X-Prometheus-Remote-Write-Version
• User-Agent
• Connection
• Keep-Alive
• Proxy-Authenticate
• Proxy-Authorization
• WWW-Authenticate

[bonifaido]

Could you please give me a pointer on where this validation should take place in the code base?

[csmarchbanks]

@roidelapluie's list seems like a good place to start at least. If problems are reported we can add more later.

You can add the validation to UnmarshalYAML for the RemoteWriteConfig. Thank you for your work!

[bonifaido]

Hm, it's not that easy to pass down that logger instance down there :) I'm trying, but if you have any suggestions I'm happy to make it that way.

The issue is that UnmarshalYAML is an implicitly called method by the parser, so there are two options:

• I set a global logger in the config package
• I pass down somehow a logger instance through the config struct chain

[roidelapluie]

You do not need a logger, just return an error.

[bonifaido]

Ah okay, I thought we should warn in the logs only, this sounds better!

[bonifaido]

I have added the header validation check accordingly.

[bonifaido]

No, I think we have to do the check case-insensitively. Let me fix it.

[csmarchbanks]

Awesome!

Just a couple small comments then a 👍 from me.

[bonifaido]

Fix according to your comments @csmarchbanks .

[roidelapluie]

thanks, LGTM. I will try that out later and let remote storage maintainers do a final review.

[csmarchbanks]

👍 Thanks!

[Harkishen-Singh]

BTW, I had a question regarding this. Was there any reason not to support headers in remote_read?

[roidelapluie]

Remote write is more common that remote read, let's see if there are any needs to do this with remote read as well. At the moment we did not receive a request related to remote read.

[Harkishen-Singh]

I think if we want to implement headers, then it should go with both the write and the read. Having just on one side, makes the system uneven in terms of functionality.

[roidelapluie]

Feel free to open an issue to track this. We won't remember if it is in a closed pull request's comments.

[Harkishen-Singh]

That would actually be nice.

[benjaminhuo]

This makes using remote storage like Thanos receiver much easier!
Thanks @bonifaido