Add options to set headers on upstream http requests #154

graemechristie · 2023-07-14T08:51:52Z

This PR adds the command line option extra-http-header, which facilitates setting arbitrary http headers on the upstream prometheus request. This is required to support queries to prometheus compatible endpoints like Grafana Mimir, which expect an "X-Scope-OrgID:" header in the request to specify the tenant to query the data for.

Additionally, I have added a rewrite-host-header-to parameter, which supports rewriting the host to a different value than the source request. This would be needed in the case where the upstream prometheus endpoint is an nginx proxy or simliar using host base proxying. This addresses the issue #135.

graemechristie · 2023-07-18T04:51:28Z

Does not seem to be alot of activity in this repo. @SuperQ could you have a look at this PR ?

graemechristie · 2023-08-01T05:53:04Z

@simonpasquier are you able to look this PR ?

simonpasquier · 2023-08-01T15:17:30Z

injectproxy/routes.go

@@ -262,6 +282,7 @@ func (sle StaticLabelEnforcer) ExtractLabel(next http.HandlerFunc) http.Handler
 }

 func NewRoutes(upstream *url.URL, label string, extractLabeler ExtractLabeler, opts ...Option) (*routes, error) {
+	//extraHttpHeaders []string, rewriteHostHeader string


to be removed?

I've removed that

main.go

injectproxy/routes.go

simonpasquier · 2023-08-01T15:34:09Z

main.go

@@ -81,6 +83,8 @@ func main() {
 		"This option is checked after Prometheus APIs, you cannot override enforced API endpoints to be not enforced with this option. Use carefully as it can easily cause a data leak if the provided path is an important "+
 		"API (like /api/v1/configuration) which isn't enforced by prom-label-proxy. NOTE: \"all\" matching paths like \"/\" or \"\" and regex are not allowed.")
 	flagset.BoolVar(&errorOnReplace, "error-on-replace", false, "When specified, the proxy will return HTTP status code 400 if the query already contains a label matcher that differs from the one the proxy would inject.")
+	flagset.Var(&extraHttpHeaders, "extra-http-header", "Additional HTTP headers to add to the upstream prometheus query in the format 'header: value'. Can be repeated multiple times for additional headers.")
+	flagset.StringVar(&rewriteHostHeader, "rewrite-host-header-to", "", "Rewrite host header to supplied value when sending the query to the upstream URL.")


I wonder if we need a dedicated arg for the Host header or if we just handle it differently in the proxy? In the latter case, you'd do prom-label-proxy --extra-http-header "Host: foo.example.com" --extra-http-header "X-Scope-OrgID: 123". It would be one less argument to remember.

I lodged this PR a long time ago ... and I cannot remember if there was a valid reason for needing the separate argument/behaviour, or I just thought it was preferable to specify the simple rewrite-host-header argument than needing to know the internal mechanics and set the header manually. Regrardless - I primarily need just the extra http headers functionality to support Mimir so I am happy to resubmit this PR with just that option, and we can add the other option in a separate PR if it is required.

@simonpasquier see my comment on line 293 above.

So conceptually, "extra-http-headers" are user defined HTTP headers outside of the well known/mandatory headers such as host:. Thus the -extra-http-headers option only covers these headers in line with how the Go SDK handles these. Host is a specific use case of a mandatory/well known header that needs to be overridden in legitimate scenarios, so that has it's own flag. if we were to allow "Host:" to be overridden with the --extra-http-header flag, we would probably want to override all of the other mandatory http header edge cases as well, for very little or no benefit.

reason to separate Host header come from lines 288-300 from routes.go

graemechristie · 2023-11-01T09:23:08Z

injectproxy/routes.go

+		Rewrite: func(r *httputil.ProxyRequest) {
+			r.SetURL(upstream)
+			if opt.rewriteHostHeader == "" {
+				r.Out.Host = r.In.Host


OK, I think this is the reason for the separate arguments. I do not think you can set well known mandatory headers like host using headers collection on the request, it has to be set using host property. You could technically scan for the key of "host" in the extra headers collection, then if found remove it from the collection and execute this logic .. but that feels hacky. I think the original implementation is the correct behaviour. @simonpasquier - Would you prefer I leave this as is or just remove the ability to set the host header all together ?

As Host header is very specific, I think it's better to let it alone, and add some treatment (iterate all extra header and check if ...) on each query seems not very optimized

…to prometheus Added option to set host header on upstream query to prometheus - addresses prometheus-community#135 Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Co-authored-by: Simon Pasquier <spasquie@redhat.com> Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

kyrianae

All seems ok to me
I tested this on my own use case and it's working, I did'nt any issue

kyrianae · 2024-01-11T07:43:17Z

injectproxy/routes.go

+		Rewrite: func(r *httputil.ProxyRequest) {
+			r.SetURL(upstream)
+			if opt.rewriteHostHeader == "" {
+				r.Out.Host = r.In.Host


As Host header is very specific, I think it's better to let it alone, and add some treatment (iterate all extra header and check if ...) on each query seems not very optimized

kyrianae · 2024-01-11T07:44:47Z

main.go

@@ -81,6 +83,8 @@ func main() {
 		"This option is checked after Prometheus APIs, you cannot override enforced API endpoints to be not enforced with this option. Use carefully as it can easily cause a data leak if the provided path is an important "+
 		"API (like /api/v1/configuration) which isn't enforced by prom-label-proxy. NOTE: \"all\" matching paths like \"/\" or \"\" and regex are not allowed.")
 	flagset.BoolVar(&errorOnReplace, "error-on-replace", false, "When specified, the proxy will return HTTP status code 400 if the query already contains a label matcher that differs from the one the proxy would inject.")
+	flagset.Var(&extraHttpHeaders, "extra-http-header", "Additional HTTP headers to add to the upstream prometheus query in the format 'header: value'. Can be repeated multiple times for additional headers.")
+	flagset.StringVar(&rewriteHostHeader, "rewrite-host-header-to", "", "Rewrite host header to supplied value when sending the query to the upstream URL.")


reason to separate Host header come from lines 288-300 from routes.go

graemechristie force-pushed the add-header-command-line-options branch from 1b520e4 to 88275aa Compare July 14, 2023 08:53

graemechristie marked this pull request as draft July 14, 2023 09:05

graemechristie force-pushed the add-header-command-line-options branch from 2163b32 to 034166d Compare July 14, 2023 14:50

graemechristie marked this pull request as ready for review July 14, 2023 14:54

SuperQ requested a review from simonpasquier July 18, 2023 08:53

simonpasquier reviewed Aug 1, 2023

View reviewed changes

graemechristie commented Nov 1, 2023

View reviewed changes

graemechristie force-pushed the add-header-command-line-options branch 2 times, most recently from 6d6e516 to 9d63218 Compare November 1, 2023 10:13

graemechristie and others added 10 commits November 1, 2023 18:14

Added command line option to set arbitrary headers on upstream query …

3c0ae3f

…to prometheus Added option to set host header on upstream query to prometheus - addresses prometheus-community#135 Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Refactored to use options pattern for new command line args

9fef355

Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Removed superfluous trimspace()

8e48586

Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Removed superfluous trimspace()

645d631

Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Update injectproxy/routes.go

d4088e8

Co-authored-by: Simon Pasquier <spasquie@redhat.com> Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Update injectproxy/routes.go

5820b25

Co-authored-by: Simon Pasquier <spasquie@redhat.com> Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Update injectproxy/routes.go

e3b7c3d

Co-authored-by: Simon Pasquier <spasquie@redhat.com> Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Update main.go

3578ebe

Co-authored-by: Simon Pasquier <spasquie@redhat.com> Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Removed unused commented code

8a31ee7

Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

Refactored validation/setting of extraHttpHeaders

be913f2

Signed-off-by: Graeme Christie <gchristie@bunnings.com.au>

graemechristie force-pushed the add-header-command-line-options branch from 9d63218 to be913f2 Compare November 1, 2023 10:15

kyrianae approved these changes Jan 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add options to set headers on upstream http requests #154

Add options to set headers on upstream http requests #154

graemechristie commented Jul 14, 2023 •

edited

Loading

graemechristie commented Jul 18, 2023

graemechristie commented Aug 1, 2023

simonpasquier Aug 1, 2023

graemechristie Nov 1, 2023

simonpasquier Aug 1, 2023

graemechristie Nov 1, 2023

graemechristie Nov 1, 2023

graemechristie Nov 13, 2023

kyrianae Jan 11, 2024

graemechristie Nov 1, 2023

kyrianae Jan 11, 2024

kyrianae left a comment

kyrianae Jan 11, 2024

kyrianae Jan 11, 2024

Add options to set headers on upstream http requests #154

Are you sure you want to change the base?

Add options to set headers on upstream http requests #154

Conversation

graemechristie commented Jul 14, 2023 • edited Loading

graemechristie commented Jul 18, 2023

graemechristie commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrianae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graemechristie commented Jul 14, 2023 •

edited

Loading