From 080dcc0add0beacd5d11ef5a7f4c9a384c3a110d Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Mon, 30 Mar 2026 22:05:43 +0000 Subject: [PATCH 1/4] feat: allow direct changes to the subnet admins for cloud engines --- rs/registry/canister/canister/canister.rs | 18 +- .../src/mutations/do_update_subnet_admins.rs | 354 +++++++++++++++--- .../canister/tests/update_subnet_admins.rs | 6 +- 3 files changed, 311 insertions(+), 67 deletions(-) diff --git a/rs/registry/canister/canister/canister.rs b/rs/registry/canister/canister/canister.rs index 2483aa72935b..80a46ee5467c 100644 --- a/rs/registry/canister/canister/canister.rs +++ b/rs/registry/canister/canister/canister.rs @@ -7,9 +7,7 @@ use dfn_core::{ use ic_base_types::{NodeId, PrincipalId}; use ic_certified_map::{AsHashTree, HashTree}; use ic_nervous_system_string::clamp_debug_len; -use ic_nns_constants::{ - GOVERNANCE_CANISTER_ID, MIGRATION_CANISTER_ID, ROOT_CANISTER_ID, SUBNET_RENTAL_CANISTER_ID, -}; +use ic_nns_constants::{GOVERNANCE_CANISTER_ID, MIGRATION_CANISTER_ID, ROOT_CANISTER_ID}; use ic_protobuf::registry::{ dc::v1::{AddOrRemoveDataCentersProposalPayload, DataCenterRecord}, node_operator::v1::NodeOperatorRecord, @@ -141,16 +139,6 @@ fn check_caller_is_canister_migration_orchestrator_and_log(method_name: &str) { ); } -fn check_caller_is_subnet_rental_canister_and_log(method_name: &str) { - let caller = dfn_core::api::caller(); - println!("{LOG_PREFIX}call: {method_name} from: {caller}"); - assert_eq!( - caller, - SUBNET_RENTAL_CANISTER_ID.into(), - "{LOG_PREFIX}Principal: {caller} is not authorized to call this method: {method_name}" - ); -} - /// Initializes the registry. /// /// The argument is expected to be a candid-encoded @@ -1296,13 +1284,13 @@ fn set_subnet_operational_level_(payload: SetSubnetOperationalLevelPayload) { #[unsafe(export_name = "canister_update update_subnet_admins")] fn update_subnet_admins() { - check_caller_is_subnet_rental_canister_and_log("update_subnet_admins"); over(candid_one, update_subnet_admins_); } #[candid_method(update, rename = "update_subnet_admins")] fn update_subnet_admins_(payload: UpdateSubnetAdminsPayload) { - registry_mut().do_update_subnet_admins(payload); + let caller = dfn_core::api::caller(); + registry_mut().do_update_subnet_admins(caller, payload); recertify_registry(); } diff --git a/rs/registry/canister/src/mutations/do_update_subnet_admins.rs b/rs/registry/canister/src/mutations/do_update_subnet_admins.rs index e00d88199f4c..a61bda00ff4c 100644 --- a/rs/registry/canister/src/mutations/do_update_subnet_admins.rs +++ b/rs/registry/canister/src/mutations/do_update_subnet_admins.rs @@ -2,6 +2,7 @@ use crate::{common::LOG_PREFIX, registry::Registry}; use candid::{CandidType, Deserialize}; use ic_base_types::{PrincipalId, SubnetId}; use ic_nervous_system_time_helpers::now_system_time; +use ic_nns_constants::SUBNET_RENTAL_CANISTER_ID; use ic_protobuf::{ registry::subnet::v1::CanisterCyclesCostSchedule, types::v1::PrincipalId as PrincipalIdPb, }; @@ -48,6 +49,14 @@ pub enum UpdateSubnetAdminsError { RateLimited { subnet_id: SubnetId, }, + CallerNotAuthorized { + caller: PrincipalId, + subnet_id: SubnetId, + }, + UnsupportedSubnetType { + subnet_id: SubnetId, + subnet_type: i32, + }, } impl std::fmt::Display for UpdateSubnetAdminsError { @@ -77,6 +86,22 @@ impl std::fmt::Display for UpdateSubnetAdminsError { short period of time. Please try again later." ) } + UpdateSubnetAdminsError::CallerNotAuthorized { caller, subnet_id } => { + write!( + f, + "Caller {caller} is not authorized to update subnet admins of subnet {subnet_id}." + ) + } + UpdateSubnetAdminsError::UnsupportedSubnetType { + subnet_id, + subnet_type, + } => { + write!( + f, + "Subnet {subnet_id} has unsupported subnet type {subnet_type} for subnet admin updates. \ + Only Application and CloudEngine subnets are supported." + ) + } } } } @@ -86,7 +111,11 @@ thread_local! { } impl Registry { - pub fn do_update_subnet_admins(&mut self, payload: UpdateSubnetAdminsPayload) { + pub fn do_update_subnet_admins( + &mut self, + caller: PrincipalId, + payload: UpdateSubnetAdminsPayload, + ) { println!("{}do_update_subnet_admins: {:?}", LOG_PREFIX, payload); let subnet_id = payload.subnet_id; @@ -103,20 +132,44 @@ impl Registry { let mut subnet_record = self.get_subnet_or_panic(subnet_id); - // Check pre-conditions that a subnet is rented before allowing subnet admin updates. - // The check is based on the expectation that a rented subnet must be an application - // subnet and must be on a "free" cycles cost schedule. - assert_eq!( - subnet_record.subnet_type, - i32::from(SubnetType::Application), - "Only application subnets are expected to be rented or have subnet admins." - ); + // Only subnets on a free cycles cost schedule are expected to have subnet admins. assert_eq!( subnet_record.canister_cycles_cost_schedule, i32::from(CanisterCyclesCostSchedule::Free), - "Only rented subnets, which are expected to be on a free cycles cost schedule, are expected to have subnet admins." + "Only subnets on a free cycles cost schedule are expected to have subnet admins." ); + // Authorization depends on subnet type: + // - Application subnets: caller must be the subnet rental canister. + // - CloudEngine subnets: caller must be one of the existing subnet admins. + if subnet_record.subnet_type == i32::from(SubnetType::Application) { + assert_eq!( + caller, + PrincipalId::from(SUBNET_RENTAL_CANISTER_ID), + "{}do_update_subnet_admins: {}", + LOG_PREFIX, + UpdateSubnetAdminsError::CallerNotAuthorized { caller, subnet_id }, + ); + } else if subnet_record.subnet_type == i32::from(SubnetType::CloudEngine) { + assert!( + subnet_record + .subnet_admins + .contains(&PrincipalIdPb::from(caller)), + "{}do_update_subnet_admins: {}", + LOG_PREFIX, + UpdateSubnetAdminsError::CallerNotAuthorized { caller, subnet_id }, + ); + } else { + panic!( + "{}do_update_subnet_admins: {}", + LOG_PREFIX, + UpdateSubnetAdminsError::UnsupportedSubnetType { + subnet_id, + subnet_type: subnet_record.subnet_type, + }, + ); + } + let current_subnet_admins = subnet_record.subnet_admins; let res = match payload.operation_type { @@ -229,7 +282,15 @@ mod tests { use maplit::btreemap; use pretty_assertions::assert_eq; - fn prepare_registry_for_update_subnet_admins_test(subnet_id: SubnetId) -> Registry { + fn subnet_rental_canister_caller() -> PrincipalId { + PrincipalId::from(SUBNET_RENTAL_CANISTER_ID) + } + + fn prepare_registry_for_update_subnet_admins_test( + subnet_id: SubnetId, + subnet_type: SubnetType, + initial_subnet_admins: Vec, + ) -> Registry { let mut registry = invariant_compliant_registry(0); let (mutate_request, node_ids_and_dkg_pks) = prepare_registry_with_nodes(1, 2); registry.maybe_apply_mutation_internal(mutate_request.mutations); @@ -240,9 +301,9 @@ mod tests { .expect("should contain at least one node ID and key"); let mut subnet_record = get_invariant_compliant_subnet_record(vec![*first_node_id]); - // Ensure subnet is considered rented. - subnet_record.subnet_type = i32::from(SubnetType::Application); + subnet_record.subnet_type = i32::from(subnet_type); subnet_record.canister_cycles_cost_schedule = i32::from(CanisterCyclesCostSchedule::Free); + subnet_record.subnet_admins = initial_subnet_admins; registry.maybe_apply_mutation_internal(add_fake_subnet( subnet_id, @@ -274,9 +335,14 @@ mod tests { } #[test] - fn can_add_or_remove_subnet_admins() { + fn application_subnet_can_add_or_remove_subnet_admins() { let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test(subnet_id); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let caller = subnet_rental_canister_caller(); let user1 = user_test_id(100).get(); let user2 = user_test_id(101).get(); @@ -287,7 +353,7 @@ mod tests { operation_type: Some(OperationType::Add(vec![user1, user2])), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); assert_updated_subnet_admins_match_expected( ®istry.get_subnet_or_panic(subnet_id).subnet_admins, @@ -300,7 +366,7 @@ mod tests { operation_type: Some(OperationType::Remove(vec![user1])), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); assert_updated_subnet_admins_match_expected( ®istry.get_subnet_or_panic(subnet_id).subnet_admins, &[PrincipalIdPb::from(user2)], @@ -308,9 +374,14 @@ mod tests { } #[test] - fn can_clear_subnet_admins() { + fn application_subnet_can_clear_subnet_admins() { let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test(subnet_id); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let caller = subnet_rental_canister_caller(); let user1 = user_test_id(100).get(); let user2 = user_test_id(101).get(); @@ -320,7 +391,7 @@ mod tests { operation_type: Some(OperationType::Add(vec![user1, user2, user3])), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); let updated_subnet_admins = registry.get_subnet_or_panic(subnet_id).subnet_admins; let expected_subnet_admins = vec![ @@ -338,7 +409,7 @@ mod tests { operation_type: Some(OperationType::Clear(EmptyRecord {})), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); assert_eq!( registry.get_subnet_or_panic(subnet_id).subnet_admins, @@ -348,36 +419,51 @@ mod tests { #[test] #[should_panic(expected = "The list of provided principals cannot be empty")] - fn can_not_add_or_remove_empty_list_of_subnet_admins() { + fn application_subnet_can_not_add_empty_list_of_subnet_admins() { let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test(subnet_id); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let caller = subnet_rental_canister_caller(); let payload = UpdateSubnetAdminsPayload { subnet_id, operation_type: Some(OperationType::Add(vec![])), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); } #[test] #[should_panic(expected = "The list of provided principals cannot be empty")] - fn can_not_remove_empty_list_of_subnet_admins() { + fn application_subnet_can_not_remove_empty_list_of_subnet_admins() { let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test(subnet_id); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let caller = subnet_rental_canister_caller(); let payload = UpdateSubnetAdminsPayload { subnet_id, operation_type: Some(OperationType::Remove(vec![])), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); } #[test] - fn can_dedup_input_when_adding_or_removing_subnet_admins() { + fn application_subnet_can_dedup_input_when_adding_or_removing_subnet_admins() { let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test(subnet_id); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let caller = subnet_rental_canister_caller(); let user1 = user_test_id(100).get(); let payload = UpdateSubnetAdminsPayload { @@ -385,7 +471,7 @@ mod tests { operation_type: Some(OperationType::Add(vec![user1, user1])), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); assert_eq!( registry.get_subnet_or_panic(subnet_id).subnet_admins, vec![PrincipalIdPb::from(user1)] @@ -396,7 +482,7 @@ mod tests { operation_type: Some(OperationType::Remove(vec![user1, user1])), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); assert_eq!( registry.get_subnet_or_panic(subnet_id).subnet_admins, vec![] @@ -407,9 +493,14 @@ mod tests { #[should_panic( expected = "Too many subnet admins. Provided: 11, Existing: 0, Max allowed: 10." )] - fn can_not_add_too_many_subnet_admins_no_existing_ones() { + fn application_subnet_can_not_add_too_many_subnet_admins_no_existing_ones() { let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test(subnet_id); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let caller = subnet_rental_canister_caller(); let mut users_to_add = Vec::new(); for i in 0..(MAX_SUBNET_ADMINS + 1) { @@ -421,14 +512,19 @@ mod tests { operation_type: Some(OperationType::Add(users_to_add)), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); } #[test] #[should_panic(expected = "Too many subnet admins. Provided: 3, Existing: 9, Max allowed: 10.")] - fn can_not_add_too_many_subnet_admins_with_existing_ones() { + fn application_subnet_can_not_add_too_many_subnet_admins_with_existing_ones() { let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test(subnet_id); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let caller = subnet_rental_canister_caller(); let mut users_to_add = Vec::new(); let mut expected_subnet_admins = Vec::new(); @@ -442,7 +538,7 @@ mod tests { subnet_id, operation_type: Some(OperationType::Add(users_to_add)), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); assert_updated_subnet_admins_match_expected( ®istry.get_subnet_or_panic(subnet_id).subnet_admins, &expected_subnet_admins, @@ -458,16 +554,21 @@ mod tests { operation_type: Some(OperationType::Add(users_to_add)), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); } #[test] #[should_panic( expected = "Too many subnet admins. Provided: 11, Existing: 0, Max allowed: 10." )] - fn can_not_remove_too_many_subnet_admins() { + fn application_subnet_can_not_remove_too_many_subnet_admins() { let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test(subnet_id); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let caller = subnet_rental_canister_caller(); let mut users_to_remove = Vec::new(); for i in 0..(MAX_SUBNET_ADMINS + 1) { @@ -479,13 +580,18 @@ mod tests { operation_type: Some(OperationType::Remove(users_to_remove)), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); } #[test] - fn can_not_add_existing_subnet_admins() { + fn application_subnet_can_not_add_existing_subnet_admins() { let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test(subnet_id); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let caller = subnet_rental_canister_caller(); let user1 = user_test_id(100).get(); let payload = UpdateSubnetAdminsPayload { @@ -493,7 +599,7 @@ mod tests { operation_type: Some(OperationType::Add(vec![user1])), }; - registry.do_update_subnet_admins(payload.clone()); + registry.do_update_subnet_admins(caller, payload.clone()); let expected_subnet_admins = vec![PrincipalIdPb::from(user1)]; assert_updated_subnet_admins_match_expected( ®istry.get_subnet_or_panic(subnet_id).subnet_admins, @@ -501,7 +607,7 @@ mod tests { ); // Attempt to add the same user again. Should be a no-op. - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); assert_updated_subnet_admins_match_expected( ®istry.get_subnet_or_panic(subnet_id).subnet_admins, &expected_subnet_admins, @@ -509,9 +615,14 @@ mod tests { } #[test] - fn can_not_remove_non_existing_subnet_admins() { + fn application_subnet_can_not_remove_non_existing_subnet_admins() { let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test(subnet_id); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let caller = subnet_rental_canister_caller(); let user1 = user_test_id(100).get(); let user2 = user_test_id(101).get(); @@ -519,7 +630,7 @@ mod tests { subnet_id, operation_type: Some(OperationType::Add(vec![user1, user2])), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); let expected_subnet_admins = vec![PrincipalIdPb::from(user1), PrincipalIdPb::from(user2)]; // Attempt to remove a user that is not in the subnet admins list. Should be a no-op. @@ -527,10 +638,155 @@ mod tests { subnet_id, operation_type: Some(OperationType::Remove(vec![user_test_id(200).get()])), }; - registry.do_update_subnet_admins(payload); + registry.do_update_subnet_admins(caller, payload); assert_updated_subnet_admins_match_expected( ®istry.get_subnet_or_panic(subnet_id).subnet_admins, &expected_subnet_admins, ); } + + #[test] + #[should_panic(expected = "is not authorized to update subnet admins")] + fn application_subnet_rejects_non_rental_canister_caller() { + let subnet_id = subnet_test_id(1); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let unauthorized_caller = user_test_id(999).get(); + + let payload = UpdateSubnetAdminsPayload { + subnet_id, + operation_type: Some(OperationType::Add(vec![user_test_id(100).get()])), + }; + + registry.do_update_subnet_admins(unauthorized_caller, payload); + } + + #[test] + fn cloud_engine_subnet_admin_can_add_or_remove_subnet_admins() { + let subnet_id = subnet_test_id(1); + let admin = user_test_id(100).get(); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::CloudEngine, + vec![PrincipalIdPb::from(admin)], + ); + + let user2 = user_test_id(101).get(); + + // Admin adds another subnet admin. + let payload = UpdateSubnetAdminsPayload { + subnet_id, + operation_type: Some(OperationType::Add(vec![user2])), + }; + + registry.do_update_subnet_admins(admin, payload); + + assert_updated_subnet_admins_match_expected( + ®istry.get_subnet_or_panic(subnet_id).subnet_admins, + &[PrincipalIdPb::from(admin), PrincipalIdPb::from(user2)], + ); + + // Admin removes the other subnet admin. + let payload = UpdateSubnetAdminsPayload { + subnet_id, + operation_type: Some(OperationType::Remove(vec![user2])), + }; + + registry.do_update_subnet_admins(admin, payload); + assert_updated_subnet_admins_match_expected( + ®istry.get_subnet_or_panic(subnet_id).subnet_admins, + &[PrincipalIdPb::from(admin)], + ); + } + + #[test] + fn cloud_engine_subnet_admin_can_clear_subnet_admins() { + let subnet_id = subnet_test_id(1); + let admin = user_test_id(100).get(); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::CloudEngine, + vec![PrincipalIdPb::from(admin)], + ); + + let payload = UpdateSubnetAdminsPayload { + subnet_id, + operation_type: Some(OperationType::Clear(EmptyRecord {})), + }; + + registry.do_update_subnet_admins(admin, payload); + + assert_eq!( + registry.get_subnet_or_panic(subnet_id).subnet_admins, + vec![] + ); + } + + #[test] + #[should_panic(expected = "is not authorized to update subnet admins")] + fn cloud_engine_subnet_rejects_non_admin_caller() { + let subnet_id = subnet_test_id(1); + let admin = user_test_id(100).get(); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::CloudEngine, + vec![PrincipalIdPb::from(admin)], + ); + let non_admin = user_test_id(999).get(); + + let payload = UpdateSubnetAdminsPayload { + subnet_id, + operation_type: Some(OperationType::Add(vec![user_test_id(200).get()])), + }; + + registry.do_update_subnet_admins(non_admin, payload); + } + + #[test] + #[should_panic(expected = "is not authorized to update subnet admins")] + fn cloud_engine_subnet_rejects_subnet_rental_canister_caller() { + let subnet_id = subnet_test_id(1); + let admin = user_test_id(100).get(); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::CloudEngine, + vec![PrincipalIdPb::from(admin)], + ); + + let payload = UpdateSubnetAdminsPayload { + subnet_id, + operation_type: Some(OperationType::Add(vec![user_test_id(200).get()])), + }; + + registry.do_update_subnet_admins(subnet_rental_canister_caller(), payload); + } + + #[test] + #[should_panic(expected = "Only subnets on a free cycles cost schedule")] + fn non_free_cycles_schedule_rejects_subnet_admin_updates() { + let subnet_id = subnet_test_id(1); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + + // Override the cycles cost schedule to Normal to simulate a non-rented subnet. + let mut subnet_record = registry.get_subnet_or_panic(subnet_id); + subnet_record.canister_cycles_cost_schedule = i32::from(CanisterCyclesCostSchedule::Normal); + registry.maybe_apply_mutation_internal(vec![upsert( + make_subnet_record_key(subnet_id).into_bytes(), + subnet_record.encode_to_vec(), + )]); + + let payload = UpdateSubnetAdminsPayload { + subnet_id, + operation_type: Some(OperationType::Add(vec![user_test_id(100).get()])), + }; + + registry.do_update_subnet_admins(subnet_rental_canister_caller(), payload); + } } diff --git a/rs/registry/canister/tests/update_subnet_admins.rs b/rs/registry/canister/tests/update_subnet_admins.rs index d458f549785a..9b1527fedcc5 100644 --- a/rs/registry/canister/tests/update_subnet_admins.rs +++ b/rs/registry/canister/tests/update_subnet_admins.rs @@ -46,12 +46,12 @@ fn test_the_anonymous_user_cannot_update_a_subnets_subnet_admins() { }; // The anonymous end-user tries to update a subnet's subnet admins, bypassing - // the subnet rental canister. This should be rejected. + // the subnet rental canister. This should be rejected because the default + // test subnet is a System subnet which is not on a free cycles cost schedule. let response: Result<(), String> = registry .update_("update_subnet_admins", candid, (payload.clone(),)) .await; - assert_matches!(response, - Err(s) if s.contains("is not authorized to call this method: update_subnet_admins")); + assert_matches!(response, Err(_)); // .. And no change should have happened to the subnet's subnet admins let subnet_admins = get_value_or_panic::( From a694f9b410e202ff6e0af752b505353270e61ed5 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Mon, 30 Mar 2026 22:06:52 +0000 Subject: [PATCH 2/4] add unreleased changes --- rs/registry/canister/unreleased_changelog.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rs/registry/canister/unreleased_changelog.md b/rs/registry/canister/unreleased_changelog.md index 38a5a40421b9..7de2f94a0048 100644 --- a/rs/registry/canister/unreleased_changelog.md +++ b/rs/registry/canister/unreleased_changelog.md @@ -9,12 +9,13 @@ on the process that this file is part of, see ## Added -* Subnet deletion endpoint. Limited to CloudEngine subnets. +* Subnet deletion endpoint. Limited to CloudEngine subnets. * Implemented the `do_split_subnet` method * Added an optional field `maximum_state_delta` to `ResourceLimits` in `CreateSubnetPayload` which, when present, sets a soft limit on the maximum (replicated) state *delta* (kept in main memory) in bytes. * Added an optional field `resource_limits` to `UpdateSubnetPayload` which, when present, sets all subnet resource limits to the provided values. +* Allow direct changes for `subnet_admins` for CloudEngine if the caller is one of the subnet admins. ## Changed From 342a3505371bd836a8045ba43428dfa29641c897 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Tue, 31 Mar 2026 21:24:24 +0000 Subject: [PATCH 3/4] suggestions --- .../src/mutations/do_update_subnet_admins.rs | 406 +++++++++++------- .../canister/tests/update_subnet_admins.rs | 3 +- 2 files changed, 255 insertions(+), 154 deletions(-) diff --git a/rs/registry/canister/src/mutations/do_update_subnet_admins.rs b/rs/registry/canister/src/mutations/do_update_subnet_admins.rs index a61bda00ff4c..a2e1ec26cee4 100644 --- a/rs/registry/canister/src/mutations/do_update_subnet_admins.rs +++ b/rs/registry/canister/src/mutations/do_update_subnet_admins.rs @@ -4,7 +4,8 @@ use ic_base_types::{PrincipalId, SubnetId}; use ic_nervous_system_time_helpers::now_system_time; use ic_nns_constants::SUBNET_RENTAL_CANISTER_ID; use ic_protobuf::{ - registry::subnet::v1::CanisterCyclesCostSchedule, types::v1::PrincipalId as PrincipalIdPb, + registry::subnet::v1::{CanisterCyclesCostSchedule, SubnetRecord}, + types::v1::PrincipalId as PrincipalIdPb, }; use ic_registry_keys::make_subnet_record_key; use ic_registry_subnet_type::SubnetType; @@ -57,6 +58,9 @@ pub enum UpdateSubnetAdminsError { subnet_id: SubnetId, subnet_type: i32, }, + WouldRemoveAllCloudEngineAdmins { + subnet_id: SubnetId, + }, } impl std::fmt::Display for UpdateSubnetAdminsError { @@ -102,6 +106,13 @@ impl std::fmt::Display for UpdateSubnetAdminsError { Only Application and CloudEngine subnets are supported." ) } + UpdateSubnetAdminsError::WouldRemoveAllCloudEngineAdmins { subnet_id } => { + write!( + f, + "Cannot remove all subnet admins from CloudEngine subnet {subnet_id}. \ + At least one admin must remain." + ) + } } } } @@ -132,43 +143,7 @@ impl Registry { let mut subnet_record = self.get_subnet_or_panic(subnet_id); - // Only subnets on a free cycles cost schedule are expected to have subnet admins. - assert_eq!( - subnet_record.canister_cycles_cost_schedule, - i32::from(CanisterCyclesCostSchedule::Free), - "Only subnets on a free cycles cost schedule are expected to have subnet admins." - ); - - // Authorization depends on subnet type: - // - Application subnets: caller must be the subnet rental canister. - // - CloudEngine subnets: caller must be one of the existing subnet admins. - if subnet_record.subnet_type == i32::from(SubnetType::Application) { - assert_eq!( - caller, - PrincipalId::from(SUBNET_RENTAL_CANISTER_ID), - "{}do_update_subnet_admins: {}", - LOG_PREFIX, - UpdateSubnetAdminsError::CallerNotAuthorized { caller, subnet_id }, - ); - } else if subnet_record.subnet_type == i32::from(SubnetType::CloudEngine) { - assert!( - subnet_record - .subnet_admins - .contains(&PrincipalIdPb::from(caller)), - "{}do_update_subnet_admins: {}", - LOG_PREFIX, - UpdateSubnetAdminsError::CallerNotAuthorized { caller, subnet_id }, - ); - } else { - panic!( - "{}do_update_subnet_admins: {}", - LOG_PREFIX, - UpdateSubnetAdminsError::UnsupportedSubnetType { - subnet_id, - subnet_type: subnet_record.subnet_type, - }, - ); - } + self.check_update_subnet_admins_caller_authorization(caller, &subnet_record, subnet_id); let current_subnet_admins = subnet_record.subnet_admins; @@ -176,21 +151,31 @@ impl Registry { Some(operation_type) => { match self.compute_new_subnet_admins(current_subnet_admins, operation_type) { Ok(new_subnet_admins) => { - subnet_record.subnet_admins = new_subnet_admins; - - let subnet_record_mutation = upsert( - make_subnet_record_key(subnet_id).into_bytes(), - subnet_record.encode_to_vec(), - ); - let mutations = vec![subnet_record_mutation]; - - // Check invariants before applying mutations - self.maybe_apply_mutation_internal(mutations); - - UPDATE_SUBNET_ADMINS_RATE_LIMITER - .with_borrow_mut(|limiter| limiter.commit(reservation, now)); - - Ok(()) + // CloudEngine subnets must always have at least one admin, + // otherwise there is no way to recover. + if subnet_record.subnet_type == i32::from(SubnetType::CloudEngine) + && new_subnet_admins.is_empty() + { + Err(UpdateSubnetAdminsError::WouldRemoveAllCloudEngineAdmins { + subnet_id, + }) + } else { + subnet_record.subnet_admins = new_subnet_admins; + + let subnet_record_mutation = upsert( + make_subnet_record_key(subnet_id).into_bytes(), + subnet_record.encode_to_vec(), + ); + let mutations = vec![subnet_record_mutation]; + + // Check invariants before applying mutations + self.maybe_apply_mutation_internal(mutations); + + UPDATE_SUBNET_ADMINS_RATE_LIMITER + .with_borrow_mut(|limiter| limiter.commit(reservation, now)); + + Ok(()) + } } Err(err) => Err(err), } @@ -208,6 +193,51 @@ impl Registry { } } + fn check_update_subnet_admins_caller_authorization( + &self, + caller: PrincipalId, + subnet_record: &SubnetRecord, + subnet_id: SubnetId, + ) { + assert_eq!( + subnet_record.canister_cycles_cost_schedule, + i32::from(CanisterCyclesCostSchedule::Free), + "Only subnets on a free cycles cost schedule are allowed to have subnet admins." + ); + + // Authorization depends on subnet type: + // - Application subnets: caller must be the subnet rental canister. + // - CloudEngine subnets: caller must be one of the existing subnet admins. + match SubnetType::try_from(subnet_record.subnet_type) { + Ok(SubnetType::Application) => { + assert_eq!( + caller, + PrincipalId::from(SUBNET_RENTAL_CANISTER_ID), + "{}", + UpdateSubnetAdminsError::CallerNotAuthorized { caller, subnet_id }, + ); + } + Ok(SubnetType::CloudEngine) => { + assert!( + subnet_record + .subnet_admins + .contains(&PrincipalIdPb::from(caller)), + "{}", + UpdateSubnetAdminsError::CallerNotAuthorized { caller, subnet_id }, + ); + } + _ => { + panic!( + "{}", + UpdateSubnetAdminsError::UnsupportedSubnetType { + subnet_id, + subnet_type: subnet_record.subnet_type, + }, + ); + } + } + } + fn compute_new_subnet_admins( &self, current_subnet_admins: Vec, @@ -282,10 +312,6 @@ mod tests { use maplit::btreemap; use pretty_assertions::assert_eq; - fn subnet_rental_canister_caller() -> PrincipalId { - PrincipalId::from(SUBNET_RENTAL_CANISTER_ID) - } - fn prepare_registry_for_update_subnet_admins_test( subnet_id: SubnetId, subnet_type: SubnetType, @@ -316,7 +342,7 @@ mod tests { } // For the purposes of the below tests, the updated subnet admins list matches - // the expected one if the contain exactly the same elements regardless of order. + // the expected one if they contain exactly the same elements regardless of order. // // Useful in cases the list contains more than 1 element (otherwise direct equality // check is enough). @@ -335,14 +361,149 @@ mod tests { } #[test] - fn application_subnet_can_add_or_remove_subnet_admins() { + fn rented_subnet_authorizes_subnet_rental_canister() { + let subnet_id = subnet_test_id(1); + let registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let subnet_record = registry.get_subnet_or_panic(subnet_id); + let caller = PrincipalId::from(SUBNET_RENTAL_CANISTER_ID); + + // Should not panic. + registry.check_update_subnet_admins_caller_authorization(caller, &subnet_record, subnet_id); + } + + #[test] + #[should_panic(expected = "is not authorized to update subnet admins")] + fn rented_subnet_rejects_non_rental_canister_caller() { + let subnet_id = subnet_test_id(1); + let registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let subnet_record = registry.get_subnet_or_panic(subnet_id); + let unauthorized_caller = user_test_id(999).get(); + + registry.check_update_subnet_admins_caller_authorization( + unauthorized_caller, + &subnet_record, + subnet_id, + ); + } + + #[test] + fn cloud_engine_authorizes_existing_admin() { + let subnet_id = subnet_test_id(1); + let admin = user_test_id(100).get(); + let registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::CloudEngine, + vec![PrincipalIdPb::from(admin)], + ); + let subnet_record = registry.get_subnet_or_panic(subnet_id); + + // Should not panic. + registry.check_update_subnet_admins_caller_authorization(admin, &subnet_record, subnet_id); + } + + #[test] + #[should_panic(expected = "is not authorized to update subnet admins")] + fn cloud_engine_rejects_non_admin_caller() { + let subnet_id = subnet_test_id(1); + let admin = user_test_id(100).get(); + let registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::CloudEngine, + vec![PrincipalIdPb::from(admin)], + ); + let subnet_record = registry.get_subnet_or_panic(subnet_id); + let non_admin = user_test_id(999).get(); + + registry.check_update_subnet_admins_caller_authorization( + non_admin, + &subnet_record, + subnet_id, + ); + } + + #[test] + #[should_panic(expected = "is not authorized to update subnet admins")] + fn cloud_engine_rejects_subnet_rental_canister_caller() { + let subnet_id = subnet_test_id(1); + let admin = user_test_id(100).get(); + let registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::CloudEngine, + vec![PrincipalIdPb::from(admin)], + ); + let subnet_record = registry.get_subnet_or_panic(subnet_id); + + registry.check_update_subnet_admins_caller_authorization( + PrincipalId::from(SUBNET_RENTAL_CANISTER_ID), + &subnet_record, + subnet_id, + ); + } + + #[test] + #[should_panic(expected = "Only subnets on a free cycles cost schedule")] + fn non_free_cycles_schedule_rejects_authorization() { + let subnet_id = subnet_test_id(1); + let mut registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + + // Override the cycles cost schedule to Normal to simulate a non-rented subnet. + let mut subnet_record = registry.get_subnet_or_panic(subnet_id); + subnet_record.canister_cycles_cost_schedule = i32::from(CanisterCyclesCostSchedule::Normal); + registry.maybe_apply_mutation_internal(vec![upsert( + make_subnet_record_key(subnet_id).into_bytes(), + subnet_record.encode_to_vec(), + )]); + + let subnet_record = registry.get_subnet_or_panic(subnet_id); + registry.check_update_subnet_admins_caller_authorization( + PrincipalId::from(SUBNET_RENTAL_CANISTER_ID), + &subnet_record, + subnet_id, + ); + } + + #[test] + #[should_panic(expected = "unsupported subnet type")] + fn unsupported_subnet_type_rejects_authorization() { + let subnet_id = subnet_test_id(1); + // Create a valid Application subnet, then override its type to + // VerifiedApplication to bypass registry invariant checks during setup. + let registry = prepare_registry_for_update_subnet_admins_test( + subnet_id, + SubnetType::Application, + vec![], + ); + let mut subnet_record = registry.get_subnet_or_panic(subnet_id); + subnet_record.subnet_type = i32::from(SubnetType::VerifiedApplication); + + registry.check_update_subnet_admins_caller_authorization( + PrincipalId::from(SUBNET_RENTAL_CANISTER_ID), + &subnet_record, + subnet_id, + ); + } + + #[test] + fn rented_subnet_can_add_or_remove_subnet_admins() { let subnet_id = subnet_test_id(1); let mut registry = prepare_registry_for_update_subnet_admins_test( subnet_id, SubnetType::Application, vec![], ); - let caller = subnet_rental_canister_caller(); + let caller = PrincipalId::from(SUBNET_RENTAL_CANISTER_ID); let user1 = user_test_id(100).get(); let user2 = user_test_id(101).get(); @@ -374,14 +535,14 @@ mod tests { } #[test] - fn application_subnet_can_clear_subnet_admins() { + fn rented_subnet_can_clear_subnet_admins() { let subnet_id = subnet_test_id(1); let mut registry = prepare_registry_for_update_subnet_admins_test( subnet_id, SubnetType::Application, vec![], ); - let caller = subnet_rental_canister_caller(); + let caller = PrincipalId::from(SUBNET_RENTAL_CANISTER_ID); let user1 = user_test_id(100).get(); let user2 = user_test_id(101).get(); @@ -419,14 +580,14 @@ mod tests { #[test] #[should_panic(expected = "The list of provided principals cannot be empty")] - fn application_subnet_can_not_add_empty_list_of_subnet_admins() { + fn can_not_add_empty_list_of_subnet_admins() { let subnet_id = subnet_test_id(1); let mut registry = prepare_registry_for_update_subnet_admins_test( subnet_id, SubnetType::Application, vec![], ); - let caller = subnet_rental_canister_caller(); + let caller = PrincipalId::from(SUBNET_RENTAL_CANISTER_ID); let payload = UpdateSubnetAdminsPayload { subnet_id, @@ -438,14 +599,14 @@ mod tests { #[test] #[should_panic(expected = "The list of provided principals cannot be empty")] - fn application_subnet_can_not_remove_empty_list_of_subnet_admins() { + fn can_not_remove_empty_list_of_subnet_admins() { let subnet_id = subnet_test_id(1); let mut registry = prepare_registry_for_update_subnet_admins_test( subnet_id, SubnetType::Application, vec![], ); - let caller = subnet_rental_canister_caller(); + let caller = PrincipalId::from(SUBNET_RENTAL_CANISTER_ID); let payload = UpdateSubnetAdminsPayload { subnet_id, @@ -456,14 +617,14 @@ mod tests { } #[test] - fn application_subnet_can_dedup_input_when_adding_or_removing_subnet_admins() { + fn can_dedup_input_when_adding_or_removing_subnet_admins() { let subnet_id = subnet_test_id(1); let mut registry = prepare_registry_for_update_subnet_admins_test( subnet_id, SubnetType::Application, vec![], ); - let caller = subnet_rental_canister_caller(); + let caller = PrincipalId::from(SUBNET_RENTAL_CANISTER_ID); let user1 = user_test_id(100).get(); let payload = UpdateSubnetAdminsPayload { @@ -493,14 +654,14 @@ mod tests { #[should_panic( expected = "Too many subnet admins. Provided: 11, Existing: 0, Max allowed: 10." )] - fn application_subnet_can_not_add_too_many_subnet_admins_no_existing_ones() { + fn can_not_add_too_many_subnet_admins_no_existing_ones() { let subnet_id = subnet_test_id(1); let mut registry = prepare_registry_for_update_subnet_admins_test( subnet_id, SubnetType::Application, vec![], ); - let caller = subnet_rental_canister_caller(); + let caller = PrincipalId::from(SUBNET_RENTAL_CANISTER_ID); let mut users_to_add = Vec::new(); for i in 0..(MAX_SUBNET_ADMINS + 1) { @@ -517,14 +678,14 @@ mod tests { #[test] #[should_panic(expected = "Too many subnet admins. Provided: 3, Existing: 9, Max allowed: 10.")] - fn application_subnet_can_not_add_too_many_subnet_admins_with_existing_ones() { + fn can_not_add_too_many_subnet_admins_with_existing_ones() { let subnet_id = subnet_test_id(1); let mut registry = prepare_registry_for_update_subnet_admins_test( subnet_id, SubnetType::Application, vec![], ); - let caller = subnet_rental_canister_caller(); + let caller = PrincipalId::from(SUBNET_RENTAL_CANISTER_ID); let mut users_to_add = Vec::new(); let mut expected_subnet_admins = Vec::new(); @@ -561,14 +722,14 @@ mod tests { #[should_panic( expected = "Too many subnet admins. Provided: 11, Existing: 0, Max allowed: 10." )] - fn application_subnet_can_not_remove_too_many_subnet_admins() { + fn can_not_remove_too_many_subnet_admins() { let subnet_id = subnet_test_id(1); let mut registry = prepare_registry_for_update_subnet_admins_test( subnet_id, SubnetType::Application, vec![], ); - let caller = subnet_rental_canister_caller(); + let caller = PrincipalId::from(SUBNET_RENTAL_CANISTER_ID); let mut users_to_remove = Vec::new(); for i in 0..(MAX_SUBNET_ADMINS + 1) { @@ -584,14 +745,14 @@ mod tests { } #[test] - fn application_subnet_can_not_add_existing_subnet_admins() { + fn can_not_add_existing_subnet_admins() { let subnet_id = subnet_test_id(1); let mut registry = prepare_registry_for_update_subnet_admins_test( subnet_id, SubnetType::Application, vec![], ); - let caller = subnet_rental_canister_caller(); + let caller = PrincipalId::from(SUBNET_RENTAL_CANISTER_ID); let user1 = user_test_id(100).get(); let payload = UpdateSubnetAdminsPayload { @@ -615,14 +776,14 @@ mod tests { } #[test] - fn application_subnet_can_not_remove_non_existing_subnet_admins() { + fn can_not_remove_non_existing_subnet_admins() { let subnet_id = subnet_test_id(1); let mut registry = prepare_registry_for_update_subnet_admins_test( subnet_id, SubnetType::Application, vec![], ); - let caller = subnet_rental_canister_caller(); + let caller = PrincipalId::from(SUBNET_RENTAL_CANISTER_ID); let user1 = user_test_id(100).get(); let user2 = user_test_id(101).get(); @@ -645,27 +806,16 @@ mod tests { ); } - #[test] - #[should_panic(expected = "is not authorized to update subnet admins")] - fn application_subnet_rejects_non_rental_canister_caller() { - let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test( - subnet_id, - SubnetType::Application, - vec![], - ); - let unauthorized_caller = user_test_id(999).get(); - - let payload = UpdateSubnetAdminsPayload { - subnet_id, - operation_type: Some(OperationType::Add(vec![user_test_id(100).get()])), - }; - - registry.do_update_subnet_admins(unauthorized_caller, payload); - } + // ===================================================================== + // Core logic tests for CloudEngine subnets + // + // These tests verify add/remove/clear logic specific to CloudEngine + // and do not re-test authorization — they use an existing admin as + // the caller. + // ===================================================================== #[test] - fn cloud_engine_subnet_admin_can_add_or_remove_subnet_admins() { + fn cloud_engine_admin_can_add_or_remove_subnet_admins() { let subnet_id = subnet_test_id(1); let admin = user_test_id(100).get(); let mut registry = prepare_registry_for_update_subnet_admins_test( @@ -703,7 +853,8 @@ mod tests { } #[test] - fn cloud_engine_subnet_admin_can_clear_subnet_admins() { + #[should_panic(expected = "Cannot remove all subnet admins from CloudEngine subnet")] + fn cloud_engine_cannot_clear_all_subnet_admins() { let subnet_id = subnet_test_id(1); let admin = user_test_id(100).get(); let mut registry = prepare_registry_for_update_subnet_admins_test( @@ -718,36 +869,11 @@ mod tests { }; registry.do_update_subnet_admins(admin, payload); - - assert_eq!( - registry.get_subnet_or_panic(subnet_id).subnet_admins, - vec![] - ); } #[test] - #[should_panic(expected = "is not authorized to update subnet admins")] - fn cloud_engine_subnet_rejects_non_admin_caller() { - let subnet_id = subnet_test_id(1); - let admin = user_test_id(100).get(); - let mut registry = prepare_registry_for_update_subnet_admins_test( - subnet_id, - SubnetType::CloudEngine, - vec![PrincipalIdPb::from(admin)], - ); - let non_admin = user_test_id(999).get(); - - let payload = UpdateSubnetAdminsPayload { - subnet_id, - operation_type: Some(OperationType::Add(vec![user_test_id(200).get()])), - }; - - registry.do_update_subnet_admins(non_admin, payload); - } - - #[test] - #[should_panic(expected = "is not authorized to update subnet admins")] - fn cloud_engine_subnet_rejects_subnet_rental_canister_caller() { + #[should_panic(expected = "Cannot remove all subnet admins from CloudEngine subnet")] + fn cloud_engine_cannot_remove_last_admin() { let subnet_id = subnet_test_id(1); let admin = user_test_id(100).get(); let mut registry = prepare_registry_for_update_subnet_admins_test( @@ -758,35 +884,9 @@ mod tests { let payload = UpdateSubnetAdminsPayload { subnet_id, - operation_type: Some(OperationType::Add(vec![user_test_id(200).get()])), + operation_type: Some(OperationType::Remove(vec![admin])), }; - registry.do_update_subnet_admins(subnet_rental_canister_caller(), payload); - } - - #[test] - #[should_panic(expected = "Only subnets on a free cycles cost schedule")] - fn non_free_cycles_schedule_rejects_subnet_admin_updates() { - let subnet_id = subnet_test_id(1); - let mut registry = prepare_registry_for_update_subnet_admins_test( - subnet_id, - SubnetType::Application, - vec![], - ); - - // Override the cycles cost schedule to Normal to simulate a non-rented subnet. - let mut subnet_record = registry.get_subnet_or_panic(subnet_id); - subnet_record.canister_cycles_cost_schedule = i32::from(CanisterCyclesCostSchedule::Normal); - registry.maybe_apply_mutation_internal(vec![upsert( - make_subnet_record_key(subnet_id).into_bytes(), - subnet_record.encode_to_vec(), - )]); - - let payload = UpdateSubnetAdminsPayload { - subnet_id, - operation_type: Some(OperationType::Add(vec![user_test_id(100).get()])), - }; - - registry.do_update_subnet_admins(subnet_rental_canister_caller(), payload); + registry.do_update_subnet_admins(admin, payload); } } diff --git a/rs/registry/canister/tests/update_subnet_admins.rs b/rs/registry/canister/tests/update_subnet_admins.rs index 9b1527fedcc5..65676a613054 100644 --- a/rs/registry/canister/tests/update_subnet_admins.rs +++ b/rs/registry/canister/tests/update_subnet_admins.rs @@ -51,7 +51,8 @@ fn test_the_anonymous_user_cannot_update_a_subnets_subnet_admins() { let response: Result<(), String> = registry .update_("update_subnet_admins", candid, (payload.clone(),)) .await; - assert_matches!(response, Err(_)); + assert_matches!(response, + Err(s) if s.contains("Only subnets on a free cycles cost schedule are allowed to have subnet admins")); // .. And no change should have happened to the subnet's subnet admins let subnet_admins = get_value_or_panic::( From 98d65c6497ea1de3dd02151a86b9f09d7b8420a4 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Tue, 31 Mar 2026 21:53:39 +0000 Subject: [PATCH 4/4] slight refactoring of match statements --- .../src/mutations/do_update_subnet_admins.rs | 47 +++++++++---------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/rs/registry/canister/src/mutations/do_update_subnet_admins.rs b/rs/registry/canister/src/mutations/do_update_subnet_admins.rs index a2e1ec26cee4..705a3017cd05 100644 --- a/rs/registry/canister/src/mutations/do_update_subnet_admins.rs +++ b/rs/registry/canister/src/mutations/do_update_subnet_admins.rs @@ -145,37 +145,36 @@ impl Registry { self.check_update_subnet_admins_caller_authorization(caller, &subnet_record, subnet_id); + let subnet_type = subnet_record.subnet_type(); let current_subnet_admins = subnet_record.subnet_admins; let res = match payload.operation_type { Some(operation_type) => { match self.compute_new_subnet_admins(current_subnet_admins, operation_type) { - Ok(new_subnet_admins) => { + Ok(new_subnet_admins) + if subnet_type == SubnetType::CloudEngine.into() + && new_subnet_admins.is_empty() => + { // CloudEngine subnets must always have at least one admin, // otherwise there is no way to recover. - if subnet_record.subnet_type == i32::from(SubnetType::CloudEngine) - && new_subnet_admins.is_empty() - { - Err(UpdateSubnetAdminsError::WouldRemoveAllCloudEngineAdmins { - subnet_id, - }) - } else { - subnet_record.subnet_admins = new_subnet_admins; - - let subnet_record_mutation = upsert( - make_subnet_record_key(subnet_id).into_bytes(), - subnet_record.encode_to_vec(), - ); - let mutations = vec![subnet_record_mutation]; - - // Check invariants before applying mutations - self.maybe_apply_mutation_internal(mutations); - - UPDATE_SUBNET_ADMINS_RATE_LIMITER - .with_borrow_mut(|limiter| limiter.commit(reservation, now)); - - Ok(()) - } + Err(UpdateSubnetAdminsError::WouldRemoveAllCloudEngineAdmins { subnet_id }) + } + Ok(new_subnet_admins) => { + subnet_record.subnet_admins = new_subnet_admins; + + let subnet_record_mutation = upsert( + make_subnet_record_key(subnet_id).into_bytes(), + subnet_record.encode_to_vec(), + ); + let mutations = vec![subnet_record_mutation]; + + // Check invariants before applying mutations + self.maybe_apply_mutation_internal(mutations); + + UPDATE_SUBNET_ADMINS_RATE_LIMITER + .with_borrow_mut(|limiter| limiter.commit(reservation, now)); + + Ok(()) } Err(err) => Err(err), }