diff --git a/Cargo.lock b/Cargo.lock index a5559f584c1..210b89c2f64 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13655,7 +13655,9 @@ name = "ic_boundary_node_system_tests" version = "0.9.0" dependencies = [ "anyhow", + "async-trait", "candid", + "canister-test", "certificate_orchestrator_interface", "ic-agent", "ic-base-types", diff --git a/rs/tests/boundary_nodes/BUILD.bazel b/rs/tests/boundary_nodes/BUILD.bazel index 9d9b4a7982c..6916aa14f96 100644 --- a/rs/tests/boundary_nodes/BUILD.bazel +++ b/rs/tests/boundary_nodes/BUILD.bazel @@ -127,6 +127,7 @@ system_test_nns( env = { "RATE_LIMIT_CANISTER_WASM_PATH": "$(rootpath //rs/boundary_node/rate_limits:rate_limit_canister)", }, + proc_macro_deps = ["@crate_index//:async-trait"], tags = [ "k8s", ], @@ -140,6 +141,7 @@ system_test_nns( "//rs/nns/constants", "//rs/nns/test_utils", "//rs/registry/subnet_type", + "//rs/rust_canisters/canister_test", "//rs/tests/boundary_nodes/utils", "//rs/tests/driver:ic-system-test-driver", "//rs/types/base_types", diff --git a/rs/tests/boundary_nodes/Cargo.toml b/rs/tests/boundary_nodes/Cargo.toml index da97cc1a076..354485874b3 100644 --- a/rs/tests/boundary_nodes/Cargo.toml +++ b/rs/tests/boundary_nodes/Cargo.toml @@ -8,7 +8,9 @@ documentation.workspace = true [dependencies] anyhow = { workspace = true } +async-trait = { workspace = true } candid = { workspace = true } +canister-test = { path = "../../rust_canisters/canister_test" } certificate_orchestrator_interface = { path = "../../boundary_node/certificate_issuance/certificate_orchestrator_interface" } ic-agent = { workspace = true } ic-base-types = { path = "../../types/base_types" } diff --git a/rs/tests/boundary_nodes/rate_limit_canister_test.rs b/rs/tests/boundary_nodes/rate_limit_canister_test.rs index 5aba85a8aea..a59efa82fd8 100644 --- a/rs/tests/boundary_nodes/rate_limit_canister_test.rs +++ b/rs/tests/boundary_nodes/rate_limit_canister_test.rs @@ -4,42 +4,55 @@ Title:: Rate-limit canister integration with API boundary nodes Goal:: Ensure rate-limit rules can be added to the canister and are properly enforced by API Boundary Nodes. Runbook: -1. Set up an Internet Computer (IC) with a system-subnet and an API boundary node. -2. Install the rate-limit canister at a specified mainnet ID. -3. Install the counter canister, which is used for testing enforced rate-limits. -4. Create an `ic-agent` instance associated with an API boundary node. -5. Verify that initially the agent can successfully interact with the counter canister by sending e.g. an update call. -6. Add a rate-limit rule to the rate-limit canister, which completely blocks requests to the counter canister. -// TODO: BOUN-1330 - investigate the reason of flakiness in Step 7, temporarily disable steps below. -7. Verify that the agent can no longer send requests to the counter canister after API boundary node enforces the new rule. -8. Add a rate-limit rule, which unblocks requests to the counter canister. -9. Verify that the agent can send requests to the counter canister again, ensuring that updated rate-limit rules are enforced correctly by API boundary nodes. +1. Set up an Internet Computer (IC) with a system-subnet and an API boundary node. +2. Install the rate-limit canister at a specified mainnet ID, without specifying an authorized_principal in the payload argument. +3. Install the counter canister, which is used for testing enforced rate-limits. +4. Create an `ic-agent` instance associated with an API boundary node. +5. Verify that initially the agent can successfully interact with the counter canister by sending e.g. an update call. +6. Try to add two rate-limit rules to the rate-limit canister, which completely block requests to two canisters: counter and rate-limit (self blocking). +7. Assert canister call fails (rejected), as authorized_principal is unset for the rate-limit canister. +8. Upgrade rate-limit canister code via proposal, specifying authorized_principal in the payload. +9. Retry step 6 and assert it succeeds. +10. Verify that the agent can no longer send requests to the counter canister after API boundary node enforces the new rule. +11. Add a rate-limit rule, which explicitly unblocks requests to the counter canister. + Setting this rule should still be possible despite the rate-limit canister being blocked itself (as there is an explicit allow-rule in the ic-boundary). +12. Verify that the agent can send requests to the counter canister again, ensuring that updated rate-limit rules are enforced correctly by API boundary nodes. end::catalog[] */ -use anyhow::Result; +use anyhow::{bail, Result}; +use async_trait::async_trait; use candid::{Decode, Encode, Principal}; +use canister_test::{Canister, Wasm}; use ic_base_types::PrincipalId; use ic_boundary_nodes_system_test_utils::{ constants::COUNTER_CANISTER_WAT, helpers::install_canisters, }; -use ic_nns_test_utils::itest_helpers::install_rust_canister_from_path; +use ic_nns_constants::{GOVERNANCE_CANISTER_ID, ROOT_CANISTER_ID}; +use ic_nns_test_utils::{ + common::modify_wasm_bytes, governance::upgrade_nns_canister_by_proposal, + itest_helpers::install_rust_canister_from_path, +}; use k256::elliptic_curve::SecretKey; use rand::{rngs::OsRng, SeedableRng}; use rand_chacha::ChaChaRng; use slog::info; -use std::{env, net::SocketAddr}; +use std::{env, net::SocketAddr, sync::Arc, time::Duration}; use tokio::runtime::Runtime; use ic_agent::{ - agent::http_transport::reqwest_transport::reqwest::Client, identity::Secp256k1Identity, Agent, - Identity, + agent::{ + http_transport::reqwest_transport::reqwest::{Client, Request, Response}, + HttpService, + }, + identity::Secp256k1Identity, + Agent, AgentError, Identity, }; use ic_registry_subnet_type::SubnetType; use ic_system_test_driver::{ driver::test_env_api::NnsInstallationBuilder, driver::{ - group::{SystemTestGroup, SystemTestSubGroup}, + group::SystemTestGroup, ic::InternetComputer, test_env::TestEnv, test_env_api::{ @@ -47,7 +60,7 @@ use ic_system_test_driver::{ IcNodeContainer, }, }, - systest, + retry_with_msg_async, systest, util::runtime_from_url, }; use rate_limits_api::{ @@ -100,7 +113,7 @@ async fn test_async(env: TestEnv) { info!( &logger, - "Step 2. Install the rate-limit canister at a specified mainnet ID {rate_limit_id}" + "Step 2. Install the rate-limit canister at a specified mainnet ID {rate_limit_id} (do not set any authorized_principal)" ); let mut rate_limit_canister = nns @@ -108,29 +121,38 @@ async fn test_async(env: TestEnv) { .await .unwrap(); + let path_to_wasm = get_dependency_path( + env::var("RATE_LIMIT_CANISTER_WASM_PATH").expect("RATE_LIMIT_CANISTER_WASM_PATH not set"), + ); + + let wasm: Wasm = Wasm::from_file(path_to_wasm.clone()); + let args = Encode!(&InitArg { registry_polling_period_secs: 5, - authorized_principal: Some(full_access_principal), + authorized_principal: None, }) .unwrap(); - info!(&logger, "Installing rate-limit canister wasm ..."); + info!( + &logger, + "Installing rate-limit canister wasm (with unset authorized_principal)..." + ); - install_rust_canister_from_path( - &mut rate_limit_canister, - get_dependency_path( - env::var("RATE_LIMIT_CANISTER_WASM_PATH") - .expect("RATE_LIMIT_CANISTER_WASM_PATH not set"), - ), - Some(args), - ) - .await; + install_rust_canister_from_path(&mut rate_limit_canister, path_to_wasm, Some(args)).await; info!( &logger, "Rate-limit canister with id={rate_limit_id} installed successfully" ); + let root = Canister::new(&nns, ROOT_CANISTER_ID); + + // set the root principal as the controller of the canister + rate_limit_canister + .set_controller(ROOT_CANISTER_ID.into()) + .await + .unwrap(); + info!(&logger, "Step 3. Installing counter canister ..."); let counter_canister_id = install_canisters( @@ -156,8 +178,8 @@ async fn test_async(env: TestEnv) { .expect("Could not create HTTP client."); let agent = Agent::builder() .with_url(format!("https://{api_bn_domain}")) - .with_http_client(client) .with_identity(full_access_identity) + .with_arc_http_middleware(Arc::new(HttpServiceNoRetry { client })) // do not use inbuilt retry logic for 429 responses .build() .unwrap(); agent.fetch_root_key().await.unwrap(); @@ -177,101 +199,202 @@ async fn test_async(env: TestEnv) { info!( &logger, - "Step 6. Add a rate-limit rule that blocks requests to counter canister" + "Step 6. Try to add two rate-limit rules that block requests to counter and rate-limit canisters" ); - set_rate_limit_rule( + let result = set_rate_limit_rules( &api_bn_agent, rate_limit_id, - RateLimitRule { - canister_id: Some(counter_canister_id), - limit: Action::Block, - ..Default::default() - }, + vec![ + InputRule { + incident_id: "b97730ac-4879-47f2-9fea-daf20b8d4b64".to_string(), + rule_raw: RateLimitRule { + canister_id: Some(counter_canister_id), + limit: Action::Block, + ..Default::default() + } + .to_bytes_json() + .unwrap(), + description: "Block requests to counter canister".to_string(), + }, + InputRule { + incident_id: "34bb6dee-9646-4543-ba62-af546ea5565b".to_string(), + rule_raw: RateLimitRule { + canister_id: Some(rate_limit_id), + limit: Action::Block, + ..Default::default() + } + .to_bytes_json() + .unwrap(), + description: "Block requests to rate-limit canister".to_string(), + }, + ], + ) + .await; + + info!( + &logger, + "Step 7. Assert canister call fails (rejected), as authorized_principal is unset for the rate-limit canister", + ); + + assert!(result.unwrap_err().contains("reject")); + + info!( + &logger, + "Step 8. Upgrade rate-limit canister code via proposal, specifying authorized_principal in the payload", + ); + + let args = Encode!(&InitArg { + registry_polling_period_secs: 5, + authorized_principal: Some(full_access_principal), + }) + .unwrap(); + + // apply a no-impact WASM modification and reinstall the canister + let new_wasm = modify_wasm_bytes(wasm.bytes().as_slice(), 42); + + upgrade_nns_canister_by_proposal( + &rate_limit_canister, + &Canister::new(&nns, GOVERNANCE_CANISTER_ID), + &root, + true, + Wasm::from_bytes(new_wasm), + Some(args), ) .await; - // TODO: BOUN-1330 - investigate the reason of flakiness in Step 7, temporarily disable steps below. - - // info!( - // &logger, - // "Step 7. Verify that the api_bn_agent can no longer send requests to the counter canister" - // ); - - // retry_with_msg_async!( - // "check_counter_canister_becomes_unreachable".to_string(), - // &logger, - // Duration::from_secs(180), - // Duration::from_secs(5), - // || async { - // match timeout( - // Duration::from_secs(2), - // api_bn_agent.update(&counter_canister_id, "write").call(), - // ) - // .await - // { - // Ok(_) => bail!("counter canister is still reachable, retrying"), - // Err(_) => Ok(()), - // } - // } - // ) - // .await - // .expect("failed to check that canister becomes unreachable"); - - // info!( - // &logger, - // "Step 8. Add a rate-limit rule, which unblocks requests to the counter canister" - // ); - - // set_rate_limit_rule( - // &api_bn_agent, - // rate_limit_id, - // RateLimitRule { - // canister_id: Some(counter_canister_id), - // limit: Action::Limit(300, Duration::from_secs(60)), - // ..Default::default() - // }, - // ) - // .await; - - // info!( - // &logger, - // "Step 9. Verify that agent can send requests to the counter canister again" - // ); - - // retry_with_msg_async!( - // "check_counter_canister_becomes_reachable".to_string(), - // &logger, - // Duration::from_secs(180), - // Duration::from_secs(5), - // || async { - // match timeout( - // Duration::from_secs(2), - // api_bn_agent.update(&counter_canister_id, "write").call(), - // ) - // .await - // { - // Ok(_) => Ok(()), - // Err(_) => bail!("counter canister is still unreachable, retrying"), - // } - // } - // ) - // .await - // .expect("failed to check that canister becomes reachable"); + info!( + &logger, + "Step 9. Assert adding two rate-limit rules to the rate-limit canister now succeeds" + ); + + set_rate_limit_rules( + &api_bn_agent, + rate_limit_id, + vec![ + InputRule { + incident_id: "b97730ac-4879-47f2-9fea-daf20b8d4b64".to_string(), + rule_raw: RateLimitRule { + canister_id: Some(counter_canister_id), + limit: Action::Block, + ..Default::default() + } + .to_bytes_json() + .unwrap(), + description: "Block requests to counter canister".to_string(), + }, + InputRule { + incident_id: "34bb6dee-9646-4543-ba62-af546ea5565b".to_string(), + rule_raw: RateLimitRule { + canister_id: Some(rate_limit_id), + limit: Action::Block, + ..Default::default() + } + .to_bytes_json() + .unwrap(), + description: "Block requests to rate-limit canister".to_string(), + }, + ], + ) + .await + .unwrap(); + + info!( + &logger, + "Step 10. Verify that the api_bn_agent can no longer send requests to the counter canister" + ); + + retry_with_msg_async!( + "check_counter_canister_becomes_unreachable".to_string(), + &logger, + Duration::from_secs(180), + Duration::from_secs(5), + || async { + match api_bn_agent + .update(&counter_canister_id, "write") + .call() + .await + { + Ok(_) => { + bail!("counter canister is still reachable, retrying"); + } + Err(error) => { + // We should observe Too Many Requests 429 http error + if let AgentError::HttpError(ref payload) = error { + if payload.status == 429 { + return Ok(()); + } + } + bail!("update call failed with unexpected error: {error:?}"); + } + } + } + ) + .await + .expect("failed to check that canister becomes unreachable"); + + info!( + &logger, + "Step 11. Add a rate-limit rule, which unblocks requests to the counter canister" + ); + + set_rate_limit_rules( + &api_bn_agent, + rate_limit_id, + vec![InputRule { + incident_id: "e6a27788-01a5-444a-9035-ab3af3ad84f3".to_string(), + rule_raw: RateLimitRule { + canister_id: Some(counter_canister_id), + limit: Action::Limit(300, Duration::from_secs(60)), + ..Default::default() + } + .to_bytes_json() + .unwrap(), + description: "Unblock requests to the counter canister".to_string(), + }], + ) + .await + .unwrap(); + + info!( + &logger, + "Step 12. Verify that agent can send requests to the counter canister again" + ); + + retry_with_msg_async!( + "check_counter_canister_becomes_reachable".to_string(), + &logger, + Duration::from_secs(180), + Duration::from_secs(5), + || async { + match api_bn_agent + .update(&counter_canister_id, "write") + .call() + .await + { + Ok(response) => { + info!(&logger, "update call succeeded with response: {response:?}"); + Ok(()) + } + Err(error) => { + info!(&logger, "update call failed with error: {error:?}"); + bail!("counter canister is still unreachable, retrying"); + } + } + } + ) + .await + .expect("failed to check that canister becomes reachable"); } -async fn set_rate_limit_rule( +async fn set_rate_limit_rules( agent: &Agent, rate_limit_canister_id: Principal, - rule: RateLimitRule, -) { + rules: Vec, +) -> Result<(), String> { let args = Encode!(&InputConfig { schema_version: 1, - rules: vec![InputRule { - incident_id: "b97730ac-4879-47f2-9fea-daf20b8d4b64".to_string(), - rule_raw: rule.to_bytes_json().unwrap(), - description: "Setting a rate-limit rule for testing".to_string(), - },], + rules, }) .unwrap(); @@ -280,9 +403,11 @@ async fn set_rate_limit_rule( .with_arg(args) .call_and_wait() .await - .unwrap(); + .map_err(|err| err.to_string())?; - let _ = Decode!(&result, AddConfigResponse).expect("failed to add new rate-limit config"); + Decode!(&result, AddConfigResponse) + .expect("failed to deserialize response") + .map_err(|err| format!("{err:?}")) } fn test(env: TestEnv) { @@ -293,7 +418,24 @@ fn test(env: TestEnv) { fn main() -> Result<()> { SystemTestGroup::new() .with_setup(setup) - .add_parallel(SystemTestSubGroup::new().add_test(systest!(test))) + .add_test(systest!(test)) .execute_from_args()?; Ok(()) } + +// The default HttpService in ic-agent retries on 429 errors, but we expect these and don't want retries. +#[derive(Debug)] +struct HttpServiceNoRetry { + client: Client, +} + +#[async_trait] +impl HttpService for HttpServiceNoRetry { + async fn call<'a>( + &'a self, + req: &'a (dyn Fn() -> Result + Send + Sync), + _max_tcp_retries: usize, + ) -> Result { + Ok(self.client.call(req, _max_tcp_retries).await?) + } +}