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.

Sign up for GitHub

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

Jump to bottom

Datasource: Fix allowed cookies to be forwarded as header to backend datasources #49541

Merged
merged 2 commits into from
May 31, 2022
Merged

Datasource: Fix allowed cookies to be forwarded as header to backend datasources #49541

merged 2 commits into from
May 31, 2022

Conversation

marefr
Copy link
Member

@marefr marefr commented May 24, 2022

What this PR does / why we need it:
Populates Cookie header in QueryDataRequest.Headers given Allowed cookies (jsonData.keepCookies) is configured for a datasource.

Which issue(s) this PR fixes:
Fixes #44250

Special notes for your reviewer:

  • Seems like it should work out of the box for Prometheus, but need to test/verify

@marefr marefr added this to the 9.0.0 milestone May 24, 2022
@marefr marefr self-assigned this May 24, 2022
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/19624

wbrowne
wbrowne reviewed May 25, 2022
View reviewed changes
pkg/services/query/query.go
@@ -149,6 +151,19 @@ func (s *Service) handleQueryData(ctx context.Context, user *models.SignedInUser
req.Headers[k] = v
}

if parsedReq.httpRequest != nil && parsedReq.httpRequest.Header.Get("Cookie") != "" && ds.JsonData != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder is now a good opportunity to refactor so that resource, query and proxy calls both use the same code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. They are different though, not sure how easy that would be. Do you have an idea?

ryantxu
ryantxu reviewed May 31, 2022
View reviewed changes
pkg/services/query/query.go
if parsedReq.httpRequest != nil && parsedReq.httpRequest.Header.Get("Cookie") != "" && ds.JsonData != nil {
keepCookieNames := []string{}

if keepCookies := ds.JsonData.Get("keepCookies"); keepCookies != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a kinda big feature to stick on the untyped jsonData

Where is this used? Is it something that is really set explicitly from the UI? It seems more like a setting attached to the whole plugin than something that would selectively apply to some instances

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an oldie part of the datasource HTTP settings for years:

grafana/packages/grafana-ui/src/components/DataSourceSettings/DataSourceHttpSettings.tsx

Lines 172 to 186 in 8fa8c9d

<div className="gf-form">
<InlineFormLabel
width={13}
tooltip="Grafana proxy deletes forwarded cookies by default. Specify cookies by name that should be forwarded to the data source."
>
Allowed cookies
</InlineFormLabel>
<TagsInput
tags={dataSourceConfig.jsonData.keepCookies}
width={40}
onChange={(cookies) =>
onSettingsChange({ jsonData: { ...dataSourceConfig.jsonData, keepCookies: cookies } })
}
/>
</div>

@marefr marefr marked this pull request as ready for review May 31, 2022 11:59
@marefr marefr requested a review from a team as a code owner May 31, 2022 11:59
@marefr marefr requested review from papagian, kylebrandt and zserge and removed request for a team May 31, 2022 11:59
@grafanabot
Copy link
Contributor

This pull request was removed from the 9.0.0-beta2 milestone because 9.0.0-beta2 is currently being released.

@grafanabot grafanabot removed this from the 9.0.0-beta2 milestone May 31, 2022
wbrowne
wbrowne approved these changes May 31, 2022
View reviewed changes
Copy link
Member

@wbrowne wbrowne left a comment

Choose a reason for hiding this comment

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

LGTM

@marefr @wbrowne
Apply suggestions from code review
f2faf20 
Co-authored-by: Will Browne <wbrowne@users.noreply.github.com>
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/20416

@marefr marefr added this to the 9.0.0-beta3 milestone May 31, 2022
@marefr
Copy link
Member Author

marefr commented May 31, 2022

Verified it works out of the box with Prometheus so going to merge this. Ping @gabor

@marefr marefr merged commit 1196b4a into main May 31, 2022
@marefr marefr deleted the 44250_fix branch May 31, 2022 15:03
@grafanabot grafanabot mentioned this pull request May 31, 2022
Merged
grafanabot pushed a commit that referenced this pull request May 31, 2022
@marefr @grafanabot
Datasource: Fix allowed cookies to be forwarded as header to backend …
cd46916 
…datasources (#49541)

Co-authored-by: Will Browne <wbrowne@users.noreply.github.com>
(cherry picked from commit 1196b4a)
marefr added a commit that referenced this pull request May 31, 2022
@grafanabot @marefr
Datasource: Fix allowed cookies to be forwarded as header to backend …
e3127d4 
…datasources (#49541) (#49935)

Co-authored-by: Will Browne <wbrowne@users.noreply.github.com>
(cherry picked from commit 1196b4a)

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
This was referenced Jun 1, 2022
Merged
Closed
@pmint93 pmint93 mentioned this pull request Dec 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryantxu ryantxu ryantxu left review comments

@wbrowne wbrowne wbrowne approved these changes

@papagian papagian Awaiting requested review from papagian papagian was automatically assigned from grafana/backend-platform

@kylebrandt kylebrandt Awaiting requested review from kylebrandt kylebrandt was automatically assigned from grafana/backend-platform

@zserge zserge Awaiting requested review from zserge zserge was automatically assigned from grafana/backend-platform

Assignees

@marefr marefr

Labels
add to changelog area/backend backport v9.0.x enterprise-failed type/bug
Projects
Plugins Platform / Grafana Community
Archived in project
Milestone
9.0.0-beta3
Development

Successfully merging this pull request may close these issues.

[grafana 8.3.x][prometheus] Allowed cookies not forwarded to the datasource
4 participants
@marefr @grafanabot @ryantxu @wbrowne