feat: allow engine-controller canister to update cloud engines directly

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<PrincipalId>) -> Vec<PrincipalId> {
+    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.
 }

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())]);
+}