Skip to content
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

Alerting: handle folder names with / safely #42947

Closed
davidmparrott opened this issue Dec 9, 2021 · 10 comments
Closed

Alerting: handle folder names with / safely #42947

davidmparrott opened this issue Dec 9, 2021 · 10 comments
Labels
area/alerting Grafana Alerting

Comments

@davidmparrott
Copy link
Contributor

davidmparrott commented Dec 9, 2021

What happened:
As originally reported in #40979 and followed up by https://github.com/grafana/hosted-grafana/issues/1667 hosted grafana alerting handles URLs in a way that breaks when attempting to use a namespace (folder) with / in it. We should consider ways to handle namespaces structured like this more safely while still allowing users to have namespaces that contain /

Possible approaches include:

  • using the UID of the folder in the namespace field
  • pass namespace as part of the payload instead of the URL
  • sluggify the resource name so it is human-readable but still URL-safe
@davidmparrott davidmparrott added area/alerting/unified area/frontend prio/critical Highest priority. Must be actively worked on as someone's top priority right now. labels Dec 9, 2021
@davidmparrott
Copy link
Contributor Author

@gillesdemey @peterholmberg can you have a look at possibly fixing this in the UI? This is high priority as CRUD operations for alerting rules are broken for anyone using a namespace with a / in it

@gillesdemey gillesdemey self-assigned this Dec 15, 2021
@gillesdemey
Copy link
Member

I've had a look at this and it looks like we are properly encoding the namespace and groupName in all requests that are being sent to the ruler API.

https://github.com/grafana/grafana/blob/85f134969e46023ec84c82f9205f8dddc01b4e6b/public/app/features/alerting/unified/api/ruler.ts

I'm not quite sure if there is anything we can do on the front-end without involving a change on the backend to resolve this one unless we transform the / to any other character before sending out the request.

At first glance it looks like we are a transparent proxy towards Cortex (https://cortexmetrics.io/docs/api/#get-rule-group) – perhaps the bug also occurs there?

@chaudyg
Copy link
Contributor

chaudyg commented Dec 20, 2021

@gillesdemey I agree grafana is properly encoding the namepace and groupName in all requests that are being sent to the ruler API. I think the problem is the fact that URL encoding / and \ can be very confusing for the backend and to path confusion. For example. go server will natively parse /api/ruler/grafana/api/v1/rules/some%2Ftest into /api/ruler/grafana/api/v1/rules/some/test. It turns out that the endpoint works in grafana because it uses a regexp to parse the url (here). For grafana instances running behind a reverse proxy like nginx, nginx might normalize the URL as well (see proxy_pass) and the request sent by the frontend will not be understood by Grafana. The two characters / and \ have meaning in a request path, and I don't think we can make the assumption the backend will understand the request the way Grafana intends to. It feels like something we should changed in grafana directly.

@gillesdemey
Copy link
Member

Thanks for investigating @chaudyg – I've dug a bit deeper and can indeed confirm that the encoded %2f characters are interpreted as a regular path delimiter (/).

We will look in to disallowing the / character in namespaces and group names in the front-end.

We also believe that there is a valid concern when it comes to the Cortex API interface – creating a groupName within a namespace is done by sending a JSON payload to a namespace

POST /api/v1/rules/testing

{"name":"foo/bar","rules":[{"record":"testing123","labels":{},"expr":"1"}]}

This will in turn create a group named foo/bar in the testing namespace, but you will not be able to remove this group because that one wants a request to be sent to DELETE /api/v1/rules/{namespace}/{groupName} and even if the group name was encoded the request would always return 404.

@gillesdemey
Copy link
Member

After some help from @pstibrany it seems like the issue is not in Cortex but perhaps in a proxy or gateway that sits in front of the Cortex setup, creating group names and deleting them via the HTTP API works fine when sent to a local Cortex setup.

@gillesdemey
Copy link
Member

Perhaps the decoding is happening somewhere in the https://github.com/grafana/backend-enterprise project, @JohnnyQQQQ investigated it and it may be decoded in the reverse proxy.

@gillesdemey gillesdemey added needs investigation for unconfirmed bugs. use type/bug for confirmed bugs, even if they "need" more investigating and removed needs investigation for unconfirmed bugs. use type/bug for confirmed bugs, even if they "need" more investigating labels Jan 4, 2022
@JohnnyQQQQ
Copy link
Member

JohnnyQQQQ commented Jan 4, 2022

We talked about this in the weekly.

The consensus is that unescaping an URL is expected behavior. We also agree that a user should be able to use whatever they want as a name.

The main problem we have here is that we will always need to provide the groupName in the URL. The only endpoint that exists to delete a namespace or group will expect the name in the URL path (DELETE /api/v1/rules/{namespace}/{groupName}).

This is also a problem for users running Grafana behind a proxy, as most proxies will unescape the URL before forwarding the request as mentioned in the comments above.

As the plain text name is used as the unique identifier, we don't have any way around it.

I think we have a few options here:

  • Save a mapping somewhere between the real name with slashes and some form of slug we use for Cortex/Loki
  • Come up with an escape sequence for slashes that we encode and decode in Grafana
  • Try to get a new endpoint upstream that will use the body payload instead of the URL
  • Forbid slashes, even if we don't want to do it

@gillesdemey
Copy link
Member

gillesdemey commented Jan 5, 2022

Since we don't have a UID to work with and sluggifying requires a migration, the only reasonable proposal left is to pass the name of the group you wish to delete either via the body (but some server implementations might ignore DELETE requests with a body even though the RFC allows it) or pass it via a querystring, ie. DELETE /api/v1/rules/namespace?groupName=foo.

Those should not be automatically decoded by proxies. Either proposal would require a change in the Cortex API since it only allows deleting groupnames via the name in the path and proxies could always decode it like @JohnnyQQQQ mentioned.

@gillesdemey gillesdemey removed their assignment Jan 5, 2022
@gillesdemey
Copy link
Member

This was discussed further with the engineering team – consensus is not to try and fix this in the current API of Cortex / Loki / Grafana and to make sure we configure all proxies properly instead.

We are instead spending our effort on an API redesign where this issue will be resolved.

@gillesdemey gillesdemey added wontfix and removed prio/critical Highest priority. Must be actively worked on as someone's top priority right now. labels Jan 11, 2022
@armandgrillet armandgrillet reopened this Jan 25, 2022
@armandgrillet armandgrillet added area/alerting Grafana Alerting and removed area/alerting/unified labels Jun 22, 2022
@konrad147
Copy link
Contributor

By adding validation, we can deal with the slashes in alert rules names, namespaces and groups but not in folders.
Unfortunately, folders are much worse, as they are not a concept exclusive to Alerting but they are used in other parts of Grafana too.
Using / in folder names is perfectly fine e.g. when you create a folder for dashboards. And then when you go to Alerting and try to use the same folder you won't see it.
Right now, we sacrifice compatibility with Grafana in exchange for compliance with Prometheus (API redesign doesn't seem to be a viable option, as it would break compatibility with Prometheus)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Grafana Alerting
Projects
Archived in project
Development

No branches or pull requests

6 participants