chore: [IC-272] always return error for fetching canister logs as ing…

…ress despite the feature flag
dfinity · Mar 8, 2024 · 334bc60 · 334bc60
1 parent c9be4e4
commit 334bc60
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 74 deletions.
diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs
@@ -429,7 +429,6 @@ impl CanisterManager {
         provisional_whitelist: &ProvisionalWhitelist,
         ingress: &SignedIngressContent,
         effective_canister_id: Option<CanisterId>,
-        canister_logging: FlagStatus,
     ) -> Result<(), UserError> {
         let method_name = ingress.method_name();
         let sender = ingress.sender();
@@ -524,24 +523,13 @@ impl CanisterManager {
                 }
             },
 
-            Ok(Ic00Method::FetchCanisterLogs) => {
-                match canister_logging {
-                    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
-                        ),
-                    ))
-                }
-            },
+            Ok(Ic00Method::FetchCanisterLogs) => Err(UserError::new(
+                ErrorCode::CanisterRejectedMessage,
+                format!(
+                    "{} API is only accessible in non-replicated mode",
+                    Ic00Method::FetchCanisterLogs
+                ),
+            )),
 
             Ok(Ic00Method::ProvisionalCreateCanisterWithCycles)
             | Ok(Ic00Method::BitcoinGetSuccessors)

diff --git a/rs/execution_environment/src/execution_environment.rs b/rs/execution_environment/src/execution_environment.rs
@@ -1230,30 +1230,15 @@ impl ExecutionEnvironment {
                 }
             }
 
-            Ok(Ic00Method::FetchCanisterLogs) => match self.config.canister_logging {
-                FlagStatus::Enabled => ExecuteSubnetMessageResult::Finished {
-                    response: Err(UserError::new(
-                        ErrorCode::CanisterRejectedMessage,
-                        format!(
-                            "{} API is only accessible in non-replicated mode",
-                            Ic00Method::FetchCanisterLogs
-                        ),
-                    )),
-                    refund: msg.take_cycles(),
-                },
-                FlagStatus::Disabled => {
-                    let err = Err(UserError::new(
-                        ErrorCode::CanisterContractViolation,
-                        format!(
-                            "{} API is not enabled on this subnet",
-                            Ic00Method::FetchCanisterLogs
-                        ),
-                    ));
-                    ExecuteSubnetMessageResult::Finished {
-                        response: err,
-                        refund: msg.take_cycles(),
-                    }
-                }
+            Ok(Ic00Method::FetchCanisterLogs) => ExecuteSubnetMessageResult::Finished {
+                response: Err(UserError::new(
+                    ErrorCode::CanisterRejectedMessage,
+                    format!(
+                        "{} API is only accessible in non-replicated mode",
+                        Ic00Method::FetchCanisterLogs
+                    ),
+                )),
+                refund: msg.take_cycles(),
             },
 
             Ok(Ic00Method::TakeCanisterSnapshot) => match self.config.canister_snapshots {
@@ -2043,7 +2028,6 @@ impl ExecutionEnvironment {
                 provisional_whitelist,
                 ingress,
                 effective_canister_id,
-                self.config.canister_logging,
             );
         }
 

diff --git a/rs/execution_environment/src/execution_environment/tests.rs b/rs/execution_environment/src/execution_environment/tests.rs
@@ -3227,8 +3227,8 @@ fn test_fetch_canister_logs_should_accept_ingress_message_disabled() {
     assert_eq!(
         result,
         Err(UserError::new(
-            ErrorCode::CanisterContractViolation,
-            "fetch_canister_logs API is not enabled on this subnet"
+            ErrorCode::CanisterRejectedMessage,
+            "fetch_canister_logs API is only accessible in non-replicated mode"
         ))
     );
 }

diff --git a/rs/execution_environment/tests/canister_logging.rs b/rs/execution_environment/tests/canister_logging.rs
@@ -75,22 +75,14 @@ fn restart_node(env: StateMachine, canister_logging: FlagStatus) -> StateMachine
 #[test]
 fn test_fetch_canister_logs_via_submit_ingress() {
     // Test fetch_canister_logs API call results depending on the feature flag.
+    let error = Err(SubmitIngressError::UserError(UserError::new(
+        ErrorCode::CanisterRejectedMessage,
+        "fetch_canister_logs API is only accessible in non-replicated mode",
+    )));
     let test_cases = vec![
         // (feature flag, expected result)
-        (
-            FlagStatus::Disabled,
-            Err(SubmitIngressError::UserError(UserError::new(
-                ErrorCode::CanisterContractViolation,
-                "fetch_canister_logs API is not enabled on this subnet",
-            ))),
-        ),
-        (
-            FlagStatus::Enabled,
-            Err(SubmitIngressError::UserError(UserError::new(
-                ErrorCode::CanisterRejectedMessage,
-                "fetch_canister_logs API is only accessible in non-replicated mode",
-            ))),
-        ),
+        (FlagStatus::Disabled, error.clone()),
+        (FlagStatus::Enabled, error),
     ];
     for (feature_flag, expected_result) in test_cases {
         let (env, canister_id) = setup(
@@ -112,22 +104,14 @@ fn test_fetch_canister_logs_via_submit_ingress() {
 #[test]
 fn test_fetch_canister_logs_via_execute_ingress() {
     // Test fetch_canister_logs API call results depending on the feature flag.
+    let error = Err(UserError::new(
+        ErrorCode::CanisterRejectedMessage,
+        "fetch_canister_logs API is only accessible in non-replicated mode",
+    ));
     let test_cases = vec![
         // (feature flag, expected result)
-        (
-            FlagStatus::Disabled,
-            Err(UserError::new(
-                ErrorCode::CanisterContractViolation,
-                "fetch_canister_logs API is not enabled on this subnet",
-            )),
-        ),
-        (
-            FlagStatus::Enabled,
-            Err(UserError::new(
-                ErrorCode::CanisterRejectedMessage,
-                "fetch_canister_logs API is only accessible in non-replicated mode",
-            )),
-        ),
+        (FlagStatus::Disabled, error.clone()),
+        (FlagStatus::Enabled, error),
     ];
     for (feature_flag, expected_result) in test_cases {
         let (env, canister_id) = setup(