From 3174ee6ab083d632c46651222aeecb9c54fe0860 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Tue, 21 Jul 2026 12:54:47 +0000 Subject: [PATCH 1/4] Do not sweep conversion to ReplicaVerion errors under the rug. --- rs/registry/helpers/src/subnet.rs | 37 ++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/rs/registry/helpers/src/subnet.rs b/rs/registry/helpers/src/subnet.rs index 109a83755ea9..48f9ffea16ee 100644 --- a/rs/registry/helpers/src/subnet.rs +++ b/rs/registry/helpers/src/subnet.rs @@ -167,7 +167,7 @@ pub trait SubnetRegistry { /// [StandardEngineReplicaVersionRecord]. Non-blank `replica_version_id` /// means the same thing as in the non-CloudEngine case. /// - /// Err is returned in various cases, but call out a couple in particular, + /// Err is returned in various cases, but we call out a few in particular, /// because the error type is unintuitive: DecodeError is returned in the /// following cases: /// @@ -176,6 +176,11 @@ pub trait SubnetRegistry { /// /// 2. Non-CloudEngine with blank replica_version_id. /// + /// 3. Replica version ID string cannot be converted to a ReplicaVersion + /// object. This means that the string contains some illegal characters. + /// In particular, only latin letters, digits, dot, dash, and underscore + /// are allowed (as of July 2026). + /// /// In practice, such data problems are prevented from happening elsewhere /// (specifically, Registry's invariants checks), but we mention them here /// for completeness. @@ -437,10 +442,29 @@ impl SubnetRegistry for T { return Ok(None); }; - // Engines may opt out of the standard deployment by setting their own - // replica_version_id; if they have, that is authoritative. + let str_to_result = |replica_version_id: &str, + case: &str| + -> Result, RegistryClientError> { + let ok = ReplicaVersion::try_from(replica_version_id) + // This wouldn't happen in practice (because of validation that + // happens elsewhere), but we handle it here anyway, because + // bugs. + .map_err(|err| DecodeError { + error: format!( + "get_replica_version({subnet_id}): {case}: '{replica_version_id}' is not a valid \ + ReplicaVersion: {err}" + ), + })?; + + Ok(Some(ok)) + }; + + // Specified directly in SubnetRecord. if !subnet_record.replica_version_id.is_empty() { - return Ok(ReplicaVersion::try_from(subnet_record.replica_version_id.as_ref()).ok()); + return str_to_result( + &subnet_record.replica_version_id, + "specified directly in SubnetRecord", + ); } // Only engines are allowed to have a blank replica_version_id (i.e. @@ -480,7 +504,10 @@ impl SubnetRegistry for T { // At this point, resolved_replica_version_id should be a git commit ID // in the ic repo. This just converts it from a raw string. - Ok(ReplicaVersion::try_from(resolved_replica_version_id.as_ref()).ok()) + str_to_result( + &resolved_replica_version_id, + "using standard engine replica version", + ) } fn get_replica_version_record( From 09ba52a4673ff1b42469c59c4ef4a239ae0463cf Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Tue, 21 Jul 2026 13:31:49 +0000 Subject: [PATCH 2/4] Added some tests for completeness. --- rs/registry/helpers/src/subnet.rs | 68 +++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/rs/registry/helpers/src/subnet.rs b/rs/registry/helpers/src/subnet.rs index 48f9ffea16ee..5d375b95198f 100644 --- a/rs/registry/helpers/src/subnet.rs +++ b/rs/registry/helpers/src/subnet.rs @@ -1021,6 +1021,74 @@ mod tests { assert_matches!(result, Err(RegistryClientError::DecodeError { .. })); } + // This wouldn't occur in practice, so this test is "just" for completeness. + #[test] + fn replica_version_id_with_illegal_characters_is_an_error() { + // Step 1: Prepare the world. A SubnetRecord whose replica_version_id + // contains a character that ReplicaVersion::try_from rejects. + let data_provider = ProtoRegistryDataProvider::new(); + data_provider + .add( + &make_subnet_record_key(subnet_id(1)), + RegistryVersion::from(2), + Some(SubnetRecord { + replica_version_id: "G@RBAGE".to_string(), + ..Default::default() + }), + ) + .unwrap(); + let registry = FakeRegistryClient::new(Arc::new(data_provider)); + registry.update_to_latest_version(); + + // Step 2: Run the code under test. + let result = registry.get_replica_version(subnet_id(1), RegistryVersion::from(2)); + + // Step 3: Verify result(s). + assert_matches!(result, Err(RegistryClientError::DecodeError { .. })); + } + + // This wouldn't occur in practice, so this test is "just" for completeness. + #[test] + fn resolved_standard_engine_replica_version_id_with_illegal_characters_is_an_error() { + // Step 1: Prepare the world. An engine subnet with a blank + // replica_version_id, resolving (via + // StandardEngineReplicaVersionRecord) to a new_replica_version_id + // that ReplicaVersion::try_from rejects. + let data_provider = ProtoRegistryDataProvider::new(); + data_provider + .add( + &make_subnet_record_key(subnet_id(1)), + RegistryVersion::from(2), + Some(SubnetRecord { + replica_version_id: "".to_string(), + subnet_type: SubnetType::CloudEngine as i32, + ..Default::default() + }), + ) + .unwrap(); + data_provider + .add( + &make_standard_engine_replica_version_record_key(), + RegistryVersion::from(2), + Some(StandardEngineReplicaVersionRecord { + new_replica_version_id: "G@RBAGE".to_string(), + old_replica_version_id: "old".to_string(), + // Guarantees priority <= this, so new_replica_version_id + // (the illegal one) is the one that gets resolved. + deployment_progress: 1.0, + }), + ) + .unwrap(); + let registry = FakeRegistryClient::new(Arc::new(data_provider)); + registry.update_to_latest_version(); + + // Step 2: Run the code under test. + let result = registry.get_replica_version(subnet_id(1), RegistryVersion::from(2)); + + // Step 3: Verify result(s). + assert_matches!(result, Err(RegistryClientError::DecodeError { .. })); + } + #[test] fn can_get_is_halted_from_subnet() { let subnet_id = subnet_id(4); From 9da943a64967590171a490363c7254d3cc2d98f7 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Tue, 21 Jul 2026 19:14:04 +0000 Subject: [PATCH 3/4] Make Orchestrator's get_replica_version method a thin wrapper around the get_replica_version method of RegistryClient. --- rs/orchestrator/BUILD.bazel | 1 + rs/orchestrator/Cargo.toml | 1 + rs/orchestrator/src/registry_helper.rs | 19 ++- rs/orchestrator/src/registry_helper_tests.rs | 140 +++++++++++++++++++ 4 files changed, 158 insertions(+), 3 deletions(-) create mode 100644 rs/orchestrator/src/registry_helper_tests.rs diff --git a/rs/orchestrator/BUILD.bazel b/rs/orchestrator/BUILD.bazel index 06df34682a00..9b28a6bb2b92 100644 --- a/rs/orchestrator/BUILD.bazel +++ b/rs/orchestrator/BUILD.bazel @@ -137,6 +137,7 @@ rust_test( "@crate_index//:assert_matches", "@crate_index//:async-stream", "@crate_index//:hyper-util", + "@crate_index//:lazy_static", "@crate_index//:libc", "@crate_index//:mockall", "@crate_index//:rcgen", diff --git a/rs/orchestrator/Cargo.toml b/rs/orchestrator/Cargo.toml index 4fa2b992e93d..59ed59cf485f 100644 --- a/rs/orchestrator/Cargo.toml +++ b/rs/orchestrator/Cargo.toml @@ -81,6 +81,7 @@ qrcode = { workspace = true } async-stream = { workspace = true } assert_matches = { workspace = true } hyper-util = { workspace = true } +lazy_static = { workspace = true } ic-crypto-temp-crypto = { path = "../crypto/temp_crypto" } ic-crypto-test-utils-canister-threshold-sigs = { path = "../crypto/test_utils/canister_threshold_sigs" } ic-crypto-test-utils-crypto-returning-ok = { path = "../crypto/test_utils/crypto_returning_ok" } diff --git a/rs/orchestrator/src/registry_helper.rs b/rs/orchestrator/src/registry_helper.rs index f0dbfae279cb..908aa3e2cb89 100644 --- a/rs/orchestrator/src/registry_helper.rs +++ b/rs/orchestrator/src/registry_helper.rs @@ -241,9 +241,18 @@ impl RegistryHelper { subnet_id: SubnetId, registry_version: RegistryVersion, ) -> OrchestratorResult { - let subnet_record = self.get_subnet_record(subnet_id, registry_version)?; - ReplicaVersion::try_from(subnet_record.replica_version_id.as_ref()) - .map_err(OrchestratorError::ReplicaVersionParseError) + let replica_version = self + .registry_client + .get_replica_version(subnet_id, registry_version)?; + + let Some(replica_version) = replica_version else { + return Err(OrchestratorError::SubnetMissingError( + subnet_id, + registry_version, + )); + }; + + Ok(replica_version) } /// Get the recalled replica versions of the given subnet in the given registry @@ -403,3 +412,7 @@ impl RegistryHelper { result.ok_or_else(|| OrchestratorError::DomainNameMissingError(self.node_id, version)) } } + +#[cfg(test)] +#[path = "./registry_helper_tests.rs"] +mod tests; diff --git a/rs/orchestrator/src/registry_helper_tests.rs b/rs/orchestrator/src/registry_helper_tests.rs new file mode 100644 index 000000000000..934b3fce72aa --- /dev/null +++ b/rs/orchestrator/src/registry_helper_tests.rs @@ -0,0 +1,140 @@ +use super::*; +use ic_logger::no_op_logger; +use ic_protobuf::registry::standard_engine_replica_version::v1::StandardEngineReplicaVersionRecord; +use ic_registry_client_fake::FakeRegistryClient; +use ic_registry_keys::{make_standard_engine_replica_version_record_key, make_subnet_record_key}; +use ic_registry_proto_data_provider::ProtoRegistryDataProvider; +use ic_types::PrincipalId; +use lazy_static::lazy_static; + +fn subnet_id(id: u64) -> SubnetId { + SubnetId::from(PrincipalId::new_subnet_test_id(id)) +} + +lazy_static! { + static ref REGISTRY_VERSION: RegistryVersion = RegistryVersion::new(42); + static ref SUBNET_ID: SubnetId = subnet_id(777); +} + +fn new_registry_helper(data_provider: ProtoRegistryDataProvider) -> RegistryHelper { + let registry_client = Arc::new(FakeRegistryClient::new(Arc::new(data_provider))); + registry_client.update_to_latest_version(); + + RegistryHelper::new( + NodeId::from(PrincipalId::new_node_test_id(1)), + registry_client, + no_op_logger(), + ) +} + +#[test] +fn test_get_replica_version_when_specified_directly() { + // Step 1: Prepare the world. + + let data_provider = ProtoRegistryDataProvider::new(); + + data_provider + .add( + &make_subnet_record_key(*SUBNET_ID), + *REGISTRY_VERSION, + Some(SubnetRecord { + replica_version_id: "some_version".to_string(), + ..Default::default() + }), + ) + .unwrap(); + + let registry = new_registry_helper(data_provider); + + // Step 2: Run the code under test. + let result = registry.get_replica_version(*SUBNET_ID, *REGISTRY_VERSION); + + // Step 3: Verify result(s). + assert_eq!( + result.unwrap(), + ReplicaVersion::try_from("some_version").unwrap(), + ); +} + +#[test] +fn test_get_replica_version_standard_engine_replica_version() { + for (deployment_progress, expected_replica_version_id) in [(1.0, "new"), (0.0, "old")] { + // Step 1: Prepare the world. + + let data_provider = ProtoRegistryDataProvider::new(); + + // CloudEngine that follows the standard replica version, i.e. has blank + // replica_version_id. + data_provider + .add( + &make_subnet_record_key(*SUBNET_ID), + *REGISTRY_VERSION, + Some(SubnetRecord { + replica_version_id: "".to_string(), + subnet_type: SubnetType::CloudEngine as i32, + ..Default::default() + }), + ) + .unwrap(); + + // Standard engine replica version. + data_provider + .add( + &make_standard_engine_replica_version_record_key(), + *REGISTRY_VERSION, + Some(StandardEngineReplicaVersionRecord { + new_replica_version_id: "new".to_string(), + old_replica_version_id: "old".to_string(), + deployment_progress, + }), + ) + .unwrap(); + + let registry = new_registry_helper(data_provider); + + // Step 2: Run the code under test. + let result = registry.get_replica_version(*SUBNET_ID, *REGISTRY_VERSION); + + // Step 3: Verify result(s). + assert_eq!( + result.unwrap(), + ReplicaVersion::try_from(expected_replica_version_id).unwrap(), + ); + } +} + +#[test] +fn get_replica_version_is_an_error_when_the_subnet_is_missing() { + // Step 1: Prepare the world. + + let data_provider = ProtoRegistryDataProvider::new(); + + // This isn't really used by this test, but in order for this test to be + // not completely trivial, we need SOME registry data. + data_provider + .add( + &make_subnet_record_key(*SUBNET_ID), + *REGISTRY_VERSION, + Some(SubnetRecord { + replica_version_id: "some_version".to_string(), + ..Default::default() + }), + ) + .unwrap(); + + let registry = new_registry_helper(data_provider); + + // Step 2: Run the code under test. + let result = registry.get_replica_version(subnet_id(0x_DEAD_BEEF), *REGISTRY_VERSION); + + // Step 3: Verify result(s). + match result { + Err(OrchestratorError::SubnetMissingError(observed_subnet_id, observed_version)) => { + assert_eq!( + (observed_subnet_id, observed_version), + (subnet_id(0x_DEAD_BEEF), *REGISTRY_VERSION), + ); + } + wrong => panic!("{wrong:?}"), + } +} From f3435a2f7219d4f4dbafdff56f5e210436f82ea0 Mon Sep 17 00:00:00 2001 From: IDX GitHub Automation Date: Tue, 21 Jul 2026 19:25:49 +0000 Subject: [PATCH 4/4] Automatically updated Cargo*.lock --- Cargo.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.lock b/Cargo.lock index 2ae7c384f850..0bf74456f38d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -19255,6 +19255,7 @@ dependencies = [ "ic-types", "idna", "indoc 1.0.9", + "lazy_static", "libc", "mockall 0.13.1", "nix 0.24.3",