From 27bd6588d68a4e94a2ab861398f4e46edbbedf25 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 28 Jun 2024 10:41:33 -0500 Subject: [PATCH] split cap into rules cap (1024) and parts cap (128) --- nexus/db-model/src/vpc_firewall_rule.rs | 36 +++++------ nexus/tests/integration_tests/vpc_firewall.rs | 59 ++++++++++++------- 2 files changed, 56 insertions(+), 39 deletions(-) diff --git a/nexus/db-model/src/vpc_firewall_rule.rs b/nexus/db-model/src/vpc_firewall_rule.rs index a345b579c6..6071dd42c7 100644 --- a/nexus/db-model/src/vpc_firewall_rule.rs +++ b/nexus/db-model/src/vpc_firewall_rule.rs @@ -211,24 +211,24 @@ pub struct VpcFirewallRule { pub priority: VpcFirewallRulePriority, } -/// Cap on the number of rules in a VPC. Also used as a max length on other vecs -/// in the rule body. +/// Cap on the number of rules in a VPC /// -/// We could have 5 constants, but why bother? The value is meant to be big -/// enough that customers will rarely run into it in practice. We just want to -/// prevent a single request from being able to write tons of rows to the DB or -/// blow up OPTE. -const MAX_FW_RULES_PER_VPC: usize = 512; +/// The choice of value is somewhat arbitrary, but the goal is to have a +/// large number that customers are unlikely to actually hit, but which still +/// meaningfully limits the ability to overload the DB with a single request. +const MAX_FW_RULES_PER_VPC: usize = 1024; -fn ensure_max_length( +/// Cap on targets and on each type of filter +const MAX_FW_RULE_PARTS: usize = 128; + +fn ensure_max_len( items: &Vec, label: &str, + max: usize, ) -> Result<(), external::Error> { - if items.len() > MAX_FW_RULES_PER_VPC { - return Err(external::Error::invalid_value( - label, - format!("max length {}", MAX_FW_RULES_PER_VPC), - )); + if items.len() > max { + let msg = format!("max length {}", max); + return Err(external::Error::invalid_value(label, msg)); } Ok(()) } @@ -247,16 +247,16 @@ impl VpcFirewallRule { }, ); - ensure_max_length(&rule.targets, "targets")?; + ensure_max_len(&rule.targets, "targets", MAX_FW_RULE_PARTS)?; if let Some(hosts) = rule.filters.hosts.as_ref() { - ensure_max_length(&hosts, "filters.hosts")?; + ensure_max_len(&hosts, "filters.hosts", MAX_FW_RULE_PARTS)?; } if let Some(ports) = rule.filters.ports.as_ref() { - ensure_max_length(&ports, "filters.ports")?; + ensure_max_len(&ports, "filters.ports", MAX_FW_RULE_PARTS)?; } if let Some(protocols) = rule.filters.protocols.as_ref() { - ensure_max_length(&protocols, "filters.protocols")?; + ensure_max_len(&protocols, "filters.protocols", MAX_FW_RULE_PARTS)?; } Ok(Self { @@ -291,7 +291,7 @@ impl VpcFirewallRule { params: external::VpcFirewallRuleUpdateParams, ) -> Result, external::Error> { ensure_no_duplicates(¶ms)?; - ensure_max_length(¶ms.rules, "rules")?; + ensure_max_len(¶ms.rules, "rules", MAX_FW_RULES_PER_VPC)?; params .rules .into_iter() diff --git a/nexus/tests/integration_tests/vpc_firewall.rs b/nexus/tests/integration_tests/vpc_firewall.rs index 2b52b8e2f5..4e75d2c2c9 100644 --- a/nexus/tests/integration_tests/vpc_firewall.rs +++ b/nexus/tests/integration_tests/vpc_firewall.rs @@ -353,23 +353,23 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { format!("/v1/vpc-firewall-rules?vpc=default&project={}", project_name); // fine with max count - let max: usize = 512; + let max_rules: usize = 1024; let rules: VpcFirewallRules = object_put( client, &url, &VpcFirewallRuleUpdateParams { - rules: (0..max).map(rule_n).collect::>(), + rules: (0..max_rules).map(rule_n).collect::>(), }, ) .await; - assert_eq!(rules.rules.len(), max); + assert_eq!(rules.rules.len(), max_rules); // fails with max + 1 let error = object_put_error( client, &url, &VpcFirewallRuleUpdateParams { - rules: (0..max + 1).map(rule_n).collect::>(), + rules: (0..max_rules + 1).map(rule_n).collect::>(), }, StatusCode::BAD_REQUEST, ) @@ -377,13 +377,15 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { assert_eq!(error.error_code, Some("InvalidValue".to_string())); assert_eq!( error.message, - format!("unsupported value for \"rules\": max length {max}") + format!("unsupported value for \"rules\": max length {max_rules}") ); /////////////////////// // TARGETS /////////////////////// + let max_parts: usize = 128; + let target = VpcFirewallRuleTarget::Vpc("default".parse().unwrap()); // fine with max @@ -392,13 +394,13 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { &url, &VpcFirewallRuleUpdateParams { rules: vec![VpcFirewallRuleUpdate { - targets: (0..max).map(|_i| target.clone()).collect(), + targets: (0..max_parts).map(|_i| target.clone()).collect(), ..base_rule.clone() }], }, ) .await; - assert_eq!(rules.rules[0].targets.len(), max); + assert_eq!(rules.rules[0].targets.len(), max_parts); // fails with max + 1 let error = object_put_error( @@ -406,7 +408,7 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { &url, &VpcFirewallRuleUpdateParams { rules: vec![VpcFirewallRuleUpdate { - targets: (0..max + 1).map(|_i| target.clone()).collect(), + targets: (0..max_parts + 1).map(|_i| target.clone()).collect(), ..base_rule.clone() }], }, @@ -416,7 +418,7 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { assert_eq!(error.error_code, Some("InvalidValue".to_string())); assert_eq!( error.message, - format!("unsupported value for \"targets\": max length {max}") + format!("unsupported value for \"targets\": max length {max_parts}") ); /////////////////////// @@ -432,7 +434,9 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { &VpcFirewallRuleUpdateParams { rules: vec![VpcFirewallRuleUpdate { filters: VpcFirewallRuleFilter { - hosts: Some((0..max).map(|_i| host.clone()).collect()), + hosts: Some( + (0..max_parts).map(|_i| host.clone()).collect(), + ), protocols: None, ports: None, }, @@ -441,7 +445,7 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { }, ) .await; - assert_eq!(rules.rules[0].filters.hosts.as_ref().unwrap().len(), max); + assert_eq!(rules.rules[0].filters.hosts.as_ref().unwrap().len(), max_parts); // fails with max + 1 let error = object_put_error( @@ -450,7 +454,9 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { &VpcFirewallRuleUpdateParams { rules: vec![VpcFirewallRuleUpdate { filters: VpcFirewallRuleFilter { - hosts: Some((0..max + 1).map(|_i| host.clone()).collect()), + hosts: Some( + (0..max_parts + 1).map(|_i| host.clone()).collect(), + ), protocols: None, ports: None, }, @@ -463,7 +469,9 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { assert_eq!(error.error_code, Some("InvalidValue".to_string())); assert_eq!( error.message, - format!("unsupported value for \"filters.hosts\": max length {max}") + format!( + "unsupported value for \"filters.hosts\": max length {max_parts}" + ) ); /////////////////////// @@ -479,7 +487,7 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { &VpcFirewallRuleUpdateParams { rules: vec![VpcFirewallRuleUpdate { filters: VpcFirewallRuleFilter { - ports: Some((0..max).map(|_i| port).collect()), + ports: Some((0..max_parts).map(|_i| port).collect()), protocols: None, hosts: None, }, @@ -488,7 +496,7 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { }, ) .await; - assert_eq!(rules.rules[0].filters.ports.as_ref().unwrap().len(), max); + assert_eq!(rules.rules[0].filters.ports.as_ref().unwrap().len(), max_parts); // fails with max + 1 let error = object_put_error( @@ -497,7 +505,7 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { &VpcFirewallRuleUpdateParams { rules: vec![VpcFirewallRuleUpdate { filters: VpcFirewallRuleFilter { - ports: Some((0..max + 1).map(|_i| port).collect()), + ports: Some((0..max_parts + 1).map(|_i| port).collect()), protocols: None, hosts: None, }, @@ -510,7 +518,9 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { assert_eq!(error.error_code, Some("InvalidValue".to_string())); assert_eq!( error.message, - format!("unsupported value for \"filters.ports\": max length {max}") + format!( + "unsupported value for \"filters.ports\": max length {max_parts}" + ) ); /////////////////////// @@ -526,7 +536,9 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { &VpcFirewallRuleUpdateParams { rules: vec![VpcFirewallRuleUpdate { filters: VpcFirewallRuleFilter { - protocols: Some((0..max).map(|_i| protocol).collect()), + protocols: Some( + (0..max_parts).map(|_i| protocol).collect(), + ), ports: None, hosts: None, }, @@ -535,7 +547,10 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { }, ) .await; - assert_eq!(rules.rules[0].filters.protocols.as_ref().unwrap().len(), max); + assert_eq!( + rules.rules[0].filters.protocols.as_ref().unwrap().len(), + max_parts + ); // fails with max + 1 let error = object_put_error( @@ -544,7 +559,9 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { &VpcFirewallRuleUpdateParams { rules: vec![VpcFirewallRuleUpdate { filters: VpcFirewallRuleFilter { - protocols: Some((0..max + 1).map(|_i| protocol).collect()), + protocols: Some( + (0..max_parts + 1).map(|_i| protocol).collect(), + ), ports: None, hosts: None, }, @@ -558,7 +575,7 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { assert_eq!( error.message, format!( - "unsupported value for \"filters.protocols\": max length {max}" + "unsupported value for \"filters.protocols\": max length {max_parts}" ) ); }