Skip to content

Commit

Permalink
it almost kind of works
Browse files Browse the repository at this point in the history
  • Loading branch information
david-crespo committed Oct 15, 2024
1 parent 1a5a2e5 commit 2fa2018
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 25 deletions.
15 changes: 9 additions & 6 deletions nexus/src/app/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,13 +187,16 @@ impl super::Nexus {
query: impl AsRef<str>,
) -> Result<Vec<oxql_types::Table>, Error> {
// Ensure the user has read access to the project
let (.., authz_project) =
let (authz_silo, authz_project) =
project_lookup.lookup_for(authz::Action::Read).await?;

let parsed_query = oximeter_db::oxql::Query::new(query.as_ref())
.map_err(|e| Error::invalid_request(e.to_string()))?;

// Check that the query only refers to the project
// Ensure the query only refers to the project
let filtered_query = format!(
"{} | filter silo_id == \"{}\" && project_id == \"{}\"",
query.as_ref(),
authz_silo.id(),
authz_project.id()
);

self.timeseries_client
.get()
Expand All @@ -204,7 +207,7 @@ impl super::Nexus {
e
))
})?
.oxql_query(query)
.oxql_query(filtered_query)
.await
.map(|result| result.tables)
.map_err(|e| match e {
Expand Down
76 changes: 57 additions & 19 deletions nexus/tests/integration_tests/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use chrono::Utc;
use dropshot::test_util::ClientTestContext;
use dropshot::ResultsPage;
use http::{Method, StatusCode};
use nexus_test_utils::background::activate_background_task;
use nexus_test_utils::http_testing::{AuthnMode, NexusRequest, RequestBuilder};
use nexus_test_utils::resource_helpers::{
create_default_ip_pool, create_disk, create_instance, create_project,
Expand Down Expand Up @@ -298,7 +299,7 @@ pub async fn project_timeseries_query(
) -> Vec<oxql_types::Table> {
execute_timeseries_query(
cptestctx,
&format!("/v1/timeseries/query/projects/{}", project),
&format!("/v1/timeseries/query/project/{}", project),
query,
)
.await
Expand Down Expand Up @@ -554,33 +555,70 @@ async fn test_project_timeseries_query(
) {
let client = &cptestctx.external_client;

create_default_ip_pool(&client).await; // needed for instance create to work

// Create two projects
let project1 = create_project(&client, "project1").await;
let project2 = create_project(&client, "project2").await;
let p1 = create_project(&client, "project1").await;
let _p2 = create_project(&client, "project2").await;

// Create resources in each project
create_instance(&client, "project1", "instance1").await;
create_instance(&client, "project2", "instance2").await;
let i1 = create_instance(&client, "project1", "instance1").await;
let _i2 = create_instance(&client, "project2", "instance2").await;

// Force a metrics collection
cptestctx.oximeter.force_collect().await;
let internal_client = &cptestctx.internal_client;

// Query for project1
let query1 = "get virtual_machine:check"; // TODO: add project to query
let result1 =
project_timeseries_query(&cptestctx, "project1", query1).await;
// get the instance metrics to show up
let _ =
activate_background_task(&internal_client, "instance_watcher").await;

// shouldn't work
let result1 =
project_timeseries_query(&cptestctx, "project2", query1).await;
// Query with no project specified
let q1 = "get virtual_machine:check";

// Query for project2
let query2 = "get virtual_machine:check";
let result2 =
project_timeseries_query(&cptestctx, "project2", query2).await;
let result = project_timeseries_query(&cptestctx, "project1", q1).await;
assert_eq!(result.len(), 1);
assert!(result[0].timeseries().len() > 0);

let result = project_timeseries_query(&cptestctx, "project2", q1).await;
assert_eq!(result.len(), 1);
assert!(result[0].timeseries().len() > 0);

// with project specified
let q2 = &format!("{} | filter project_id == \"{}\"", q1, p1.identity.id);

let result = project_timeseries_query(&cptestctx, "project1", q2).await;
assert_eq!(result.len(), 1);
assert!(result[0].timeseries().len() > 0);

let result = project_timeseries_query(&cptestctx, "project2", q2).await;
assert_eq!(result.len(), 1);
assert_eq!(result[0].timeseries().len(), 0);

// with instance specified
let q3 = &format!("{} | filter instance_id == \"{}\"", q1, i1.identity.id);

// project containing instance gives me something
let result = project_timeseries_query(&cptestctx, "project1", q3).await;
assert_eq!(result.len(), 1);
assert_eq!(result[0].timeseries().len(), 1);

// should be empty or error
let result = project_timeseries_query(&cptestctx, "project2", q3).await;
assert_eq!(result.len(), 1);
assert_eq!(result[0].timeseries().len(), 0);

// Query with no project specified
// Query with nonexistent project
// Also test at least once with project ID in path instead of project name

// try a project in your silo that you can't read
// try a project in another silo

// now try it adversarially — use a project you can read in the path, but
// try to access a fleet metric... if I can find one that will work

// let q4 = "get hardware_component:temperature";
// let result = dbg!(timeseries_query(&cptestctx, q4).await);
// let result =
// dbg!(project_timeseries_query(&cptestctx, "project2", q4).await);
}

#[nexus_test]
Expand Down

0 comments on commit 2fa2018

Please sign in to comment.