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

httpclient: Don't forward HTTP headers by default #679

Merged
merged 2 commits into from
May 4, 2023

Conversation

marefr
Copy link
Member

@marefr marefr commented May 4, 2023

Based on discussion in #612 enable automatic forwarding of HTTP headers did introduce problems for several plugins. This disables automatic forwarding of HTTP headers and requires httpclient.Options.ForwardHTTPHeaders to be configued to true for HTTP headers to be forwarded. This doesn't affect core Grafana plugins since we already override the default middlewares there.

Consider the alternative of removing adding the httpclient.ContextualMiddleware to httpclient.DefaultMiddlewares. Then you would had to do something like this

opt := httpclient.Options{
  Middlewares: []httpclient.Middleware{...httpclient.DefaultMiddlewares, httpclient.ContextualMiddleware},
}

Feels like there might be other use cases for httpclient.ContextualMiddleware in the future why having it as default make sense and why I opted for these changes instead.

Would have to update the following if we merge:

@marefr marefr requested a review from a team as a code owner May 4, 2023 09:53
@marefr marefr requested review from wbrowne and xnyo and removed request for a team May 4, 2023 09:53
@marefr marefr self-assigned this May 4, 2023
// included in backend.QueryDataRequest, backend.CallResourceRequest,
// backend.CheckHealthRequest, e.g. based on if Allowed cookies or
// Forward OAuth Identity is configured.
ForwardHTTPHeaders bool
Copy link
Member Author

Choose a reason for hiding this comment

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

Hard time explaining what this does and/or why/when you would use it 🤷

Copy link
Member

@xnyo xnyo left a comment

Choose a reason for hiding this comment

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

LGTM great work! ✨

Once this is merged I will take care of updating the plugin example since I plan to do some work over there soon already.

I agree this needs to be documented properly as a breaking change as some plugins may rely on this feature even if it was introduced relatively recently.

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 marefr merged commit 3e2ff58 into main May 4, 2023
2 checks passed
@marefr marefr deleted the disable_default_forwarding_headers branch May 4, 2023 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants