Skip to content

Return bad request when the tenant resolving fail #6715

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

Merged

Conversation

SungJin1212
Copy link
Member

This PR changes to return status code 400(Bad Request) when the tenant resolution fail.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • [NA] Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@justinjung04
Copy link
Contributor

The change looks good to me, but does this mean we lack negative e2e tests? I'm surprised this used to return 200.

@SungJin1212
Copy link
Member Author

SungJin1212 commented Apr 23, 2025

@justinjung04
I have noticed the current query frontend returns 200 when we input an invalid tenant ID with no message.

Also, if we use the SingleResolver, we don't check the tenantID validation. (https://cortexmetrics.io/docs/guides/limitations/#tenant-id-naming)

For example, we can push with the user name ++.
스크린샷 2025-04-23 오전 10 01 15

We can change to validate tenantID to the SingleResolver, but I'm worrying that existing invalid tenantIDs are no longer available..

@yeya24 @friedrichg
Is it intended behavior for the SingleResolver to not validate invalid tenantIDs like +?

Copy link
Contributor

@justinjung04 justinjung04 left a comment

Choose a reason for hiding this comment

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

I've check other places where we call tenant.TenantIDs and everywhere all except frontend transport handler returns the error (or throws http.Error). So I think handling it in handler.go makes sense.

I was self-debating whether this would be considered as a breaking change, but this feels more like a bugfix or enhancement, just adding proper error handlering instead of error swallowing.

Thank you for catching it.

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Good catch!

@yeya24 yeya24 merged commit d1dcdf0 into cortexproject:master Apr 24, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants