.*: Apply webrouteprefix on debug and metric endpoints #4471

wbh1 · 2021-07-22T18:48:06Z

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

This is a continuation of the work started by @ranjithkumar007 in #2524.

I've completed the rebase, fixed prefix handling in Thanos Store, added prefix handling logic to Thanos Compact (the flags were there but ignored, previously), and added additional testing around prefix behaviors in e2e tests.

Related issues:

Web route prefix is not applied on debug endpoints. #2486
/metrics and probe routes does not respect web.route-prefix #1700
- Note: @bwplotka advocated for keeping the prefix off of these endpoints in this issue, but I agree with @FUSAKLA's opinion that it's making a decision for the user.
- I think it's simpler to serve the whole app as specified via the command line flags, and users may block endpoints that they do not want others to access (same as if they don't prefix the routes).
- Prometheus prefixes all endpoints (including /metrics and /debug/*), so this PR makes Thanos consistent with that.
Thanos rule doesn't honor --web.external-prefix #3260

Essentially, it prefixes the debug and metrics endpoints with the value of --web.route-prefix if available.

For those like me who have Thanos components behind a reverse proxy (e.g. https://example.com/thanos/query) then this will break their access to some endpoints (e.g. /metrics, /-/ready and /-/healthy) if they hard coded around it like we did (see below).

Bad:

   # This will break
    location = /thanos/query/metrics {
       proxy_pass http://127.0.0.1:10902/metrics;
    }

Good:

   # This will work
    location /thanos/query/ {
        proxy_pass http://127.0.0.1:10902/thanos/query/;
    }

Verification

Verified with local tests and deployment of a build to our own servers to verify that the endpoints are served as expected both with and without --web.external-prefix (and thereby --web.route-prefix) being set.

Additionally, added more e2e tests to verify this works as expected on the components that support setting a prefix.

wbh1 · 2021-07-22T21:09:55Z

I've rerun the failed e2e test TestCompactWithStoreGateway without issue locally without any changes, so I think that might just be flaky. I've been unable to make TestRule succeed, but I don't see anything that I changed that would've affected it.

ranjithkumar007 · 2021-07-23T05:35:48Z

Thank you @wbh1 for fixing this.

wiardvanrij

It LGTM, but I also have that feeling about "can't we do this smarter?". Still, looks good and thank you!

wiardvanrij · 2021-07-28T11:48:10Z

CHANGELOG.md

@@ -14,6 +14,8 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re

 ### Fixed

+[#4471](https://github.com/thanos-io/thanos/pull/4471) **breaking:** Query/Rule/Compact/Store: Serve `/debug/*`, `/-/ready`, `/-/healthy`, and `/metrics` routes under the prefix from `--web.route-prefix`, if set.


Just to clarify it, let's start with **breaking** if --web.route-prefix is set: serves endpoints x,y,z now actually under the route-prefix. WDYT?

That makes sense to me. I'll update it!

@wiardvanrij - updated ✅ . Thanks!

wbh1 · 2021-07-28T13:17:20Z

It LGTM, but I also have that feeling about "can't we do this smarter?". Still, looks good and thank you!

I agree that there's likely a better way to do it that involves more refactoring -- maybe with including a webConfig for all components? Something to be addressed in a future PR, perhaps 😛

thanos/cmd/thanos/config.go

Lines 156 to 161 in 83419bc

    
           type webConfig struct { 
        
           	routePrefix      string 
        
           	externalPrefix   string 
        
           	prefixHeaderName string 
        
           	disableCORS      bool 
        
           }

wbh1 · 2021-08-04T15:25:24Z

Hey @wiardvanrij - anything else that you need for this?

wiardvanrij · 2021-08-04T19:24:37Z

Hey @wiardvanrij - anything else that you need for this?

No, still LGTM. I will ping others to get this reviewed/merged :) Thanks for picking this up!

wiardvanrij · 2021-08-04T19:28:42Z

It LGTM, but I also have that feeling about "can't we do this smarter?". Still, looks good and thank you!

I agree that there's likely a better way to do it that involves more refactoring -- maybe with including a webConfig for all components? Something to be addressed in a future PR, perhaps 😛

thanos/cmd/thanos/config.go

Lines 156 to 161 in 83419bc

type webConfig struct {

routePrefix string

externalPrefix string

prefixHeaderName string

disableCORS bool

}

I think this makes sense. Would you be interested to write an issue for it and then perhaps implement it? I'm asking for an issue so that it can be (re)viewed before you work a lot on it.

yeya24 · 2021-08-05T06:01:54Z

The code change looks good. But this is really a breaking change and might impact our users. cc @thanos-io/thanos-maintainers for review.

wbh1 · 2021-09-10T17:31:24Z

Resurfacing this discussion @yeya24 😄 Could you ping again?

stale · 2021-11-25T21:09:48Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

wbh1 · 2021-11-25T22:12:13Z

Still relevant. Needs review.

stale · 2022-03-02T10:26:21Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

wbh1 · 2022-03-14T13:15:55Z

Still relevant. Needs review.

GiedriusS · 2022-04-20T12:24:10Z

Will take a look soon, sorry for the delay. Could you please rebase this?

Signed-off-by: Will Hegedus <whegedus@linode.com>

GiedriusS

Thank you for your work! However, historically we've had many "broken" implementations of the prefix functionality. So, could you please add some test cases to TestQueryExternalPrefixWithoutReverseProxy or some similar test that would test out these changes? Namely, we are looking to automatically test whether /metrics and /debug endpoints are successfully accessible with these changes just like we would expect.

This adds additional endpoints that were not previously behind the routePrefix to the testing suite and improves documentation of those tests. Signed-off-by: Will Hegedus <whegedus@linode.com>

This will now return errors if an HTTP error (status >= 400) is returned by the chrome driver. Additionally, the Chrome context is now modified to inherit the deadline from the running test suite. Signed-off-by: Will Hegedus <whegedus@linode.com>

Validate that the --web.route-prefix flag adjusts expected routes. Signed-off-by: Will Hegedus <whegedus@linode.com>

Thanos Compact supports the flags for webconf, but previously was not actually doing anything with them. Signed-off-by: Will Hegedus <whegedus@linode.com>

Refactors Thanos Store to use the registerFlag function to ensure it receives all flags that are applicable to webConfig. Previously, only 3/4 were manually defined. Signed-off-by: Will Hegedus <whegedus@linode.com>

Previously, routes were not getting the value of the --web.*-prefix flags applied. Signed-off-by: Will Hegedus <whegedus@linode.com>

Signed-off-by: Will Hegedus <whegedus@linode.com>

wbh1 · 2022-05-09T14:00:29Z

Alright, @GiedriusS -- this should be ready for review now.

I've added additional e2e testing for the components that actually support the prefix (Compact, Query, Rule, and Store), which helped catch bugs in how Store and Compact were handling route prefixes so thank you for that 😁.

Unrelated to the merging of this PR, but related to the topic of it:

In working on this PR, I've realized how each component largely handles the setting up of its routing configuration which makes changes like this difficult. Which leads me to want to make an effort in the future to consolidate the HTTP server/routing configuration for all components to ensure they're all consistent with the way they serve their routes.

Part of this work is already done in how some components use the webConfig struct, and also the prometheus/common/route package (example in Store). This leads to a lot of repeated code (see the similarities in Rule and Query).

Is this an idea that should go through the proposal process? Looking for next steps before starting any effort towards it.

stale · 2022-07-10T11:06:32Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

wbh1 · 2022-07-11T16:42:22Z

Not stale. Just need to get around to rebasing it again.

GiedriusS · 2022-07-11T17:03:11Z

Sorry for the delay, I was on vacation. Will take a look in a bit & respond to you

stale · 2022-09-21T00:13:15Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

wbh1 force-pushed the webroute branch 2 times, most recently from e1990ff to 41f9805 Compare July 22, 2021 20:17

wiardvanrij previously approved these changes Jul 28, 2021

View reviewed changes

wbh1 dismissed wiardvanrij’s stale review via 0d799c8 July 28, 2021 13:21

wbh1 force-pushed the webroute branch from 0d799c8 to d5a48f4 Compare July 28, 2021 13:22

wbh1 requested a review from wiardvanrij July 28, 2021 13:44

wbh1 force-pushed the webroute branch 2 times, most recently from 2a4788b to a2fed5b Compare July 29, 2021 17:00

stale bot added the stale label Nov 25, 2021

stale bot removed the stale label Nov 25, 2021

stale bot added the stale label Mar 2, 2022

stale bot removed the stale label Mar 14, 2022

wbh1 force-pushed the webroute branch from a2fed5b to 64c2315 Compare April 20, 2022 18:09

pull-request-size bot added the size/M label Apr 20, 2022

wbh1 force-pushed the webroute branch from 5f9ab0b to 896453f Compare April 20, 2022 19:06

Apply webrouteprefix on all endpoints

79c0d95

Signed-off-by: Will Hegedus <whegedus@linode.com>

wbh1 force-pushed the webroute branch from 896453f to ac5d09a Compare April 20, 2022 19:12

wbh1 force-pushed the webroute branch from ac5d09a to 9f57ca0 Compare April 20, 2022 19:30

wbh1 added 2 commits April 20, 2022 15:34

add CHANGELOG entry

4b0ce7b

Signed-off-by: Will Hegedus <whegedus@linode.com>

fix tests to account for URL prefix

bdd8da6

Signed-off-by: Will Hegedus <whegedus@linode.com>

wbh1 force-pushed the webroute branch from 9f57ca0 to bdd8da6 Compare April 20, 2022 19:35

GiedriusS reviewed May 4, 2022

View reviewed changes

pull-request-size bot added size/L and removed size/M labels May 8, 2022

wbh1 added 6 commits May 8, 2022 14:04

tests: update query tests for route prefixes

9be439f

This adds additional endpoints that were not previously behind the routePrefix to the testing suite and improves documentation of those tests. Signed-off-by: Will Hegedus <whegedus@linode.com>

tests: add tests for routePrefix on Thanos Ruler

b81ea62

Validate that the --web.route-prefix flag adjusts expected routes. Signed-off-by: Will Hegedus <whegedus@linode.com>

fix: Prefix compact's routes with routePrefix

92122c6

Thanos Compact supports the flags for webconf, but previously was not actually doing anything with them. Signed-off-by: Will Hegedus <whegedus@linode.com>

feat: support all webConfig flags in Thanos Store

e3e04fd

Refactors Thanos Store to use the registerFlag function to ensure it receives all flags that are applicable to webConfig. Previously, only 3/4 were manually defined. Signed-off-by: Will Hegedus <whegedus@linode.com>

fix: correctly prefix routes in Thanos Store

fcfcc9a

Previously, routes were not getting the value of the --web.*-prefix flags applied. Signed-off-by: Will Hegedus <whegedus@linode.com>

wbh1 force-pushed the webroute branch 2 times, most recently from e03c279 to e9e8bf0 Compare May 8, 2022 21:24

wbh1 added 3 commits May 9, 2022 09:20

style: fix go-lint errors in e2e tests

fc9c46a

Signed-off-by: Will Hegedus <whegedus@linode.com>

docs: update flags for Thanos Store

47c85de

Signed-off-by: Will Hegedus <whegedus@linode.com>

docs: clarify changelog

f18fa62

Signed-off-by: Will Hegedus <whegedus@linode.com>

wbh1 force-pushed the webroute branch from e9e8bf0 to f18fa62 Compare May 9, 2022 13:21

stale bot added the stale label Jul 10, 2022

stale bot removed the stale label Jul 11, 2022

stale bot added the stale label Sep 21, 2022

stale bot closed this Oct 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.*: Apply webrouteprefix on debug and metric endpoints #4471

.*: Apply webrouteprefix on debug and metric endpoints #4471

wbh1 commented Jul 22, 2021 •

edited

Loading

wbh1 commented Jul 22, 2021

ranjithkumar007 commented Jul 23, 2021

wiardvanrij left a comment •

edited

Loading

wiardvanrij Jul 28, 2021 •

edited

Loading

wbh1 Jul 28, 2021

wbh1 Jul 28, 2021 •

edited

Loading

wbh1 commented Jul 28, 2021

wbh1 commented Aug 4, 2021

wiardvanrij commented Aug 4, 2021

wiardvanrij commented Aug 4, 2021

yeya24 commented Aug 5, 2021

wbh1 commented Sep 10, 2021

stale bot commented Nov 25, 2021

wbh1 commented Nov 25, 2021

stale bot commented Mar 2, 2022

wbh1 commented Mar 14, 2022

GiedriusS commented Apr 20, 2022

GiedriusS left a comment

wbh1 commented May 9, 2022

stale bot commented Jul 10, 2022

wbh1 commented Jul 11, 2022

GiedriusS commented Jul 11, 2022

stale bot commented Sep 21, 2022

		@@ -14,6 +14,8 @@ We use breaking :warning: to mark changes that are not backward compatible (re

		### Fixed

		[#4471](https://github.com/thanos-io/thanos/pull/4471) breaking: Query/Rule/Compact/Store: Serve `/debug/*`, `/-/ready`, `/-/healthy`, and `/metrics` routes under the prefix from `--web.route-prefix`, if set.

.*: Apply webrouteprefix on debug and metric endpoints #4471

.*: Apply webrouteprefix on debug and metric endpoints #4471

Conversation

wbh1 commented Jul 22, 2021 • edited Loading

Changes

Verification

wbh1 commented Jul 22, 2021

ranjithkumar007 commented Jul 23, 2021

wiardvanrij left a comment • edited Loading

Choose a reason for hiding this comment

wiardvanrij Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

wbh1 Jul 28, 2021

Choose a reason for hiding this comment

wbh1 Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

wbh1 commented Jul 28, 2021

wbh1 commented Aug 4, 2021

wiardvanrij commented Aug 4, 2021

wiardvanrij commented Aug 4, 2021

yeya24 commented Aug 5, 2021

wbh1 commented Sep 10, 2021

stale bot commented Nov 25, 2021

wbh1 commented Nov 25, 2021

stale bot commented Mar 2, 2022

wbh1 commented Mar 14, 2022

GiedriusS commented Apr 20, 2022

GiedriusS left a comment

Choose a reason for hiding this comment

wbh1 commented May 9, 2022

stale bot commented Jul 10, 2022

wbh1 commented Jul 11, 2022

GiedriusS commented Jul 11, 2022

stale bot commented Sep 21, 2022

wbh1 commented Jul 22, 2021 •

edited

Loading

wiardvanrij left a comment •

edited

Loading

wiardvanrij Jul 28, 2021 •

edited

Loading

wbh1 Jul 28, 2021 •

edited

Loading