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

Plugins: Preserve trailing slash in plugin proxy #86859

Merged
merged 4 commits into from
Jun 5, 2024
Merged

Plugins: Preserve trailing slash in plugin proxy #86859

merged 4 commits into from
Jun 5, 2024

Conversation

marefr
Copy link
Member

@marefr marefr commented Apr 24, 2024

Fix to allow preserving trailing slash in plugin proxy if the incoming URL contains a trailing slash. Behind feature toggle since unsure if this could cause issue for existing app plugins.

Fixes #86851

@marefr marefr self-assigned this Apr 24, 2024
@marefr marefr requested review from grafanabot and a team as code owners April 24, 2024 13:48
@marefr marefr requested review from wbrowne, andresmgot and xnyo and removed request for a team April 24, 2024 13:48
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone Apr 24, 2024
{
Name: "pluginProxyPreserveTrailingSlash",
Description: "Preserve plugin proxy trailing slash.",
Stage: FeatureStageExperimental,
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of an experimental feature that needs to be turned on, could we consider this a bug fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, because otherwise we can never turn it on. Since we can never be certain which plugins it may affect.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can add Expression: "true" to enable it by default. In case of error, it's easier to change a flag than rolling back or pinning instances so it's safer this way.

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 enabled by default now

@abannachGrafana
Copy link
Contributor

Since this is a bug fix, can we backport it into the previous version? That would allow us to leverage the fix in cloud without having to wait a month for the slow channel to roll out.

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, maybe enabling the flag by default.

{
Name: "pluginProxyPreserveTrailingSlash",
Description: "Preserve plugin proxy trailing slash.",
Stage: FeatureStageExperimental,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add Expression: "true" to enable it by default. In case of error, it's easier to change a flag than rolling back or pinning instances so it's safer this way.

@andresmgot
Copy link
Contributor

Since this is a bug fix, can we backport it into the previous version? That would allow us to leverage the fix in cloud without having to wait a month for the slow channel to roll out.

Cloud does not work with versions. Even if backported you would still need to wait a month for the fix to reach the slow channel. I don't think we need to backport this but up to you.

@abannachGrafana
Copy link
Contributor

Since this is a bug fix, can we backport it into the previous version? That would allow us to leverage the fix in cloud without having to wait a month for the slow channel to roll out.

Cloud does not work with versions. Even if backported you would still need to wait a month for the fix to reach the slow channel. I don't think we need to backport this but up to you.

Interesting, How do we get urgent fixes out to cloud then?

@andresmgot
Copy link
Contributor

Interesting, How do we get urgent fixes out to cloud then?

I believe there is a process to request slow to be moved forward if necessary (or moving affected instances to a different channel). You can reach folks in #grafana-backend-services for more info.

Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions!

@github-actions github-actions bot added the stale Issue with no recent activity label May 26, 2024
@andresmgot
Copy link
Contributor

ping @marefr if you want to update/merge this.

@andresmgot andresmgot removed the stale Issue with no recent activity label May 27, 2024
@marefr marefr requested a review from andresmgot June 4, 2024 18:48
@marefr
Copy link
Member Author

marefr commented Jun 4, 2024

@andresmgot made it default true. Wasn't sure about the stage, but went with FeatureStageGeneralAvailability 🤷

@marefr marefr merged commit fe3e591 into main Jun 5, 2024
19 checks passed
@marefr marefr deleted the fix_86851 branch June 5, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🚀 Shipped
Development

Successfully merging this pull request may close these issues.

Plugin Proxy Routing: routes don't preserve trailing slash
4 participants