feat: EXC-2057: Update canister status to include environment variables (#5843)

This PR updates the canister status to expose canister environment
variables.
  
It also refactors the code to use the EnvironmentVariables type instead
of the raw type, moving the struct to the /types module and updating the
relevant code and tests accordingly.
 
Follow-up: environment variables validation, expanded tests coverage,
improvements.

Author: AlexandraZapuc

rs/embedders/src/wasmtime_embedder/system_api/sandbox_safe_system_state.rs

@@ -789,7 +789,7 @@ impl SandboxSafeSystemState {
             system_state.memory_allocation,
             system_state.wasm_memory_threshold,
             compute_allocation,
-            system_state.environment_variables.clone(),
+            system_state.environment_variables.clone().into(),
             system_state.balance(),
             system_state.reserved_balance(),
             system_state.reserved_balance_limit(),

rs/execution_environment/src/canister_manager.rs

@@ -325,8 +325,7 @@ impl CanisterManager {
         }
         if let Some(environment_variables) = settings.environment_variables() {
             if self.environment_variables_flag == FlagStatus::Enabled {
-                canister.system_state.environment_variables =
-                    environment_variables.get_environment_variables().clone();
+                canister.system_state.environment_variables = environment_variables.clone();
             }
         }
     }
@@ -865,6 +864,7 @@ impl CanisterManager {
                 .egress_payload_size,
             wasm_memory_limit.map(|x| x.get()),
             wasm_memory_threshold.get(),
+            canister.system_state.environment_variables.clone(),
         ))
     }
 

rs/execution_environment/src/canister_manager/tests.rs

@@ -13,7 +13,7 @@ use crate::{
 };
 use assert_matches::assert_matches;
 use candid::{CandidType, Decode, Encode};
-use ic_base_types::{NumSeconds, PrincipalId};
+use ic_base_types::{EnvironmentVariables, NumSeconds, PrincipalId};
 use ic_config::{
     execution_environment::{
         Config, CANISTER_GUARANTEED_CALLBACK_QUOTA, DEFAULT_WASM_MEMORY_LIMIT,
@@ -35,11 +35,12 @@ use ic_management_canister_types_private::{
     CanisterChange, CanisterChangeDetails, CanisterChangeOrigin, CanisterIdRecord,
     CanisterInstallMode, CanisterInstallModeV2, CanisterSettingsArgsBuilder,
     CanisterSnapshotResponse, CanisterStatusResultV2, CanisterStatusType, CanisterUpgradeOptions,
-    ChunkHash, ClearChunkStoreArgs, CreateCanisterArgs, EmptyBlob, InstallCodeArgsV2,
-    LoadCanisterSnapshotArgs, Method, NodeMetricsHistoryArgs, NodeMetricsHistoryResponse,
-    OnLowWasmMemoryHookStatus, Payload, RenameCanisterArgs, RenameToArgs, StoredChunksArgs,
-    StoredChunksReply, SubnetInfoArgs, SubnetInfoResponse, TakeCanisterSnapshotArgs,
-    UpdateSettingsArgs, UploadChunkArgs, UploadChunkReply, WasmMemoryPersistence, IC_00,
+    ChunkHash, ClearChunkStoreArgs, CreateCanisterArgs, EmptyBlob, EnvironmentVariable,
+    InstallCodeArgsV2, LoadCanisterSnapshotArgs, Method, NodeMetricsHistoryArgs,
+    NodeMetricsHistoryResponse, OnLowWasmMemoryHookStatus, Payload, RenameCanisterArgs,
+    RenameToArgs, StoredChunksArgs, StoredChunksReply, SubnetInfoArgs, SubnetInfoResponse,
+    TakeCanisterSnapshotArgs, UpdateSettingsArgs, UploadChunkArgs, UploadChunkReply,
+    WasmMemoryPersistence, IC_00,
 };
 use ic_metrics::MetricsRegistry;
 use ic_registry_provisional_whitelist::ProvisionalWhitelist;
@@ -1297,6 +1298,36 @@ fn get_canister_status_of_stopping_canister() {
     });
 }
 
+#[test]
+fn canister_status_with_environment_variables() {
+    let mut test = ExecutionTestBuilder::new()
+        .with_environment_variables_flag(FlagStatus::Enabled)
+        .build();
+    let environment_variables = btreemap![
+        "TEST_VAR".to_string() => "test_value".to_string(),
+        "TEST_VAR2".to_string() => "test_value2".to_string(),
+    ];
+
+    let settings = CanisterSettingsArgsBuilder::new()
+        .with_environment_variables(environment_variables.clone())
+        .build();
+    let canister_id = test
+        .create_canister_with_settings(*INITIAL_CYCLES, settings)
+        .unwrap();
+    let status = test.canister_status(canister_id);
+    let reply = get_reply(status);
+    let status = Decode!(&reply, CanisterStatusResultV2).unwrap();
+
+    let expected_env_vars = environment_variables
+        .into_iter()
+        .map(|(name, value)| EnvironmentVariable { name, value })
+        .collect::<Vec<_>>();
+    assert_eq!(
+        status.settings().environment_variables(),
+        &expected_env_vars
+    );
+}
+
 #[test]
 fn set_controller_with_incorrect_controller() {
     let mut test = ExecutionTestBuilder::new().build();
@@ -6667,16 +6698,17 @@ fn test_environment_variables_are_changed_via_create_canister() {
         })
         .build();
 
-    let mut env_vars = BTreeMap::new();
-    env_vars.insert("KEY1".to_string(), "VALUE1".to_string());
-    env_vars.insert("KEY2".to_string(), "VALUE2".to_string());
+    let env_vars = EnvironmentVariables::new(BTreeMap::from([
+        ("KEY1".to_string(), "VALUE1".to_string()),
+        ("KEY2".to_string(), "VALUE2".to_string()),
+    ]));
 
     // Create canister with environment variables.
     let canister_id = test
         .create_canister_with_settings(
             Cycles::new(1_000_000_000_000_000),
             CanisterSettingsArgsBuilder::new()
-                .with_environment_variables(env_vars.clone())
+                .with_environment_variables(env_vars.clone().into())
                 .build(),
         )
         .unwrap();
@@ -6696,15 +6728,16 @@ fn test_environment_variables_are_updated_on_update_settings() {
         .build();
     let canister_id = test.create_canister(Cycles::new(1_000_000_000_000_000));
 
-    let mut env_vars = BTreeMap::new();
-    env_vars.insert("KEY1".to_string(), "VALUE1".to_string());
-    env_vars.insert("KEY2".to_string(), "VALUE2".to_string());
+    let env_vars = EnvironmentVariables::new(BTreeMap::from([
+        ("KEY1".to_string(), "VALUE1".to_string()),
+        ("KEY2".to_string(), "VALUE2".to_string()),
+    ]));
 
     // Set environment variables via `update_settings`.
     let args = UpdateSettingsArgs {
         canister_id: canister_id.get(),
         settings: CanisterSettingsArgsBuilder::new()
-            .with_environment_variables(env_vars.clone())
+            .with_environment_variables(env_vars.clone().into())
             .build(),
         sender_canister_version: None,
     };
@@ -6739,9 +6772,10 @@ fn test_environment_variables_are_not_set_when_disabled() {
         .build();
 
     // Create environment variables.
-    let mut env_vars = BTreeMap::new();
-    env_vars.insert("KEY1".to_string(), "VALUE1".to_string());
-    env_vars.insert("KEY2".to_string(), "VALUE2".to_string());
+    let env_vars = BTreeMap::from([
+        ("KEY1".to_string(), "VALUE1".to_string()),
+        ("KEY2".to_string(), "VALUE2".to_string()),
+    ]);
 
     // Create canister with environment variables.
     let canister_id = test
@@ -6755,7 +6789,10 @@ fn test_environment_variables_are_not_set_when_disabled() {
 
     // Verify environment variables are not set.
     let canister = test.canister_state(canister_id);
-    assert_eq!(canister.system_state.environment_variables, BTreeMap::new());
+    assert_eq!(
+        canister.system_state.environment_variables,
+        EnvironmentVariables::new(BTreeMap::new())
+    );
 
     // Set environment variables via `update_settings`.
     let args = UpdateSettingsArgs {
@@ -6770,7 +6807,10 @@ fn test_environment_variables_are_not_set_when_disabled() {
 
     // Verify environment variables are not set.
     let canister = test.canister_state(canister_id);
-    assert_eq!(canister.system_state.environment_variables, BTreeMap::new());
+    assert_eq!(
+        canister.system_state.environment_variables,
+        EnvironmentVariables::new(BTreeMap::new())
+    );
 }
 
 /// Creates and deploys a pair of universal canisters with the second canister being controlled by the first one

rs/execution_environment/src/canister_settings.rs

@@ -1,16 +1,15 @@
-use ic_base_types::{NumBytes, NumSeconds};
-use ic_crypto_sha2::Sha256;
+use ic_base_types::{EnvironmentVariables, 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_private::{CanisterSettingsArgs, LogVisibilityV2, HASH_LENGTH};
+use ic_management_canister_types_private::{CanisterSettingsArgs, LogVisibilityV2};
 use ic_replicated_state::MessageMemoryUsage;
 use ic_types::{
     ComputeAllocation, Cycles, InvalidComputeAllocationError, InvalidMemoryAllocationError,
     MemoryAllocation, PrincipalId,
 };
 use num_traits::{cast::ToPrimitive, SaturatingSub};
-use std::{collections::BTreeMap, convert::TryFrom};
+use std::convert::TryFrom;
 
 use crate::canister_manager::types::CanisterManagerError;
 
@@ -422,46 +421,6 @@ impl ValidatedCanisterSettings {
     }
 }
 
-#[derive(Clone)]
-pub struct EnvironmentVariables {
-    environment_variables: BTreeMap<String, String>,
-}
-
-impl EnvironmentVariables {
-    pub fn new(environment_variables: BTreeMap<String, String>) -> Self {
-        Self {
-            environment_variables,
-        }
-    }
-
-    pub fn hash(&self) -> [u8; HASH_LENGTH] {
-        // Create a vector to store the hashes of key-value pairs
-        let mut hashes: Vec<Vec<u8>> = Vec::new();
-
-        // 1. For each key-value pair, hash the key and value, and concatenate the hashes.
-        for (key, value) in &self.environment_variables {
-            let mut key_hash = Sha256::hash(key.as_bytes()).to_vec();
-            let mut value_hash = Sha256::hash(value.as_bytes()).to_vec();
-            key_hash.append(&mut value_hash);
-            hashes.push(key_hash);
-        }
-        // 2. Sort the concatenated hashes.
-        hashes.sort();
-
-        // 3. Concatenate the sorted hashes, and hash the result.
-        let mut hasher = Sha256::new();
-        for hash in hashes {
-            hasher.write(&hash);
-        }
-
-        hasher.finish()
-    }
-
-    pub fn get_environment_variables(&self) -> BTreeMap<String, String> {
-        self.environment_variables.clone()
-    }
-}
-
 /// Validates the new canisters settings:
 /// - memory allocation:
 ///     - it cannot be lower than the current canister memory usage.

rs/execution_environment/src/lib.rs

@@ -15,7 +15,6 @@ mod types;
 pub mod util;
 
 use crate::ingress_filter::IngressFilterServiceImpl;
-pub use canister_settings::EnvironmentVariables;
 pub use execution_environment::{
     as_num_instructions, as_round_instructions, execute_canister, CompilationCostHandling,
     ExecuteMessageResult, ExecutionEnvironment, ExecutionResponse, RoundInstructions, RoundLimits,

rs/execution_environment/tests/canister_history.rs

@@ -2,12 +2,11 @@ use ic00::{
     CanisterSettingsArgsBuilder, CanisterSnapshotResponse, LoadCanisterSnapshotArgs,
     TakeCanisterSnapshotArgs,
 };
-use ic_base_types::PrincipalId;
+use ic_base_types::{EnvironmentVariables, PrincipalId};
 use ic_config::flag_status::FlagStatus;
 use ic_config::{execution_environment::Config as HypervisorConfig, subnet_config::SubnetConfig};
 use ic_crypto_sha2::Sha256;
 use ic_error_types::{ErrorCode, UserError};
-use ic_execution_environment::EnvironmentVariables;
 use ic_management_canister_types_private::CanisterInstallMode::{Install, Reinstall, Upgrade};
 use ic_management_canister_types_private::{
     self as ic00, CanisterChange, CanisterChangeDetails, CanisterChangeOrigin, CanisterIdRecord,
@@ -1253,12 +1252,15 @@ fn check_environment_variables_for_create_canister_history(
     let canister_state = state.canister_state(&canister_id).unwrap();
     match environment_variables_flag {
         FlagStatus::Enabled => {
-            assert_eq!(&canister_state.system_state.environment_variables, env_vars);
+            assert_eq!(
+                canister_state.system_state.environment_variables,
+                EnvironmentVariables::new(env_vars.clone())
+            );
         }
         FlagStatus::Disabled => {
             assert_eq!(
                 canister_state.system_state.environment_variables,
-                BTreeMap::new()
+                EnvironmentVariables::new(BTreeMap::new())
             );
         }
     }
@@ -1337,7 +1339,10 @@ fn canister_history_tracking_env_vars_update_settings() {
     // Verify the environment variables of the canister state.
     let state = env.get_latest_state();
     let canister_state = state.canister_state(&canister_id).unwrap();
-    assert_eq!(canister_state.system_state.environment_variables, env_vars);
+    assert_eq!(
+        canister_state.system_state.environment_variables,
+        EnvironmentVariables::new(env_vars)
+    );
 }
 
 #[test]

rs/replica_tests/tests/canister_lifecycle.rs

@@ -732,6 +732,7 @@ fn can_get_canister_information() {
                 0u128,
                 Some(DEFAULT_WASM_MEMORY_LIMIT.get()),
                 0u64,
+                Default::default(),
             )
         );
 
@@ -797,6 +798,7 @@ fn can_get_canister_information() {
                     0u128,
                     Some(DEFAULT_WASM_MEMORY_LIMIT.get()),
                     0u64,
+                    Default::default(),
                 ),
                 CanisterStatusResultV2::decode(&res).unwrap(),
                 2 * BALANCE_EPSILON,

rs/replicated_state/src/canister_state/system_state.rs

@@ -15,7 +15,7 @@ use crate::{
     PageMap, StateError,
 };
 pub use call_context_manager::{CallContext, CallContextAction, CallContextManager, CallOrigin};
-use ic_base_types::NumSeconds;
+use ic_base_types::{EnvironmentVariables, NumSeconds};
 use ic_error_types::RejectCode;
 use ic_interfaces::execution_environment::HypervisorError;
 use ic_logger::{error, ReplicaLogger};
@@ -405,7 +405,7 @@ pub struct SystemState {
     pub snapshots_memory_usage: NumBytes,
 
     /// Environment variables.
-    pub environment_variables: BTreeMap<String, String>,
+    pub environment_variables: EnvironmentVariables,
 }
 
 /// A wrapper around the different canister statuses.
@@ -849,7 +849,7 @@ impl SystemState {
             wasm_memory_limit,
             next_snapshot_id,
             snapshots_memory_usage,
-            environment_variables,
+            environment_variables: EnvironmentVariables::new(environment_variables),
         };
         system_state.check_invariants().unwrap_or_else(|msg| {
             metrics.observe_broken_soft_invariant(msg);

rs/state_manager/src/tip.rs

@@ -1312,7 +1312,11 @@ fn serialize_canister_protos_to_checkpoint_readwrite(
             wasm_memory_limit: canister_state.system_state.wasm_memory_limit,
             next_snapshot_id: canister_state.system_state.next_snapshot_id,
             snapshots_memory_usage: canister_state.system_state.snapshots_memory_usage,
-            environment_variables: canister_state.system_state.environment_variables.clone(),
+            environment_variables: canister_state
+                .system_state
+                .environment_variables
+                .clone()
+                .into(),
         }
         .into(),
     )?;

rs/test_utilities/state/src/lib.rs

@@ -1,4 +1,4 @@
-use ic_base_types::NumSeconds;
+use ic_base_types::{EnvironmentVariables, NumSeconds};
 use ic_btc_replica_types::BitcoinAdapterRequestWrapper;
 use ic_management_canister_types_private::{
     CanisterStatusType, EcdsaCurve, EcdsaKeyId, LogVisibilityV2, MasterPublicKeyId,
@@ -471,7 +471,7 @@ impl SystemStateBuilder {
         mut self,
         environment_variables: BTreeMap<String, String>,
     ) -> Self {
-        self.system_state.environment_variables = environment_variables;
+        self.system_state.environment_variables = EnvironmentVariables::new(environment_variables);
         self
     }