-
Notifications
You must be signed in to change notification settings - Fork 99
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 options to set headers on upstream http requests #154
base: main
Are you sure you want to change the base?
Changes from all commits
3c0ae3f
9fef355
8e48586
645d631
d4088e8
5820b25
e3b7c3d
3578ebe
8a31ee7
be913f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,6 +64,8 @@ func main() { | |
enableLabelAPIs bool | ||
unsafePassthroughPaths string // Comma-delimited string. | ||
errorOnReplace bool | ||
extraHttpHeaders arrayFlags | ||
rewriteHostHeader string | ||
) | ||
|
||
flagset := flag.NewFlagSet(os.Args[0], flag.ExitOnError) | ||
|
@@ -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", "HTTP header to add to the upstream query in the format 'header: value'. Can be repeated multiple times.") | ||
flagset.StringVar(&rewriteHostHeader, "rewrite-host-header-to", "", "Rewrite host header to supplied value when sending the query to the upstream URL.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @simonpasquier see my comment on line 293 above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reason to separate Host header come from lines 288-300 from routes.go |
||
|
||
//nolint: errcheck // Parse() will exit on error. | ||
flagset.Parse(os.Args[1:]) | ||
|
@@ -126,6 +130,18 @@ func main() { | |
opts = append(opts, injectproxy.WithErrorOnReplace()) | ||
} | ||
|
||
for _, headerArg := range extraHttpHeaders { | ||
header, val, found := strings.Cut(headerArg, ":") | ||
if !found || len(strings.TrimSpace(header)) == 0 || len(strings.TrimSpace(val)) == 0 { | ||
log.Fatalf("extra-http-header %s is not in the format 'key:value'", headerArg) | ||
} | ||
opts = append(opts, injectproxy.WithExtraHttpHeader(header, val)) | ||
} | ||
|
||
if len(rewriteHostHeader) > 0 { | ||
opts = append(opts, injectproxy.WithRewriteHostHeader(rewriteHostHeader)) | ||
} | ||
|
||
var extractLabeler injectproxy.ExtractLabeler | ||
switch { | ||
case len(labelValues) > 0: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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