From bdc84cf48c6d6d425c8cab7a45fedbd6410adde9 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 4 Feb 2022 16:22:59 -0500 Subject: [PATCH] don't paginate rules endpoint, use same response view for GET and PUT --- common/src/api/external/mod.rs | 18 +++--- nexus/src/db/datastore.rs | 4 +- nexus/src/external_api/http_entrypoints.rs | 14 ++--- nexus/src/nexus.rs | 13 ++-- nexus/tests/integration_tests/vpc_firewall.rs | 30 ++++----- openapi/nexus.json | 62 ++----------------- 6 files changed, 39 insertions(+), 102 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index a585a2173e0..34d9c15409d 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1415,6 +1415,14 @@ pub struct VpcFirewallRule { pub vpc_id: Uuid, } +/** + * Collection of a [`Vpc`]'s firewall rules + */ +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct VpcFirewallRules { + pub rules: Vec, +} + /// A single rule in a VPC firewall #[derive(Clone, Debug, Deserialize, PartialEq, Serialize, JsonSchema)] pub struct VpcFirewallRuleUpdate { @@ -1446,15 +1454,7 @@ pub struct VpcFirewallRuleUpdateParams { pub rules: Vec, } -/** - * Response to an update replacing `Vpc`'s firewall - */ -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct VpcFirewallRuleUpdateResult { - pub rules: Vec, -} - -impl FromIterator for VpcFirewallRuleUpdateResult { +impl FromIterator for VpcFirewallRules { fn from_iter(iter: T) -> Self where T: IntoIterator, diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index cc14293d587..51dfbd54cce 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2043,13 +2043,13 @@ impl DataStore { pub async fn vpc_list_firewall_rules( &self, vpc_id: &Uuid, - pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { use db::schema::vpc_firewall_rule::dsl; - paginated(dsl::vpc_firewall_rule, dsl::name, &pagparams) + dsl::vpc_firewall_rule .filter(dsl::time_deleted.is_null()) .filter(dsl::vpc_id.eq(*vpc_id)) + .order(dsl::name.asc()) .select(VpcFirewallRule::as_select()) .load_async(self.pool()) .await diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index d1a4455d2ad..8b13b93a8d9 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -52,9 +52,8 @@ use omicron_common::api::external::RouterRouteCreateParams; use omicron_common::api::external::RouterRouteKind; use omicron_common::api::external::RouterRouteUpdateParams; use omicron_common::api::external::Saga; -use omicron_common::api::external::VpcFirewallRule; use omicron_common::api::external::VpcFirewallRuleUpdateParams; -use omicron_common::api::external::VpcFirewallRuleUpdateResult; +use omicron_common::api::external::VpcFirewallRules; use omicron_common::api::external::VpcRouter; use omicron_common::api::external::VpcRouterKind; use ref_cast::RefCast; @@ -1460,15 +1459,13 @@ async fn subnets_ips_get( }] async fn vpc_firewall_rules_get( rqctx: Arc>>, - query_params: Query, path_params: Path, -) -> Result>, HttpError> { +) -> Result, HttpError> { // TODO: Check If-Match and fail if the ETag doesn't match anymore. // Without this check, if firewall rules change while someone is listing // the rules, they will see a mix of the old and new rules. let apictx = rqctx.context(); let nexus = &apictx.nexus; - let query = query_params.into_inner(); let path = path_params.into_inner(); let handler = async { let rules = nexus @@ -1476,14 +1473,12 @@ async fn vpc_firewall_rules_get( &path.organization_name, &path.project_name, &path.vpc_name, - &data_page_params_for(&rqctx, &query)? - .map_name(|n| Name::ref_cast(n)), ) .await? .into_iter() .map(|rule| rule.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, rules)?)) + Ok(HttpResponseOk(rules)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -1500,8 +1495,9 @@ async fn vpc_firewall_rules_put( rqctx: Arc>>, path_params: Path, router_params: TypedBody, -) -> Result, HttpError> { +) -> Result, HttpError> { // TODO: Check If-Match and fail if the ETag doesn't match anymore. + // TODO: limit size of the ruleset because the GET endpoint is not paginated let apictx = rqctx.context(); let nexus = &apictx.nexus; let path = path_params.into_inner(); diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 0f1c9681258..1345d21e3f6 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -51,7 +51,7 @@ use omicron_common::api::external::RouterRouteKind; use omicron_common::api::external::RouterRouteUpdateParams; use omicron_common::api::external::UpdateResult; use omicron_common::api::external::VpcFirewallRuleUpdateParams; -use omicron_common::api::external::VpcFirewallRuleUpdateResult; +use omicron_common::api::external::VpcFirewallRules; use omicron_common::api::external::VpcRouterKind; use omicron_common::api::internal::nexus; use omicron_common::api::internal::nexus::DiskRuntimeState; @@ -1735,16 +1735,13 @@ impl Nexus { organization_name: &Name, project_name: &Name, vpc_name: &Name, - pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { let vpc = self .project_lookup_vpc(organization_name, project_name, vpc_name) .await?; - let subnets = self - .db_datastore - .vpc_list_firewall_rules(&vpc.id(), pagparams) - .await?; - Ok(subnets) + let rules = + self.db_datastore.vpc_list_firewall_rules(&vpc.id()).await?; + Ok(rules) } pub async fn vpc_update_firewall_rules( @@ -1753,7 +1750,7 @@ impl Nexus { project_name: &Name, vpc_name: &Name, params: &VpcFirewallRuleUpdateParams, - ) -> UpdateResult { + ) -> UpdateResult { let vpc = self .project_lookup_vpc(organization_name, project_name, vpc_name) .await?; diff --git a/nexus/tests/integration_tests/vpc_firewall.rs b/nexus/tests/integration_tests/vpc_firewall.rs index 2b9ba0faabb..17c54a3bfad 100644 --- a/nexus/tests/integration_tests/vpc_firewall.rs +++ b/nexus/tests/integration_tests/vpc_firewall.rs @@ -9,13 +9,13 @@ use omicron_common::api::external::{ VpcFirewallRuleAction, VpcFirewallRuleDirection, VpcFirewallRuleFilter, VpcFirewallRuleHostFilter, VpcFirewallRulePriority, VpcFirewallRuleProtocol, VpcFirewallRuleStatus, VpcFirewallRuleTarget, - VpcFirewallRuleUpdate, VpcFirewallRuleUpdateParams, + VpcFirewallRuleUpdate, VpcFirewallRuleUpdateParams, VpcFirewallRules, }; use omicron_nexus::external_api::views::Vpc; use std::convert::TryFrom; use uuid::Uuid; -use dropshot::test_util::{object_delete, object_get, objects_list_page}; +use dropshot::test_util::{object_delete, object_get}; use nexus_test_utils::resource_helpers::{ create_organization, create_project, create_vpc, @@ -39,13 +39,14 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { let default_vpc_url = format!("{}/default", vpcs_url); let default_vpc = object_get::(client, &default_vpc_url).await; - let default_vpc_firewall = format!("{}/default/firewall/rules", vpcs_url); - let rules = - objects_list_page::(client, &default_vpc_firewall) - .await - .items; + let default_vpc_firewall = format!("{}/firewall/rules", default_vpc_url); + let rules = object_get::(client, &default_vpc_firewall) + .await + .rules; + dbg!("d"); assert!(rules.iter().all(|r| r.vpc_id == default_vpc.identity.id)); assert!(is_default_firewall_rules(&rules)); + dbg!("e"); // Create another VPC and make sure it gets the default rules. let other_vpc = "second-vpc"; @@ -53,9 +54,7 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { format!("{}/{}/firewall/rules", vpcs_url, other_vpc); let vpc2 = create_vpc(&client, &org_name, &project_name, &other_vpc).await; let rules = - objects_list_page::(client, &other_vpc_firewall) - .await - .items; + object_get::(client, &other_vpc_firewall).await.rules; assert!(rules.iter().all(|r| r.vpc_id == vpc2.identity.id)); assert!(is_default_firewall_rules(&rules)); @@ -107,10 +106,9 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { .unwrap(); // Make sure the firewall is changed - let rules = - objects_list_page::(client, &default_vpc_firewall) - .await - .items; + let rules = object_get::(client, &default_vpc_firewall) + .await + .rules; assert!(!is_default_firewall_rules(&rules)); assert_eq!(rules.len(), new_rules.len()); assert_eq!(rules[0].identity.name, "allow-icmp"); @@ -118,9 +116,7 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { // Make sure the other firewall is unchanged let rules = - objects_list_page::(client, &other_vpc_firewall) - .await - .items; + object_get::(client, &other_vpc_firewall).await.rules; assert!(is_default_firewall_rules(&rules)); // DELETE is unspported diff --git a/openapi/nexus.json b/openapi/nexus.json index 1d39c41bec2..0d84e07cb05 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -1729,36 +1729,6 @@ "summary": "List firewall rules for a VPC.", "operationId": "vpc_firewall_rules_get", "parameters": [ - { - "in": "query", - "name": "limit", - "schema": { - "nullable": true, - "description": "Maximum number of items returned by a single call", - "type": "integer", - "format": "uint32", - "minimum": 1 - }, - "style": "form" - }, - { - "in": "query", - "name": "page_token", - "schema": { - "nullable": true, - "description": "Token returned by previous call to retreive the subsequent page", - "type": "string" - }, - "style": "form" - }, - { - "in": "query", - "name": "sort_by", - "schema": { - "$ref": "#/components/schemas/NameSortMode" - }, - "style": "form" - }, { "in": "path", "name": "organization_name", @@ -1793,13 +1763,12 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/VpcFirewallRuleResultsPage" + "$ref": "#/components/schemas/VpcFirewallRules" } } } } - }, - "x-dropshot-pagination": true + } }, "put": { "tags": [ @@ -1852,7 +1821,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/VpcFirewallRuleUpdateResult" + "$ref": "#/components/schemas/VpcFirewallRules" } } } @@ -5158,27 +5127,6 @@ "ICMP" ] }, - "VpcFirewallRuleResultsPage": { - "description": "A single page of results", - "type": "object", - "properties": { - "items": { - "description": "list of items on this page of results", - "type": "array", - "items": { - "$ref": "#/components/schemas/VpcFirewallRule" - } - }, - "next_page": { - "nullable": true, - "description": "token used to fetch the next page of results (if any)", - "type": "string" - } - }, - "required": [ - "items" - ] - }, "VpcFirewallRuleStatus": { "type": "string", "enum": [ @@ -5333,8 +5281,8 @@ "rules" ] }, - "VpcFirewallRuleUpdateResult": { - "description": "Response to an update replacing `Vpc`'s firewall", + "VpcFirewallRules": { + "description": "Collection of a [`Vpc`]'s firewall rules", "type": "object", "properties": { "rules": {