Skip to content

Commit

Permalink
raise cap to 512 and use it for targets and filters too
Browse files Browse the repository at this point in the history
  • Loading branch information
@david-crespo
david-crespo committed Jun 19, 2024
1 parent 75f5b67 commit a4b3ba0
Show file tree
Hide file tree
Showing 3 changed files with 301 additions and 16 deletions.
50 changes: 43 additions & 7 deletions nexus/db-model/src/vpc_firewall_rule.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,20 +211,55 @@ 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.
/// We could have 5 constants, but don't bother unless they need to be
/// different. 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;

fn ensure_max_length<T>(
items: &Vec<T>,
label: &str,
) -> 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),
));
}
Ok(())
}

impl VpcFirewallRule {
pub fn new(
rule_id: Uuid,
vpc_id: Uuid,
rule: &external::VpcFirewallRuleUpdate,
) -> Self {
) -> Result<Self, external::Error> {
let identity = VpcFirewallRuleIdentity::new(
rule_id,
external::IdentityMetadataCreateParams {
name: rule.name.clone(),
description: rule.description.clone(),
},
);
Self {

ensure_max_length(&rule.targets, "targets")?;

if let Some(hosts) = rule.filters.hosts.as_ref() {
ensure_max_length(&hosts, "filters.hosts")?;
}
if let Some(ports) = rule.filters.ports.as_ref() {
ensure_max_length(&ports, "filters.ports")?;
}
if let Some(protocols) = rule.filters.protocols.as_ref() {
ensure_max_length(&protocols, "filters.protocols")?;
}

Ok(Self {
identity,
vpc_id,
status: rule.status.into(),
Expand All @@ -248,19 +283,20 @@ impl VpcFirewallRule {
}),
action: rule.action.into(),
priority: rule.priority.into(),
}
})
}

pub fn vec_from_params(
vpc_id: Uuid,
params: external::VpcFirewallRuleUpdateParams,
) -> Result<Vec<VpcFirewallRule>, external::Error> {
ensure_no_duplicates(&params)?;
Ok(params
ensure_max_length(&params.rules, "rules")?;
params
.rules
.iter()
.map(|rule| VpcFirewallRule::new(Uuid::new_v4(), vpc_id, rule))
.collect())
.into_iter()
.map(|rule| VpcFirewallRule::new(Uuid::new_v4(), vpc_id, &rule))
.collect()
}
}

Expand Down
20 changes: 12 additions & 8 deletions nexus/db-queries/src/db/datastore/vpc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,20 +195,24 @@ impl DataStore {
.map(|rule| (rule.name().clone(), rule))
.collect::<BTreeMap<_, _>>();

fw_rules.entry(DNS_VPC_FW_RULE.name.clone()).or_insert_with(|| {
VpcFirewallRule::new(
// these have to be done this way because the contructor returns a result
if !fw_rules.contains_key(&DNS_VPC_FW_RULE.name) {
let rule = VpcFirewallRule::new(
Uuid::new_v4(),
*SERVICES_VPC_ID,
&DNS_VPC_FW_RULE,
)
});
fw_rules.entry(NEXUS_VPC_FW_RULE.name.clone()).or_insert_with(|| {
VpcFirewallRule::new(
)?;
fw_rules.insert(DNS_VPC_FW_RULE.name.clone(), rule);
}

if !fw_rules.contains_key(&NEXUS_VPC_FW_RULE.name) {
let rule = VpcFirewallRule::new(
Uuid::new_v4(),
*SERVICES_VPC_ID,
&NEXUS_VPC_FW_RULE,
)
});
)?;
fw_rules.insert(NEXUS_VPC_FW_RULE.name.clone(), rule);
}

let rules = fw_rules
.into_values()
Expand Down
247 changes: 246 additions & 1 deletion nexus/tests/integration_tests/vpc_firewall.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use http::method::Method;
use http::StatusCode;
use nexus_test_utils::http_testing::{AuthnMode, NexusRequest};
use nexus_test_utils::resource_helpers::{
create_project, create_vpc, object_get, object_put_error,
create_project, create_vpc, object_get, object_put, object_put_error,
};
use nexus_test_utils_macros::nexus_test;
use nexus_types::external_api::views::Vpc;
Expand Down Expand Up @@ -321,3 +321,248 @@ async fn test_firewall_rules_same_name(cptestctx: &ControlPlaneTestContext) {
assert_eq!(error.error_code, Some("InvalidValue".to_string()));
assert_eq!(error.message, "unsupported value for \"rules\": Rule names must be unique. Duplicates: [\"dupe\"]");
}

#[nexus_test]
async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) {
let client = &cptestctx.external_client;

let project_name = "my-project";
create_project(&client, &project_name).await;

let base_rule = VpcFirewallRuleUpdate {
name: "my-rule".parse().unwrap(),
description: "".to_string(),
status: VpcFirewallRuleStatus::Enabled,
direction: VpcFirewallRuleDirection::Inbound,
targets: vec![],
filters: VpcFirewallRuleFilter {
hosts: None,
protocols: None,
ports: None,
},
action: VpcFirewallRuleAction::Allow,
priority: VpcFirewallRulePriority(65534),
};

let rule_n = |i: usize| VpcFirewallRuleUpdate {
name: format!("rule{}", i).parse().unwrap(),
..base_rule.clone()
};

let url =
format!("/v1/vpc-firewall-rules?vpc=default&project={}", project_name);

// fine with max count
let max: usize = 512;
let rules: VpcFirewallRules = object_put(
client,
&url,
&VpcFirewallRuleUpdateParams {
rules: (0..max).map(rule_n).collect::<Vec<_>>(),
},
)
.await;
assert_eq!(rules.rules.len(), max);

// fails with max + 1
let error = object_put_error(
client,
&url,
&VpcFirewallRuleUpdateParams {
rules: (0..max + 1).map(rule_n).collect::<Vec<_>>(),
},
StatusCode::BAD_REQUEST,
)
.await;
assert_eq!(error.error_code, Some("InvalidValue".to_string()));
assert_eq!(
error.message,
format!("unsupported value for \"rules\": max length {max}")
);

///////////////////////
// TARGETS
///////////////////////

let target = VpcFirewallRuleTarget::Vpc("default".parse().unwrap());

// fine with max
let rules: VpcFirewallRules = object_put(
client,
&url,
&VpcFirewallRuleUpdateParams {
rules: vec![VpcFirewallRuleUpdate {
targets: (0..max).map(|_i| target.clone()).collect(),
..base_rule.clone()
}],
},
)
.await;
assert_eq!(rules.rules[0].targets.len(), max);

// fails with max + 1
let error = object_put_error(
client,
&url,
&VpcFirewallRuleUpdateParams {
rules: vec![VpcFirewallRuleUpdate {
targets: (0..max + 1).map(|_i| target.clone()).collect(),
..base_rule.clone()
}],
},
StatusCode::BAD_REQUEST,
)
.await;
assert_eq!(error.error_code, Some("InvalidValue".to_string()));
assert_eq!(
error.message,
format!("unsupported value for \"targets\": max length {max}")
);

///////////////////////
// HOST FILTERS
///////////////////////

let host = VpcFirewallRuleHostFilter::Vpc("default".parse().unwrap());

// fine with max
let rules: VpcFirewallRules = object_put(
client,
&url,
&VpcFirewallRuleUpdateParams {
rules: vec![VpcFirewallRuleUpdate {
filters: VpcFirewallRuleFilter {
hosts: Some((0..max).map(|_i| host.clone()).collect()),
protocols: None,
ports: None,
},
..base_rule.clone()
}],
},
)
.await;
assert_eq!(rules.rules[0].filters.hosts.as_ref().unwrap().len(), max);

// fails with max + 1
let error = object_put_error(
client,
&url,
&VpcFirewallRuleUpdateParams {
rules: vec![VpcFirewallRuleUpdate {
filters: VpcFirewallRuleFilter {
hosts: Some((0..max + 1).map(|_i| host.clone()).collect()),
protocols: None,
ports: None,
},
..base_rule.clone()
}],
},
StatusCode::BAD_REQUEST,
)
.await;
assert_eq!(error.error_code, Some("InvalidValue".to_string()));
assert_eq!(
error.message,
format!("unsupported value for \"filters.hosts\": max length {max}")
);

///////////////////////
// PORT FILTERS
///////////////////////

let port: L4PortRange = "1234".parse().unwrap();

// fine with max
let rules: VpcFirewallRules = object_put(
client,
&url,
&VpcFirewallRuleUpdateParams {
rules: vec![VpcFirewallRuleUpdate {
filters: VpcFirewallRuleFilter {
ports: Some((0..max).map(|_i| port.clone()).collect()),
protocols: None,
hosts: None,
},
..base_rule.clone()
}],
},
)
.await;
assert_eq!(rules.rules[0].filters.ports.as_ref().unwrap().len(), max);

// fails with max + 1
let error = object_put_error(
client,
&url,
&VpcFirewallRuleUpdateParams {
rules: vec![VpcFirewallRuleUpdate {
filters: VpcFirewallRuleFilter {
ports: Some((0..max + 1).map(|_i| port.clone()).collect()),
protocols: None,
hosts: None,
},
..base_rule.clone()
}],
},
StatusCode::BAD_REQUEST,
)
.await;
assert_eq!(error.error_code, Some("InvalidValue".to_string()));
assert_eq!(
error.message,
format!("unsupported value for \"filters.ports\": max length {max}")
);

///////////////////////
// PROTOCOL FILTERS
///////////////////////

let protocol = VpcFirewallRuleProtocol::Tcp;

// fine with max
let rules: VpcFirewallRules = object_put(
client,
&url,
&VpcFirewallRuleUpdateParams {
rules: vec![VpcFirewallRuleUpdate {
filters: VpcFirewallRuleFilter {
protocols: Some(
(0..max).map(|_i| protocol.clone()).collect(),
),
ports: None,
hosts: None,
},
..base_rule.clone()
}],
},
)
.await;
assert_eq!(rules.rules[0].filters.protocols.as_ref().unwrap().len(), max);

// fails with max + 1
let error = object_put_error(
client,
&url,
&VpcFirewallRuleUpdateParams {
rules: vec![VpcFirewallRuleUpdate {
filters: VpcFirewallRuleFilter {
protocols: Some(
(0..max + 1).map(|_i| protocol.clone()).collect(),
),
ports: None,
hosts: None,
},
..base_rule.clone()
}],
},
StatusCode::BAD_REQUEST,
)
.await;
assert_eq!(error.error_code, Some("InvalidValue".to_string()));
assert_eq!(
error.message,
format!(
"unsupported value for \"filters.protocols\": max length {max}"
)
);
}

0 comments on commit a4b3ba0

Please sign in to comment.