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

allow using {{ .URL }} inside "routes" section in plugin.json #80858

Merged
merged 5 commits into from Feb 2, 2024

Conversation

Slach
Copy link
Contributor

@Slach Slach commented Jan 19, 2024

What is this feature?
Detail description is #80856

Why do we need this feature?

Looks like current implementation contains bug

Who is this feature for?

For plugin developers which can't store all settings into jsonData
and try to use standard @grafana/ui DataSourceHttpSettings component

For plugin developers when data source destination server doesn't allow additional path in proxyfied request URL.

Which issue(s) does this PR fix?:

Fixes #80856

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.

@Slach Slach requested a review from a team as a code owner January 19, 2024 08:04
@Slach Slach requested review from wbrowne, andresmgot and oshirohugo and removed request for a team January 19, 2024 08:04
@grafana-pr-automation grafana-pr-automation bot added area/backend pr/external This PR is from external contributor labels Jan 19, 2024
@Slach
Copy link
Contributor Author

Slach commented Jan 19, 2024

@wbrowne @andresmgot @oshirohugo could you sugget how to pass Changelog Check and Milestone Check? It looks like manual approve

Slach added a commit to Altinity/clickhouse-grafana that referenced this pull request Jan 19, 2024
Slach added a commit to Altinity/clickhouse-grafana that referenced this pull request Jan 19, 2024
@CLAassistant
Copy link

CLAassistant commented Jan 19, 2024

CLA assistant check
All committers have signed the CLA.

@Slach Slach mentioned this pull request Jan 19, 2024
25 tasks
@Slach Slach requested a review from andresmgot January 19, 2024 18:20
Slach added a commit to Slach/grafana that referenced this pull request Jan 22, 2024
@andresmgot andresmgot added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Jan 22, 2024
@andresmgot andresmgot added this to the 10.4.x milestone Jan 22, 2024
@andresmgot
Copy link
Contributor

It seems there is a specific test that prevents the request from being modified at all:

https://github.com/grafana/grafana/blob/main/pkg/api/pluginproxy/ds_proxy_test.go#L151-L159

This was introduced here: #27653 which seemed to be only interested in keeping the Host and Schema but also protected the Path. Seems unnecessary but do you know if this may break anything @marefr ?

@Slach
Copy link
Contributor Author

Slach commented Jan 22, 2024

@andresmgot i can't figure out
why test-backend failed, but no-one test failed here - https://drone.grafana.net/grafana/grafana/155812/1/7

@andresmgot
Copy link
Contributor

@andresmgot i can't figure out why test-backend failed, but no-one test failed here - https://drone.grafana.net/grafana/grafana/155812/1/7

It did fail, you need to expand the logs:

--- FAIL: TestDataSourceProxy_routeRule (0.65s)
    --- FAIL: TestDataSourceProxy_routeRule/Plugin_with_routes (0.00s)
        --- FAIL: TestDataSourceProxy_routeRule/Plugin_with_routes/When_matching_route_path_with_no_url (0.00s)
            ds_proxy_test.go:158: 
                	Error Trace:	/drone/src/pkg/api/pluginproxy/ds_proxy_test.go:158
                	Error:      	Not equal: 
                	            	expected: "http://localhost/asd"
                	            	actual  : "http://localhost"
                	            	
                	            	Diff:
                	            	--- Expected
                	            	+++ Actual
                	            	@@ -1 +1 @@
                	            	-http://localhost/asd
                	            	+http://localhost
                	Test:       	TestDataSourceProxy_routeRule/Plugin_with_routes/When_matching_route_path_with_no_url
FAIL
coverage: 65.8% of statements
FAIL	github.com/grafana/grafana/pkg/api/pluginproxy	0.935s

@Slach
Copy link
Contributor Author

Slach commented Jan 23, 2024

@andresmgot thanks for logs
so, what will we do?
should I expand template data to allow pass URL or should i change test behavior?

@Slach Slach changed the title replace prefix path from url when using routes section without url allow using {{ .URL }} inside "routes" section in plugin.json Jan 28, 2024
@Slach
Copy link
Contributor Author

Slach commented Jan 28, 2024

@andresmgot @marefr I implemented allowing to use {{ .URL }} instead of changing behavior with proxyPath.
Please review it.

@Slach Slach requested a review from andresmgot January 28, 2024 02:34
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, do you mind adding a test to cover this use case? You can add the following:

--- a/pkg/api/pluginproxy/ds_proxy_test.go
+++ b/pkg/api/pluginproxy/ds_proxy_test.go
@@ -158,6 +158,36 @@ func TestDataSourceProxy_routeRule(t *testing.T) {
                        assert.Equal(t, "http://localhost/asd", req.URL.String())
                })
 
+               t.Run("When matching route path and has setting url", func(t *testing.T) {
+                       ctx, req := setUp()
+                       proxy, err := setupDSProxyTest(t, ctx, ds, routes, "api/common/some/method")
+                       require.NoError(t, err)
+                       proxy.matchedRoute = &plugins.Route{
+                               Path: "api/common",
+                               URL:  "{{.URL}}",
+                               Headers: []plugins.Header{
+                                       {Name: "x-header", Content: "my secret {{.SecureJsonData.key}}"},
+                               },
+                               URLParams: []plugins.URLParam{
+                                       {Name: "{{.JsonData.queryParam}}", Content: "{{.SecureJsonData.key}}"},
+                               },
+                       }
+
+                       dsInfo := DSInfo{
+                               ID:       ds.ID,
+                               Updated:  ds.Updated,
+                               JSONData: jd,
+                               DecryptedSecureJSONData: map[string]string{
+                                       "key": "123",
+                               },
+                               URL: "https://dynamic.grafana.com",
+                       }
+                       ApplyRoute(proxy.ctx.Req.Context(), req, proxy.proxyPath, proxy.matchedRoute, dsInfo, proxy.cfg)
+
+                       assert.Equal(t, "https://dynamic.grafana.com/some/method?apiKey=123", req.URL.String())
+                       assert.Equal(t, "my secret 123", req.Header.Get("x-header"))
+               })
+

Slach added a commit to Slach/grafana that referenced this pull request Feb 1, 2024
@Slach
Copy link
Contributor Author

Slach commented Feb 1, 2024

@andresmgot test added, thanks for suggestion

@Slach
Copy link
Contributor Author

Slach commented Feb 1, 2024

@andresmgot hot to approve continuous-integration/drone/pr github workflow ?

@Slach Slach requested a review from andresmgot February 1, 2024 12:01
@andresmgot
Copy link
Contributor

hi @Slach, can you merge the latest main here to deal with the CI issues?

@Slach
Copy link
Contributor Author

Slach commented Feb 2, 2024

merge and rebase, done
@andresmgot

@andresmgot
Copy link
Contributor

Thank you for the contribution.

@andresmgot andresmgot merged commit 702e228 into grafana:main Feb 2, 2024
11 checks passed
Ukochka pushed a commit that referenced this pull request Feb 14, 2024
…in.json (#80858)

Signed-off-by: Slach <bloodjazman@gmail.com>
Co-authored-by: Andres Martinez Gotor <andres.mgotor@gmail.com>
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes pr/external This PR is from external contributor
Projects
None yet
5 participants