Apply webrouteprefix on debug endpoints #2524

ranjithkumar007 · 2020-04-25T11:20:38Z

Fixes #2486
Signed-off-by: ranjithkumar007 ranjith.dakshana2015@gmail.com

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

Verification

Verified locally by checking the URLs with and without webrouteprefix.

ranjithkumar007 · 2020-04-25T11:24:00Z

Also, there seems some issue with js/CSS not being loaded properly when webrouteprefix is provided. Attached screenshots below

Without any webrouteprefix:

With webrouteprefix

Should I open an issue for this?

yeya24 · 2020-04-25T14:44:02Z

The current solution for this is when web route prefix is provided, then web external prefix should also be provided, to make the static resources in the valid path.

yeya24 · 2020-04-25T14:45:28Z

cmd/thanos/bucket.go

@@ -335,7 +335,8 @@ func registerBucketWeb(m map[string]setupFunc, root *kingpin.CmdClause, name str
 			prober.NewInstrumentation(comp, logger, extprom.WrapRegistererWithPrefix("thanos_", reg)),
 		)

-		srv := httpserver.New(logger, reg, comp, httpProbe,
+		srv := httpserver.New(logger, reg, comp, "/",


Do we want webRoutePrefix for all components, instead of just using / here? cc @GiedriusS @bwplotka

I am not sure if it is required for all components.
@bwplotka any inputs here?

ranjithkumar007 · 2020-04-25T14:48:53Z

The current solution for this is when web route prefix is provided, then web external prefix should also be provided, to make the static resources in the valid path.

Got it. So, I think the above screenshots shouldn't be an issue.

bwplotka

Thanks, looks good, but I think we can simplify by using path.Join to concatenate the URL paths.

bwplotka · 2020-05-18T09:28:31Z

cmd/thanos/compact.go

@@ -247,7 +247,8 @@ func runCompact(
 		prober.NewInstrumentation(component, logger, extprom.WrapRegistererWithPrefix("thanos_", reg)),
 	)

-	srv := httpserver.New(logger, reg, component, httpProbe,
+	srv := httpserver.New(logger, reg, component, "/",


Why not we route as well?

I checked existing compacter flags in which there was no web.route-prefix. I was not sure whether I should add web.route-prefix flag to all components.

Should I add web.route-prefix to all components?

May be we can do it in a separate PR.

I think it would be nice to have --web.route-prefix in other components as well, but let's leave it for a separate PR.

pkg/server/http/http.go

ranjithkumar007 · 2020-06-27T13:00:59Z

failing e2e unrelated to PR

squat · 2020-06-27T13:07:41Z

Rerunning :)

yeya24 · 2020-06-27T13:49:06Z

@ranjithkumar007 E2E still failing.

Would you mind fixing this e2e test?

##[error]    TestQueryRoutePrefix: query_test.go:184: query_test.go:184:
        
         unexpected error: the service querier-1 is not ready; err: got no expected status code: 404, expected: 200

I think the problem is here https://github.com/thanos-io/thanos/blob/master/test/e2e/e2ethanos/services.go#L170, you should add the web route prefix to readiness probe path as well.

ranjithkumar007 · 2020-06-27T13:52:16Z

@ranjithkumar007 E2E still failing.

Would you mind fixing this e2e test?
##[error]    TestQueryRoutePrefix: query_test.go:184: query_test.go:184:
        
         unexpected error: the service querier-1 is not ready; err: got no expected status code: 404, expected: 200
      
I think the problem is here https://github.com/thanos-io/thanos/blob/master/test/e2e/e2ethanos/services.go#L170, you should add the web route prefix to readiness probe path as well.

Thanks @yeya24 .
Got it.. I will fix that.

yeya24

Great! Looks good and CI passed.
@kakkoyun Would you mind giving some 👀 on this pr? I am not sure if this pr affects something in the kube-thanos repo.

Signed-off-by: ranjithkumar007 <ranjith.dakshana2015@gmail.com>

Signed-off-by: Ranjith Kumar <ranjith.dakshana2015@gmail.com>

kakkoyun

These changes look good regarding kube-thanos. We just need to change a mixin we have in queried specs https://github.com/thanos-io/kube-thanos/blob/3b7d80d643dec546f1bfa8dc18b38460d0d57f53/jsonnet/kube-thanos/kube-thanos-query.libsonnet#L142-L164
We can do this after these changes are merged.

Signed-off-by: Ranjith Kumar <ranjith.dakshana2015@gmail.com>

onprem

Looks great. LGTM 🎉

ranjithkumar007 · 2020-07-16T08:58:19Z

Anything left here?

squat · 2020-08-31T18:12:41Z

@ranjithkumar007 sorry we let this get so stale. Can you please rebase?

CC @prmsrswt for review considering web route prefix changes

onprem · 2020-08-31T21:53:15Z

There are some changes in route and external prefix handling in master branch which are not present in this PR so I'll test again after a rebase, but I think these changes are good to go 💯.

yeya24 · 2020-09-01T13:18:39Z

@ranjithkumar007 Hello. Sorry for the delay, but this pr is pretty close now. Can you please rebase master and fix the conflict? Thank you!

onprem · 2020-09-02T12:31:42Z

The new component Query Frontend also uses the httpserver.New function which was modified in this PR. @ranjithkumar007 can you update the code to use web.route-prefix there as well? Thanks.

yeya24 · 2020-09-02T13:03:26Z

@prmsrswt Thanks for this! It is fine for Query frontend now. I need to investigate whether route prefix breaks anything in Query frontend first. If it doesn't bring any risk, then I will add it in another pr.

@ranjithkumar007 Can you please fix the CI failures?

onprem · 2020-09-02T13:13:09Z

The CI failures are because Query Frontend now has to use the modified HTTP server, which expects web route prefix as an argument. As @yeya24 said, we can pass an empty string for now in the case of Query Frontend and then investigate it further if it makes sense.

yeya24 · 2020-09-02T13:18:37Z

The CI failures are because Query Frontend now has to use the modified HTTP server, which expects web route prefix as an argument. As @yeya24 said, we can pass an empty string for now in the case of Query Frontend and then investigate it further if it makes sense.

Oh yes, I missed that the function signature is changed 👍

kakkoyun · 2020-09-08T08:26:45Z

@ranjithkumar007 Friendly ping. Any update on this?

kakkoyun · 2020-10-31T10:00:41Z

@ranjithkumar007 Any update? We can take this over if you're busy.

@yeya24 @prmsrswt What do you think? Shall we take this one over?

ranjithkumar007 · 2020-11-07T05:52:13Z

@ranjithkumar007 Any update? We can take this over if you're busy.

Sorry, I wasn't getting time outside of work. Please feel free to take over.

stale · 2021-01-06T10:10:18Z

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.

ranjithkumar007 mentioned this pull request Apr 25, 2020

rule: /-/reload is inaccessible on v0.12.1 when no prefix has been specified #2514

Closed

yeya24 reviewed Apr 25, 2020

View reviewed changes

ranjithkumar007 force-pushed the webroute branch from 516b385 to e5b6d9b Compare May 2, 2020 05:38

bwplotka reviewed May 18, 2020

View reviewed changes

yeya24 mentioned this pull request May 25, 2020

Web route prefix is not applied on debug endpoints. #2486

Closed

ranjithkumar007 force-pushed the webroute branch from e5b6d9b to b772db1 Compare June 25, 2020 08:26

ranjithkumar007 force-pushed the webroute branch from beef968 to 5eab2d7 Compare June 27, 2020 14:51

yeya24 approved these changes Jun 27, 2020

View reviewed changes

ranjithkumar007 added 4 commits June 28, 2020 10:23

apply webrouteprefix on debug endpoints

c85e927

Signed-off-by: ranjithkumar007 <ranjith.dakshana2015@gmail.com>

fix ci

f149e3c

Signed-off-by: Ranjith Kumar <ranjith.dakshana2015@gmail.com>

fix routeprefix in readinessprobe

37609c7

Signed-off-by: Ranjith Kumar <ranjith.dakshana2015@gmail.com>

update changelog

4127475

Signed-off-by: Ranjith Kumar <ranjith.dakshana2015@gmail.com>

ranjithkumar007 force-pushed the webroute branch from cfd00b3 to 4127475 Compare June 28, 2020 04:54

kakkoyun reviewed Jun 30, 2020

View reviewed changes

fix merge conflicts

b872d9c

Signed-off-by: Ranjith Kumar <ranjith.dakshana2015@gmail.com>

onprem approved these changes Jul 2, 2020

View reviewed changes

Merge branch 'master' into webroute

8049f14

stale bot added the stale label Jan 6, 2021

stale bot closed this Jan 13, 2021

wbh1 mentioned this pull request Jul 22, 2021

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

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply webrouteprefix on debug endpoints #2524

Apply webrouteprefix on debug endpoints #2524

ranjithkumar007 commented Apr 25, 2020 •

edited

Loading

ranjithkumar007 commented Apr 25, 2020 •

edited

Loading

yeya24 commented Apr 25, 2020

yeya24 Apr 25, 2020

ranjithkumar007 May 2, 2020

ranjithkumar007 commented Apr 25, 2020 •

edited

Loading

bwplotka left a comment

bwplotka May 18, 2020

ranjithkumar007 May 18, 2020 •

edited

Loading

ranjithkumar007 May 18, 2020

ranjithkumar007 Jun 25, 2020

ranjithkumar007 Jun 27, 2020

onprem Jul 2, 2020

ranjithkumar007 commented Jun 27, 2020

squat commented Jun 27, 2020

yeya24 commented Jun 27, 2020

ranjithkumar007 commented Jun 27, 2020

yeya24 left a comment

kakkoyun left a comment

onprem left a comment

ranjithkumar007 commented Jul 16, 2020

squat commented Aug 31, 2020

onprem commented Aug 31, 2020

yeya24 commented Sep 1, 2020

onprem commented Sep 2, 2020

yeya24 commented Sep 2, 2020 •

edited

Loading

onprem commented Sep 2, 2020

yeya24 commented Sep 2, 2020

kakkoyun commented Sep 8, 2020

kakkoyun commented Oct 31, 2020

ranjithkumar007 commented Nov 7, 2020

stale bot commented Jan 6, 2021

Apply webrouteprefix on debug endpoints #2524

Apply webrouteprefix on debug endpoints #2524

Conversation

ranjithkumar007 commented Apr 25, 2020 • edited Loading

Verification

ranjithkumar007 commented Apr 25, 2020 • edited Loading

yeya24 commented Apr 25, 2020

yeya24 Apr 25, 2020

Choose a reason for hiding this comment

ranjithkumar007 May 2, 2020

Choose a reason for hiding this comment

ranjithkumar007 commented Apr 25, 2020 • edited Loading

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka May 18, 2020

Choose a reason for hiding this comment

ranjithkumar007 May 18, 2020 • edited Loading

Choose a reason for hiding this comment

ranjithkumar007 May 18, 2020

Choose a reason for hiding this comment

ranjithkumar007 Jun 25, 2020

Choose a reason for hiding this comment

ranjithkumar007 Jun 27, 2020

Choose a reason for hiding this comment

onprem Jul 2, 2020

Choose a reason for hiding this comment

ranjithkumar007 commented Jun 27, 2020

squat commented Jun 27, 2020

yeya24 commented Jun 27, 2020

ranjithkumar007 commented Jun 27, 2020

yeya24 left a comment

Choose a reason for hiding this comment

kakkoyun left a comment

Choose a reason for hiding this comment

onprem left a comment

Choose a reason for hiding this comment

ranjithkumar007 commented Jul 16, 2020

squat commented Aug 31, 2020

onprem commented Aug 31, 2020

yeya24 commented Sep 1, 2020

onprem commented Sep 2, 2020

yeya24 commented Sep 2, 2020 • edited Loading

onprem commented Sep 2, 2020

yeya24 commented Sep 2, 2020

kakkoyun commented Sep 8, 2020

kakkoyun commented Oct 31, 2020

ranjithkumar007 commented Nov 7, 2020

stale bot commented Jan 6, 2021

ranjithkumar007 commented Apr 25, 2020 •

edited

Loading

ranjithkumar007 commented Apr 25, 2020 •

edited

Loading

ranjithkumar007 commented Apr 25, 2020 •

edited

Loading

ranjithkumar007 May 18, 2020 •

edited

Loading

yeya24 commented Sep 2, 2020 •

edited

Loading