[nexus] Project-scoped OxQL endpoint - #6873
Merged
Merged
Conversation
david-crespo
force-pushed
the
project-scoped-oxql
branch
from
October 24, 2024 00:37
2fa2018 to
67eb7e0
Compare
david-crespo
force-pushed
the
project-scoped-oxql
branch
2 times, most recently
from
November 6, 2024 16:49
02d0476 to
75b035f
Compare
david-crespo
force-pushed
the
project-scoped-oxql
branch
from
November 12, 2024 21:27
75b035f to
5f92e34
Compare
david-crespo
force-pushed
the
project-scoped-oxql
branch
2 times, most recently
from
November 13, 2024 00:18
8952807 to
5d69621
Compare
david-crespo
commented
Nov 13, 2024
david-crespo
commented
Nov 13, 2024
| query.as_ref(), | ||
| authz_silo.id(), | ||
| authz_project.id() | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
This is the crux of the PR.
david-crespo
commented
Nov 13, 2024
david-crespo
force-pushed
the
project-scoped-oxql
branch
from
November 13, 2024 17:07
b707fd0 to
5fd8be9
Compare
david-crespo
marked this pull request as ready for review
November 13, 2024 17:46
david-crespo
force-pushed
the
project-scoped-oxql
branch
2 times, most recently
from
November 13, 2024 18:58
6d9d1fa to
f3e8e0c
Compare
david-crespo
added a commit
that referenced
this pull request
Nov 13, 2024
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`.
david-crespo
force-pushed
the
project-scoped-oxql
branch
from
November 13, 2024 23:03
f3e8e0c to
cd4b011
Compare
bnaecker
approved these changes
Dec 3, 2024
bnaecker
left a comment
Collaborator
There was a problem hiding this comment.
Looks good, thanks for doing this! One small comment, but otherwise ready to go I think.
| .await | ||
| .map(|result| result.tables) | ||
| .map_err(|e| match e { | ||
| oximeter_db::Error::DatabaseUnavailable(_) => { |
Collaborator
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
Thanks, I'll rebase on main and do that.
# Conflicts: # nexus/tests/integration_tests/metrics.rs
david-crespo
force-pushed
the
project-scoped-oxql
branch
from
December 3, 2024 22:19
20a7147 to
f4b2313
Compare
This file contains hidden or 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
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.
#7047 moves the existing
/v1/timeseries/queryendpoint (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/querywith required project query paramAdd a list schemas endpoint that only lists schemas with aproject_idfieldMove 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_idon 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_scopeis 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
hiddenon 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.