feat(httpclient): support appending the same header multiple times #830

sathieu · 2023-12-05T10:28:02Z

What this PR does / why we need it:

See #829

Which issue(s) this PR fixes:

Fixes #829

Special notes for your reviewer:

CLAassistant · 2023-12-05T10:28:13Z

All committers have signed the CLA.

marefr · 2023-12-08T10:30:58Z

Please sync with main and resolve CI failure and we'll review this. Thanks

mage -v lint
go: downloading github.com/magefile/mage v1.15.0
Running target: Lint
exec: golangci-lint "run" "./..."
experimental/authclient/authclient_test.go:1: : # github.com/grafana/grafana-plugin-sdk-go/experimental/authclient_test [github.com/grafana/grafana-plugin-sdk-go/experimental/authclient.test]
experimental/authclient/authclient_test.go:34:15: cannot use map[string]string{…} (value of type map[string]string) as map[string][]string value in struct literal
experimental/authclient/authclient_test.go:57:17: cannot use map[string]string{…} (value of type map[string]string) as map[string][]string value in struct literal
experimental/authclient/authclient_test.go:84:15: cannot use map[string]string{…} (value of type map[string]string) as map[string][]string value in struct literal
experimental/authclient/authclient_test.go:106:15: cannot use map[string]string{…} (value of type map[string]string) as map[string][]string value in struct literal (typecheck)
package authclient_test
Error: running "golangci-lint run ./..." failed with exit code 1

sathieu · 2023-12-11T10:08:31Z

@marefr I've fixed the test. back to you 🏸 !

andresmgot

I think that in general, it makes sense to use the same type than the upstream HTTP header: https://pkg.go.dev/net/http#Header but I am worried about the breaking change. If we do this, we should prepare at least an adapting PR for grafana/grafana.

Regarding the original issue described in #829, I wonder if a possible workaround would be to define the multiple headers as a comma-separated list. @sathieu would that work?

andresmgot · 2023-12-13T12:33:55Z

backend/http_settings.go

@@ -16,7 +16,7 @@ type HTTPSettings struct {
 	BasicAuthEnabled  bool
 	BasicAuthUser     string
 	BasicAuthPassword string
-	Headers           map[string]string
+	Headers           map[string][]string


I would just go ahead and define this as a http.Header

Good idea @andresmgot.

I have done that, and renamed from Headers to Header, as I broke API anyway (and this is now same as net/http.

sathieu · 2023-12-14T21:31:43Z

Thanks for your review @andresmgot.

I think that in general, it makes sense to use the same type than the upstream HTTP header: https://pkg.go.dev/net/http#Header but I am worried about the breaking change. If we do this, we should prepare at least an adapting PR for grafana/grafana.

I prepared a PR in grafana at grafana/grafana#79543.

Regarding the original issue described in #829, I wonder if a possible workaround would be to define the multiple headers as a comma-separated list. @sathieu would that work?

Unfortunately, comma-separated header doesn't work for prom-label-proxy (see code). So this PR is needed.

Back to you!

andresmgot

Thanks, one minor comment here

backend/http_settings.go

andresmgot

LGTM, any other thought @marefr ?

See grafana/grafana-plugin-sdk-go#830

academo · 2024-02-28T08:47:13Z

@andresmgot @marefr is there anything blocking this PR from merging?

marefr

LGTM, but a question before merging

marefr · 2024-03-06T14:15:04Z

backend/http_settings.go

@@ -273,7 +274,7 @@ func parseHTTPSettings(jsonData json.RawMessage, secureJSONData map[string]strin

 		if key, exists := dat[headerNameSuffix]; exists {
 			if value, exists := secureJSONData[headerValueSuffix]; exists {
-				s.Headers[key.(string)] = value
+				s.Header.Add(key.(string), value)


Don't understand why key needs to be cast to a string - isn't it already a string since dat is map[string]interface{}?

dat is map[string]interface{}, so key := dat[headerNameSuffix] is interface{}.

On the other side secureJSONData is map[string]string, so value doesn't need cast.

Oh yeah I see that now 👍

marefr · 2024-03-06T14:16:10Z

@sathieu please sync with main branch to resolve conflicts and see my comment. Thanks

sathieu · 2024-03-08T14:22:26Z

@marefr I've rebased and resolved conflicts. Casting is needed.

Back to you 🏓

andresmgot · 2024-03-11T11:21:59Z

hi @sathieu, there are some other files that need an update: https://drone.grafana.net/grafana/grafana-plugin-sdk-go/1981/1/3

Also, can you re-open / refresh grafana/grafana#79543? (sorry, it got closed due to inactivity)

marefr · 2024-03-11T13:46:33Z

@sathieu back to you. Not sure whether we really should remove Headers and add Header. I would suggest to keep Headers. WDYT? Also, lint problems

mage -v lint
go: downloading github.com/magefile/mage v1.15.0
Running target: Lint
exec: golangci-lint "run" "./..."
backend/httpclient/basic_auth_middleware.go:1: : # github.com/grafana/grafana-plugin-sdk-go/backend/httpclient [github.com/grafana/grafana-plugin-sdk-go/backend/httpclient.test]
backend/httpclient/custom_headers_middleware_test.go:67:48: unknown field Headers in struct literal of type Options (typecheck)
package httpclient
backend/httpclient/contextual_middleware_example_test.go:8:2: could not import github.com/grafana/grafana-plugin-sdk-go/backend/httpclient (-: # github.com/grafana/grafana-plugin-sdk-go/backend/httpclient [github.com/grafana/grafana-plugin-sdk-go/backend/httpclient.test]
backend/httpclient/custom_headers_middleware_test.go:67:48: unknown field Headers in struct literal of type Options) (typecheck)
	"github.com/grafana/grafana-plugin-sdk-go/backend/httpclient"
	^
Error: running "golangci-lint run ./..." failed with exit code 1
``

See grafana#829

See grafana/grafana-plugin-sdk-go#830

sathieu · 2024-03-11T14:51:17Z

@andresmgot :

hi @sathieu, there are some other files that need an update: https://drone.grafana.net/grafana/grafana-plugin-sdk-go/1981/1/3

Fixed, rebased and pushed.

Also, can you re-open / refresh grafana/grafana#79543? (sorry, it got closed due to inactivity)

I can't reopen. I've created a new PR: grafana/grafana#84193

sathieu · 2024-03-11T14:53:50Z

@marefr

I think Header is more appropriate, like http.Request, but I can change to Headers if you want.

marefr

LGTM sure let's use Header

sathieu requested a review from a team as a code owner December 5, 2023 10:28

sathieu requested review from wbrowne, marefr and xnyo and removed request for a team December 5, 2023 10:28

sathieu mentioned this pull request Dec 5, 2023

Allow to set the same header multiple time in Prometheus Datascources #829

Closed

sathieu force-pushed the multiple-same-headers branch 2 times, most recently from 7067bef to 0314907 Compare December 5, 2023 10:45

sathieu force-pushed the multiple-same-headers branch from 0314907 to f8fe86a Compare December 11, 2023 10:02

sathieu force-pushed the multiple-same-headers branch from f8fe86a to c6c0ded Compare December 11, 2023 10:11

andresmgot reviewed Dec 13, 2023

View reviewed changes

sathieu force-pushed the multiple-same-headers branch from c6c0ded to bbc71f5 Compare December 14, 2023 20:58

sathieu mentioned this pull request Dec 14, 2023

chore: Upgrade SDK to support multiple headers with same name grafana/grafana#79543

Closed

3 tasks

andresmgot reviewed Dec 15, 2023

View reviewed changes

backend/http_settings.go Outdated Show resolved Hide resolved

sathieu force-pushed the multiple-same-headers branch from bbc71f5 to 3dee90d Compare December 18, 2023 20:26

andresmgot approved these changes Dec 19, 2023

View reviewed changes

sathieu added a commit to sathieu/grafana that referenced this pull request Dec 19, 2023

chore: Upgrade SDK to support multiple headers with same name

a36748d

See grafana/grafana-plugin-sdk-go#830

marefr approved these changes Mar 6, 2024

View reviewed changes

marefr assigned marefr and andresmgot Mar 6, 2024

sathieu force-pushed the multiple-same-headers branch from 3dee90d to 5a057a4 Compare March 8, 2024 14:19

feat(httpclient): support appending the same header multiple times

d132ddf

See grafana#829

sathieu force-pushed the multiple-same-headers branch from 5a057a4 to d132ddf Compare March 11, 2024 14:34

sathieu added a commit to sathieu/grafana that referenced this pull request Mar 11, 2024

chore: Upgrade SDK to support multiple headers with same name

19c3bd4

See grafana/grafana-plugin-sdk-go#830

sathieu mentioned this pull request Mar 11, 2024

chore: Upgrade SDK to support multiple headers with same name grafana/grafana#84193

Closed

3 tasks

marefr approved these changes Mar 12, 2024

View reviewed changes

andresmgot merged commit a9a0737 into grafana:main Mar 12, 2024
2 of 3 checks passed

wbrowne mentioned this pull request Mar 20, 2024

Prometheus: Prevent duplicate registration of custom header middleware grafana/grafana#84860

Merged

grafana-delivery-bot bot mentioned this pull request Mar 25, 2024

[v10.4.x] Prometheus: Prevent duplicate registration of custom header middleware grafana/grafana#85120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(httpclient): support appending the same header multiple times #830

feat(httpclient): support appending the same header multiple times #830

sathieu commented Dec 5, 2023

CLAassistant commented Dec 5, 2023 •

edited

Loading

marefr commented Dec 8, 2023 •

edited

Loading

sathieu commented Dec 11, 2023

andresmgot left a comment

andresmgot Dec 13, 2023

sathieu Dec 14, 2023

sathieu commented Dec 14, 2023

andresmgot left a comment

andresmgot left a comment

academo commented Feb 28, 2024

marefr left a comment

marefr Mar 6, 2024

sathieu Mar 8, 2024 •

edited

Loading

marefr Mar 11, 2024

marefr commented Mar 6, 2024

sathieu commented Mar 8, 2024

andresmgot commented Mar 11, 2024

marefr commented Mar 11, 2024

sathieu commented Mar 11, 2024

sathieu commented Mar 11, 2024

marefr left a comment

feat(httpclient): support appending the same header multiple times #830

feat(httpclient): support appending the same header multiple times #830

Conversation

sathieu commented Dec 5, 2023

CLAassistant commented Dec 5, 2023 • edited Loading

marefr commented Dec 8, 2023 • edited Loading

sathieu commented Dec 11, 2023

andresmgot left a comment

Choose a reason for hiding this comment

andresmgot Dec 13, 2023

Choose a reason for hiding this comment

sathieu Dec 14, 2023

Choose a reason for hiding this comment

sathieu commented Dec 14, 2023

andresmgot left a comment

Choose a reason for hiding this comment

andresmgot left a comment

Choose a reason for hiding this comment

academo commented Feb 28, 2024

marefr left a comment

Choose a reason for hiding this comment

marefr Mar 6, 2024

Choose a reason for hiding this comment

sathieu Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

marefr Mar 11, 2024

Choose a reason for hiding this comment

marefr commented Mar 6, 2024

sathieu commented Mar 8, 2024

andresmgot commented Mar 11, 2024

marefr commented Mar 11, 2024

sathieu commented Mar 11, 2024

sathieu commented Mar 11, 2024

marefr left a comment

Choose a reason for hiding this comment

CLAassistant commented Dec 5, 2023 •

edited

Loading

marefr commented Dec 8, 2023 •

edited

Loading

sathieu Mar 8, 2024 •

edited

Loading