-
Notifications
You must be signed in to change notification settings - Fork 3.6k
feat: Add endpoint returning tenant configured limits #17101
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
base: main
Are you sure you want to change the base?
Conversation
pkg/loki/loki.go
Outdated
@@ -505,6 +503,7 @@ func (t *Loki) bindConfigEndpoint(opts RunOpts) { | |||
configEndpointHandlerFn = opts.CustomConfigEndpointHandlerFn | |||
} | |||
t.Server.HTTP.Path("/config").Methods("GET").HandlerFunc(configEndpointHandlerFn) | |||
t.Server.HTTP.Path("/config/tenant/{tenant}/limits").Methods("GET").HandlerFunc(t.tenantLimitsHandler()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we have a root prefix for paths like this? given that this is now going to be something relied upon by other services I think the path should have a version in it, if that's not already provided by an additional prefix somewhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't add any prefix to that route. My concern with adding it under /loki/api/v1
is that I think we risk to somehow accidentally exposte the endpoint and make it public. I see this more like an operator endpoint similar to the /config
or /ring
ones.
I agree adding versioning here makes sense, what about /config/tenant/v1/{tenant}/limits
?
pkg/loki/loki.go
Outdated
@@ -505,6 +503,7 @@ func (t *Loki) bindConfigEndpoint(opts RunOpts) { | |||
configEndpointHandlerFn = opts.CustomConfigEndpointHandlerFn | |||
} | |||
t.Server.HTTP.Path("/config").Methods("GET").HandlerFunc(configEndpointHandlerFn) | |||
t.Server.HTTP.Path("/config/tenant/{tenant}/limits").Methods("GET").HandlerFunc(t.tenantLimitsHandler()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also wary about how this changes our multi-tenant paradigm, might it be better to not have a tenant path param and instead use headers the way we do multi-tenant in the rest of our API's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, good idea 👍 I will get the tenant from the header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few questions with this approach, I think we should have a discussion before merging this
Pushed your suggestions Ed, this is how the endpoint looks like now
|
What this PR does / why we need it:
This PR adds a new API endpoint
GET /config/tenant/{tenant}/limits
that returns the configured limits (merged with the defaults) of a tenant.Note, this may be useful to implement the tenants limits page in the new operator UI
loki/pkg/ui/frontend/src/config/routes.tsx
Lines 94 to 98 in dbf2bef