diff --git a/rs/engine_controller/canister/canister.rs b/rs/engine_controller/canister/canister.rs index a91daab8ff15..724e9b01ec5e 100644 --- a/rs/engine_controller/canister/canister.rs +++ b/rs/engine_controller/canister/canister.rs @@ -7,7 +7,8 @@ use candid::Principal; use ic_base_types::{NodeId, PrincipalId, SubnetId}; use ic_cdk::{api::msg_caller, call::Call, init, post_upgrade, println, update}; use ic_engine_controller::{ - CreateEngineArgs, DeleteEngineArgs, EngineControllerInitArgs, NewSubnet, + CreateEngineArgs, DeleteEngineArgs, DeployGuestosToAllSubnetNodesPayload, + EngineControllerInitArgs, NewSubnet, UpdateSubnetPayload, }; use ic_nns_constants::REGISTRY_CANISTER_ID; use ic_protobuf::registry::subnet::v1::SubnetFeatures; @@ -186,6 +187,180 @@ async fn delete_engine(args: DeleteEngineArgs) -> Result<(), String> { response } +/// Validates that the only fields set on the proxied `UpdateSubnetPayload` +/// are the ones the engine controller is allowed to manage: `subnet_admins` +/// and `is_halted` (subnet halting / unhalting). Every other `Option<_>` +/// field must be `None`, and the single non-optional knob +/// (`set_gossip_config_to_default`) must hold its default value (`false`). +/// The required `subnet_id` is exempt because it merely identifies the target. +/// +/// This keeps the surface of `update_subnet` deliberately tiny: only the +/// fields the engine controller is intended to manage flow through. Adding a +/// new allowed field is a conscious, code-level decision. +fn ensure_only_allowed_fields_set(payload: &UpdateSubnetPayload) -> Result<(), String> { + let UpdateSubnetPayload { + subnet_id: _, + // The fields we allow. + subnet_admins: _, + is_halted: _, + + max_ingress_bytes_per_message, + max_ingress_bytes_per_block, + max_ingress_messages_per_block, + max_block_payload_size, + unit_delay_millis, + initial_notary_delay_millis, + dkg_interval_length, + dkg_dealings_per_block, + start_as_nns, + subnet_type, + halt_at_cup_height, + features, + resource_limits, + chain_key_config, + chain_key_signing_enable, + chain_key_signing_disable, + max_number_of_canisters, + ssh_readonly_access, + ssh_backup_access, + max_artifact_streams_per_peer, + max_chunk_wait_ms, + max_duplicity, + max_chunk_size, + receive_check_cache_size, + pfn_evaluation_period_ms, + registry_poll_period_ms, + retransmission_request_ms, + set_gossip_config_to_default, + } = payload; + + // Build up a list of fields the caller is trying to set so the error is + // actionable. The check is purely structural: any `Some(_)` (or a + // non-default bool) is treated as "the caller tried to update this". + let mut disallowed: Vec<&'static str> = vec![]; + macro_rules! check_none { + ($field:expr, $name:literal) => { + if $field.is_some() { + disallowed.push($name); + } + }; + } + check_none!( + max_ingress_bytes_per_message, + "max_ingress_bytes_per_message" + ); + check_none!(max_ingress_bytes_per_block, "max_ingress_bytes_per_block"); + check_none!( + max_ingress_messages_per_block, + "max_ingress_messages_per_block" + ); + check_none!(max_block_payload_size, "max_block_payload_size"); + check_none!(unit_delay_millis, "unit_delay_millis"); + check_none!(initial_notary_delay_millis, "initial_notary_delay_millis"); + check_none!(dkg_interval_length, "dkg_interval_length"); + check_none!(dkg_dealings_per_block, "dkg_dealings_per_block"); + check_none!(start_as_nns, "start_as_nns"); + check_none!(subnet_type, "subnet_type"); + check_none!(halt_at_cup_height, "halt_at_cup_height"); + check_none!(features, "features"); + check_none!(resource_limits, "resource_limits"); + check_none!(chain_key_config, "chain_key_config"); + check_none!(chain_key_signing_enable, "chain_key_signing_enable"); + check_none!(chain_key_signing_disable, "chain_key_signing_disable"); + check_none!(max_number_of_canisters, "max_number_of_canisters"); + check_none!(ssh_readonly_access, "ssh_readonly_access"); + check_none!(ssh_backup_access, "ssh_backup_access"); + check_none!( + max_artifact_streams_per_peer, + "max_artifact_streams_per_peer" + ); + check_none!(max_chunk_wait_ms, "max_chunk_wait_ms"); + check_none!(max_duplicity, "max_duplicity"); + check_none!(max_chunk_size, "max_chunk_size"); + check_none!(receive_check_cache_size, "receive_check_cache_size"); + check_none!(pfn_evaluation_period_ms, "pfn_evaluation_period_ms"); + check_none!(registry_poll_period_ms, "registry_poll_period_ms"); + check_none!(retransmission_request_ms, "retransmission_request_ms"); + if *set_gossip_config_to_default { + disallowed.push("set_gossip_config_to_default"); + } + + if disallowed.is_empty() { + Ok(()) + } else { + Err(format!( + "Updating these fields via the engine controller is not allowed: {}. \ + Only `subnet_admins` and `is_halted` may be updated.", + disallowed.join(", ") + )) + } +} + +/// Ensures that the configured `AUTHORIZED_CALLER` (the engine controller's +/// "super admin") is always present in the resulting admin list, even if the +/// caller forgot to include it. +fn normalize_subnet_admins(admins: Vec) -> Vec { + let super_admin = PrincipalId(AUTHORIZED_CALLER.with(|c| *c.borrow())); + let mut admins = admins; + if !admins.contains(&super_admin) { + admins.push(super_admin); + } + admins +} + +/// Proxies to the registry's `update_subnet` endpoint. Only `subnet_admins` +/// and `is_halted` may be updated through this path; every other field must be +/// left at its default value (`None` / `false`) or the call is rejected. +/// +/// The `subnet_admins` list is always normalized to include the engine +/// controller's authorized caller (the super admin), so callers cannot +/// accidentally lock the controller out of the subnet. +#[update] +async fn update_subnet(payload: UpdateSubnetPayload) -> Result<(), String> { + ensure_authorized()?; + ensure_only_allowed_fields_set(&payload)?; + + // Normalize `subnet_admins` so the super admin is always present. + // The caller may omit the field entirely (no change requested), but if + // they do supply one, we treat it as the source of truth and add the + // super admin if missing. + #[allow(unused_mut)] + let mut payload = payload; + if let Some(admins) = payload.subnet_admins { + payload.subnet_admins = Some(normalize_subnet_admins(admins)); + } + + Call::unbounded_wait(REGISTRY_CANISTER_ID.into(), "update_subnet") + .with_arg(payload) + .await + .map_err(|e| format!("registry.update_subnet call failed: {e:?}"))? + .candid::<()>() + .map_err(|e| format!("Failed to decode registry response: {e}"))?; + + Ok(()) +} + +/// Proxies to the registry's `deploy_guestos_to_all_subnet_nodes` endpoint, +/// which is the registry path for updating a subnet's replica version. +#[update] +async fn deploy_guestos_to_all_subnet_nodes( + payload: DeployGuestosToAllSubnetNodesPayload, +) -> Result<(), String> { + ensure_authorized()?; + + Call::unbounded_wait( + REGISTRY_CANISTER_ID.into(), + "deploy_guestos_to_all_subnet_nodes", + ) + .with_arg(payload) + .await + .map_err(|e| format!("registry.deploy_guestos_to_all_subnet_nodes call failed: {e:?}"))? + .candid::<()>() + .map_err(|e| format!("Failed to decode registry response: {e}"))?; + + Ok(()) +} + fn main() { // This block is intentionally left blank. } diff --git a/rs/engine_controller/canister/tests.rs b/rs/engine_controller/canister/tests.rs index 0cae6e0e5f5c..08af9914cc13 100644 --- a/rs/engine_controller/canister/tests.rs +++ b/rs/engine_controller/canister/tests.rs @@ -1,5 +1,46 @@ use super::*; use candid_parser::utils::{CandidSource, service_equal}; +use ic_base_types::{PrincipalId, SubnetId}; + +/// Builds an `UpdateSubnetPayload` where every optional field is `None` and +/// every non-optional knob has its default. The given `subnet_id` identifies +/// the target; the caller can flip individual fields on the returned struct +/// to set up a specific test case. +fn empty_update_payload() -> UpdateSubnetPayload { + UpdateSubnetPayload { + subnet_id: SubnetId::new(PrincipalId::new_user_test_id(1)), + max_ingress_bytes_per_message: None, + max_ingress_bytes_per_block: None, + max_ingress_messages_per_block: None, + max_block_payload_size: None, + unit_delay_millis: None, + initial_notary_delay_millis: None, + dkg_interval_length: None, + dkg_dealings_per_block: None, + start_as_nns: None, + subnet_type: None, + is_halted: None, + halt_at_cup_height: None, + features: None, + resource_limits: None, + chain_key_config: None, + chain_key_signing_enable: None, + chain_key_signing_disable: None, + max_number_of_canisters: None, + ssh_readonly_access: None, + ssh_backup_access: None, + subnet_admins: None, + max_artifact_streams_per_peer: None, + max_chunk_wait_ms: None, + max_duplicity: None, + max_chunk_size: None, + receive_check_cache_size: None, + pfn_evaluation_period_ms: None, + registry_poll_period_ms: None, + retransmission_request_ms: None, + set_gossip_config_to_default: false, + } +} /// This is NOT affected by /// @@ -32,3 +73,102 @@ fn test_implemented_interface_matches_declared_interface_exactly() { let result = service_equal(declared_interface, implemented_interface); assert!(result.is_ok(), "{:?}\n\n", result.unwrap_err()); } + +#[test] +fn ensure_only_allowed_fields_set_accepts_empty_payload() { + // Pure no-op: no fields set at all. We don't actually want to allow this + // in practice (the call would be useless), but the validator's job is + // purely structural — it must accept any payload where the only mutated + // fields are the allowed ones. + ensure_only_allowed_fields_set(&empty_update_payload()) + .expect("an empty payload must pass the structural check"); +} + +#[test] +fn ensure_only_allowed_fields_set_accepts_subnet_admins_only() { + let mut payload = empty_update_payload(); + payload.subnet_admins = Some(vec![PrincipalId::new_user_test_id(42)]); + ensure_only_allowed_fields_set(&payload).expect("subnet_admins-only payload must be allowed"); +} + +#[test] +fn ensure_only_allowed_fields_set_accepts_is_halted() { + for is_halted in [true, false] { + let mut payload = empty_update_payload(); + payload.is_halted = Some(is_halted); + ensure_only_allowed_fields_set(&payload) + .unwrap_or_else(|e| panic!("is_halted={is_halted} payload must be allowed: {e}")); + } +} + +#[test] +fn ensure_only_allowed_fields_set_accepts_subnet_admins_and_is_halted() { + let mut payload = empty_update_payload(); + payload.subnet_admins = Some(vec![PrincipalId::new_user_test_id(42)]); + payload.is_halted = Some(true); + ensure_only_allowed_fields_set(&payload) + .expect("subnet_admins + is_halted payload must be allowed"); +} + +#[test] +fn ensure_only_allowed_fields_set_rejects_other_fields() { + let mut payload = empty_update_payload(); + payload.max_number_of_canisters = Some(100); + // `halt_at_cup_height` is intentionally *not* part of the allowed surface, + // even though it is halting-adjacent: only `is_halted` is. + payload.halt_at_cup_height = Some(true); + let err = ensure_only_allowed_fields_set(&payload).expect_err("should reject"); + assert!( + err.contains("max_number_of_canisters"), + "error must mention disallowed field: {err}" + ); + assert!( + err.contains("halt_at_cup_height"), + "error must mention disallowed field: {err}" + ); +} + +#[test] +fn ensure_only_allowed_fields_set_rejects_non_default_gossip_flag() { + let mut payload = empty_update_payload(); + payload.set_gossip_config_to_default = true; + let err = ensure_only_allowed_fields_set(&payload).expect_err("should reject"); + assert!( + err.contains("set_gossip_config_to_default"), + "error must mention the non-default bool: {err}" + ); +} + +#[test] +fn normalize_subnet_admins_adds_super_admin_when_missing() { + let other = PrincipalId::new_user_test_id(7); + let normalized = normalize_subnet_admins(vec![other]); + + let super_admin = PrincipalId(default_authorized_caller()); + assert!( + normalized.contains(&super_admin), + "super admin must be present after normalization" + ); + assert!( + normalized.contains(&other), + "other admins must be preserved" + ); +} + +#[test] +fn normalize_subnet_admins_keeps_list_intact_when_super_admin_present() { + let super_admin = PrincipalId(default_authorized_caller()); + let other = PrincipalId::new_user_test_id(8); + let input = vec![other, super_admin]; + let normalized = normalize_subnet_admins(input.clone()); + assert_eq!( + normalized, input, + "list must not be reordered or duplicated when super admin is already present" + ); +} + +#[test] +fn normalize_subnet_admins_handles_empty_input() { + let normalized = normalize_subnet_admins(vec![]); + assert_eq!(normalized, vec![PrincipalId(default_authorized_caller())]); +} diff --git a/rs/engine_controller/engine_controller.did b/rs/engine_controller/engine_controller.did index 71d96a68ac0d..153c0f23bf30 100644 --- a/rs/engine_controller/engine_controller.did +++ b/rs/engine_controller/engine_controller.did @@ -21,7 +21,95 @@ type EngineControllerInitArgs = record { initial_dkg_subnet_id : opt principal; }; +type SubnetType = variant { + application; + verified_application; + system; + cloud_engine; +}; + +type SubnetFeatures = record { + canister_sandboxing : bool; + http_requests : bool; + sev_enabled : opt bool; +}; + +type ResourceLimits = record { + maximum_state_size : opt nat64; + maximum_state_delta : opt nat64; +}; + +// Mirror of the subset of registry types referenced by UpdateSubnetPayload. +// The engine controller forwards the payload unchanged to the registry, but +// only `subnet_id` and `subnet_admins` may be set; every other field must be +// `null`/`false` or the call is rejected. +type EcdsaCurve = variant { secp256k1 }; +type EcdsaKeyId = record { curve : EcdsaCurve; name : text }; +type SchnorrAlgorithm = variant { bip340secp256k1; ed25519 }; +type SchnorrKeyId = record { algorithm : SchnorrAlgorithm; name : text }; +type VetKdCurve = variant { bls12_381_g2 }; +type VetKdKeyId = record { curve : VetKdCurve; name : text }; +type MasterPublicKeyId = variant { + Ecdsa : EcdsaKeyId; + Schnorr : SchnorrKeyId; + VetKd : VetKdKeyId; +}; + +type KeyConfig = record { + key_id : opt MasterPublicKeyId; + pre_signatures_to_create_in_advance : opt nat32; + max_queue_size : opt nat32; +}; + +type ChainKeyConfig = record { + key_configs : vec KeyConfig; + signature_request_timeout_ns : opt nat64; + idkg_key_rotation_period_ms : opt nat64; + max_parallel_pre_signature_transcripts_in_creation : opt nat32; +}; + +type UpdateSubnetPayload = record { + subnet_id : principal; + max_ingress_bytes_per_message : opt nat64; + max_ingress_bytes_per_block : opt nat64; + max_ingress_messages_per_block : opt nat64; + max_block_payload_size : opt nat64; + unit_delay_millis : opt nat64; + initial_notary_delay_millis : opt nat64; + dkg_interval_length : opt nat64; + dkg_dealings_per_block : opt nat64; + start_as_nns : opt bool; + subnet_type : opt SubnetType; + is_halted : opt bool; + halt_at_cup_height : opt bool; + features : opt SubnetFeatures; + resource_limits : opt ResourceLimits; + chain_key_config : opt ChainKeyConfig; + chain_key_signing_enable : opt vec MasterPublicKeyId; + chain_key_signing_disable : opt vec MasterPublicKeyId; + max_number_of_canisters : opt nat64; + ssh_readonly_access : opt vec text; + ssh_backup_access : opt vec text; + subnet_admins : opt vec principal; + max_artifact_streams_per_peer : opt nat32; + max_chunk_wait_ms : opt nat32; + max_duplicity : opt nat32; + max_chunk_size : opt nat32; + receive_check_cache_size : opt nat32; + pfn_evaluation_period_ms : opt nat32; + registry_poll_period_ms : opt nat32; + retransmission_request_ms : opt nat32; + set_gossip_config_to_default : bool; +}; + +type DeployGuestosToAllSubnetNodesPayload = record { + subnet_id : principal; + replica_version_id : text; +}; + service : (opt EngineControllerInitArgs) -> { create_engine : (CreateEngineArgs) -> (CreateEngineResult); delete_engine : (DeleteEngineArgs) -> (Result); + update_subnet : (UpdateSubnetPayload) -> (Result); + deploy_guestos_to_all_subnet_nodes : (DeployGuestosToAllSubnetNodesPayload) -> (Result); } diff --git a/rs/engine_controller/src/lib.rs b/rs/engine_controller/src/lib.rs index 6c79e52f4c4b..644bce74ba7b 100644 --- a/rs/engine_controller/src/lib.rs +++ b/rs/engine_controller/src/lib.rs @@ -10,6 +10,12 @@ use serde::Deserialize; // have to depend on `registry-canister` directly just to decode it. pub use registry_canister::mutations::do_create_subnet::NewSubnet; +// Re-export the payload types accepted by the proxy endpoints +// (`update_subnet` and `deploy_guestos_to_all_subnet_nodes`) so clients +// don't have to depend on `registry-canister` directly to construct them. +pub use registry_canister::mutations::do_deploy_guestos_to_all_subnet_nodes::DeployGuestosToAllSubnetNodesPayload; +pub use registry_canister::mutations::do_update_subnet::UpdateSubnetPayload; + #[derive(Clone, Debug, Default, CandidType, Deserialize)] pub struct EngineControllerInitArgs { /// If `Some`, replaces the default authorized caller; if `None`, the diff --git a/rs/registry/canister/canister/canister.rs b/rs/registry/canister/canister/canister.rs index d128079e1def..b3517e71dd9c 100644 --- a/rs/registry/canister/canister/canister.rs +++ b/rs/registry/canister/canister/canister.rs @@ -553,13 +553,14 @@ fn revise_elected_replica_versions_(payload: ReviseElectedGuestosVersionsPayload #[unsafe(export_name = "canister_update deploy_guestos_to_all_subnet_nodes")] fn deploy_guestos_to_all_subnet_nodes() { - check_caller_is_governance_and_log("deploy_guestos_to_all_subnet_nodes"); + check_caller_is_governance_or_engine_controller_and_log("deploy_guestos_to_all_subnet_nodes"); over(candid_one, deploy_guestos_to_all_subnet_nodes_); } #[candid_method(update, rename = "deploy_guestos_to_all_subnet_nodes")] fn deploy_guestos_to_all_subnet_nodes_(payload: DeployGuestosToAllSubnetNodesPayload) { - registry_mut().do_deploy_guestos_to_all_subnet_nodes(payload); + let caller = dfn_core::api::caller(); + registry_mut().do_deploy_guestos_to_all_subnet_nodes(caller, payload); recertify_registry(); } @@ -869,7 +870,7 @@ fn remove_node_operators_(payload: RemoveNodeOperatorsPayload) { #[unsafe(export_name = "canister_update update_subnet")] fn update_subnet() { - check_caller_is_governance_and_log("update_subnet"); + check_caller_is_governance_or_engine_controller_and_log("update_subnet"); over(candid_one, |payload: UpdateSubnetPayload| { update_subnet_(payload) }); @@ -877,7 +878,8 @@ fn update_subnet() { #[candid_method(update, rename = "update_subnet")] fn update_subnet_(payload: UpdateSubnetPayload) { - registry_mut().do_update_subnet(payload); + let caller = dfn_core::api::caller(); + registry_mut().do_update_subnet(caller, payload); recertify_registry(); } diff --git a/rs/registry/canister/src/common/test_helpers.rs b/rs/registry/canister/src/common/test_helpers.rs index eca76cf75c39..ce8f6bc9cd01 100644 --- a/rs/registry/canister/src/common/test_helpers.rs +++ b/rs/registry/canister/src/common/test_helpers.rs @@ -4,18 +4,20 @@ use crate::mutations::node_management::do_add_node::connection_endpoint_from_str use crate::registry::Registry; use ic_base_types::{NodeId, PrincipalId, SubnetId}; use ic_nns_test_utils::registry::{ - create_subnet_threshold_signing_pubkey_and_cup_mutations, invariant_compliant_mutation, - new_node_keys_and_node_id, + TEST_ID, create_subnet_threshold_signing_pubkey_and_cup_mutations, + invariant_compliant_mutation, new_node_keys_and_node_id, }; use ic_protobuf::registry::crypto::v1::PublicKey; use ic_protobuf::registry::node::v1::NodeRecord; use ic_protobuf::registry::node::v1::{IPv4InterfaceConfig, NodeRewardType}; use ic_protobuf::registry::node_operator::v1::NodeOperatorRecord; -use ic_protobuf::registry::subnet::v1::SubnetListRecord; -use ic_protobuf::registry::subnet::v1::SubnetRecord; +use ic_protobuf::registry::subnet::v1::{ + CanisterCyclesCostSchedule, SubnetListRecord, SubnetRecord, +}; use ic_registry_keys::make_node_operator_record_key; use ic_registry_keys::make_subnet_list_record_key; use ic_registry_keys::make_subnet_record_key; +use ic_registry_subnet_type::SubnetType; use ic_registry_transport::pb::v1::{ RegistryAtomicMutateRequest, RegistryMutation, registry_mutation::Type, }; @@ -204,6 +206,76 @@ pub fn prepare_registry_raw( (mutate_request, node_ids_and_dkg_pks) } +/// Prepares the mutations that add a CloudEngine subnet to a registry that was +/// initialized with [`invariant_compliant_mutation`] / [`invariant_compliant_registry`]. +/// +/// This first creates `node_count` fresh type-4 nodes (CloudEngine subnets may +/// only contain type-4 nodes) and then a CloudEngine subnet record made up of +/// those nodes, on the `Free` cost schedule (both required for CloudEngine +/// subnets). The returned request is meant to be pushed as an additional init +/// mutate request on top of the invariant-compliant base; it also returns the +/// id of the new CloudEngine subnet. +pub fn prepare_registry_with_cloud_engine_subnet( + node_count: u64, + starting_mutation_id: u8, +) -> (RegistryAtomicMutateRequest, SubnetId) { + // CloudEngine subnets may only contain type-4 nodes (enforced by the + // `check_node_type4_iff_cloud_engine` invariant). + let (nodes_request, node_ids_and_dkg_pks) = prepare_registry_with_nodes_and_reward_type( + starting_mutation_id, + node_count, + NodeRewardType::Type4, + ); + let mut mutations = nodes_request.mutations; + + // CloudEngine subnets are not charged cycles, i.e. they use the `Free` cost + // schedule. + let subnet_record = SubnetRecord { + membership: node_ids_and_dkg_pks + .keys() + .map(|node_id| node_id.get().to_vec()) + .collect(), + subnet_type: i32::from(SubnetType::CloudEngine), + canister_cycles_cost_schedule: i32::from(CanisterCyclesCostSchedule::Free), + replica_version_id: ReplicaVersion::default().to_string(), + unit_delay_millis: 600, + ..Default::default() + }; + + // The invariant-compliant base contains a single system subnet (`TEST_ID`); + // append the new CloudEngine subnet to the subnet list. + let cloud_engine_subnet_id = subnet_test_id(TEST_ID + 1); + let subnet_list_record = SubnetListRecord { + subnets: vec![ + subnet_test_id(TEST_ID).get().to_vec(), + cloud_engine_subnet_id.get().to_vec(), + ], + }; + + mutations.push(upsert( + make_subnet_list_record_key().as_bytes(), + subnet_list_record.encode_to_vec(), + )); + mutations.push(upsert( + make_subnet_record_key(cloud_engine_subnet_id).as_bytes(), + subnet_record.encode_to_vec(), + )); + mutations.append( + &mut create_subnet_threshold_signing_pubkey_and_cup_mutations( + cloud_engine_subnet_id, + &node_ids_and_dkg_pks, + ), + ); + + ( + RegistryAtomicMutateRequest { + mutations, + preconditions: vec![], + }, + cloud_engine_subnet_id, + ) +} + pub fn registry_create_subnet_with_nodes( registry: &mut Registry, node_ids_and_dkg_pks: &BTreeMap, diff --git a/rs/registry/canister/src/mutations/do_deploy_guestos_to_all_subnet_nodes.rs b/rs/registry/canister/src/mutations/do_deploy_guestos_to_all_subnet_nodes.rs index c72db03a65f5..3901908b74a2 100644 --- a/rs/registry/canister/src/mutations/do_deploy_guestos_to_all_subnet_nodes.rs +++ b/rs/registry/canister/src/mutations/do_deploy_guestos_to_all_subnet_nodes.rs @@ -6,7 +6,8 @@ use candid::{CandidType, Deserialize}; #[cfg(target_arch = "wasm32")] use dfn_core::println; use ic_base_types::{PrincipalId, SubnetId}; -use ic_protobuf::registry::subnet::v1::SubnetRecord; +use ic_nns_constants::ENGINE_CONTROLLER_CANISTER_ID; +use ic_protobuf::registry::subnet::v1::{SubnetRecord, SubnetType as SubnetTypePb}; use ic_registry_keys::make_subnet_record_key; use ic_registry_transport::pb::v1::{RegistryMutation, RegistryValue, registry_mutation}; use prost::Message; @@ -15,14 +16,32 @@ use serde::Serialize; impl Registry { pub fn do_deploy_guestos_to_all_subnet_nodes( &mut self, + caller: PrincipalId, payload: DeployGuestosToAllSubnetNodesPayload, ) { - println!("{LOG_PREFIX}do_deploy_guestos_to_all_subnet_nodes: {payload:?}"); + println!( + "{LOG_PREFIX}do_deploy_guestos_to_all_subnet_nodes: caller={caller}, payload={payload:?}" + ); + + let subnet_id = SubnetId::from(payload.subnet_id); + + // The engine controller canister is only allowed to mutate CloudEngine + // subnets. Other authorized callers (governance) can update any subnet. + if caller == ENGINE_CONTROLLER_CANISTER_ID.get() { + let subnet_record = self.get_subnet_or_panic(subnet_id); + assert_eq!( + subnet_record.subnet_type, + i32::from(SubnetTypePb::CloudEngine), + "{LOG_PREFIX}do_deploy_guestos_to_all_subnet_nodes: engine controller may only \ + deploy GuestOS to CloudEngine subnets; subnet {subnet_id} has subnet_type {:?}", + subnet_record.subnet_type, + ); + } check_replica_version_is_elected(self, &payload.replica_version_id); // Get the subnet record - let subnet_key = make_subnet_record_key(SubnetId::from(payload.subnet_id)); + let subnet_key = make_subnet_record_key(subnet_id); let mutation = match self.get(subnet_key.as_bytes(), self.latest_version()) { Some(RegistryValue { value: subnet_record_vec, @@ -48,6 +67,109 @@ impl Registry { } } +#[cfg(test)] +mod tests { + use super::*; + use crate::common::test_helpers::{ + add_fake_subnet, get_invariant_compliant_subnet_record, invariant_compliant_registry, + prepare_registry_with_cloud_engine_subnet, prepare_registry_with_nodes, + }; + use ic_nns_constants::{ENGINE_CONTROLLER_CANISTER_ID, GOVERNANCE_CANISTER_ID}; + use ic_protobuf::registry::subnet::v1::SubnetType as SubnetTypePb; + use ic_registry_subnet_type::SubnetType; + use ic_test_utilities_types::ids::subnet_test_id; + use ic_types::ReplicaVersion; + use maplit::btreemap; + + /// Creates a registry with a single non-CloudEngine subnet of the given + /// `subnet_type`. For CloudEngine subnets, use + /// [`prepare_registry_with_cloud_engine_subnet`] directly. + fn make_registry_with_non_cloud_engine_subnet(subnet_type: SubnetType) -> (Registry, SubnetId) { + assert_ne!( + subnet_type, + SubnetType::CloudEngine, + "use prepare_registry_with_cloud_engine_subnet for CloudEngine subnets", + ); + 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); + + let mut subnet_list_record = registry.get_subnet_list_record(); + + let (first_node_id, first_dkg_pk) = node_ids_and_dkg_pks + .iter() + .next() + .expect("should contain at least one node ID"); + + let mut subnet_record = get_invariant_compliant_subnet_record(vec![*first_node_id]); + subnet_record.subnet_type = i32::from(SubnetTypePb::from(subnet_type)); + + let subnet_id = subnet_test_id(3000); + registry.maybe_apply_mutation_internal(add_fake_subnet( + subnet_id, + &mut subnet_list_record, + subnet_record, + &btreemap!(*first_node_id => first_dkg_pk.clone()), + )); + + (registry, subnet_id) + } + + fn deploy_payload(subnet_id: SubnetId) -> DeployGuestosToAllSubnetNodesPayload { + DeployGuestosToAllSubnetNodesPayload { + subnet_id: subnet_id.get(), + replica_version_id: ReplicaVersion::default().to_string(), + } + } + + #[test] + fn engine_controller_can_deploy_to_cloud_engine_subnet() { + let mut registry = invariant_compliant_registry(0); + let (mutate_request, subnet_id) = prepare_registry_with_cloud_engine_subnet(1, 2); + registry.maybe_apply_mutation_internal(mutate_request.mutations); + + registry.do_deploy_guestos_to_all_subnet_nodes( + ENGINE_CONTROLLER_CANISTER_ID.get(), + deploy_payload(subnet_id), + ); + + let subnet_record = registry.get_subnet_or_panic(subnet_id); + assert_eq!( + subnet_record.replica_version_id, + ReplicaVersion::default().to_string() + ); + } + + #[test] + #[should_panic(expected = "engine controller may only deploy GuestOS to CloudEngine subnets")] + fn engine_controller_cannot_deploy_to_non_cloud_engine_subnet() { + let (mut registry, subnet_id) = + make_registry_with_non_cloud_engine_subnet(SubnetType::Application); + + registry.do_deploy_guestos_to_all_subnet_nodes( + ENGINE_CONTROLLER_CANISTER_ID.get(), + deploy_payload(subnet_id), + ); + } + + #[test] + fn governance_can_deploy_to_non_cloud_engine_subnet() { + let (mut registry, subnet_id) = + make_registry_with_non_cloud_engine_subnet(SubnetType::Application); + + registry.do_deploy_guestos_to_all_subnet_nodes( + GOVERNANCE_CANISTER_ID.get(), + deploy_payload(subnet_id), + ); + + let subnet_record = registry.get_subnet_or_panic(subnet_id); + assert_eq!( + subnet_record.replica_version_id, + ReplicaVersion::default().to_string() + ); + } +} + /// The argument of a command to update the replica version of a single subnet /// to a specific version. /// diff --git a/rs/registry/canister/src/mutations/do_update_subnet.rs b/rs/registry/canister/src/mutations/do_update_subnet.rs index 5e90e35d0e4f..b88f19e29f7d 100644 --- a/rs/registry/canister/src/mutations/do_update_subnet.rs +++ b/rs/registry/canister/src/mutations/do_update_subnet.rs @@ -3,10 +3,11 @@ use candid::{CandidType, Deserialize}; use dfn_core::println; use ic_base_types::{PrincipalId, SubnetId, subnet_id_into_protobuf}; use ic_management_canister_types_private::MasterPublicKeyId; +use ic_nns_constants::ENGINE_CONTROLLER_CANISTER_ID; use ic_protobuf::{ registry::subnet::v1::{ ResourceLimits as ResourceLimitsPb, SubnetFeatures as SubnetFeaturesPb, - SubnetRecord as SubnetRecordPb, + SubnetRecord as SubnetRecordPb, SubnetType as SubnetTypePb, }, types::v1::PrincipalId as PrincipalIdPb, }; @@ -22,17 +23,36 @@ use std::collections::HashSet; /// Updates the subnet's configuration in the registry. /// -/// This method is called by the governance canister, after a proposal -/// for updating a new subnet has been accepted. +/// This method is called by: +/// * the governance canister, after a proposal for updating a subnet has +/// been accepted (no scope restriction); and +/// * the engine controller canister, which may only target CloudEngine +/// subnets and is further restricted to a small subset of fields (see +/// [`ensure_engine_controller_payload_scope`]). impl Registry { - pub fn do_update_subnet(&mut self, payload: UpdateSubnetPayload) { - println!("{}do_update_subnet: {:?}", LOG_PREFIX, payload); + pub fn do_update_subnet(&mut self, caller: PrincipalId, payload: UpdateSubnetPayload) { + println!("{LOG_PREFIX}do_update_subnet: caller={caller}, payload={payload:?}"); + + let subnet_id = payload.subnet_id; + + // The engine controller canister is only allowed to mutate CloudEngine + // subnets, and only a small subset of fields. Other authorized callers + // (governance) can update any subnet and any field. + if caller == ENGINE_CONTROLLER_CANISTER_ID.get() { + let subnet_record = self.get_subnet_or_panic(subnet_id); + assert_eq!( + subnet_record.subnet_type, + i32::from(SubnetTypePb::CloudEngine), + "{LOG_PREFIX}do_update_subnet: engine controller may only update CloudEngine \ + subnets; subnet {subnet_id} has subnet_type {:?}", + subnet_record.subnet_type, + ); + ensure_engine_controller_payload_scope(&payload); + } self.validate_update_payload_chain_key_config(&payload); self.validate_update_sev_feature(&payload); - let subnet_id = payload.subnet_id; - let new_subnet_record = merge_subnet_record(self.get_subnet_or_panic(subnet_id), payload.clone()); @@ -213,6 +233,109 @@ impl Registry { } } +/// Defence-in-depth check that the engine controller canister never reaches +/// `do_update_subnet` with anything other than the small set of fields it is +/// allowed to manage (currently `subnet_admins` and `is_halted`). The engine +/// controller proxy already enforces this, but mirroring the check here keeps +/// the registry's invariants self-contained and prevents future drift if the +/// proxy's surface ever changes. +/// +/// Uses exhaustive destructuring so adding a new field to `UpdateSubnetPayload` +/// will fail to compile here until it is explicitly classified as +/// allowed-or-disallowed. +fn ensure_engine_controller_payload_scope(payload: &UpdateSubnetPayload) { + let UpdateSubnetPayload { + subnet_id: _, + // The fields the engine controller is allowed to set. + subnet_admins: _, + is_halted: _, + + max_ingress_bytes_per_message, + max_ingress_bytes_per_block, + max_ingress_messages_per_block, + max_block_payload_size, + unit_delay_millis, + initial_notary_delay_millis, + dkg_interval_length, + dkg_dealings_per_block, + start_as_nns, + subnet_type, + halt_at_cup_height, + features, + resource_limits, + chain_key_config, + chain_key_signing_enable, + chain_key_signing_disable, + max_number_of_canisters, + ssh_readonly_access, + ssh_backup_access, + max_artifact_streams_per_peer, + max_chunk_wait_ms, + max_duplicity, + max_chunk_size, + receive_check_cache_size, + pfn_evaluation_period_ms, + registry_poll_period_ms, + retransmission_request_ms, + set_gossip_config_to_default, + } = payload; + + let mut disallowed: Vec<&'static str> = vec![]; + macro_rules! check_none { + ($field:expr, $name:literal) => { + if $field.is_some() { + disallowed.push($name); + } + }; + } + check_none!( + max_ingress_bytes_per_message, + "max_ingress_bytes_per_message" + ); + check_none!(max_ingress_bytes_per_block, "max_ingress_bytes_per_block"); + check_none!( + max_ingress_messages_per_block, + "max_ingress_messages_per_block" + ); + check_none!(max_block_payload_size, "max_block_payload_size"); + check_none!(unit_delay_millis, "unit_delay_millis"); + check_none!(initial_notary_delay_millis, "initial_notary_delay_millis"); + check_none!(dkg_interval_length, "dkg_interval_length"); + check_none!(dkg_dealings_per_block, "dkg_dealings_per_block"); + check_none!(start_as_nns, "start_as_nns"); + check_none!(subnet_type, "subnet_type"); + check_none!(halt_at_cup_height, "halt_at_cup_height"); + check_none!(features, "features"); + check_none!(resource_limits, "resource_limits"); + check_none!(chain_key_config, "chain_key_config"); + check_none!(chain_key_signing_enable, "chain_key_signing_enable"); + check_none!(chain_key_signing_disable, "chain_key_signing_disable"); + check_none!(max_number_of_canisters, "max_number_of_canisters"); + check_none!(ssh_readonly_access, "ssh_readonly_access"); + check_none!(ssh_backup_access, "ssh_backup_access"); + check_none!( + max_artifact_streams_per_peer, + "max_artifact_streams_per_peer" + ); + check_none!(max_chunk_wait_ms, "max_chunk_wait_ms"); + check_none!(max_duplicity, "max_duplicity"); + check_none!(max_chunk_size, "max_chunk_size"); + check_none!(receive_check_cache_size, "receive_check_cache_size"); + check_none!(pfn_evaluation_period_ms, "pfn_evaluation_period_ms"); + check_none!(registry_poll_period_ms, "registry_poll_period_ms"); + check_none!(retransmission_request_ms, "retransmission_request_ms"); + if *set_gossip_config_to_default { + disallowed.push("set_gossip_config_to_default"); + } + + assert!( + disallowed.is_empty(), + "{LOG_PREFIX}do_update_subnet: engine controller may only update \ + `subnet_admins` and `is_halted`, but the following fields were also \ + set: {disallowed:?}", + ); +} + /// The payload of a proposal to update an existing subnet's configuration. /// /// See /rs/protobuf/def/registry/subnet/v1/subnet.proto @@ -540,6 +663,7 @@ mod tests { EcdsaCurve, EcdsaKeyId, SchnorrAlgorithm, SchnorrKeyId, VetKdCurve, VetKdKeyId, }; use ic_nervous_system_common_test_keys::{TEST_USER1_PRINCIPAL, TEST_USER2_PRINCIPAL}; + use ic_nns_constants::GOVERNANCE_CANISTER_ID; use ic_protobuf::registry::subnet::v1::{ CanisterCyclesCostSchedule, ChainKeyConfig as ChainKeyConfigPb, KeyConfig as KeyConfigPb, SubnetRecord as SubnetRecordPb, @@ -935,7 +1059,7 @@ mod tests { payload.chain_key_signing_enable = Some(vec![MasterPublicKeyId::Ecdsa(key)]); // Should panic because we are trying to enable a key that hasn't previously held it - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } #[test] @@ -990,7 +1114,7 @@ mod tests { payload.chain_key_signing_enable = Some(vec![MasterPublicKeyId::Ecdsa(key)]); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } #[test] @@ -1080,7 +1204,7 @@ mod tests { max_parallel_pre_signature_transcripts_in_creation: None, }); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } /// Returns an invariant-compliant Registry instance and an ID of a subnet @@ -1158,7 +1282,7 @@ mod tests { sev_enabled: Some(true), }); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } #[test] @@ -1175,7 +1299,7 @@ mod tests { sev_enabled: Some(false), }); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } /// Regression test: a payload with `features = Some(_)` but @@ -1195,7 +1319,7 @@ mod tests { sev_enabled: None, }); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } #[test] @@ -1210,7 +1334,7 @@ mod tests { sev_enabled: Some(true), }); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); let subnet_features = registry .get_subnet_or_panic(subnet_id) @@ -1233,7 +1357,7 @@ mod tests { }); // Should not panic because we are not changing SEV-related subnet features. - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } #[test] @@ -1244,7 +1368,7 @@ mod tests { { let mut payload = make_empty_update_payload(subnet_id); payload.features = None; - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } // Enable non-SEV-related features that can be enabled after the subnet was created. @@ -1255,7 +1379,7 @@ mod tests { http_requests: true, sev_enabled: None, }); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } } @@ -1322,7 +1446,7 @@ mod tests { payload.chain_key_signing_enable = Some(vec![master_public_key_held_by_subnet.clone()]); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); // Make sure it's actually in the list of enabled chain keys. assert!( @@ -1351,7 +1475,7 @@ mod tests { payload.chain_key_signing_disable = Some(vec![master_public_key_held_by_subnet.clone()]); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); // Ensure it's now removed from list of enabled subnets. assert!( @@ -1438,7 +1562,7 @@ mod tests { payload.chain_key_signing_disable = Some(vec![MasterPublicKeyId::Ecdsa(key.clone())]); // Should panic because we are trying to enable/disable same key - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } #[test] @@ -1509,7 +1633,7 @@ mod tests { ..make_empty_update_payload(subnet_id) }; - registry.do_update_subnet(payload.clone()); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload.clone()); // Try to update the subnet by adding a new key and removing one of the existing keys let payload = UpdateSubnetPayload { @@ -1521,7 +1645,7 @@ mod tests { }; // Should panic because we are trying to delete an existing key - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } #[test] @@ -1541,7 +1665,7 @@ mod tests { None, ); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } #[test] @@ -1561,7 +1685,7 @@ mod tests { Some(99), ); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } fn update_subnet_payload_with_key_config( @@ -1622,7 +1746,7 @@ mod tests { let mut payload = make_empty_update_payload(subnet_id); payload.subnet_admins = Some(vec![user1, user2]); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); assert_eq!( registry.get_subnet_or_panic(subnet_id).subnet_admins, @@ -1639,7 +1763,7 @@ mod tests { // First set an admin. let mut payload = make_empty_update_payload(subnet_id); payload.subnet_admins = Some(vec![user1]); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); assert_eq!( registry.get_subnet_or_panic(subnet_id).subnet_admins, vec![PrincipalIdPb::from(user1)], @@ -1648,7 +1772,7 @@ mod tests { // Then clear it via Some(vec![]). let mut payload = make_empty_update_payload(subnet_id); payload.subnet_admins = Some(vec![]); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); assert_eq!( registry.get_subnet_or_panic(subnet_id).subnet_admins, Vec::::new(), @@ -1663,11 +1787,11 @@ mod tests { let mut payload = make_empty_update_payload(subnet_id); payload.subnet_admins = Some(vec![user1]); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); // A subsequent update with `subnet_admins: None` must not change the list. let payload = make_empty_update_payload(subnet_id); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); assert_eq!( registry.get_subnet_or_panic(subnet_id).subnet_admins, @@ -1688,7 +1812,7 @@ mod tests { let mut payload = make_empty_update_payload(subnet_id); payload.subnet_admins = Some(admins); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); } #[test] @@ -1703,6 +1827,96 @@ mod tests { let mut payload = make_empty_update_payload(subnet_id); payload.subnet_admins = Some(vec![*TEST_USER1_PRINCIPAL]); - registry.do_update_subnet(payload); + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); + } + + /// Builds a registry that already has a `CloudEngine` subnet, suitable + /// for exercising the engine-controller permission checks on + /// `do_update_subnet`. + fn make_registry_with_cloud_engine_subnet() -> (Registry, SubnetId) { + use crate::common::test_helpers::prepare_registry_with_cloud_engine_subnet; + + let mut registry = invariant_compliant_registry(0); + let (mutate_request, subnet_id) = prepare_registry_with_cloud_engine_subnet(1, 2); + registry.maybe_apply_mutation_internal(mutate_request.mutations); + (registry, subnet_id) + } + + #[test] + fn engine_controller_can_update_cloud_engine_subnet_admins() { + use ic_nns_constants::ENGINE_CONTROLLER_CANISTER_ID; + + let (mut registry, subnet_id) = make_registry_with_cloud_engine_subnet(); + + let new_admins = vec![*TEST_USER1_PRINCIPAL, *TEST_USER2_PRINCIPAL]; + let mut payload = make_empty_update_payload(subnet_id); + payload.subnet_admins = Some(new_admins.clone()); + + registry.do_update_subnet(ENGINE_CONTROLLER_CANISTER_ID.get(), payload); + + let subnet_record = registry.get_subnet_or_panic(subnet_id); + let stored: Vec = subnet_record + .subnet_admins + .into_iter() + .map(|p| PrincipalId::try_from(p.raw.as_slice()).unwrap()) + .collect(); + assert_eq!(stored, new_admins); + } + + #[test] + #[should_panic(expected = "engine controller may only update CloudEngine subnets")] + fn engine_controller_cannot_update_non_cloud_engine_subnet() { + use ic_nns_constants::ENGINE_CONTROLLER_CANISTER_ID; + + // Default fixture is an Application subnet. + let (mut registry, subnet_id) = make_registry_for_update_subnet_tests(); + + let mut payload = make_empty_update_payload(subnet_id); + payload.subnet_admins = Some(vec![*TEST_USER1_PRINCIPAL]); + + registry.do_update_subnet(ENGINE_CONTROLLER_CANISTER_ID.get(), payload); + } + + #[test] + #[should_panic(expected = "engine controller may only update `subnet_admins` and `is_halted`")] + fn engine_controller_cannot_update_disallowed_fields() { + use ic_nns_constants::ENGINE_CONTROLLER_CANISTER_ID; + + let (mut registry, subnet_id) = make_registry_with_cloud_engine_subnet(); + + let mut payload = make_empty_update_payload(subnet_id); + // `max_number_of_canisters` is outside the engine controller's scope. + payload.max_number_of_canisters = Some(123); + + registry.do_update_subnet(ENGINE_CONTROLLER_CANISTER_ID.get(), payload); + } + + #[test] + fn engine_controller_can_set_is_halted() { + use ic_nns_constants::ENGINE_CONTROLLER_CANISTER_ID; + + let (mut registry, subnet_id) = make_registry_with_cloud_engine_subnet(); + + let mut payload = make_empty_update_payload(subnet_id); + payload.is_halted = Some(true); + + registry.do_update_subnet(ENGINE_CONTROLLER_CANISTER_ID.get(), payload); + + assert!(registry.get_subnet_or_panic(subnet_id).is_halted); + } + + #[test] + fn governance_is_not_restricted_to_cloud_engine_subnets() { + // Sanity check: governance must still be able to update non-CloudEngine + // subnets (it goes through the same code path now). + let (mut registry, subnet_id) = make_registry_for_update_subnet_tests(); + + let mut payload = make_empty_update_payload(subnet_id); + payload.max_number_of_canisters = Some(123); + + registry.do_update_subnet(GOVERNANCE_CANISTER_ID.get(), payload); + + let subnet_record = registry.get_subnet_or_panic(subnet_id); + assert_eq!(subnet_record.max_number_of_canisters, 123); } } diff --git a/rs/registry/canister/tests/common/test_helpers.rs b/rs/registry/canister/tests/common/test_helpers.rs index c3d41cb2d474..f63ad19464b5 100644 --- a/rs/registry/canister/tests/common/test_helpers.rs +++ b/rs/registry/canister/tests/common/test_helpers.rs @@ -213,6 +213,11 @@ pub fn prepare_registry_with_nodes_from_template( /// subnets). The returned request is meant to be pushed as an additional init /// mutate request on top of the invariant-compliant base; it also returns the id /// of the new CloudEngine subnet. +/// +/// NOTE: A unit-test-local copy of this helper with the same name and +/// signature lives at `src/common/test_helpers.rs` because the latter is +/// `#[cfg(test)]` and not accessible from integration tests. Keep them in +/// sync. pub fn prepare_registry_with_cloud_engine_subnet( node_count: u64, starting_mutation_id: u8, diff --git a/rs/registry/canister/unreleased_changelog.md b/rs/registry/canister/unreleased_changelog.md index 93c14851bbe2..b74d0b6fdf37 100644 --- a/rs/registry/canister/unreleased_changelog.md +++ b/rs/registry/canister/unreleased_changelog.md @@ -30,6 +30,13 @@ on the process that this file is part of, see * The `create_subnet` and `delete_subnet` endpoints can now be called by the engine controller canister (`si2b5-pyaaa-aaaaa-aaaja-cai`) in addition to the governance canister. +* The `update_subnet` and `deploy_guestos_to_all_subnet_nodes` endpoints can now + also be called by the engine controller canister + (`si2b5-pyaaa-aaaaa-aaaja-cai`) in addition to the governance canister. When + invoked by the engine controller, both endpoints are restricted to acting on + `CloudEngine` subnets only — any attempt to target a subnet of a different + type is rejected. Calls from the governance canister are unaffected and may + still target subnets of any type. * **SEV on existing subnets:** Reverted — `sev_enabled` can once again only be set at subnet creation; any update_subnet proposal that would change the effective `sev_enabled` value (in either direction, including via wholesale `features` replacement with `sev_enabled` left unset) is rejected.