diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index a7cb8cb3344..527327450aa 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1256,7 +1256,7 @@ pub struct RouterRoute { pub identity: IdentityMetadata, /// The VPC Router to which the route belongs. - pub router_id: Uuid, + pub vpc_router_id: Uuid, /// Describes the kind of router. Set at creation. `read-only` pub kind: RouterRouteKind, diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 5a72569ee99..a936946bf95 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -687,14 +687,14 @@ CREATE TABLE omicron.public.router_route ( /* Indicates that the object has been deleted */ time_deleted TIMESTAMPTZ, - router_id UUID NOT NULL, + vpc_router_id UUID NOT NULL, kind omicron.public.router_route_kind NOT NULL, target STRING(128) NOT NULL, destination STRING(128) NOT NULL ); CREATE UNIQUE INDEX ON omicron.public.router_route ( - router_id, + vpc_router_id, name ) WHERE time_deleted IS NULL; diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 641c2f181ee..31875701639 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2854,7 +2854,7 @@ impl DataStore { use db::schema::router_route::dsl; paginated(dsl::router_route, dsl::name, pagparams) .filter(dsl::time_deleted.is_null()) - .filter(dsl::router_id.eq(authz_router.id())) + .filter(dsl::vpc_router_id.eq(authz_router.id())) .select(RouterRoute::as_select()) .load_async::( self.pool_authorized(opctx).await?, @@ -2863,98 +2863,17 @@ impl DataStore { .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } - /// Fetches a RouterRoute from the database and returns both the database - /// row and an [`authz::RouterRoute`] for doing authz checks - /// - /// See [`DataStore::organization_lookup_noauthz()`] for intended use cases - /// and caveats. - // TODO-security See the note on organization_lookup_noauthz(). - async fn route_lookup_noauthz( - &self, - authz_vpc_router: &authz::VpcRouter, - route_name: &Name, - ) -> LookupResult<(authz::RouterRoute, RouterRoute)> { - use db::schema::router_route::dsl; - dsl::router_route - .filter(dsl::time_deleted.is_null()) - .filter(dsl::router_id.eq(authz_vpc_router.id())) - .filter(dsl::name.eq(route_name.clone())) - .select(RouterRoute::as_select()) - .get_result_async(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::RouterRoute, - LookupType::ByName(route_name.as_str().to_owned()), - ), - ) - }) - .map(|r| { - ( - authz_vpc_router.child_generic( - ResourceType::RouterRoute, - r.id(), - LookupType::ByName(route_name.to_string()), - ), - r, - ) - }) - } - - /// Lookup a RouterRoute by name and return the full database record, along - /// with an [`authz::RouterRoute`] for subsequent authorization checks - pub async fn route_fetch( - &self, - opctx: &OpContext, - authz_vpc_router: &authz::VpcRouter, - name: &Name, - ) -> LookupResult<(authz::RouterRoute, RouterRoute)> { - let (authz_route, db_route) = - self.route_lookup_noauthz(authz_vpc_router, name).await?; - opctx.authorize(authz::Action::Read, &authz_route).await?; - Ok((authz_route, db_route)) - } - - /// Look up the id for a RouterRoute based on its name - /// - /// Returns an [`authz::RouterRoute`] (which makes the id available). - /// - /// Like the other "lookup_by_path()" functions, this function does no authz - /// checks. - pub async fn route_lookup_by_path( - &self, - organization_name: &Name, - project_name: &Name, - vpc_name: &Name, - router_name: &Name, - route_name: &Name, - ) -> LookupResult { - let authz_vpc_router = self - .vpc_router_lookup_by_path( - organization_name, - project_name, - vpc_name, - router_name, - ) - .await?; - self.vpc_router_lookup_noauthz(&authz_vpc_router, route_name) - .await - .map(|(v, _)| v) - } - pub async fn router_create_route( &self, opctx: &OpContext, authz_router: &authz::VpcRouter, route: RouterRoute, ) -> CreateResult { - assert_eq!(authz_router.id(), route.router_id); + assert_eq!(authz_router.id(), route.vpc_router_id); opctx.authorize(authz::Action::CreateChild, authz_router).await?; use db::schema::router_route::dsl; - let router_id = route.router_id; + let router_id = route.vpc_router_id; let name = route.name().clone(); VpcRouter::insert_resource( diff --git a/nexus/src/db/lookup.rs b/nexus/src/db/lookup.rs index 379401d0166..bf9d232eb38 100644 --- a/nexus/src/db/lookup.rs +++ b/nexus/src/db/lookup.rs @@ -204,6 +204,21 @@ impl<'a> LookupPath<'a> { pub fn disk_id(self, id: Uuid) -> Disk<'a> { Disk { key: Key::Id(Root { lookup_root: self }, id) } } + + /// Select a resource of type Vpc, identified by its id + pub fn vpc_id(self, id: Uuid) -> Vpc<'a> { + Vpc { key: Key::Id(Root { lookup_root: self }, id) } + } + + /// Select a resource of type VpcRouter, identified by its id + pub fn vpc_router_id(self, id: Uuid) -> VpcRouter<'a> { + VpcRouter { key: Key::Id(Root { lookup_root: self }, id) } + } + + /// Select a resource of type RouterRoute, identified by its id + pub fn router_route_id(self, id: Uuid) -> RouterRoute<'a> { + RouterRoute { key: Key::Id(Root { lookup_root: self }, id) } + } } /// Describes a node along the selection path of a resource @@ -244,7 +259,7 @@ lookup_resource! { lookup_resource! { name = "Project", ancestors = [ "Organization" ], - children = [ "Disk", "Instance" ], + children = [ "Disk", "Instance", "Vpc" ], authz_kind = Typed } @@ -262,6 +277,27 @@ lookup_resource! { authz_kind = Generic } +lookup_resource! { + name = "Vpc", + ancestors = [ "Organization", "Project" ], + children = [ "VpcRouter" ], + authz_kind = Generic +} + +lookup_resource! { + name = "VpcRouter", + ancestors = [ "Organization", "Project", "Vpc" ], + children = [ "RouterRoute" ], + authz_kind = Generic +} + +lookup_resource! { + name = "RouterRoute", + ancestors = [ "Organization", "Project", "Vpc", "VpcRouter" ], + children = [], + authz_kind = Generic +} + #[cfg(test)] mod test { use super::Instance; diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index 5f51ec218b1..084de900961 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -1810,7 +1810,7 @@ impl DatastoreCollection for VpcRouter { type CollectionId = Uuid; type GenerationNumberColumn = vpc_router::dsl::rcgen; type CollectionTimeDeletedColumn = vpc_router::dsl::time_deleted; - type CollectionIdColumn = router_route::dsl::router_id; + type CollectionIdColumn = router_route::dsl::vpc_router_id; } #[derive(AsChangeset)] @@ -1922,7 +1922,7 @@ pub struct RouterRoute { identity: RouterRouteIdentity, pub kind: RouterRouteKind, - pub router_id: Uuid, + pub vpc_router_id: Uuid, pub target: RouteTarget, pub destination: RouteDestination, } @@ -1930,14 +1930,14 @@ pub struct RouterRoute { impl RouterRoute { pub fn new( route_id: Uuid, - router_id: Uuid, + vpc_router_id: Uuid, kind: external::RouterRouteKind, params: external::RouterRouteCreateParams, ) -> Self { let identity = RouterRouteIdentity::new(route_id, params.identity); Self { identity, - router_id, + vpc_router_id, kind: RouterRouteKind(kind), target: RouteTarget(params.target), destination: RouteDestination::new(params.destination), @@ -1949,7 +1949,7 @@ impl Into for RouterRoute { fn into(self) -> external::RouterRoute { external::RouterRoute { identity: self.identity(), - router_id: self.router_id, + vpc_router_id: self.vpc_router_id, kind: self.kind.0, target: self.target.0.clone(), destination: self.destination.state().clone(), diff --git a/nexus/src/db/schema.rs b/nexus/src/db/schema.rs index 4353f5e375f..f37e3ea7c63 100644 --- a/nexus/src/db/schema.rs +++ b/nexus/src/db/schema.rs @@ -324,7 +324,7 @@ table! { time_modified -> Timestamptz, time_deleted -> Nullable, kind -> crate::db::model::RouterRouteKindEnum, - router_id -> Uuid, + vpc_router_id -> Uuid, target -> Text, destination -> Text, } diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 3ad7de6efe7..31f4c2614cf 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -2535,20 +2535,15 @@ impl Nexus { router_name: &Name, route_name: &Name, ) -> LookupResult { - let authz_router = self - .db_datastore - .vpc_router_lookup_by_path( - organization_name, - project_name, - vpc_name, - router_name, - ) + let (.., db_route) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .vpc_name(vpc_name) + .vpc_router_name(router_name) + .router_route_name(route_name) + .fetch() .await?; - Ok(self - .db_datastore - .route_fetch(&opctx, &authz_router, route_name) - .await? - .1) + Ok(db_route) } #[allow(clippy::too_many_arguments)] @@ -2594,19 +2589,16 @@ impl Nexus { router_name: &Name, route_name: &Name, ) -> DeleteResult { - let authz_router = self - .db_datastore - .vpc_router_lookup_by_path( - organization_name, - project_name, - vpc_name, - router_name, - ) - .await?; - let (authz_route, db_route) = self - .db_datastore - .route_fetch(opctx, &authz_router, route_name) - .await?; + let (.., authz_route, db_route) = + LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .vpc_name(vpc_name) + .vpc_router_name(router_name) + .router_route_name(route_name) + .fetch() + .await?; + // Only custom routes can be deleted // TODO Shouldn't this constraint be checked by the database query? if db_route.kind.0 != RouterRouteKind::Custom { @@ -2629,19 +2621,15 @@ impl Nexus { route_name: &Name, params: &RouterRouteUpdateParams, ) -> UpdateResult { - let authz_router = self - .db_datastore - .vpc_router_lookup_by_path( - organization_name, - project_name, - vpc_name, - router_name, - ) - .await?; - let (authz_route, db_route) = self - .db_datastore - .route_fetch(opctx, &authz_router, route_name) - .await?; + let (.., authz_route, db_route) = + LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .vpc_name(vpc_name) + .vpc_router_name(router_name) + .router_route_name(route_name) + .fetch() + .await?; // TODO: Write a test for this once there's a way to test it (i.e. // subnets automatically register to the system router table) match db_route.kind.0 { diff --git a/openapi/nexus.json b/openapi/nexus.json index 6d337a41cee..6a823a04b48 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -5707,11 +5707,6 @@ } ] }, - "router_id": { - "description": "The VPC Router to which the route belongs.", - "type": "string", - "format": "uuid" - }, "target": { "$ref": "#/components/schemas/RouteTarget" }, @@ -5724,6 +5719,11 @@ "description": "timestamp when this resource was last modified", "type": "string", "format": "date-time" + }, + "vpc_router_id": { + "description": "The VPC Router to which the route belongs.", + "type": "string", + "format": "uuid" } }, "required": [ @@ -5732,10 +5732,10 @@ "id", "kind", "name", - "router_id", "target", "time_created", - "time_modified" + "time_modified", + "vpc_router_id" ] }, "RouterRouteCreateParams": {