diff --git a/rs/config/src/execution_environment.rs b/rs/config/src/execution_environment.rs index 1667a0b7cb6..9490d72dc4f 100644 --- a/rs/config/src/execution_environment.rs +++ b/rs/config/src/execution_environment.rs @@ -250,6 +250,10 @@ pub struct Config { /// Indicates whether canister backup and restore feature is enabled or not. pub canister_snapshots: FlagStatus, + + // TODO(IC-272): remove this flag once the feature is enabled by default. + /// Indicates whether fetching canister logs API is enabled or not. + pub fetch_canister_logs: FlagStatus, } impl Default for Config { @@ -319,6 +323,7 @@ impl Default for Config { wasm_chunk_store: FlagStatus::Enabled, stop_canister_timeout_duration: STOP_CANISTER_TIMEOUT_DURATION, canister_snapshots: FlagStatus::Disabled, + fetch_canister_logs: FlagStatus::Disabled, } } } diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index cbaa3b59040..adeeea7b979 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -17,8 +17,8 @@ use ic_cycles_account_manager::{CyclesAccountManager, ResourceSaturation}; use ic_error_types::{ErrorCode, RejectCode, UserError}; use ic_ic00_types::{ CanisterChangeDetails, CanisterChangeOrigin, CanisterInstallModeV2, CanisterStatusResultV2, - CanisterStatusType, InstallChunkedCodeArgs, InstallCodeArgsV2, LogVisibility, - Method as Ic00Method, StoredChunksReply, UploadChunkReply, + CanisterStatusType, InstallChunkedCodeArgs, InstallCodeArgsV2, Method as Ic00Method, + StoredChunksReply, UploadChunkReply, }; use ic_interfaces::execution_environment::{ CanisterOutOfCyclesError, HypervisorError, IngressHistoryWriter, SubnetAvailableMemory, @@ -437,6 +437,7 @@ impl CanisterManager { provisional_whitelist: &ProvisionalWhitelist, ingress: &SignedIngressContent, effective_canister_id: Option, + fetch_canister_logs: FlagStatus, ) -> Result<(), UserError> { let method_name = ingress.method_name(); let sender = ingress.sender(); @@ -524,7 +525,7 @@ impl CanisterManager { )), } }, - None => Err(UserError::new( + None => Err(UserError::new( ErrorCode::InvalidManagementPayload, format!("Failed to decode payload for ic00 method: {}", method_name), )), @@ -532,25 +533,21 @@ impl CanisterManager { }, Ok(Ic00Method::FetchCanisterLogs) => { - match effective_canister_id { - Some(canister_id) => { - let canister = state.canister_state(&canister_id).ok_or_else(|| UserError::new( - ErrorCode::CanisterNotFound, - format!("Canister {} not found", canister_id), - ))?; - match canister.log_visibility() { - LogVisibility::Public => Ok(()), - LogVisibility::Controllers if canister.controllers().contains(&sender.get()) => Ok(()), - LogVisibility::Controllers => Err(UserError::new( - ErrorCode::CanisterRejectedMessage, - format!("Caller {} is not allowed to call ic00 method {}", sender, method_name), - )), - } - }, - None => Err(UserError::new( - ErrorCode::InvalidManagementPayload, - format!("Failed to decode payload for ic00 method: {}", method_name), + match fetch_canister_logs { + FlagStatus::Enabled => Err(UserError::new( + ErrorCode::CanisterRejectedMessage, + format!( + "{} API is only accessible in non-replicated mode", + Ic00Method::FetchCanisterLogs + ), )), + FlagStatus::Disabled => Err(UserError::new( + ErrorCode::CanisterContractViolation, + format!( + "{} API is not enabled on this subnet", + Ic00Method::FetchCanisterLogs + ), + )) } }, diff --git a/rs/execution_environment/src/execution_environment.rs b/rs/execution_environment/src/execution_environment.rs index 667f653392c..7d3400a30b4 100644 --- a/rs/execution_environment/src/execution_environment.rs +++ b/rs/execution_environment/src/execution_environment.rs @@ -1102,17 +1102,28 @@ impl ExecutionEnvironment { Some((res, msg.take_cycles())) } - Ok(Ic00Method::FetchCanisterLogs) => Some(( - // TODO(IC-272). - Err(UserError::new( - ErrorCode::CanisterRejectedMessage, - format!( - "{} API is not yet implemented.", - Ic00Method::FetchCanisterLogs - ), + Ok(Ic00Method::FetchCanisterLogs) => match self.config.fetch_canister_logs { + FlagStatus::Enabled => Some(( + Err(UserError::new( + ErrorCode::CanisterRejectedMessage, + format!( + "{} API is only accessible in non-replicated mode", + Ic00Method::FetchCanisterLogs + ), + )), + msg.take_cycles(), )), - msg.take_cycles(), - )), + FlagStatus::Disabled => { + let err = Err(UserError::new( + ErrorCode::CanisterContractViolation, + format!( + "{} API is not enabled on this subnet", + Ic00Method::FetchCanisterLogs + ), + )); + Some((err, msg.take_cycles())) + } + }, Ok(Ic00Method::TakeCanisterSnapshot) => match self.config.canister_snapshots { FlagStatus::Enabled => { @@ -1872,6 +1883,7 @@ impl ExecutionEnvironment { provisional_whitelist, ingress, effective_canister_id, + self.config.fetch_canister_logs, ); } diff --git a/rs/execution_environment/src/execution_environment/tests.rs b/rs/execution_environment/src/execution_environment/tests.rs index 083c229b129..b14ccf09faa 100644 --- a/rs/execution_environment/src/execution_environment/tests.rs +++ b/rs/execution_environment/src/execution_environment/tests.rs @@ -3272,9 +3272,13 @@ fn test_canister_settings_log_visibility_set_to_public() { } #[test] -fn test_fetch_canister_logs_should_accept_ingress_message_log_visibility_public_succeeds() { +fn test_fetch_canister_logs_should_accept_ingress_message_disabled() { // Arrange. - let mut test = ExecutionTestBuilder::new().build(); + // - disable the fetch_canister_logs API + // - set the log visibility to public so any user can read the logs + let mut test = ExecutionTestBuilder::new() + .with_fetch_canister_logs(FlagStatus::Disabled) + .build(); let canister_id = test.universal_canister().unwrap(); let not_a_controller = user_test_id(42); test.set_log_visibility(canister_id, LogVisibility::Public) @@ -3290,17 +3294,27 @@ fn test_fetch_canister_logs_should_accept_ingress_message_log_visibility_public_ .encode(), ); // Assert. - assert_eq!(result, Ok(())); + // Expect error because the API is disabled. + assert_eq!( + result, + Err(UserError::new( + ErrorCode::CanisterContractViolation, + "fetch_canister_logs API is not enabled on this subnet" + )) + ); } #[test] -fn test_fetch_canister_logs_should_accept_ingress_message_log_visibility_invalid_controller_fails() -{ +fn test_fetch_canister_logs_should_accept_ingress_message_enabled() { // Arrange. - let mut test = ExecutionTestBuilder::new().build(); + // - enable the fetch_canister_logs API + // - set the log visibility to public so any user can read the logs + let mut test = ExecutionTestBuilder::new() + .with_fetch_canister_logs(FlagStatus::Enabled) + .build(); let canister_id = test.universal_canister().unwrap(); let not_a_controller = user_test_id(42); - test.set_log_visibility(canister_id, LogVisibility::Controllers) + test.set_log_visibility(canister_id, LogVisibility::Public) .unwrap(); // Act. test.set_user_id(not_a_controller); @@ -3313,37 +3327,12 @@ fn test_fetch_canister_logs_should_accept_ingress_message_log_visibility_invalid .encode(), ); // Assert. + // Expect error since `should_accept_ingress_message` is only called in replicated mode which is not supported. assert_eq!( result, Err(UserError::new( ErrorCode::CanisterRejectedMessage, - format!( - "Caller {not_a_controller} is not allowed to call ic00 method fetch_canister_logs" - ), + "fetch_canister_logs API is only accessible in non-replicated mode" )) ); } - -#[test] -fn test_fetch_canister_logs_should_accept_ingress_message_log_visibility_valid_controller_succeeds() -{ - // Arrange. - let mut test = ExecutionTestBuilder::new().build(); - let canister_id = test.universal_canister().unwrap(); - let controller = user_test_id(42); - test.set_log_visibility(canister_id, LogVisibility::Controllers) - .unwrap(); - test.set_controller(canister_id, controller.get()).unwrap(); - // Act. - test.set_user_id(controller); - let result = test.should_accept_ingress_message( - test.state().metadata.own_subnet_id.into(), - Method::FetchCanisterLogs, - FetchCanisterLogsRequest { - canister_id: canister_id.into(), - } - .encode(), - ); - // Assert. - assert_eq!(result, Ok(())); -} diff --git a/rs/execution_environment/tests/fetch_canister_logs.rs b/rs/execution_environment/tests/fetch_canister_logs.rs new file mode 100644 index 00000000000..6ea171bc570 --- /dev/null +++ b/rs/execution_environment/tests/fetch_canister_logs.rs @@ -0,0 +1,115 @@ +use ic_config::execution_environment::Config as ExecutionConfig; +use ic_config::flag_status::FlagStatus; +use ic_config::subnet_config::SubnetConfig; +use ic_ic00_types::{ + CanisterInstallMode, CanisterSettingsArgsBuilder, FetchCanisterLogsRequest, LogVisibility, + Payload, +}; +use ic_registry_subnet_type::SubnetType; +use ic_state_machine_tests::{ + ErrorCode, PrincipalId, StateMachine, StateMachineBuilder, StateMachineConfig, UserError, +}; +use ic_test_utilities::universal_canister::UNIVERSAL_CANISTER_WASM; +use ic_types::{CanisterId, Cycles}; + +fn setup(fetch_canister_logs: FlagStatus) -> (StateMachine, CanisterId) { + let subnet_type = SubnetType::Application; + let config = StateMachineConfig::new( + SubnetConfig::new(subnet_type), + ExecutionConfig { + fetch_canister_logs, + ..ExecutionConfig::default() + }, + ); + let env = StateMachineBuilder::new() + .with_config(Some(config)) + .with_subnet_type(subnet_type) + .with_checkpoints_enabled(false) + .build(); + let canister_id = + env.create_canister_with_cycles(None, Cycles::from(100_000_000_000_u128), None); + env.install_wasm_in_mode( + canister_id, + CanisterInstallMode::Install, + UNIVERSAL_CANISTER_WASM.to_vec(), + vec![], + ) + .unwrap(); + + (env, canister_id) +} + +#[test] +fn test_fetch_canister_logs_ingress_disabled() { + // Arrange. + // - disable the fetch_canister_logs API + // - set the log visibility to public so that any user can read the logs + let (env, canister_id) = setup(FlagStatus::Disabled); + let not_a_controller = PrincipalId::new_user_test_id(42); + env.update_settings( + &canister_id, + CanisterSettingsArgsBuilder::new() + .with_log_visibility(LogVisibility::Public) + .build(), + ) + .unwrap(); + // Act. + // Make an update call. + let result = env.execute_ingress_as( + not_a_controller, + CanisterId::ic_00(), + "fetch_canister_logs", + FetchCanisterLogsRequest { + canister_id: canister_id.into(), + } + .encode(), + ); + // Assert. + // Expect to get an error because the fetch_canister_logs API is disabled, + // despite the fact that the log visibility is set to public. + assert_eq!( + result, + Err(UserError::new( + ErrorCode::CanisterContractViolation, + "fetch_canister_logs API is not enabled on this subnet" + )) + ); +} + +#[test] +fn test_fetch_canister_logs_ingress_enabled() { + // Arrange. + // - enable the fetch_canister_logs API + // - set the log visibility to public so that any user can read the logs + let (env, canister_id) = setup(FlagStatus::Enabled); + let not_a_controller = PrincipalId::new_user_test_id(42); + env.update_settings( + &canister_id, + CanisterSettingsArgsBuilder::new() + .with_log_visibility(LogVisibility::Public) + .build(), + ) + .unwrap(); + // Act. + // Make an update call. + let result = env.execute_ingress_as( + not_a_controller, + CanisterId::ic_00(), + "fetch_canister_logs", + FetchCanisterLogsRequest { + canister_id: canister_id.into(), + } + .encode(), + ); + // Assert. + // Expect error because an update calls are not allowed. + assert_eq!( + result, + Err(UserError::new( + ErrorCode::CanisterRejectedMessage, + "fetch_canister_logs API is only accessible in non-replicated mode" + )) + ); +} + +// TODO(IC-272): add query call tests. diff --git a/rs/test_utilities/execution_environment/src/lib.rs b/rs/test_utilities/execution_environment/src/lib.rs index 75bba59dcc2..8d459beca68 100644 --- a/rs/test_utilities/execution_environment/src/lib.rs +++ b/rs/test_utilities/execution_environment/src/lib.rs @@ -1922,6 +1922,11 @@ impl ExecutionTestBuilder { self } + pub fn with_fetch_canister_logs(mut self, status: FlagStatus) -> Self { + self.execution_config.fetch_canister_logs = status; + self + } + pub fn with_time(mut self, time: Time) -> Self { self.time = time; self