feat: add support for multiple enforced labels - #400
Conversation
simonpasquier
left a comment
simonpasquier
left a comment
There was a problem hiding this comment.
Thanks for starting this. 2 remarks
- Let's try to change only what's required. I get that help text could get better and be reworded but it should be part of a separate PR.
- I think that we should improve the proposed UX.
| -insecure-listen-address 127.0.0.1:8080 | ||
| ``` | ||
|
|
||
| The same positional pairing applies when repeating `-query-param` instead of |
There was a problem hiding this comment.
IMHO it isn't a very good user experience as it's easy to get wrong. Maybe we've reached the limit of what we can configure using CLI arguments and need to support file-based configuration?
There was a problem hiding this comment.
I thought about that and I agree that the positional pairing would be easy to get wrong. I didn't want to change any existing or introduce any new config params, but a new file based config would be the cleanest imo.
I can rework the PR to include a file based config.
9cd5f17 to
245cffb
Compare
|
This needs a rebase |
245cffb to
cd30cc2
Compare
|
rebased onto main, and added a file-based configuration for multiple enforced labels. the logic for the old cli arguments remains unchanged. |
917cb5e to
3f4a367
Compare
| } | ||
|
|
||
| // LabelConfig declares a single enforced label and the source of its values. | ||
| // Exactly one of Header, QueryParam or Values must be set. |
There was a problem hiding this comment.
is there a use case where someone would need to mix the source of enforced label values? E.g. label foo comes from the query parameters and label bar comes from HTTP headers. Maybe static values are a legit case though.
There was a problem hiding this comment.
in our case we have one prom-label-proxy instance for a sensitive label enforcement like X-Tenant and another chained instance with query parameters where the user can apply their own filtering in the grafana datasource for a datacenter label. it costs nothing and I couldn't think of a reason to restrict this in the config.
| func assertSingleLabelValue(next http.HandlerFunc) http.HandlerFunc { | ||
| // assertSingleLabelValue verifies that each enforced label has only one value. | ||
| // If not, it will reply with "422 Unprocessable Content". | ||
| func (r *routes) assertSingleLabelValue(next http.HandlerFunc) http.HandlerFunc { |
There was a problem hiding this comment.
assertSingleLabelValue needs access to all r.labels so making it a method made sense to me.
| for _, enforcedMatcher := range ms.labelMatchers { | ||
| res = append(res, enforcedMatcher) | ||
| // Iterate in a deterministic order so that the injected matchers are | ||
| // always rendered the same way. |
There was a problem hiding this comment.
please make it a separate PR
There was a problem hiding this comment.
actually on second thought, I don't believe this is needed at all. this is sorted in the prometheus parser anyway
| // | ||
| // The label and extractLabeler arguments declare the first enforced label. They | ||
| // may be left empty and nil when the labels are declared with | ||
| // WithEnforcedLabel() or WithConfig() instead. |
There was a problem hiding this comment.
While I understand and appreciate the attention towards downstream users I feel that it complicates the API.
Can we have a separate constructor maybe and NewRoutes becoming a shim?
func NewRoutesWithLabelers(upstream *url.URL, labelers []LabelEnforcer, opts ...Option) {
...
}
NewRoutes(upstream *url.URL, labelers []LabelEnforcer, opts ...Option) {
// build labelers list.
...
return NewRoutesWithLabelers(...)
}
There was a problem hiding this comment.
i will add NewRoutesWithLabelers and removed WithEnforcedLabel and WithConfig
3f4a367 to
57b5d78
Compare
Signed-off-by: Lukas Boettcher <1340215+lukasboettcher@users.noreply.github.com>
31ff85c to
e303a37
Compare
3 participants
This PR adds support for enforcing multiple ExtractLabelers in a single prom-label-proxy instance.
The goal is to keep backwards compatibility for the CLI and API interface.
Multiple ExtractLabelers can be created by specifying
--labeland--query-paramor--header-namearguments multiple times. The ordering is important to associate labels with their value sources. The--label-valueflag is excluded from this feature due to the ambiguity of mapping static label values to multiple labels.Disclaimer: this PR was created with the help of AI and manually refined
fixes: #190