Accept label value as regexp

[sathieu]

Fixes: #82

[simonpasquier]

Thanks for tackling this! Reading the issue, I have the impression that supporting regexp label values only for --label-value was enough (e.g. only for static values). Do you have a use case for supporting the same for --query-param and --header-name?

Given the security implications (e.g. it's easy to get the regexp wrong and expose too many metrics by accident), I'd be more inclined to limit the scope for now.

[sathieu]

Thanks for tackling this! Reading the issue, I have the impression that supporting regexp label values only for --label-value was enough (e.g. only for static values). Do you have a use case for supporting the same for --query-param and --header-name?

Yes.

Here is my usecase : I have multiple (soft-)tenants in a Kubernetes cluster. Each can include a list of namespace ([app1, app2]) or a list of namespace regexps ([tenant1-.+]). For the first case, Grafana is not able to pass the same header multiple times (see grafana/grafana-plugin-sdk-go#829), so here regexp support is a workaround. For the second case (less common), I really need regexp support.

Alternatively, a new tenant label could have been set at scrapping time. This is waiting for Scrape classes support in Prometheus Operator.

Given the security implications (e.g. it's easy to get the regexp wrong and expose too many metrics by accident), I'd be more inclined to limit the scope for now.

I understand. But I need it 😉 . Maybe add a warning in the docs?

[simonpasquier]

Thanks for the details, it helps a lot! I could agree to the feature being marked as experimental with an very clear warning in the CLI help + README.md file.

The proxy needs also to validate that the regexp is valid. Also what should the proxy's response when it gets several label values? My gut feeling is that it should fail.

[sathieu]

Thanks @simonpasquier I've rebased, added warnings in README, added checks for invalid regex and multiple values.

Back to you 🏓 !

[simonpasquier]

(suggestion) I'd use a static label value to make it more "visible".

Suggested change

	-header-name X-Namespace \
	-label namespace \
	-label-value '^foo-.+$' \
	-label namespace \

[simonpasquier]

I wonder if we shouldn't anchor the regexp by default (like Prometheus does for relabelings)? It would at least prevent too-permissive configurations.
It'd also be good to require that the compiled regex doesn't match the empty string.

[sathieu]

I wonder if we shouldn't anchor the regexp by default (like Prometheus does for relabelings)? It would at least prevent too-permissive configurations.

Prometheus anchors already.

It'd also be good to require that the compiled regex doesn't match the empty string.

Good idea.

[sathieu]

@simonpasquier I've merged your suggestions, added tests for regexp matching empty strings, and added tests early when using -label-value.

[sathieu]

@simonpasquier Thanks for your review. I've merged your suggestions, and silence API is now returning 501 Not Implemented when regex match is enabled. I'll try to add support for multiple values in silences in a future PR.

Back to you 🏸 !

[simonpasquier]

one last comment...

[simonpasquier]

(suggestion) to avoid duplicating the mux.Handle() calls, we might do something like this:

errs.Add(
    mux.Handle("/api/v2/silences", r.errorIfRegexpMatch(
        r.el.ExtractLabel(...

[...]

 func (r *routes) errorIfRegexpMatch(next http.HandlerFunc) http.HandlerFunc {
    return func(w http.ResponseWriter, req *http.Request) {
        if r.regexMatch {
            prometheusAPIError(w, "support for regex match not implemented", http.StatusNotImplemented)
            return
        }

        return next(w, req)
    }
}

[sathieu]

Done @simonpasquier. Back to you ⚾ !

[simonpasquier]

Thanks! 👍 for me.
@squat do you have any concern with this addition? From my side, the warning in the README.md is clear enough that users know about the potential issues if they mess up with their regexp.