change injected PromQL label from labels.MatchEqual to labels. MatchRegexp to support multiple namespaces

[dongyingbo]

Problem

This proxy is used together with kube-rbac-proxy for OCP monitoring multiple tenancy.

kube-rbac-proxy supports to validate user against multiple namespaces via query like namespace=kube-system&namespace=openshift-monitoring. But this proxy support only one namespace. With the query above, its source code,

func (r *routes) ServeHTTP(w http.ResponseWriter, req *http.Request) {
	lvalue := req.URL.Query().Get(r.label)
        ....
}       

will extract only namespace kube-system. And source code,

	err = SetRecursive(expr, []*labels.Matcher{
		{
			Name:  r.label,
			Type:  labels.MatchEqual,
			Value: mustLabelValue(req.Context()),
		},
	})

will add labels.MatchEqual.

Suggestion

1. In the first piece of source code, extract all query values and construct a RegExpr string like kube-system|openshift-monitoring.
2. In the second piece of source code, use labels.MatchRegexp instead of labels.MatchEqual

[dongyingbo]

/cc simonpasquier May you take a look when you are available?

[lilic]

Thanks for your issue! Sorry for the late reply. I think having multi namespace support is a good idea, I know that kube-rbac-proxy has this already so maybe Frederic has some ideas around how this should look like in prom-label-proxy as well.

cc @brancz @simonpasquier

[brancz]

If multiple values is the use case, I would prefer if we made the query parameter repeatable instead of using a regex. The regex is going to be difficult security wise. Repeated namespaces would have the same effect though, so that should work equally well. I'm happy to have that addition if it doesn't already work.

[simonpasquier]

Regex matchers should be safe if we escape values with regex.QuoteMeta though?

[dongyingbo]

Thanks for your reply @lilic @brancz and @simonpasquier !

I realized that this proxy does not meet our requirements for building Grafana on top of it. The main problem is that I have to update the PromQL string from Grafana before it is passing onto kube-rbac-proy. So PromQL will be updated twice and it seems not good to me.

So I created one new project and initial source code is uploaded here https://github.com/IBM/ibm-grafana-ocpthanos-proxy. The proxy will be used as sidecar of Grafana and it will update PromQL string directly.

[lilic]

@dongyingbo

So PromQL will be updated twice and it seems not good to me.

Why does this not seem good?

[dongyingbo]

@lilic The PromQL string will go through the path,

|Grafana|--metric_name{namespace="name1"}-->|proxy|--metric_name?namespace=name1-->|kube-rbac-proxy+prom-label-proxy|--metric_name{namespace="name1"}-->|thanos-querier|

1. In the path above, user has to implement their own proxy to parse and update query from Grafana so that it meets the requirement of kube-rbac-proxy. I think it is a burden.
2. The same PromQL string will have to be updated twice. If I have to implement my own proxy I would prefer to make it work in one proxy.

[denogio]

Any plans to support multiple tenants in a single query?

[volka]

I would also have a usecase for this, as I have a proxy application that will authenticate my users and based on this, would create a label value that matches the "groups" of values a user has access to. E.g. if I have a user with access rights for GROUP1 and GROUP2, but not GROUP3, I would create a label group_id=~"|GROUP1|GROUP2". Note the first "empty" selector to also allow metrics that are not tagged with a group at all. Only parts of my metrics are sensitive to this group filtering, and for those the metrics exporters will generate the correct labels.

My application has full control of the query string, so the application would be the one to set the label value and could e.g. strip any other mention of the "group_id" label. It could also ensure the generated regex is correct / the group names are unproblematic.

I would suggest to support this with a configuration parameter to allow regex requests, and clearly state the security issue in case the label value can be manipulated by the user.

Do you see any other issues with this? If I posted a PR, would it have a chance to be merged?

[simonpasquier]

Closed by #115