-
Notifications
You must be signed in to change notification settings - Fork 40
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
[nexus] Project-scoped OxQL endpoint #6873
Conversation
2fa2018
to
67eb7e0
Compare
02d0476
to
75b035f
Compare
75b035f
to
5f92e34
Compare
8952807
to
5d69621
Compare
query.as_ref(), | ||
authz_silo.id(), | ||
authz_project.id() | ||
); |
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.
This is the crux of the PR.
b707fd0
to
5fd8be9
Compare
6d9d1fa
to
f3e8e0c
Compare
Discussed this with @bnaecker a couple of weeks ago. Was going to do this in #6873 but I decided to split it out because it's just noise. The idea here is that (for now) rather than getting elaborate with permissions on the OxQL endpoint a la #5298, we just keep the existing endpoint fleet-viewer scoped and move it to `/v1/system/timeseries/query` with all the other operator endpoints. #6873 goes on top of this one and adds a project-scoped endpoint at `/v1/timeseries/query?project=my-proj`.
f3e8e0c
to
cd4b011
Compare
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.
Looks good, thanks for doing this! One small comment, but otherwise ready to go I think.
nexus/src/app/metrics.rs
Outdated
.await | ||
.map(|result| result.tables) | ||
.map_err(|e| match e { | ||
oximeter_db::Error::DatabaseUnavailable(_) => { |
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 think we might need to put the Connection
error variant in here too. We get that when qorb
fails to loan out a connection from its pool.
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.
Thanks, I'll rebase on main and do that.
# Conflicts: # nexus/tests/integration_tests/metrics.rs
20a7147
to
f4b2313
Compare
#7047 moves the existing
/v1/timeseries/query
endpoint (which requires the fleet viewer role) to/v1/system/timeseries/query
. This PR is on top of #7047 and adds/v1/timeseries/query?project=my-project
, using a sneaky trick to let us ensure that the user has read permissions on a project before we let them see metrics for that project. See #5298 (comment) for a discussion of the OxQL authz problem more broadly./v1/timeseries/query
with required project query paramAdd a list schemas endpoint that only lists schemas with aproject_id
fieldMove schema filtering logic inside oximeter clientFully test schema list endpointThe trick
| filter silo_id == "<silo_id>" && project_id == "<project_id>"
on the end of whatever query they passed inIt sounds silly, but I talked it over with @bnaecker and we couldn't find any holes. If the user tries to fetch metrics from another project inside their query, the query will end up with the filter
project_id == "def" && project_id == "xyz"
and the result set will always be empty. If they try to query a metric that doesn't have aproject_id
on it, it will error out. It works!API design
Initially I had the endpoint as
/v1/timeseries/query/project/{project}
. This is really horrible. It should be/projects/
, but that doesn't feel any better. I also considered/v1/projects/{project}/timeseries/query
, which has some precedent:omicron/nexus/external-api/output/nexus_tags.txt
Lines 59 to 64 in 45f5f1c
But it also feels awful.
I like the query param approach. Right now, there are only fleet-scoped metrics and project-scoped metrics, so requiring the project ID makes sense, but the query params are nicely flexible in that we could make project optional or add other optional fields and just do different auth checks based on what's passed in. Neither path nor operation ID mention projects.
Examples of fleet- and project-scoped metrics
omicron/oximeter/oximeter/schema/virtual-machine.toml
Lines 3 to 18 in 45f5f1c
omicron/oximeter/oximeter/schema/switch-data-link.toml
Lines 3 to 19 in 45f5f1c
No list endpoint yet
I backed out the schema list endpoint. We can't list project-scoped schemas because
authz_scope
is not in the database! See #5942. Currently the schema list endpoint hard-codesauthz_scope: Fleet
.omicron/oximeter/db/src/model/from_block.rs
Line 142 in 69de8b6
I am using the tag
hidden
on the query endpoint so that it goes in the OpenAPI definition (so I can use it in the console) but it will not show up on the docs site.