[nexus] Move existing OxQL endpoints under /v1/system/*
#7047
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
david-crespo
changed the title
/v1/system/*
david-crespo
changed the title
/v1/system/*/v1/system/*
2 tasks
david-crespo
commented
Nov 13, 2024
| @@ -170,6 +168,8 @@ ip_pool_view GET /v1/system/ip-pools/{pool} | |||
| API operations found with tag "system/metrics" | |||
| OPERATION ID METHOD URL PATH | |||
| system_metric GET /v1/system/metrics/{metric_name} | |||
| system_timeseries_query POST /v1/system/timeseries/query | |||
| system_timeseries_schema_list GET /v1/system/timeseries/schema | |||
There was a problem hiding this comment.
I think that's OK, just need to update the CLI as well.
bnaecker
reviewed
Nov 13, 2024
Comment on lines
+8498
to
+8499
| "summary": "Run timeseries query", | ||
| "description": "Queries are written in OxQL.", |
There was a problem hiding this comment.
Do we want the summary or description to reflect that this is for fleet-scoped timeseries? Not sure if it's relevant or important yet, given that's the only scope we have today.
There was a problem hiding this comment.
Well, currently it is technically for all timeseries in the system. I'm honestly not sure whether we want to change that in the future. Generally we try not to make it too easy for operators to peek inside silos, with a few exceptions like being able to list instances on a given sled.
bnaecker
approved these changes
Nov 13, 2024
david-crespo
added a commit
that referenced
this pull request
Dec 3, 2024
#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. - [x] Add `/v1/timeseries/query` with required project query param - [x] Integration tests showing the authz works in an expected scenarios - ~~Add a list schemas endpoint that only lists schemas with a `project_id` field~~ - ~~Move schema filtering logic inside oximeter client~~ - ~~Fully test schema list endpoint~~ ## The trick 1. Require the user to say (in a query param) what project they're interested in 2. Look up that project and make sure they can read it 3. Jam `| filter silo_id == "<silo_id>" && project_id == "<project_id>"` on the end of whatever query they passed in It 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 a `project_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: https://github.com/oxidecomputer/omicron/blob/45f5f1cc2c7eec2a1de0a5143e85e0794134f175/nexus/external-api/output/nexus_tags.txt#L59-L64 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. <details> <summary>Examples of fleet- and project-scoped metrics</summary https://github.com/oxidecomputer/omicron/blob/45f5f1cc2c7eec2a1de0a5143e85e0794134f175/oximeter/oximeter/schema/virtual-machine.toml#L3-L18 https://github.com/oxidecomputer/omicron/blob/45f5f1cc2c7eec2a1de0a5143e85e0794134f175/oximeter/oximeter/schema/switch-data-link.toml#L3-L19 </details> ## 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-codes `authz_scope: Fleet`. https://github.com/oxidecomputer/omicron/blob/69de8b6288fde36fbcd6cabb4d632d62851230ad/oximeter/db/src/model/from_block.rs#L142 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/querywith 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.