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

fix: serve everything under path prefix when set #3186

Merged
merged 4 commits into from
Jun 2, 2023

Conversation

maxbrunet
Copy link
Member

Serve everything under path prefix when --path-prefix is set, including /metrics and /debug/pprof, and nothing directly from /

@maxbrunet maxbrunet requested a review from a team as a code owner May 27, 2023 20:21
Copy link
Contributor

@thorfour thorfour left a comment

Choose a reason for hiding this comment

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

LGTM

@brancz
Copy link
Member

brancz commented May 27, 2023

I feel like this was this way for a reason before. I think the way it was intended to be used is to have a rewrite rule by any proxy in front of parca.

Comment on lines -130 to -133
if pathPrefix != "" {
internalMux.Mount(pathPrefix+"/api", grpcWebMux)
}
internalMux.Mount("/api", grpcWebMux)
Copy link
Member Author

Choose a reason for hiding this comment

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

We could use a rewrite rule, but what's the purpose of this then? It feels like the other paths were overlooked. Otherwise, I'd remove this if-block and we do not change anything in the router when pathPrefix is set. WDYT?

Copy link
Member

@metalmatze metalmatze left a comment

Choose a reason for hiding this comment

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

I guess the idea was that because the API is part of our public API the prefix would be adjusted, whereas the internal APIs would stay the same. It would be more clear if those routers where served on different ports, but we have it on the same one.

Given we have it on the same port and use the same router in the end, I can see how being consistent with the prefix makes sense. 👍

pkg/server/server.go Outdated Show resolved Hide resolved
@brancz
Copy link
Member

brancz commented Jun 2, 2023

Ah ok yes I think in the past we played around with separate ports. Lgtm then

@metalmatze metalmatze enabled auto-merge (squash) June 2, 2023 14:41
@metalmatze metalmatze merged commit cf0105a into parca-dev:main Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants