feat: Allow changing subnet_admins via UpdateSubnetPayload

rs/nns/integration_tests/src/subnet_handler.rs

@@ -96,6 +96,7 @@ fn test_submit_and_accept_update_subnet_proposal() {
                 max_number_of_canisters: Some(200),
                 ssh_readonly_access: Some(vec!["pub_key_0".to_string()]),
                 ssh_backup_access: Some(vec!["pub_key_1".to_string()]),
+                subnet_admins: None,
                 chain_key_config: None,
                 chain_key_signing_enable: None,
                 chain_key_signing_disable: None,

rs/registry/admin/bin/update_subnet.rs

@@ -13,7 +13,7 @@ use ic_nns_common::types::NeuronId;
 use ic_registry_nns_data_provider::registry::RegistryCanister;
 use ic_registry_resource_limits::ResourceLimits;
 use ic_registry_subnet_features::SubnetFeatures;
-use ic_types::SubnetId;
+use ic_types::{PrincipalId, SubnetId};
 use registry_canister::mutations::do_update_subnet;
 use std::collections::BTreeMap;
 use std::{collections::HashSet, path::PathBuf};
@@ -175,6 +175,12 @@ pub(crate) struct ProposeToUpdateSubnetCmd {
     #[clap(long, num_args(1..))]
     ssh_backup_access: Option<Vec<String>>,
 
+    /// If set, replaces the subnet's admin list with the given principals
+    /// (passing zero principals clears the list). When unset, the existing
+    /// admin list is left unchanged.
+    #[clap(long, num_args(0..))]
+    pub subnet_admins: Option<Vec<PrincipalId>>,
+
     /// If set, the created proposal will contain a desired override of that
     /// field to the value set. See `ProposeToCreateSubnetCmd` for the semantic
     /// of this field.
@@ -374,6 +380,7 @@ impl ProposeToUpdateSubnetCmd {
 
             ssh_readonly_access: self.ssh_readonly_access.clone(),
             ssh_backup_access: self.ssh_backup_access.clone(),
+            subnet_admins: self.subnet_admins.clone(),
             max_number_of_canisters: self.max_number_of_canisters,
 
             chain_key_config,
@@ -443,6 +450,7 @@ mod tests {
             max_number_of_canisters: None,
             ssh_readonly_access: None,
             ssh_backup_access: None,
+            subnet_admins: None,
             chain_key_config: None,
             chain_key_signing_enable: None,
             chain_key_signing_disable: None,
@@ -481,6 +489,7 @@ mod tests {
             resource_limits: None,
             ssh_readonly_access: None,
             ssh_backup_access: None,
+            subnet_admins: None,
             max_number_of_canisters: None,
         }
     }
@@ -556,6 +565,7 @@ mod tests {
         assert_eq!(
             cmd.new_payload_for_subnet(subnet_id, subnet_record),
             do_update_subnet::UpdateSubnetPayload {
+                subnet_admins: None,
                 chain_key_config: Some(do_update_subnet::ChainKeyConfig {
                     // Note the order is not important so long as it is deterministic. As a matter
                     // of fact, the order is lexicographic, as the implementation uses `BTreeSet`.
@@ -666,6 +676,7 @@ mod tests {
         assert_eq!(
             cmd.new_payload_for_subnet(subnet_id, subnet_record),
             do_update_subnet::UpdateSubnetPayload {
+                subnet_admins: None,
                 chain_key_config: Some(do_update_subnet::ChainKeyConfig {
                     key_configs: vec![
                         do_update_subnet::KeyConfig {
@@ -757,6 +768,7 @@ mod tests {
         assert_eq!(
             cmd.new_payload_for_subnet(subnet_id, subnet_record),
             do_update_subnet::UpdateSubnetPayload {
+                subnet_admins: None,
                 chain_key_config: Some(do_update_subnet::ChainKeyConfig {
                     key_configs: vec![
                         // Existed before, now being updated.
@@ -847,6 +859,7 @@ mod tests {
             do_update_subnet::UpdateSubnetPayload {
                 resource_limits: expected_resource_limits
                     .map(|resource_limits| resource_limits.into()),
+                subnet_admins: None,
                 ..make_empty_update_payload(subnet_id)
             },
         );

rs/registry/canister/canister/registry.did

@@ -486,6 +486,7 @@ type UpdateSubnetPayload = record {
   max_artifact_streams_per_peer : opt nat32;
   subnet_type : opt SubnetType;
   ssh_readonly_access : opt vec text;
+  subnet_admins : opt vec principal;
   chain_key_config : opt ChainKeyConfig;
   chain_key_signing_enable : opt vec MasterPublicKeyId;
   chain_key_signing_disable : opt vec MasterPublicKeyId;

rs/registry/canister/canister/registry_test.did

@@ -486,6 +486,7 @@ type UpdateSubnetPayload = record {
   max_artifact_streams_per_peer : opt nat32;
   subnet_type : opt SubnetType;
   ssh_readonly_access : opt vec text;
+  subnet_admins : opt vec principal;
   chain_key_config : opt ChainKeyConfig;
   chain_key_signing_enable : opt vec MasterPublicKeyId;
   chain_key_signing_disable : opt vec MasterPublicKeyId;

rs/registry/canister/src/invariants/mod.rs

@@ -10,5 +10,5 @@ mod node_operator;
 mod node_record;
 mod replica_version;
 mod routing_table;
-mod subnet;
+pub(crate) mod subnet;
 mod unassigned_nodes_config;

rs/registry/canister/src/invariants/subnet.rs

@@ -17,6 +17,9 @@ use ic_protobuf::registry::{
 use ic_registry_keys::{SUBNET_RECORD_KEY_PREFIX, make_subnet_record_key};
 use prost::Message;
 
+/// The maximum number of subnet admins a subnet may have.
+pub const MAX_SUBNET_ADMINS: usize = 10;
+
 /// Subnet invariants hold iff:
 ///    * Each SSH key access list does not contain more than 50 keys
 ///    * Subnet membership contains no repetition
@@ -30,7 +33,8 @@ use prost::Message;
 ///         * consist of nodes with reward type 4
 ///    * Conversely, only cloud engines can have nodes with reward type 4
 ///    * SEV-enabled subnets consist of SEV-enabled nodes only (i.e. nodes with a chip ID in the node record)
-///    * Only rented subnets can have subnet admins set to a non-empty list
+///    * Only rented or cloud engine subnets can have subnet admins set to a non-empty list
+///    * No subnet has more than `MAX_SUBNET_ADMINS` subnet admins
 pub(crate) fn check_subnet_invariants(
     snapshot: &RegistrySnapshot,
 ) -> Result<(), InvariantCheckError> {
@@ -176,7 +180,8 @@ pub(crate) fn check_subnet_invariants(
     Ok(())
 }
 
-// Checks that only rented subnets or cloud engine subnets can have admins.
+// Checks that only rented subnets or cloud engine subnets can have admins, and
+// that no subnet has more than `MAX_SUBNET_ADMINS` admins.
 fn check_subnet_admins_invariant(
     subnet_record: &SubnetRecord,
     subnet_id: SubnetId,
@@ -202,6 +207,17 @@ fn check_subnet_admins_invariant(
             source: None,
         });
     }
+
+    if subnet_record.subnet_admins.len() > MAX_SUBNET_ADMINS {
+        return Err(InvariantCheckError {
+            msg: format!(
+                "Subnet {subnet_id:} has {} subnet admins, which exceeds the maximum of {MAX_SUBNET_ADMINS}",
+                subnet_record.subnet_admins.len()
+            ),
+            source: None,
+        });
+    }
+
     Ok(())
 }
 

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

@@ -270,6 +270,43 @@ fn non_rented_application_subnets_cannot_have_subnet_admins() {
     );
 }
 
+#[test]
+fn subnets_cannot_have_more_than_max_subnet_admins() {
+    let system_subnet_id = subnet_test_id(1);
+    let test_subnet_id = subnet_test_id(2);
+    let (mut snapshot, mut test_subnet_record) =
+        setup_minimal_registry_snapshot_for_check_subnet_invariants(
+            system_subnet_id,
+            test_subnet_id,
+            1,     // num_nodes_in_test_subnet
+            false, // with_chip_id
+        );
+
+    // Make the subnet rented so the type/cost-schedule check passes.
+    test_subnet_record.subnet_type = i32::from(SubnetType::Application);
+    test_subnet_record.canister_cycles_cost_schedule = i32::from(CanisterCyclesCostSchedule::Free);
+
+    // Exactly `MAX_SUBNET_ADMINS` admins is allowed.
+    test_subnet_record.subnet_admins = (0..MAX_SUBNET_ADMINS as u64)
+        .map(|i| PrincipalIdPb::from(user_test_id(i).get()))
+        .collect();
+    snapshot.insert(
+        make_subnet_record_key(test_subnet_id).into_bytes(),
+        test_subnet_record.encode_to_vec(),
+    );
+    check_subnet_invariants(&snapshot).unwrap();
+
+    // One more than `MAX_SUBNET_ADMINS` is not.
+    test_subnet_record.subnet_admins = (0..(MAX_SUBNET_ADMINS as u64 + 1))
+        .map(|i| PrincipalIdPb::from(user_test_id(i).get()))
+        .collect();
+    snapshot.insert(
+        make_subnet_record_key(test_subnet_id).into_bytes(),
+        test_subnet_record.encode_to_vec(),
+    );
+    assert_non_compliant_record(&snapshot, "subnet admins, which exceeds the maximum of");
+}
+
 #[test]
 fn cloud_engine_subnets_must_have_type4_nodes() {
     let system_subnet_id = subnet_test_id(1);

rs/registry/canister/src/lib.rs

@@ -12,5 +12,5 @@ pub mod registry;
 pub mod registry_lifecycle;
 pub mod storage;
 
-mod invariants;
+pub(crate) mod invariants;
 mod rate_limits;