feat: [EXC-1675] migrate replica to no-op LogVisibilityV2 (#768)

This PR migrates replica to using a no-op `LogVisibilityV2` instead of
`LogVisibility`.

The difference between the two is in `AllowedViewers` enum variant which
is currently ignored, so it's expected for V2 to behave the same way as
V1.

rs/execution_environment/src/canister_settings.rs

@@ -2,7 +2,7 @@ use ic_base_types::{NumBytes, NumSeconds};
 use ic_cycles_account_manager::{CyclesAccountManager, ResourceSaturation};
 use ic_error_types::{ErrorCode, UserError};
 use ic_interfaces::execution_environment::SubnetAvailableMemory;
-use ic_management_canister_types::{CanisterSettingsArgs, LogVisibility};
+use ic_management_canister_types::{CanisterSettingsArgs, LogVisibilityV2};
 use ic_types::{
     ComputeAllocation, Cycles, InvalidComputeAllocationError, InvalidMemoryAllocationError,
     MemoryAllocation, PrincipalId,
@@ -25,7 +25,7 @@ pub(crate) struct CanisterSettings {
     pub(crate) wasm_memory_threshold: Option<NumBytes>,
     pub(crate) freezing_threshold: Option<NumSeconds>,
     pub(crate) reserved_cycles_limit: Option<Cycles>,
-    pub(crate) log_visibility: Option<LogVisibility>,
+    pub(crate) log_visibility: Option<LogVisibilityV2>,
     pub(crate) wasm_memory_limit: Option<NumBytes>,
 }
 
@@ -37,7 +37,7 @@ impl CanisterSettings {
         wasm_memory_threshold: Option<NumBytes>,
         freezing_threshold: Option<NumSeconds>,
         reserved_cycles_limit: Option<Cycles>,
-        log_visibility: Option<LogVisibility>,
+        log_visibility: Option<LogVisibilityV2>,
         wasm_memory_limit: Option<NumBytes>,
     ) -> Self {
         Self {
@@ -76,7 +76,7 @@ impl CanisterSettings {
         self.reserved_cycles_limit
     }
 
-    pub fn log_visibility(&self) -> Option<&LogVisibility> {
+    pub fn log_visibility(&self) -> Option<&LogVisibilityV2> {
         self.log_visibility.as_ref()
     }
 
@@ -179,7 +179,7 @@ pub(crate) struct CanisterSettingsBuilder {
     wasm_memory_threshold: Option<NumBytes>,
     freezing_threshold: Option<NumSeconds>,
     reserved_cycles_limit: Option<Cycles>,
-    log_visibility: Option<LogVisibility>,
+    log_visibility: Option<LogVisibilityV2>,
     wasm_memory_limit: Option<NumBytes>,
 }
 
@@ -253,7 +253,7 @@ impl CanisterSettingsBuilder {
         }
     }
 
-    pub fn with_log_visibility(self, log_visibility: LogVisibility) -> Self {
+    pub fn with_log_visibility(self, log_visibility: LogVisibilityV2) -> Self {
         Self {
             log_visibility: Some(log_visibility),
             ..self
@@ -348,7 +348,7 @@ pub(crate) struct ValidatedCanisterSettings {
     freezing_threshold: Option<NumSeconds>,
     reserved_cycles_limit: Option<Cycles>,
     reservation_cycles: Cycles,
-    log_visibility: Option<LogVisibility>,
+    log_visibility: Option<LogVisibilityV2>,
     wasm_memory_limit: Option<NumBytes>,
 }
 
@@ -381,7 +381,7 @@ impl ValidatedCanisterSettings {
         self.reservation_cycles
     }
 
-    pub fn log_visibility(&self) -> Option<&LogVisibility> {
+    pub fn log_visibility(&self) -> Option<&LogVisibilityV2> {
         self.log_visibility.as_ref()
     }
 

rs/execution_environment/src/execution_environment/tests.rs

@@ -7,7 +7,7 @@ use ic_management_canister_types::{
     self as ic00, BitcoinGetUtxosArgs, BitcoinNetwork, BoundedHttpHeaders, CanisterChange,
     CanisterHttpRequestArgs, CanisterIdRecord, CanisterStatusResultV2, CanisterStatusType,
     DerivationPath, EcdsaCurve, EcdsaKeyId, EmptyBlob, FetchCanisterLogsRequest, HttpMethod,
-    LogVisibility, MasterPublicKeyId, Method, Payload as Ic00Payload,
+    LogVisibilityV2, MasterPublicKeyId, Method, Payload as Ic00Payload,
     ProvisionalCreateCanisterWithCyclesArgs, ProvisionalTopUpCanisterArgs, SchnorrAlgorithm,
     SchnorrKeyId, TakeCanisterSnapshotArgs, TransformContext, TransformFunc, IC_00,
 };
@@ -3524,7 +3524,7 @@ fn test_canister_settings_log_visibility_default_controllers() {
     // Assert.
     assert_eq!(
         canister_status.settings().log_visibility(),
-        &LogVisibility::Controllers
+        &LogVisibilityV2::Controllers
     );
 }
 
@@ -3537,7 +3537,7 @@ fn test_canister_settings_log_visibility_create_with_settings() {
         .create_canister_with_settings(
             Cycles::new(1_000_000_000),
             ic00::CanisterSettingsArgsBuilder::new()
-                .with_log_visibility(LogVisibility::Public)
+                .with_log_visibility(LogVisibilityV2::Public)
                 .build(),
         )
         .unwrap();
@@ -3546,7 +3546,7 @@ fn test_canister_settings_log_visibility_create_with_settings() {
     // Assert.
     assert_eq!(
         canister_status.settings().log_visibility(),
-        &LogVisibility::Public
+        &LogVisibilityV2::Public
     );
 }
 
@@ -3556,14 +3556,14 @@ fn test_canister_settings_log_visibility_set_to_public() {
     let mut test = ExecutionTestBuilder::new().build();
     let canister_id = test.create_canister(Cycles::new(1_000_000_000));
     // Act.
-    test.set_log_visibility(canister_id, LogVisibility::Public)
+    test.set_log_visibility(canister_id, LogVisibilityV2::Public)
         .unwrap();
     let result = test.canister_status(canister_id);
     let canister_status = CanisterStatusResultV2::decode(&get_reply(result)).unwrap();
     // Assert.
     assert_eq!(
         canister_status.settings().log_visibility(),
-        &LogVisibility::Public
+        &LogVisibilityV2::Public
     );
 }
 
@@ -3574,7 +3574,7 @@ fn test_fetch_canister_logs_should_accept_ingress_message() {
     let mut test = ExecutionTestBuilder::new().build();
     let canister_id = test.universal_canister().unwrap();
     let not_a_controller = user_test_id(42);
-    test.set_log_visibility(canister_id, LogVisibility::Public)
+    test.set_log_visibility(canister_id, LogVisibilityV2::Public)
         .unwrap();
     // Act.
     test.set_user_id(not_a_controller);

rs/execution_environment/src/query_handler.rs

@@ -50,7 +50,7 @@ use tower::{util::BoxCloneService, Service};
 
 pub(crate) use self::query_scheduler::{QueryScheduler, QuerySchedulerFlag};
 use ic_management_canister_types::{
-    FetchCanisterLogsRequest, FetchCanisterLogsResponse, LogVisibility, Payload, QueryMethod,
+    FetchCanisterLogsRequest, FetchCanisterLogsResponse, LogVisibilityV2, Payload, QueryMethod,
 };
 
 /// Convert an object into CBOR binary.
@@ -282,6 +282,10 @@ impl InternalHttpQueryHandler {
     }
 }
 
+// TODO(EXC-1678): remove after release.
+/// Feature flag to enable/disable allowed viewers for canister log visibility.
+const ALLOWED_VIEWERS_ENABLED: bool = false;
+
 fn fetch_canister_logs(
     sender: PrincipalId,
     state: &ReplicatedState,
@@ -295,10 +299,19 @@ fn fetch_canister_logs(
         )
     })?;
 
-    match canister.log_visibility() {
-        LogVisibility::Public => Ok(()),
-        LogVisibility::Controllers if canister.controllers().contains(&sender) => Ok(()),
-        LogVisibility::Controllers => Err(UserError::new(
+    let log_visibility = match canister.log_visibility() {
+        // If the feature is disabled override `AllowedViewers` with default value.
+        LogVisibilityV2::AllowedViewers(_) if !ALLOWED_VIEWERS_ENABLED => {
+            &LogVisibilityV2::default()
+        }
+        other => other,
+    };
+    match log_visibility {
+        LogVisibilityV2::Public => Ok(()),
+        LogVisibilityV2::Controllers if canister.controllers().contains(&sender) => Ok(()),
+        LogVisibilityV2::AllowedViewers(principals) if principals.get().contains(&sender) => Ok(()),
+        LogVisibilityV2::AllowedViewers(_) if canister.controllers().contains(&sender) => Ok(()),
+        LogVisibilityV2::AllowedViewers(_) | LogVisibilityV2::Controllers => Err(UserError::new(
             ErrorCode::CanisterRejectedMessage,
             format!(
                 "Caller {} is not allowed to query ic00 method {}",

rs/execution_environment/tests/canister_logging.rs

@@ -1,9 +1,9 @@
 use ic_config::execution_environment::Config as ExecutionConfig;
 use ic_config::subnet_config::SubnetConfig;
 use ic_management_canister_types::{
-    self as ic00, CanisterIdRecord, CanisterInstallMode, CanisterLogRecord, CanisterSettingsArgs,
-    CanisterSettingsArgsBuilder, DataSize, EmptyBlob, FetchCanisterLogsRequest,
-    FetchCanisterLogsResponse, LogVisibility, Payload,
+    self as ic00, BoundedAllowedViewers, CanisterIdRecord, CanisterInstallMode, CanisterLogRecord,
+    CanisterSettingsArgs, CanisterSettingsArgsBuilder, DataSize, EmptyBlob,
+    FetchCanisterLogsRequest, FetchCanisterLogsResponse, LogVisibilityV2, Payload,
 };
 use ic_registry_subnet_type::SubnetType;
 use ic_state_machine_tests::{
@@ -79,7 +79,7 @@ fn setup_with_controller(wasm: Vec<u8>) -> (StateMachine, CanisterId, PrincipalI
     let controller = PrincipalId::new_user_test_id(42);
     let (env, canister_id) = setup_and_install_wasm(
         CanisterSettingsArgsBuilder::new()
-            .with_log_visibility(LogVisibility::Controllers)
+            .with_log_visibility(LogVisibilityV2::Controllers)
             .with_controllers(vec![controller])
             .build(),
         wasm,
@@ -111,7 +111,7 @@ fn fetch_canister_logs(
 fn test_fetch_canister_logs_via_submit_ingress() {
     let (env, canister_id) = setup_and_install_wasm(
         CanisterSettingsArgsBuilder::new()
-            .with_log_visibility(LogVisibility::Public)
+            .with_log_visibility(LogVisibilityV2::Public)
             .build(),
         wat_canister().build_wasm(),
     );
@@ -135,7 +135,7 @@ fn test_fetch_canister_logs_via_execute_ingress() {
     // Test fetch_canister_logs API call results.
     let (env, canister_id) = setup_and_install_wasm(
         CanisterSettingsArgsBuilder::new()
-            .with_log_visibility(LogVisibility::Public)
+            .with_log_visibility(LogVisibilityV2::Public)
             .build(),
         wat_canister().build_wasm(),
     );
@@ -159,7 +159,7 @@ fn test_fetch_canister_logs_via_query_call() {
     // Test fetch_canister_logs API call results.
     let (env, canister_id) = setup_and_install_wasm(
         CanisterSettingsArgsBuilder::new()
-            .with_log_visibility(LogVisibility::Public)
+            .with_log_visibility(LogVisibilityV2::Public)
             .build(),
         wat_canister().build_wasm(),
     );
@@ -230,37 +230,63 @@ fn test_fetch_canister_logs_via_composite_query_call() {
 #[test]
 fn test_log_visibility_of_fetch_canister_logs() {
     // Test combinations of log_visibility and sender for fetch_canister_logs API call.
-    let controller = PrincipalId::new_user_test_id(27);
-    let not_a_controller = PrincipalId::new_user_test_id(42);
+    let controller = PrincipalId::new_user_test_id(1);
+    let not_a_controller = PrincipalId::new_user_test_id(2);
+    let allowed_viewer = PrincipalId::new_user_test_id(3);
+    let not_allowed_viewer = PrincipalId::new_user_test_id(4);
+    let allowed_viewers = BoundedAllowedViewers::new(vec![allowed_viewer]);
     let ok = Ok(WasmResult::Reply(
         FetchCanisterLogsResponse {
             canister_log_records: vec![],
         }
         .encode(),
     ));
-    let error = Err(UserError::new(
-        ErrorCode::CanisterRejectedMessage,
-        format!(
-            "Caller {not_a_controller} is not allowed to query ic00 method fetch_canister_logs"
-        ),
-    ));
+    fn not_allowed_error(caller: &PrincipalId) -> Result<WasmResult, UserError> {
+        Err(UserError::new(
+            ErrorCode::CanisterRejectedMessage,
+            format!("Caller {caller} is not allowed to query ic00 method fetch_canister_logs"),
+        ))
+    }
     let test_cases = vec![
         // (log_visibility, sender, expected_result)
-        (LogVisibility::Public, controller, ok.clone()),
-        (LogVisibility::Public, not_a_controller, ok.clone()),
-        (LogVisibility::Controllers, controller, ok),
-        (LogVisibility::Controllers, not_a_controller, error),
+        (LogVisibilityV2::Public, controller, ok.clone()),
+        (LogVisibilityV2::Public, not_a_controller, ok.clone()),
+        (LogVisibilityV2::Controllers, controller, ok.clone()),
+        (
+            LogVisibilityV2::Controllers,
+            not_a_controller,
+            not_allowed_error(&not_a_controller),
+        ),
+        (
+            LogVisibilityV2::AllowedViewers(allowed_viewers.clone()),
+            allowed_viewer,
+            // TODO(EXC-1675): when disabled works as for controllers, change to ok when enabled.
+            not_allowed_error(&allowed_viewer),
+        ),
+        (
+            LogVisibilityV2::AllowedViewers(allowed_viewers.clone()),
+            not_allowed_viewer,
+            not_allowed_error(&not_allowed_viewer),
+        ),
+        (
+            LogVisibilityV2::AllowedViewers(allowed_viewers),
+            controller,
+            ok,
+        ),
     ];
     for (log_visibility, sender, expected_result) in test_cases {
         let (env, canister_id) = setup_and_install_wasm(
             CanisterSettingsArgsBuilder::new()
-                .with_log_visibility(log_visibility)
+                .with_log_visibility(log_visibility.clone())
                 .with_controllers(vec![controller])
                 .build(),
             wat_canister().build_wasm(),
         );
         let actual_result = fetch_canister_logs(&env, sender, canister_id);
-        assert_eq!(actual_result, expected_result);
+        assert_eq!(
+            actual_result, expected_result,
+            "Failed for log_visibility: {log_visibility:?}, sender: {sender}"
+        );
     }
 }
 
@@ -730,7 +756,7 @@ fn test_logging_debug_print_persists_over_upgrade() {
 fn test_logging_trap_at_install_start() {
     let (env, canister_id) = setup(
         CanisterSettingsArgsBuilder::new()
-            .with_log_visibility(LogVisibility::Public)
+            .with_log_visibility(LogVisibilityV2::Public)
             .build(),
     );
     env.advance_time(TIME_STEP);
@@ -763,7 +789,7 @@ fn test_logging_trap_at_install_start() {
 fn test_logging_trap_at_install_init() {
     let (env, canister_id) = setup(
         CanisterSettingsArgsBuilder::new()
-            .with_log_visibility(LogVisibility::Public)
+            .with_log_visibility(LogVisibilityV2::Public)
             .build(),
     );
     env.advance_time(TIME_STEP);