don't paginate rules endpoint, use same response view for GET and PUT

oxidecomputer · Feb 4, 2022 · bdc84cf · bdc84cf
1 parent d9207c0
commit bdc84cf
Show file tree

Hide file tree

Showing 6 changed files with 39 additions and 102 deletions.
diff --git 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<VpcFirewallRule>,
+}
+
 /// 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<VpcFirewallRuleUpdate>,
 }
 
-/**
- * Response to an update replacing `Vpc`'s firewall
- */
-#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
-pub struct VpcFirewallRuleUpdateResult {
-    pub rules: Vec<VpcFirewallRule>,
-}
-
-impl FromIterator<VpcFirewallRule> for VpcFirewallRuleUpdateResult {
+impl FromIterator<VpcFirewallRule> for VpcFirewallRules {
     fn from_iter<T>(iter: T) -> Self
     where
         T: IntoIterator<Item = VpcFirewallRule>,

diff --git 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<VpcFirewallRule> {
         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
@@ -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,30 +1459,26 @@ async fn subnets_ips_get(
 }]
 async fn vpc_firewall_rules_get(
     rqctx: Arc<RequestContext<Arc<ServerContext>>>,
-    query_params: Query<PaginatedByName>,
     path_params: Path<VpcPathParam>,
-) -> Result<HttpResponseOk<ResultsPage<VpcFirewallRule>>, HttpError> {
+) -> Result<HttpResponseOk<VpcFirewallRules>, 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
             .vpc_list_firewall_rules(
                 &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<RequestContext<Arc<ServerContext>>>,
     path_params: Path<VpcPathParam>,
     router_params: TypedBody<VpcFirewallRuleUpdateParams>,
-) -> Result<HttpResponseOk<VpcFirewallRuleUpdateResult>, HttpError> {
+) -> Result<HttpResponseOk<VpcFirewallRules>, 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
@@ -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<db::model::VpcFirewallRule> {
         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<VpcFirewallRuleUpdateResult> {
+    ) -> UpdateResult<VpcFirewallRules> {
         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
@@ -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,23 +39,22 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) {
     let default_vpc_url = format!("{}/default", vpcs_url);
     let default_vpc = object_get::<Vpc>(client, &default_vpc_url).await;
 
-    let default_vpc_firewall = format!("{}/default/firewall/rules", vpcs_url);
-    let rules =
-        objects_list_page::<VpcFirewallRule>(client, &default_vpc_firewall)
-            .await
-            .items;
+    let default_vpc_firewall = format!("{}/firewall/rules", default_vpc_url);
+    let rules = object_get::<VpcFirewallRules>(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";
     let other_vpc_firewall =
         format!("{}/{}/firewall/rules", vpcs_url, other_vpc);
     let vpc2 = create_vpc(&client, &org_name, &project_name, &other_vpc).await;
     let rules =
-        objects_list_page::<VpcFirewallRule>(client, &other_vpc_firewall)
-            .await
-            .items;
+        object_get::<VpcFirewallRules>(client, &other_vpc_firewall).await.rules;
     assert!(rules.iter().all(|r| r.vpc_id == vpc2.identity.id));
     assert!(is_default_firewall_rules(&rules));
 
@@ -107,20 +106,17 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) {
         .unwrap();
 
     // Make sure the firewall is changed
-    let rules =
-        objects_list_page::<VpcFirewallRule>(client, &default_vpc_firewall)
-            .await
-            .items;
+    let rules = object_get::<VpcFirewallRules>(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");
     assert_eq!(rules[1].identity.name, "deny-all-incoming");
 
     // Make sure the other firewall is unchanged
     let rules =
-        objects_list_page::<VpcFirewallRule>(client, &other_vpc_firewall)
-            .await
-            .items;
+        object_get::<VpcFirewallRules>(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
@@ -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": {