Skip to content

Commit dbae8fe

Browse files
authored
chore(node): chip_id invariant check (#9218)
NODE-1896 Adds an invariant to the registry to ensure that each node's chip ID is unique.
1 parent f8e7248 commit dbae8fe

File tree

2 files changed

+110
-4
lines changed

2 files changed

+110
-4
lines changed

rs/registry/canister/src/invariants/node_record.rs

Lines changed: 55 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,39 @@
1-
use crate::invariants::common::{InvariantCheckError, RegistrySnapshot, get_all_node_records};
1+
use std::collections::{BTreeMap, HashMap};
2+
3+
use ic_base_types::NodeId;
24
use ic_nns_common::registry::MAX_NUM_SSH_KEYS;
5+
use ic_protobuf::registry::node::v1::NodeRecord;
6+
7+
use crate::invariants::common::{
8+
InvariantCheckError, RegistrySnapshot, get_node_records_from_snapshot,
9+
};
310

411
pub(crate) fn check_node_record_invariants(
512
snapshot: &RegistrySnapshot,
613
) -> Result<(), InvariantCheckError> {
7-
for node_record in get_all_node_records(snapshot) {
14+
let node_records = get_node_records_from_snapshot(snapshot);
15+
16+
check_ssh_key_limits(&node_records)?;
17+
check_chip_ids_are_unique(&node_records)?;
18+
19+
Ok(())
20+
}
21+
22+
fn check_ssh_key_limits(
23+
node_records: &BTreeMap<NodeId, NodeRecord>,
24+
) -> Result<(), InvariantCheckError> {
25+
for node_record in node_records.values() {
826
// Enforce that the ssh_node_state_write_access field does not have too many elements.
927
if node_record.ssh_node_state_write_access.len() > MAX_NUM_SSH_KEYS {
1028
return Err(InvariantCheckError {
1129
msg: format!(
12-
"The `ssh_node_state_write_access` field of a `NodeReocrd` has too many elements. \
13-
{MAX_NUM_SSH_KEYS} is the maximum allowed; whereas, the `NodeRecord` with `chip_id`=\
30+
"The `ssh_node_state_write_access` field of a `NodeRecord` has too many elements. \
31+
{MAX_NUM_SSH_KEYS} is the maximum allowed; whereas, the `NodeRecord` with `http`=\
1432
{} had {} elements \
1533
in this field.",
1634
node_record
1735
.http
36+
.as_ref()
1837
.map(|http| format!("{http:?}"))
1938
.unwrap_or_else(|| "?-UNKNOWN-?".to_string()),
2039
node_record.ssh_node_state_write_access.len()
@@ -27,5 +46,37 @@ pub(crate) fn check_node_record_invariants(
2746
Ok(())
2847
}
2948

49+
fn check_chip_ids_are_unique(
50+
node_records: &BTreeMap<NodeId, NodeRecord>,
51+
) -> Result<(), InvariantCheckError> {
52+
let mut chip_id_to_node: HashMap<Vec<u8>, NodeId> = HashMap::new();
53+
54+
for (node_id, node_record) in node_records {
55+
// Skip nodes without a chip_id (non-SEV nodes), or with an empty one.
56+
// Rejecting a malformed (empty) chip_id is not this check's job; here
57+
// we only care about uniqueness among meaningful values.
58+
let chip_id = match node_record.chip_id {
59+
Some(ref id) if !id.is_empty() => id,
60+
_ => continue,
61+
};
62+
63+
if let Some(prev_node_id) = chip_id_to_node.get(chip_id) {
64+
return Err(InvariantCheckError {
65+
msg: format!(
66+
"chip_id {} is assigned to multiple nodes: {} and {}",
67+
hex::encode(chip_id),
68+
prev_node_id,
69+
node_id,
70+
),
71+
source: None,
72+
});
73+
}
74+
75+
chip_id_to_node.insert(chip_id.clone(), *node_id);
76+
}
77+
78+
Ok(())
79+
}
80+
3081
#[cfg(test)]
3182
mod tests;

rs/registry/canister/src/invariants/node_record/tests.rs

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,3 +54,58 @@ fn test_ssh_node_state_write_access_not_too_long() {
5454
assert!(message.contains(key_word), "{err:?}");
5555
}
5656
}
57+
58+
#[test]
59+
fn test_chip_id_uniqueness_distinct_chip_ids() {
60+
let mut snapshot = RegistrySnapshot::new();
61+
62+
snapshot.insert(
63+
make_node_record_key(NodeId::from(PrincipalId::new_user_test_id(1))).into_bytes(),
64+
NodeRecord {
65+
chip_id: Some(vec![0xAA; 64]),
66+
..Default::default()
67+
}
68+
.encode_to_vec(),
69+
);
70+
snapshot.insert(
71+
make_node_record_key(NodeId::from(PrincipalId::new_user_test_id(2))).into_bytes(),
72+
NodeRecord {
73+
chip_id: Some(vec![0xBB; 64]),
74+
..Default::default()
75+
}
76+
.encode_to_vec(),
77+
);
78+
79+
check_node_record_invariants(&snapshot).unwrap();
80+
}
81+
82+
#[test]
83+
fn test_chip_id_uniqueness_duplicate_chip_ids() {
84+
let mut snapshot = RegistrySnapshot::new();
85+
let duplicate_chip_id = vec![0xCC; 64];
86+
87+
snapshot.insert(
88+
make_node_record_key(NodeId::from(PrincipalId::new_user_test_id(1))).into_bytes(),
89+
NodeRecord {
90+
chip_id: Some(duplicate_chip_id.clone()),
91+
..Default::default()
92+
}
93+
.encode_to_vec(),
94+
);
95+
snapshot.insert(
96+
make_node_record_key(NodeId::from(PrincipalId::new_user_test_id(2))).into_bytes(),
97+
NodeRecord {
98+
chip_id: Some(duplicate_chip_id.clone()),
99+
..Default::default()
100+
}
101+
.encode_to_vec(),
102+
);
103+
104+
let err = check_node_record_invariants(&snapshot).unwrap_err();
105+
assert!(err.msg.contains("multiple nodes"), "{}", err.msg);
106+
assert!(
107+
err.msg.contains(&hex::encode(&duplicate_chip_id)),
108+
"{}",
109+
err.msg
110+
);
111+
}

0 commit comments

Comments
 (0)