Plugins: Fix colon in CallResource URL returning an error when creating plugin resource request

[GiedriusS]

url.Parse() does not handle the given input correctly when the input contains a colon character. The user will see the following error message when trying to use remote cluster in Elasticsearch:

level=warn msg="Failed for create plugin resource request" error="parse \"foo-*,*:foo-*/_mapping\": first path segment in URL cannot contain colon" traceID=

As far as I can tell, we only want to set the path here + rawquery so avoid url.Parse() altogether.

What is this feature?

N/A

Why do we need this feature?

N/A

Who is this feature for?

N/A

Which issue(s) does this PR fix?:

Fixes #79745

Special notes for your reviewer:

Please check that:

• It works as expected from a user's perspective.
• If this is a pre-GA feature, it is behind a feature toggle.
• The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

[GiedriusS]

@zserge @mildwonkey @idafurjes anyone?

[ivanahuckova]

@grafana/backend-platform could you please review as this will fix issue with Elasticsearch data source and cross-cluster search which is currently broken #79745.

[grafana-delivery-bot]

This PR must be merged before a backport PR will be created.

[grafana-delivery-bot]

This PR must be merged before a backport PR will be created.

[xnyo]

Hi and thank you for your contribution!

From a CallResource point of view, the changes make sense to me. From my testing and by comparing with the curent main, we are only interested in the relative path, which is Path + RawQuery.

I have also merged the latest main branch so CI can run fine.

However, if I try to reproduce the original issue described in #79745 (adding an Elasticsearch datasource with foo,othernode:foo index), the /resources/foo,othernode:foo/_mapping request still fails with a 500 error:

ERROR[01-24|16:02:03] Failed to parse data source path         logger=tsdb.elasticsearch endpoint=callResource pluginId=elasticsearch dsName=elasticsearch-1 dsUID=b2d54c90-8ff5-403f-940d-e0937ff68240 uname=admin error="parse \"foo,othernode:foo/_mapping\": first path segment in URL cannot contain colon" url=foo,othernode:foo/_mapping
ERROR[01-24|16:02:03] Plugin Request Completed                 logger=plugin.instrumentation endpoint=callResource pluginId=elasticsearch dsName=elasticsearch-1 dsUID=b2d54c90-8ff5-403f-940d-e0937ff68240 uname=admin status=error duration=72.045µs eventName=grafana-data-egress time_before_plugin_request=723.449µs error="[plugin.downstreamError] client: failed to call resources: parse \"foo,othernode:foo/_mapping\": first path segment in URL cannot contain colon" statusSource=plugin
ERROR[01-24|16:02:03] Internal server error                    logger=context userId=1 orgId=1 uname=admin error="[plugin.downstreamError] client: failed to call resources: parse \"foo,othernode:foo/_mapping\": first path segment in URL cannot contain colon" remote_addr=127.0.0.1 traceID=
ERROR[01-24|16:02:03] Request Completed                        logger=context userId=1 orgId=1 uname=admin method=GET path=/api/datasources/uid/b2d54c90-8ff5-403f-940d-e0937ff68240/resources/foo,othernode:foo/_mapping status=500 remote_addr=127.0.0.1 time_ms=0 duration=859.996µs size=116 referer=http://127.0.0.1:3000/connections/datasources/edit/b2d54c90-8ff5-403f-940d-e0937ff68240 handler=/api/datasources/uid/:uid/resources/*

This is because the Elasticsearch plugin itself uses url.Parse to extract the index name from the URL, here:

grafana/pkg/tsdb/elasticsearch/elasticsearch.go

Lines 210 to 214 in 0cf6b94

	resourcePath, err := url.Parse(req.Path)
	if err != nil {
	logger.Error("Failed to parse data source path", "error", err, "url", req.Path)
	return err
	}

So this would need to be changed as well. Alternatively, I would also consider using a POST request instead, and putting the index name in the body rather than in the URL.

cc @ivanahuckova

[ivanahuckova]

Elastic API expects GET request https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-get-mapping.html so I'd prefer to use that. Also I like the idea of keeping this simple and just fixing using GET on both grafana resource call and when sending to Elastic. So if this PR is good from your perspective, feel free to merge and I create additional PR to tackle the logic in Elastic. 🙂

[GiedriusS]

Thanks for testing out and seeing if this helps, I had a hard time running Grafana on my computer so I couldn't test thoroughly if this PR is the only one that needs to be merged for fixing the original problem. 🙇 I am deeply grateful for your work

[xnyo]

Elastic API expects GET request https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-get-mapping.html so I'd prefer to use that. Also I like the idea of keeping this simple and just fixing using GET on both grafana resource call and when sending to Elastic. So if this PR is good from your perspective, feel free to merge and I create additional PR to tackle the logic in Elastic. 🙂

Another option could be encoding the index name in base64 so it's url-safe.

If we decide to keep the current request, this PR makes sense to me from a CallResource point of view, but I would also wait on @marefr or @wbrowne before merging to double check that I am not missing anything. @ivanahuckova then you can go ahead and do the changes on the Elasticsearch plugin side.

[marefr]

Elastic API expects GET request https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-get-mapping.html so I'd prefer to use that. Also I like the idea of keeping this simple and just fixing using GET on both grafana resource call and when sending to Elastic. So if this PR is good from your perspective, feel free to merge and I create additional PR to tackle the logic in Elastic. 🙂

Another option could be encoding the index name in base64 so it's url-safe.

Can we URL encode the index before making the request or doesn't matter? Don't you have to do that before calling the ES mapping endpoint as well? foo-*,*:foo-* => foo-%2A%2C%2A%3Afoo-%2A

If we decide to keep the current request, this PR makes sense to me from a CallResource point of view, but I would also wait on @marefr or @wbrowne before merging to double check that I am not missing anything. @ivanahuckova then you can go ahead and do the changes on the Elasticsearch plugin side.

Think this make sense, but curious why we used url.Parse in the first place and if it was for some specific use case. I guess to make sure URL is valid but also that all fields of URL would be populated correctly. Would using https://pkg.go.dev/net/url#ParseRequestURI instead be a better solution? Would be good with a test perhaps verifying that query parameters are properly included in the CallResourceRequest.URL field since that's what the code seems to solve?

[ivanahuckova]

Can we URL encode the index before making the request or doesn't matter?

I tried encoding with https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent but it doesn't work and returns the same error ☹️

WARN [01-24|17:28:17] Failed for create plugin resource request logger=context traceID=cc1e20ca445cf305254be397b84e9f5e userId=1 orgId=1 uname=admin error="parse \"abc,def:ghi/_mapping\": first path segment in URL cannot contain colon" traceID=cc1e20ca445cf305254be397b84e9f5e

[xnyo]

Would using https://pkg.go.dev/net/url#ParseRequestURI instead be a better solution?

I tried url.ParseRequestURI(rawURL) instead of url.Parse(rawURL) and it returns the same error. Under the hood they both call the same function that checks for colons (url.parse), so unfortunately it doesn't work.

Would be good with a test perhaps verifying that query parameters are properly included in the CallResourceRequest.URL field since that's what the code seems to solve?

I can add a test case for query params, I tested those manually and they were working fine but a test case is definitely a good idea 👍

[marefr]

Was curious and backtracked the introduction of this code from 3,5 years ago and of course by me 5b951c7 #25472 😅 No tests or anything so hard to tell why exactly I added it, but I guess I realized somehow query parameters was not forwarded correctly.

grafana/pkg/plugins/backendplugin/manager.go

Lines 281 to 292 in c0f3b29

	clonedReq := reqCtx.Req.Clone(reqCtx.Req.Context())
	rawURL := path
	if clonedReq.URL.RawQuery != "" {
	rawURL += "?" + clonedReq.URL.RawQuery
	}
	urlPath, err := url.Parse(rawURL)
	if err != nil {
	handleCallResourceError(err, reqCtx)
	return
	}
	clonedReq.URL = urlPath
	err = m.callResourceInternal(reqCtx.Resp, clonedReq, pCtx)

[xnyo]

Would be good with a test perhaps verifying that query parameters are properly included in the CallResourceRequest.URL field since that's what the code seems to solve?

I have added more tests to ensure query parameters are forwarded correctly

[marefr]

LGTM

[ivanahuckova]

@xnyo @marefr some tests are failing in this PR, so we are not able to merge. Whenever you have time, could you please have a look. 🙂

[xnyo]

@ivanahuckova CI is now passing and we can merge. I don't think this requires backporting (not a critical bugfix), WDYT?

[ivanahuckova]

I don't think this requires backporting (not a critical bugfix), WDYT?

+1

[xnyo]

@ivanahuckova PR is now merged. You can now make the required changes in the Elasticsearch plugin to handle colons on the plugin's side 🥳