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.

Sign up for GitHub

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

Jump to bottom

Query UI: Add tenant box #6867

Merged
merged 7 commits into from Feb 27, 2024
Merged

Query UI: Add tenant box #6867

merged 7 commits into from Feb 27, 2024

Conversation

jacobbaungard
Copy link
Contributor

@jacobbaungard jacobbaungard commented Nov 1, 2023 •
edited

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

Changes

  • Tenant input box added to the Query UI, in order to be able to specify which tenant the query should use. This is especially useful for setups were tenancy enforcement is enabled (Query: add optional tenancy enforcement #6756).

tenant-ui

Verification

  • Manually verified that:
    • No tenant header is sent, if textbox is left empty
    • tenant HTTP header is sent if text is not empty
    • The tenant header name changes with the query.tenant-header option
    • The tenant input is added to the URL query params so one can link to the identical query

Todo

@yeya24
Copy link
Contributor

yeya24 commented Nov 2, 2023

Overall I think the UI is neat. But I do have one question. I assume this feature is mainly used for testing purpose. Is it correct?
For production, having users entering arbitrary tenant value seems not safe.

yeya24
yeya24 reviewed Nov 2, 2023
View reviewed changes
pkg/ui/react-app/src/pages/graph/Panel.tsx
bsSize="sm"
onChange={this.handleChangeTenant}
placeholder={`${defaultTenant}`}
value={options.tenant}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need any validation for the tenant value? Or we want to rely on Thanos backend to do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's currently just the following validation in the backend:

thanos/pkg/tenancy/tenancy.go

Line 38 in ec6b8b6

func IsTenantValid(tenant string) error {
that seems fine to just do in the backend imo.

I'm not sure if it makes sense to do any more format validation of the tenant? For receive this is also the only thing we have in place.

@jacobbaungard
Copy link
Contributor Author

Overall I think the UI is neat. But I do have one question. I assume this feature is mainly used for testing purpose. Is it correct?
For production, having users entering arbitrary tenant value seems not safe.

In general for the tenancy features, Thanos doesn't do any authentication. If someone has access to the query API, there is nothing stopping anyone from setting an arbitrary tenant with the HTTP headers. It's assumed that you will put something in front of the query API to authenticate users and set the correct tenant.

For the UI specifically, at least as implemented here, I think it should not be exposed directly to end-users if tenancy enforcement is in place. If needed in the future, I think it could be possible to implement this in a way which would allow someone to setup a proxy in front of the UI that first authenticate users, and then set the tenant before reaching the UI, but I am not sure if anyone has that usecase.

@jacobbaungard
Copy link
Contributor Author

Rebased on latest main, and also added changelog. Currently the auto-completetion will always use the default tenant, but we need an improvement in the Prometheus library to implement that. I would suggest we can merge this meanwhile (or at latest shortly after #6756 is merged)

@jacobbaungard jacobbaungard marked this pull request as ready for review November 10, 2023 10:47
@jacobbaungard
Query UI: Add tenant box
2562f1b 
With this commit as tenant box is added to the query UI. It can be used
to specify which tenant to use when making a query.

Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>
@jacobbaungard
Re-compile static react app
dc536e4 
Recompiles the static react app as now needed following:
thanos-io#6900

Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>
@jacobbaungard
Copy link
Contributor Author

Rebased on main and re-generated the react ui as needed following: #6900

douglascamata
douglascamata approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@douglascamata douglascamata left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@jacobbaungard
Merge branch 'upstream/main' into tenant-ui-box
d0e669e 
Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>
@jacobbaungard
Merge branch 'upstream/main' into tenant-ui-box
1925e72 
Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>
saswatamcode
saswatamcode previously approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@saswatamcode saswatamcode left a comment •
edited

Choose a reason for hiding this comment

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

LGTM! Let's merge this. Let's make this textbox optional in a follow-up PR, which is tied to enforcement options

@jacobbaungard
Merge 'upstream/main' into tenant-ui-box
11ed083
@jacobbaungard
Move changelog item to appropiate future release
1e81a8d 
After merging it was under the 0.34 release.

Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>
@jacobbaungard
Move query path tenancy proposal to done
f21e51b 
Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>
@saswatamcode saswatamcode enabled auto-merge (squash) February 27, 2024 15:20
@saswatamcode saswatamcode merged commit 360d24d into thanos-io:main Feb 27, 2024
20 checks passed
@jacobbaungard jacobbaungard mentioned this pull request Mar 6, 2024
Merged
2 tasks
jnyi pushed a commit to jnyi/thanos that referenced this pull request Apr 4, 2024
@jacobbaungard @jnyi
Query UI: Add tenant box (thanos-io#6867)
a8ece96 
* Query UI: Add tenant box

With this commit as tenant box is added to the query UI. It can be used
to specify which tenant to use when making a query.

Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>

* Re-compile static react app

Recompiles the static react app as now needed following:
thanos-io#6900

Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>

* Move changelog item to appropiate future release

After merging it was under the 0.34 release.

Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>

* Move query path tenancy proposal to done

Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>

---------

Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yeya24 yeya24 yeya24 left review comments

@douglascamata douglascamata douglascamata approved these changes

@saswatamcode saswatamcode saswatamcode approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jacobbaungard @yeya24 @douglascamata @saswatamcode