diff --git a/rs/nns/governance/api/src/types.rs b/rs/nns/governance/api/src/types.rs index e970d8e4a7c4..e76511846f2b 100644 --- a/rs/nns/governance/api/src/types.rs +++ b/rs/nns/governance/api/src/types.rs @@ -4292,6 +4292,12 @@ pub enum NnsFunction { /// `SetupInitialDKG` requests without an explicit subnet id are routed to the /// calling subnet (NNS). SetDefaultInitialDkgSubnet = 58, + /// Deploy a GuestOS version to an explicit list of subnets at once. The + /// proposal changes the GuestOS version used on every subnet in the provided + /// list. The version must be contained in the list of elected GuestOS + /// versions. The upgrade is completed when each subnet creates the next + /// regular CUP. + UpdateGuestosVersionForSubnets = 59, } impl NnsFunction { /// String value of the enum field names used in the ProtoBuf definition. @@ -4380,6 +4386,9 @@ impl NnsFunction { NnsFunction::SetDefaultInitialDkgSubnet => { "NNS_FUNCTION_SET_DEFAULT_INITIAL_DKG_SUBNET" } + NnsFunction::UpdateGuestosVersionForSubnets => { + "NNS_FUNCTION_UPDATE_GUESTOS_VERSION_FOR_SUBNETS" + } } } /// Creates an enum from field names used in the ProtoBuf definition. @@ -4463,6 +4472,9 @@ impl NnsFunction { "NNS_FUNCTION_SPLIT_SUBNET" => Some(Self::SplitSubnet), "NNS_FUNCTION_DELETE_SUBNET" => Some(Self::DeleteSubnet), "NNS_FUNCTION_SET_DEFAULT_INITIAL_DKG_SUBNET" => Some(Self::SetDefaultInitialDkgSubnet), + "NNS_FUNCTION_UPDATE_GUESTOS_VERSION_FOR_SUBNETS" => { + Some(Self::UpdateGuestosVersionForSubnets) + } _ => None, } } diff --git a/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto b/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto index 71b5afff82f4..e9f7061f2718 100644 --- a/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto +++ b/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto @@ -511,6 +511,13 @@ enum NnsFunction { // `SetupInitialDKG` requests without an explicit subnet id are routed to the // calling subnet (NNS). NNS_FUNCTION_SET_DEFAULT_INITIAL_DKG_SUBNET = 58; + + // Deploy a GuestOS version to an explicit list of subnets at once. The + // proposal changes the GuestOS version used on every subnet in the provided + // list. The version must be contained in the list of elected GuestOS + // versions. The upgrade is completed when each subnet creates the next + // regular CUP. + NNS_FUNCTION_UPDATE_GUESTOS_VERSION_FOR_SUBNETS = 59; } // Payload of a proposal that calls a function on another NNS diff --git a/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs b/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs index 3a5611e1b0b2..7f7f34d0d246 100644 --- a/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs +++ b/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs @@ -5286,6 +5286,12 @@ pub enum NnsFunction { /// `SetupInitialDKG` requests without an explicit subnet id are routed to the /// calling subnet (NNS). SetDefaultInitialDkgSubnet = 58, + /// Deploy a GuestOS version to an explicit list of subnets at once. The + /// proposal changes the GuestOS version used on every subnet in the provided + /// list. The version must be contained in the list of elected GuestOS + /// versions. The upgrade is completed when each subnet creates the next + /// regular CUP. + UpdateGuestosVersionForSubnets = 59, } impl NnsFunction { /// String value of the enum field names used in the ProtoBuf definition. @@ -5362,6 +5368,9 @@ impl NnsFunction { Self::SplitSubnet => "NNS_FUNCTION_SPLIT_SUBNET", Self::DeleteSubnet => "NNS_FUNCTION_DELETE_SUBNET", Self::SetDefaultInitialDkgSubnet => "NNS_FUNCTION_SET_DEFAULT_INITIAL_DKG_SUBNET", + Self::UpdateGuestosVersionForSubnets => { + "NNS_FUNCTION_UPDATE_GUESTOS_VERSION_FOR_SUBNETS" + } } } /// Creates an enum from field names used in the ProtoBuf definition. @@ -5445,6 +5454,9 @@ impl NnsFunction { "NNS_FUNCTION_SPLIT_SUBNET" => Some(Self::SplitSubnet), "NNS_FUNCTION_DELETE_SUBNET" => Some(Self::DeleteSubnet), "NNS_FUNCTION_SET_DEFAULT_INITIAL_DKG_SUBNET" => Some(Self::SetDefaultInitialDkgSubnet), + "NNS_FUNCTION_UPDATE_GUESTOS_VERSION_FOR_SUBNETS" => { + Some(Self::UpdateGuestosVersionForSubnets) + } _ => None, } } diff --git a/rs/nns/governance/src/pb/conversions/mod.rs b/rs/nns/governance/src/pb/conversions/mod.rs index c0ecae923268..ba60b6007dd5 100644 --- a/rs/nns/governance/src/pb/conversions/mod.rs +++ b/rs/nns/governance/src/pb/conversions/mod.rs @@ -3885,6 +3885,9 @@ impl From for api::NnsFunction { pb::NnsFunction::SetDefaultInitialDkgSubnet => { api::NnsFunction::SetDefaultInitialDkgSubnet } + pb::NnsFunction::UpdateGuestosVersionForSubnets => { + api::NnsFunction::UpdateGuestosVersionForSubnets + } } } } @@ -3985,6 +3988,9 @@ impl From for pb::NnsFunction { api::NnsFunction::SetDefaultInitialDkgSubnet => { pb::NnsFunction::SetDefaultInitialDkgSubnet } + api::NnsFunction::UpdateGuestosVersionForSubnets => { + pb::NnsFunction::UpdateGuestosVersionForSubnets + } } } } diff --git a/rs/nns/governance/src/proposals/execute_nns_function.rs b/rs/nns/governance/src/proposals/execute_nns_function.rs index 7af600eed28e..d5a14120bdcf 100644 --- a/rs/nns/governance/src/proposals/execute_nns_function.rs +++ b/rs/nns/governance/src/proposals/execute_nns_function.rs @@ -459,6 +459,7 @@ pub enum ValidNnsFunction { SplitSubnet, DeleteSubnet, SetDefaultInitialDkgSubnet, + UpdateGuestosVersionForSubnets, } impl ValidNnsFunction { @@ -468,6 +469,7 @@ impl ValidNnsFunction { ValidNnsFunction::HardResetNnsRootToVersion | ValidNnsFunction::ReviseElectedGuestosVersions | ValidNnsFunction::DeployGuestosToAllSubnetNodes + | ValidNnsFunction::UpdateGuestosVersionForSubnets ) } @@ -506,6 +508,9 @@ impl ValidNnsFunction { ValidNnsFunction::DeployGuestosToAllSubnetNodes => { (REGISTRY_CANISTER_ID, "deploy_guestos_to_all_subnet_nodes") } + ValidNnsFunction::UpdateGuestosVersionForSubnets => { + (REGISTRY_CANISTER_ID, "update_guestos_version_for_subnets") + } ValidNnsFunction::ReviseElectedHostosVersions => { (REGISTRY_CANISTER_ID, "revise_elected_hostos_versions") } @@ -630,6 +635,7 @@ impl ValidNnsFunction { ValidNnsFunction::DeployHostosToSomeNodes | ValidNnsFunction::DeployGuestosToAllSubnetNodes + | ValidNnsFunction::UpdateGuestosVersionForSubnets | ValidNnsFunction::DeployGuestosToSomeApiBoundaryNodes | ValidNnsFunction::DeployGuestosToAllUnassignedNodes => Topic::IcOsVersionDeployment, @@ -667,6 +673,9 @@ impl ValidNnsFunction { ValidNnsFunction::UpdateConfigOfSubnet => "Update Subnet Config", ValidNnsFunction::AssignNoid => "Assign Node Operator ID (NOID)", ValidNnsFunction::DeployGuestosToAllSubnetNodes => "Deploy GuestOS To All Subnet Nodes", + ValidNnsFunction::UpdateGuestosVersionForSubnets => { + "Update GuestOS Version For Subnets" + } ValidNnsFunction::ClearProvisionalWhitelist => "Clear Provisional Whitelist", ValidNnsFunction::RemoveNodesFromSubnet => "Remove Node from Subnet", ValidNnsFunction::SetAuthorizedSubnetworks => "Set Authorized Subnets", @@ -767,6 +776,12 @@ impl ValidNnsFunction { The version must be contained in the list of elected GuestOS versions.\n\n\ The upgrade is completed when the subnet creates the next regular CUP." } + ValidNnsFunction::UpdateGuestosVersionForSubnets => { + "Deploy a GuestOS version to an explicit list of subnets at once. The proposal \ + changes the GuestOS version used on every subnet in the provided list.\n\n\ + The version must be contained in the list of elected GuestOS versions.\n\n\ + The upgrade is completed when each subnet creates the next regular CUP." + } ValidNnsFunction::ClearProvisionalWhitelist => { "Clear the provisional whitelist, which allows the listed principals to create \ Canisters with cycles. The mechanism is only needed for bootstrap and testing and \ @@ -1036,6 +1051,9 @@ impl TryFrom for ValidNnsFunction { NnsFunction::SetDefaultInitialDkgSubnet => { Ok(ValidNnsFunction::SetDefaultInitialDkgSubnet) } + NnsFunction::UpdateGuestosVersionForSubnets => { + Ok(ValidNnsFunction::UpdateGuestosVersionForSubnets) + } // Obsolete functions - based on check_obsolete NnsFunction::BlessReplicaVersion | NnsFunction::RetireReplicaVersion => { diff --git a/rs/nns/governance/src/proposals/execute_nns_function_tests.rs b/rs/nns/governance/src/proposals/execute_nns_function_tests.rs index bf13e1666b06..0c5c054a5e95 100644 --- a/rs/nns/governance/src/proposals/execute_nns_function_tests.rs +++ b/rs/nns/governance/src/proposals/execute_nns_function_tests.rs @@ -124,6 +124,26 @@ fn test_execute_nns_function_try_from_errors() { } } +#[test] +fn test_update_guestos_version_for_subnets_routing() { + use ic_nns_constants::REGISTRY_CANISTER_ID; + + let valid = ValidExecuteNnsFunction::try_from(ExecuteNnsFunction { + nns_function: NnsFunction::UpdateGuestosVersionForSubnets as i32, + payload: vec![], + }) + .expect("UpdateGuestosVersionForSubnets should be a valid NNS function"); + + assert_eq!( + valid.nns_function, + ValidNnsFunction::UpdateGuestosVersionForSubnets + ); + assert_eq!( + valid.nns_function.canister_and_function(), + (REGISTRY_CANISTER_ID, "update_guestos_version_for_subnets"), + ); +} + // This tests a "normal" NNS function where the payload is translated through a candid file fetched // by the `canister_metadata` method on the management canister. #[tokio::test] diff --git a/rs/nns/governance/unreleased_changelog.md b/rs/nns/governance/unreleased_changelog.md index 25478837d207..e16e96a9dc7e 100644 --- a/rs/nns/governance/unreleased_changelog.md +++ b/rs/nns/governance/unreleased_changelog.md @@ -13,6 +13,9 @@ on the process that this file is part of, see proposes to set or unset the default subnet to which `SetupInitialDKG` management canister calls are routed when no subnet is specified explicitly in the request. +* Added a new `NnsFunction` variant `UpdateGuestosVersionForSubnets`, which + proposes to deploy a given (elected) GuestOS version to an explicit list of + subnets at once. ## Changed diff --git a/rs/registry/canister/canister/canister.rs b/rs/registry/canister/canister/canister.rs index d128079e1def..cb71f23848f6 100644 --- a/rs/registry/canister/canister/canister.rs +++ b/rs/registry/canister/canister/canister.rs @@ -66,6 +66,7 @@ use registry_canister::{ do_update_elected_hostos_versions::{ ReviseElectedHostosVersionsPayload, UpdateElectedHostosVersionsPayload, }, + do_update_guestos_version_for_subnets::UpdateGuestosVersionForSubnetsPayload, do_update_node_operator_config::UpdateNodeOperatorConfigPayload, do_update_node_operator_config_directly::UpdateNodeOperatorConfigDirectlyPayload, do_update_nodes_hostos_version::{ @@ -563,6 +564,18 @@ fn deploy_guestos_to_all_subnet_nodes_(payload: DeployGuestosToAllSubnetNodesPay recertify_registry(); } +#[unsafe(export_name = "canister_update update_guestos_version_for_subnets")] +fn update_guestos_version_for_subnets() { + check_caller_is_governance_and_log("update_guestos_version_for_subnets"); + over(candid_one, update_guestos_version_for_subnets_); +} + +#[candid_method(update, rename = "update_guestos_version_for_subnets")] +fn update_guestos_version_for_subnets_(payload: UpdateGuestosVersionForSubnetsPayload) { + registry_mut().do_update_guestos_version_for_subnets(payload); + recertify_registry(); +} + // TODO[NNS1-3000]: Remove this endpoint once mainnet NNS Governance starts calling the new // TODO[NNS1-3000]: `revise_elected_hostos_versions` endpoint. #[unsafe(export_name = "canister_update update_elected_hostos_versions")] diff --git a/rs/registry/canister/canister/registry.did b/rs/registry/canister/canister/registry.did index 9aea72cb4b98..c5662556510f 100644 --- a/rs/registry/canister/canister/registry.did +++ b/rs/registry/canister/canister/registry.did @@ -441,6 +441,11 @@ type UpdateFirewallRulesPayload = record { rules : vec FirewallRule; }; +type UpdateGuestosVersionForSubnetsPayload = record { + subnet_ids : vec principal; + replica_version_id : text; +}; + type UpdateNodeDirectlyPayload = record { idkg_dealing_encryption_pk : opt blob; }; @@ -655,6 +660,7 @@ service : { update_elected_hostos_versions : (UpdateElectedHostosVersionsPayload) -> (); revise_elected_hostos_versions : (ReviseElectedHostosVersionsPayload) -> (); update_firewall_rules : (UpdateFirewallRulesPayload) -> (); + update_guestos_version_for_subnets : (UpdateGuestosVersionForSubnetsPayload) -> (); update_node_directly : (UpdateNodeDirectlyPayload) -> (); update_node_domain_directly : (UpdateNodeDomainDirectlyPayload) -> (UpdateNodeDomainDirectlyResponse); update_node_ipv4_config_directly : (UpdateNodeIPv4ConfigDirectlyPayload) -> ( diff --git a/rs/registry/canister/canister/registry_test.did b/rs/registry/canister/canister/registry_test.did index 386a330a1972..70f5377501ce 100644 --- a/rs/registry/canister/canister/registry_test.did +++ b/rs/registry/canister/canister/registry_test.did @@ -441,6 +441,11 @@ type UpdateFirewallRulesPayload = record { rules : vec FirewallRule; }; +type UpdateGuestosVersionForSubnetsPayload = record { + subnet_ids : vec principal; + replica_version_id : text; +}; + type UpdateNodeDirectlyPayload = record { idkg_dealing_encryption_pk : opt blob; }; @@ -655,6 +660,7 @@ service : { update_elected_hostos_versions : (UpdateElectedHostosVersionsPayload) -> (); revise_elected_hostos_versions : (ReviseElectedHostosVersionsPayload) -> (); update_firewall_rules : (UpdateFirewallRulesPayload) -> (); + update_guestos_version_for_subnets : (UpdateGuestosVersionForSubnetsPayload) -> (); update_node_directly : (UpdateNodeDirectlyPayload) -> (); update_node_domain_directly : (UpdateNodeDomainDirectlyPayload) -> (UpdateNodeDomainDirectlyResponse); update_node_ipv4_config_directly : (UpdateNodeIPv4ConfigDirectlyPayload) -> ( diff --git a/rs/registry/canister/src/mutations/do_update_guestos_version_for_subnets.rs b/rs/registry/canister/src/mutations/do_update_guestos_version_for_subnets.rs new file mode 100644 index 000000000000..2c72d650a2d8 --- /dev/null +++ b/rs/registry/canister/src/mutations/do_update_guestos_version_for_subnets.rs @@ -0,0 +1,246 @@ +use crate::{ + common::LOG_PREFIX, + mutations::common::{check_replica_version_is_elected, has_duplicates}, + registry::Registry, +}; + +use candid::{CandidType, Deserialize}; +#[cfg(target_arch = "wasm32")] +use dfn_core::println; +use ic_base_types::{PrincipalId, SubnetId}; +use ic_registry_keys::make_subnet_record_key; +use ic_registry_transport::pb::v1::{RegistryMutation, registry_mutation}; +use prost::Message; +use serde::Serialize; + +impl Registry { + /// Sets the `replica_version_id` of every subnet in `payload.subnet_ids` to + /// the given (elected) GuestOS version. + /// + /// Unlike `do_deploy_guestos_to_all_subnet_nodes`, which targets a single + /// subnet, this updates an explicit list of subnets in one atomic registry + /// mutation: either all listed subnets move to the new version, or none do. + /// Subnets that are already on the requested version are skipped to avoid + /// churning the registry with no-op updates. + pub fn do_update_guestos_version_for_subnets( + &mut self, + payload: UpdateGuestosVersionForSubnetsPayload, + ) { + println!("{LOG_PREFIX}do_update_guestos_version_for_subnets: {payload:?}"); + + check_replica_version_is_elected(self, &payload.replica_version_id); + + assert!( + !payload.subnet_ids.is_empty(), + "{LOG_PREFIX}do_update_guestos_version_for_subnets: subnet_ids must not be empty.", + ); + assert!( + !has_duplicates(&payload.subnet_ids), + "{LOG_PREFIX}do_update_guestos_version_for_subnets: subnet_ids must not contain duplicates.", + ); + + let mut mutations = vec![]; + for subnet_id in &payload.subnet_ids { + let subnet_id = SubnetId::from(*subnet_id); + let mut subnet_record = self + .get_subnet(subnet_id, self.latest_version()) + .unwrap_or_else(|err| { + panic!("{LOG_PREFIX}do_update_guestos_version_for_subnets: {err}") + }); + + if subnet_record.replica_version_id == payload.replica_version_id { + continue; + } + + subnet_record.replica_version_id = payload.replica_version_id.clone(); + mutations.push(RegistryMutation { + mutation_type: registry_mutation::Type::Update as i32, + key: make_subnet_record_key(subnet_id).into_bytes(), + value: subnet_record.encode_to_vec(), + }); + } + + println!( + "{LOG_PREFIX}do_update_guestos_version_for_subnets: updating {} of {} requested subnet(s) to version {}", + mutations.len(), + payload.subnet_ids.len(), + payload.replica_version_id, + ); + + // Check invariants before applying mutations. An empty mutation list is + // a no-op (e.g. when all listed subnets are already on the requested + // version). + self.maybe_apply_mutation_internal(mutations) + } +} + +/// The argument of a command to update the GuestOS (replica) version of a +/// specific list of subnets to a single version. +/// +/// The subnets will be mutated only if the given version is, indeed, elected. +#[derive(Clone, Eq, PartialEq, Debug, CandidType, Deserialize, Serialize)] +pub struct UpdateGuestosVersionForSubnetsPayload { + /// The subnets to update. + pub subnet_ids: Vec, // SubnetId See NNS-73 + /// The new GuestOS (replica) version to use. + pub replica_version_id: String, +} + +#[cfg(test)] +mod tests { + use ic_base_types::{NodeId, PrincipalId, SubnetId}; + use ic_protobuf::registry::crypto::v1::PublicKey; + use ic_protobuf::registry::replica_version::v1::ReplicaVersionRecord; + use ic_registry_keys::make_replica_version_key; + use ic_registry_transport::insert; + use ic_test_utilities_types::ids::subnet_test_id; + use maplit::btreemap; + use prost::Message; + + use crate::common::test_helpers::{ + add_fake_subnet, get_invariant_compliant_subnet_record, invariant_compliant_registry, + prepare_registry_with_nodes, + }; + use crate::registry::Registry; + + use super::UpdateGuestosVersionForSubnetsPayload; + + const TARGET_VERSION: &str = "version"; + + /// Elects `TARGET_VERSION` so the update passes the elected-version check. + fn elect_target_version(registry: &mut Registry) { + registry.maybe_apply_mutation_internal(vec![insert( + make_replica_version_key(TARGET_VERSION), + ReplicaVersionRecord { + release_package_sha256_hex: "".into(), + release_package_urls: vec![], + guest_launch_measurements: None, + } + .encode_to_vec(), + )]); + } + + /// Adds an application subnet backed by `node_id` and returns its id. + fn add_subnet( + registry: &mut Registry, + node_id: NodeId, + dkg_pk: PublicKey, + subnet_index: u64, + ) -> SubnetId { + let subnet_record = get_invariant_compliant_subnet_record(vec![node_id]); + let mut subnet_list_record = registry.get_subnet_list_record(); + let subnet_id = subnet_test_id(subnet_index); + registry.maybe_apply_mutation_internal(add_fake_subnet( + subnet_id, + &mut subnet_list_record, + subnet_record, + &btreemap!(node_id => dkg_pk), + )); + subnet_id + } + + fn replica_version_of(registry: &Registry, subnet_id: SubnetId) -> String { + registry.get_subnet_or_panic(subnet_id).replica_version_id + } + + /// Sets up a registry with three application subnets and elects the target + /// version. Returns the three subnet ids. + fn registry_with_three_subnets() -> (Registry, [SubnetId; 3]) { + let mut registry = invariant_compliant_registry(0); + elect_target_version(&mut registry); + + let (mutate_request, nodes) = prepare_registry_with_nodes(1, 3); + registry.maybe_apply_mutation_internal(mutate_request.mutations); + + let mut nodes = nodes.into_iter(); + let (n1, pk1) = nodes.next().unwrap(); + let (n2, pk2) = nodes.next().unwrap(); + let (n3, pk3) = nodes.next().unwrap(); + + let s1 = add_subnet(&mut registry, n1, pk1, 1001); + let s2 = add_subnet(&mut registry, n2, pk2, 1002); + let s3 = add_subnet(&mut registry, n3, pk3, 1003); + (registry, [s1, s2, s3]) + } + + #[test] + #[should_panic(expected = "'version' is NOT elected")] + fn should_panic_if_version_not_elected() { + let mut registry = invariant_compliant_registry(0); + + registry.do_update_guestos_version_for_subnets(UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![subnet_test_id(1001).get()], + replica_version_id: TARGET_VERSION.into(), + }); + } + + #[test] + #[should_panic(expected = "subnet_ids must not be empty")] + fn should_panic_on_empty_subnet_list() { + let mut registry = invariant_compliant_registry(0); + elect_target_version(&mut registry); + + registry.do_update_guestos_version_for_subnets(UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![], + replica_version_id: TARGET_VERSION.into(), + }); + } + + #[test] + #[should_panic(expected = "must not contain duplicates")] + fn should_panic_on_duplicate_subnets() { + let (mut registry, [s1, ..]) = registry_with_three_subnets(); + + registry.do_update_guestos_version_for_subnets(UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![s1.get(), s1.get()], + replica_version_id: TARGET_VERSION.into(), + }); + } + + #[test] + #[should_panic(expected = "not found in the registry")] + fn should_panic_on_unknown_subnet() { + let mut registry = invariant_compliant_registry(0); + elect_target_version(&mut registry); + + registry.do_update_guestos_version_for_subnets(UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![PrincipalId::new_subnet_test_id(12345)], + replica_version_id: TARGET_VERSION.into(), + }); + } + + #[test] + fn should_update_only_listed_subnets() { + let (mut registry, [s1, s2, s3]) = registry_with_three_subnets(); + + registry.do_update_guestos_version_for_subnets(UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![s1.get(), s2.get()], + replica_version_id: TARGET_VERSION.into(), + }); + + assert_eq!(replica_version_of(®istry, s1), TARGET_VERSION); + assert_eq!(replica_version_of(®istry, s2), TARGET_VERSION); + // The subnet that was not listed must be untouched. + assert_ne!(replica_version_of(®istry, s3), TARGET_VERSION); + } + + #[test] + fn should_be_noop_when_listed_subnets_already_on_version() { + let (mut registry, [s1, ..]) = registry_with_three_subnets(); + + // Bring s1 to the target version first. + registry.do_update_guestos_version_for_subnets(UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![s1.get()], + replica_version_id: TARGET_VERSION.into(), + }); + let version_before = registry.latest_version(); + + // Re-applying for the same subnet produces no mutation, hence no bump. + registry.do_update_guestos_version_for_subnets(UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![s1.get()], + replica_version_id: TARGET_VERSION.into(), + }); + + assert_eq!(registry.latest_version(), version_before); + } +} diff --git a/rs/registry/canister/src/mutations/mod.rs b/rs/registry/canister/src/mutations/mod.rs index 871e33b0cf6a..47b683c58296 100644 --- a/rs/registry/canister/src/mutations/mod.rs +++ b/rs/registry/canister/src/mutations/mod.rs @@ -27,6 +27,7 @@ pub mod do_split_subnet; pub mod do_swap_node_in_subnet_directly; pub mod do_update_api_boundary_nodes_version; pub mod do_update_elected_hostos_versions; +pub mod do_update_guestos_version_for_subnets; pub mod do_update_node_directly; pub mod do_update_node_operator_config; pub mod do_update_node_operator_config_directly; diff --git a/rs/registry/canister/tests/update_guestos_version_for_subnets.rs b/rs/registry/canister/tests/update_guestos_version_for_subnets.rs new file mode 100644 index 000000000000..13f8795e1701 --- /dev/null +++ b/rs/registry/canister/tests/update_guestos_version_for_subnets.rs @@ -0,0 +1,239 @@ +use candid::{Encode, Principal}; +use ic_base_types::{PrincipalId, SubnetId}; +use ic_nervous_system_integration_tests::pocket_ic_helpers::nns::registry::decode_registry_value; +use ic_nns_constants::{GOVERNANCE_CANISTER_ID, REGISTRY_CANISTER_ID}; +use ic_nns_test_utils::itest_helpers::{ + forward_call_via_universal_canister, set_up_registry_canister, set_up_universal_canister, + state_machine_test_on_nns_subnet, +}; +use ic_nns_test_utils::registry::invariant_compliant_mutation_as_atomic_req; +use ic_protobuf::registry::subnet::v1::{SubnetListRecord as SubnetListRecordPb, SubnetRecord}; +use ic_registry_keys::{make_subnet_list_record_key, make_subnet_record_key}; +use pocket_ic::PocketIcBuilder; +use pocket_ic::nonblocking::PocketIc; +use registry_canister::{ + init::RegistryCanisterInitPayloadBuilder, + mutations::{ + do_revise_elected_replica_versions::ReviseElectedGuestosVersionsPayload, + do_update_guestos_version_for_subnets::UpdateGuestosVersionForSubnetsPayload, + }, +}; + +mod common; + +use common::test_helpers::install_registry_canister_with_payload_builder; + +const MOCK_HASH: &str = "acdcacdcacdcacdcacdcacdcacdcacdcacdcacdcacdcacdcacdcacdcacdcacdc"; +const NEW_VERSION: &str = "version_43"; + +/// Installs an invariant-compliant registry (which already contains a single, +/// non-CloudEngine subnet) and returns the running `PocketIc` together with the +/// principal of an existing subnet that can be used as an update target. +async fn setup_with_existing_subnet() -> (PocketIc, Principal) { + let pocket_ic = PocketIcBuilder::new().with_nns_subnet().build_async().await; + + let mut builder = RegistryCanisterInitPayloadBuilder::new(); + builder.push_init_mutate_request(invariant_compliant_mutation_as_atomic_req(0)); + install_registry_canister_with_payload_builder(&pocket_ic, builder.build(), true).await; + + let subnet_list_record = + decode_registry_value::(&pocket_ic, make_subnet_list_record_key()) + .await; + let subnet_id = subnet_list_record + .subnets + .first() + .expect("expected the invariant-compliant registry to contain at least one subnet"); + let subnet_id = Principal::try_from(subnet_id.as_slice()).unwrap(); + + (pocket_ic, subnet_id) +} + +/// Elects `NEW_VERSION` by submitting a `revise_elected_replica_versions` call +/// as the governance canister. +async fn elect_new_version(pocket_ic: &PocketIc) { + let payload = ReviseElectedGuestosVersionsPayload { + replica_version_to_elect: Some(NEW_VERSION.into()), + release_package_sha256_hex: Some(MOCK_HASH.into()), + release_package_urls: vec!["http://release_package.tar.zst".into()], + guest_launch_measurements: None, + replica_versions_to_unelect: vec![], + }; + pocket_ic + .update_call( + REGISTRY_CANISTER_ID.get().0, + GOVERNANCE_CANISTER_ID.get().0, + "revise_elected_replica_versions", + Encode!(&payload).unwrap(), + ) + .await + .expect("failed to elect a new replica version"); +} + +fn subnet_replica_version(record: &SubnetRecord) -> &str { + &record.replica_version_id +} + +#[tokio::test] +async fn test_the_anonymous_user_cannot_update_guestos_version_for_subnets() { + let (pocket_ic, subnet_id) = setup_with_existing_subnet().await; + + let payload = UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![PrincipalId::from(subnet_id)], + replica_version_id: NEW_VERSION.into(), + }; + + // The anonymous end-user tries to update subnets via an ingress message, + // bypassing governance. This should be rejected by the authorization check. + let response = pocket_ic + .update_call( + REGISTRY_CANISTER_ID.get().0, + PrincipalId::new_anonymous().0, + "update_guestos_version_for_subnets", + Encode!(&payload).unwrap(), + ) + .await; + + assert!( + response.as_ref().is_err_and(|err| err + .reject_message + .contains("is not authorized to call this method: update_guestos_version_for_subnets")), + "Expected an authorization rejection, but got {response:?}" + ); +} + +#[tokio::test] +async fn test_an_unauthorized_principal_cannot_update_guestos_version_for_subnets() { + let (pocket_ic, subnet_id) = setup_with_existing_subnet().await; + + // A principal that is not the governance canister, calling via an ingress + // message. + let unauthorized_caller = PrincipalId::new_user_test_id(1); + assert_ne!(unauthorized_caller, GOVERNANCE_CANISTER_ID.get()); + + let payload = UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![PrincipalId::from(subnet_id)], + replica_version_id: NEW_VERSION.into(), + }; + + let response = pocket_ic + .update_call( + REGISTRY_CANISTER_ID.get().0, + unauthorized_caller.0, + "update_guestos_version_for_subnets", + Encode!(&payload).unwrap(), + ) + .await; + + assert!( + response.as_ref().is_err_and(|err| err + .reject_message + .contains("is not authorized to call this method: update_guestos_version_for_subnets")), + "Expected an authorization rejection, but got {response:?}" + ); +} + +#[test] +fn test_a_canister_other_than_governance_cannot_update_guestos_version_for_subnets() { + state_machine_test_on_nns_subnet(|runtime| async move { + // An attacker canister tries to update subnets via an inter-canister + // call. Going through a real canister (rather than an ingress message) + // ensures the access control cannot be bypassed by, e.g., only guarding + // ingress messages in `inspect_message`. + let attacker_canister = set_up_universal_canister(&runtime).await; + assert_ne!( + attacker_canister.canister_id(), + ic_nns_constants::GOVERNANCE_CANISTER_ID + ); + + let registry = set_up_registry_canister( + &runtime, + RegistryCanisterInitPayloadBuilder::new() + .push_init_mutate_request(invariant_compliant_mutation_as_atomic_req(0)) + .build(), + ) + .await; + + let payload = UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![PrincipalId::new_subnet_test_id(999)], + replica_version_id: NEW_VERSION.into(), + }; + + // The attacker canister's call should have no effect. + assert!( + !forward_call_via_universal_canister( + &attacker_canister, + ®istry, + "update_guestos_version_for_subnets", + Encode!(&payload).unwrap() + ) + .await + ); + + Ok(()) + }); +} + +#[tokio::test] +async fn test_governance_can_update_guestos_version_for_subnets() { + let (pocket_ic, subnet_id) = setup_with_existing_subnet().await; + let subnet_key = make_subnet_record_key(SubnetId::from(PrincipalId::from(subnet_id))); + + // The subnet must not already be on the new version, otherwise the update + // would be a no-op and this test would not prove anything. + let before = decode_registry_value::(&pocket_ic, &subnet_key).await; + assert_ne!(subnet_replica_version(&before), NEW_VERSION); + + elect_new_version(&pocket_ic).await; + + let payload = UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![PrincipalId::from(subnet_id)], + replica_version_id: NEW_VERSION.into(), + }; + pocket_ic + .update_call( + REGISTRY_CANISTER_ID.get().0, + GOVERNANCE_CANISTER_ID.get().0, + "update_guestos_version_for_subnets", + Encode!(&payload).unwrap(), + ) + .await + .expect("governance update_guestos_version_for_subnets call was unexpectedly rejected"); + + let after = decode_registry_value::(&pocket_ic, &subnet_key).await; + assert_eq!(subnet_replica_version(&after), NEW_VERSION); +} + +#[tokio::test] +async fn test_unelected_version_is_rejected() { + let (pocket_ic, subnet_id) = setup_with_existing_subnet().await; + let subnet_key = make_subnet_record_key(SubnetId::from(PrincipalId::from(subnet_id))); + let before = decode_registry_value::(&pocket_ic, &subnet_key).await; + + let payload = UpdateGuestosVersionForSubnetsPayload { + subnet_ids: vec![PrincipalId::from(subnet_id)], + replica_version_id: "unelected".into(), + }; + + let response = pocket_ic + .update_call( + REGISTRY_CANISTER_ID.get().0, + GOVERNANCE_CANISTER_ID.get().0, + "update_guestos_version_for_subnets", + Encode!(&payload).unwrap(), + ) + .await; + + assert!( + response + .as_ref() + .is_err_and(|err| err.reject_message.contains("is NOT elected")), + "Expected a rejection because the version is not elected, but got {response:?}" + ); + + // The subnet record must be unchanged. + let after = decode_registry_value::(&pocket_ic, &subnet_key).await; + assert_eq!( + subnet_replica_version(&after), + subnet_replica_version(&before) + ); +} diff --git a/rs/registry/canister/unreleased_changelog.md b/rs/registry/canister/unreleased_changelog.md index 93c14851bbe2..a6a7b0c35d61 100644 --- a/rs/registry/canister/unreleased_changelog.md +++ b/rs/registry/canister/unreleased_changelog.md @@ -21,6 +21,11 @@ on the process that this file is part of, see * Added `vcpu_type` to `GuestLaunchMeasurementMetadata` to record the virtual CPU type used for a guest launch measurement. * Added a new endpoint `get_subnet` to the registry canister, returning the subnet record of a given subnet. +* Added a new endpoint `update_guestos_version_for_subnets` to the registry + canister, which sets the `replica_version_id` of an explicit list of subnets + to a given (elected) GuestOS version in a single atomic mutation. Subnets not + in the list are left untouched, and subnets already on the requested version + are skipped. ## Changed