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

query: Fix external prefix and add an e2e test for it #2800

Merged
merged 12 commits into from
Jul 2, 2020

Conversation

onprem
Copy link
Member

@onprem onprem commented Jun 24, 2020

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

Changes

  • Test --web.external-prefix using a simple reverse proxy
  • Test both --web.external-prefix and --web.route-prefix in a single scenario, i.e. when both are set simultaneously.

Changes in externalPrefix and routePrefix handling

  • Now, --web.route-prefix defaults to --web.external-prefix. This behavior allows a user to use thanos with and without a reverse proxy if --web.external-prefix is set, and it is in line with what Prometheus does.
  • Updated both, the old UI and the new React UI to account for --web.external-prefix

Turns out, --web.external-prefix is still broken, and as expected, these tests would fail unless we fix it.
It should work now.

This PR changes how --web.external-prefix works, so it can make the UI inaccessible to some users who were relying on some workarounds or hacks before (like setting --web.external-prefix=".").

Verification

Ran GOTEST_OPTS="-run TestQuery" make test-e2e locally to test all e2e tests related to Query component.

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
@onprem onprem marked this pull request as ready for review June 27, 2020 19:58
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
@onprem onprem changed the title query: e2e: Test external prefix query: Fix external prefix and add an e2e test for it Jun 27, 2020
@onprem
Copy link
Member Author

onprem commented Jun 27, 2020

Now that we are more in line with what Prometheus does, the following guides also works for Thanos.

The only difference is that, instead of using the Prometheus flag --web.external-url one needs to use --web.external-prefix flag in Thanos. So prometheus --web.external-url http://localhost:19090/prometheus/ becomes thanos query --web.external-prefix=thanos for Thanos.

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Awesome work! Just a few comments on how to improve this 🎉

test/e2e/query_test.go Show resolved Hide resolved
test/e2e/query_test.go Show resolved Hide resolved
test/e2e/e2ethanos/helpers.go Outdated Show resolved Hide resolved
cmd/thanos/query.go Outdated Show resolved Hide resolved
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
@onprem
Copy link
Member Author

onprem commented Jun 30, 2020

Seems like Netlify is having some problems.

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

The code looks good! I think we should another iteration on the documentation and then we can merge this :P Mostly grammar nits + renamed Thanos querier to Thanos Query to be consistent.

docs/operating/reverse-proxy.md Outdated Show resolved Hide resolved
docs/operating/reverse-proxy.md Outdated Show resolved Hide resolved
docs/operating/reverse-proxy.md Outdated Show resolved Hide resolved
docs/operating/reverse-proxy.md Outdated Show resolved Hide resolved
docs/operating/reverse-proxy.md Outdated Show resolved Hide resolved
docs/operating/reverse-proxy.md Outdated Show resolved Hide resolved
docs/operating/reverse-proxy.md Outdated Show resolved Hide resolved
docs/operating/reverse-proxy.md Outdated Show resolved Hide resolved
docs/operating/reverse-proxy.md Outdated Show resolved Hide resolved
docs/operating/reverse-proxy.md Outdated Show resolved Hide resolved
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

LGTM. Even tested with a reverse proxy with the example configuration in the documentation and it works with these changes whereas with the master version you get redirected to http://localhost/thanos/thanos/graph with identical configuration.
I'll give @squat and others time to review these changes if they want to before merging this. Awesome stuff!

docs/operating/reverse-proxy.md Outdated Show resolved Hide resolved
cmd/thanos/query.go Outdated Show resolved Hide resolved
test/e2e/e2ethanos/helpers.go Outdated Show resolved Hide resolved
Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

Looks great overall, Prem! Great work and thanks for the docs! Just some small nits

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

Great 🎉

@GiedriusS GiedriusS merged commit c121d58 into thanos-io:master Jul 2, 2020
@onprem onprem deleted the test-external-prefix branch July 2, 2020 09:43
@GiedriusS
Copy link
Member

Oops, fixing up the CHANGELOG in #2833.

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.

3 participants