Add matcher based tenancy to api/v1/rules

[saswatamcode]

Currently, a call to the /api/metrics/v1/{tenant}/api/v1/rules endpoint, returns rules for all tenants, as it proxies the request to Thanos Querier.

This PR adds tenant isolation to /api/metrics/v1/{tenant}/api/v1/rules endpoint, by adapting prom-label-proxy code to modify the API response from the Thanos Query api/v1/rules endpoint and filter via the tenant label (this is possible as we enforce tenant labels on rules). So each tenant can only retrieve their own rules.

Relevant issue: https://issues.redhat.com/browse/MON-2141

[squat]

Hey @saswatamcode, regarding:

a call to the /api/metrics/v1/{tenant}/api/v1/rules endpoint, returns rules for all tenants

I don't quite agree with this statement. I think the ticket that you linked in the PR description is incorrect. The rules API is already tenant-aware: the /rules API is filtering results so that we only get the rules that were defined by one single tenant. Furthermore, the client must be authorized to read metrics from the specified tenant in order to fetch the rules. Please look here:

api/api/metrics/v1/rules.go

Line 34 in 9401841

resp, err := rh.client.ListRules(r.Context(), tenant)

However, as I mentioned in #191

Note: this is a distinct problem from ensuring that the rules written by
a tenant are guaranteed to only include data from that tenant.
Currently, the rules from a tenant can select data from any tenant. This
is a distinct problem and should absolutely be tackled in a follow up.

I think this is the problem you, and the Jira ticket, are trying to address?

[saswatamcode]

Hi @squat! :)
Yes, the naming is a bit confusing, as both deal with rules, but let me clarify the purpose of this ticket/PR,

• Currently, the rules API which hits the Rules storage backend is tenant-aware. A tenant can get and set rules in YAML format, via GET and PUT requests to the api/metrics/v1/{tenant}/api/v1/rules/raw endpoint of the API.
• However when a tenant sends a GET request to api/metrics/v1/{tenant}/api/v1/rules, the request is proxied by the API to the read upstream endpoint (for eg. Thanos Querier). The querier sends back all rules of all tenants in JSON. This is handled using uiProxy. This is what this ticket/PR aims to solve.

Maybe I should change the PR title?

Note: this is a distinct problem from ensuring that the rules written by
a tenant are guaranteed to only include data from that tenant.
Currently, the rules from a tenant can select data from any tenant. This
is a distinct problem and should absolutely be tackled in a follow up.

This is still a distinct problem. :)

[squat]

ack. I wasn't aware of this second endpoint. hmm this endpoint seems to be essentially part of an attempt trying to turn Observatorium into an equivalent Prometheus/Thanos Querier replacement

[bill3tt]

Nice PR @saswatamcode ✊ I think the changes you have proposed would work, but I am wondering if there is a different solution that better maintains existing properties of the API 🤔

As @squat alludes to - the API was always designed and intended to be a light-weight authentication and authorization proxy that enforced the relevant tenant configurations on requests. With this in mind, it feels like adding rule specific logic in this PR start to take us down a path we have tried our best to resist so far!

Did you consider any upstream / Thanos first approaches? For example if the Thanos Query rules API supported matchers (like /labels does), then the API would then append the right labels and proxy the request forward.

This would be more work (requires change in Rules proto), but feels like it would maintain the lightweight API properties and could also be useful for Thanos.

[saswatamcode]

Hi @ianbillett!
Yes, I did consider the upstream first approach before, while discussing this with @onprem. 🙂

Adding matcher support to Thanos Query rules API is certainly possible but I was a bit hesitant to go with that, as this would make the Query rules API diverge from the Prometheus rules API (one would have matcher support while the other wouldn't).

Also, I found precedent that bits of prom-label-proxy were already adapted in Observatorium API here, so I decided to go with this.

If the upstream approach seems better for this use case, I can raise a PR to Thanos and update this PR once merged. WDYT? 🙂

[bwplotka]

SGTM for changing Prom and Thanos API first (:

Sounds like this is a great starting point: prometheus/prometheus#9627

[bill3tt]

We just discussed this issue at the Thanos Community hours meeting 👍

We're happy with the idea of adding matchers to the Thanos /rules calls. Though this would make Thanos differ from the Prometheus API that is ok, as long as all valid Prometheus requests are still compatible. In fact, there is an upstream Prometheus issue prometheus/prometheus#9627 that is proposing the same thing.

Next steps are to follow up with the upstream issue first, and if we can't progress with that then add it in Thanos 👍

edit: @bwplotka beat me to the summary 😅

[squat]

one thing I'm curious about is why we want to add this as a first-class feature in the Thanos/Prometheus API vs making the prom-label proxy part of the Observatorium infrastructure/deployment and add the enforcement there?

[saswatamcode]

I think adding this as a first-class feature to Thanos/Prometheus API would be better as it solves a couple of issues, 🙂

• There are already issues asking for label-based matcher support in rules API upstream
• By adding it here, we avoid having another moving part in our deployment but don't compromise on tenancy

[saswatamcode]

This can be merged once thanos#5111 makes its way to main! 🙂

[matej-g]

Looks good, thank you @saswatamcode 👍