diff --git a/CHANGELOG.md b/CHANGELOG.md index 48ee145a8..3aa4ea649 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ air-gapped signing * fix: `icp canister logs` output formats are corrected. `--json` now emits machine-readable JSON and the default emits the human-readable lines (the two were swapped), and `--follow --json` emits newline-delimited JSON, one record per line, streamed as each record arrives. This is breaking for scripts: parsing the default output as JSON now requires `--json`, and consumers of `--follow --json` must read one JSON object per line. * fix: `icp canister status` again falls back on the publicly readable state-tree information when the caller may not read the status. Replicas now reject those calls with `IC0542`, which the fallback did not recognise, so the command failed with `Error looking up canister ` instead of printing the controllers and module hash. `IC0541`, returned on subnets with administrators, is now recognised too, and the fallback no longer depends on whether the rejection arrives certified or uncertified. * fix: `icp canister settings sync` now applies the settings the selected environment declares, not only the canister's base settings. It resolved the canister from the project's pre-override record, so a `settings:` block under `environments:` — including the one a vendored member declares for its own environments — was ignored: a setting declared only there was never written, and the command reported success having changed nothing. `icp deploy` already read the environment's record, so the two disagreed about the same canister in the same environment. +* fix: a deploy that targets only some of a project's canisters no longer unwires them from the ones it did not target. `PUBLIC_CANISTER_ID:` variables are computed from the ids in the environment's store and written as the canister's whole variable list, so a run that could not see a referenced canister's id — it lies outside the run's scope, or was deployed from another project root, which is how a workspace member deployed on its own sees its dependencies — silently dropped the variable a full deploy had stamped, and the canister read an empty value from then on. Such a variable now keeps the value it has, and the run reports which canister it could not resolve. ## Experimental diff --git a/crates/icp-cli/tests/dependency_tests.rs b/crates/icp-cli/tests/dependency_tests.rs index ca0cdceef..ec863089d 100644 --- a/crates/icp-cli/tests/dependency_tests.rs +++ b/crates/icp-cli/tests/dependency_tests.rs @@ -2,7 +2,11 @@ use indoc::formatdoc; use predicates::{prelude::PredicateBooleanExt, str::contains}; use crate::common::{ENVIRONMENT_RANDOM_PORT, NETWORK_RANDOM_PORT, TestContext, clients}; -use icp::{fs::write_string, prelude::*}; +use icp::{ + fs::{read_to_string, write_string}, + prelude::*, +}; +use std::collections::BTreeMap; mod common; @@ -524,3 +528,150 @@ async fn deploy_with_shared_dependency_dedups_to_one_instance() { ); } } + +/// The canister id store the CLI keeps for `env` under `project_dir`. +fn ids_store_path(project_dir: &Path, env: &str) -> PathBuf { + for sub in ["cache", "data"] { + let p = project_dir.join(format!(".icp/{sub}/mappings/{env}.ids.json")); + if p.exists() { + return p; + } + } + panic!("no canister id store for environment '{env}' under {project_dir}/.icp"); +} + +/// A binding whose target has no id in the store must not unwire the canister. +/// A deploy scoped to one canister cannot recompute an id it cannot see — the +/// dependency was deployed from another project root, or lies outside this +/// run's scope — so it keeps the id already stamped and says why, instead of +/// replacing the canister's variables with a set that silently omits it. +#[tokio::test] +async fn scoped_deploy_keeps_a_dependency_id_it_cannot_resolve() { + let ctx = TestContext::new(); + let project_dir = ctx.create_project_dir("icp"); + let wasm = ctx.make_asset("example_icp_mo.wasm"); + + let dep_dir = project_dir.join("vendor/openemail"); + std::fs::create_dir_all(&dep_dir).expect("failed to create dependency dir"); + let dep_manifest = formatdoc! {r#" + canisters: + - name: backend + build: + steps: + - type: script + command: cp '{wasm}' "$ICP_WASM_OUTPUT_PATH" + + environments: + - name: random-environment + "#}; + write_string(&dep_dir.join("icp.yaml"), &dep_manifest) + .expect("failed to write dependency manifest"); + + let pm = formatdoc! {r#" + canisters: + - name: app + build: + steps: + - type: script + command: cp '{wasm}' "$ICP_WASM_OUTPUT_PATH" + + dependencies: + - name: openemail + path: ./vendor/openemail + + {NETWORK_RANDOM_PORT} + {ENVIRONMENT_RANDOM_PORT} + "#}; + write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest"); + + let _g = ctx.start_network_in(&project_dir, "random-network").await; + ctx.ping_until_healthy(&project_dir, "random-network"); + + clients::icp(&ctx, &project_dir, Some("random-environment".to_string())) + .mint_cycles(200 * TRILLION); + + // A full workspace deploy stamps the real dependency id. + ctx.icp() + .current_dir(&project_dir) + .args(["deploy", "--environment", "random-environment"]) + .assert() + .success(); + + let assert = ctx + .icp() + .current_dir(&project_dir) + .args([ + "canister", + "status", + "--environment", + "random-environment", + "vendor/openemail:backend", + "--id-only", + ]) + .assert() + .success(); + let openemail_id = String::from_utf8(assert.get_output().stdout.clone()) + .expect("canister id should be valid utf-8") + .trim() + .to_string(); + assert!( + !openemail_id.is_empty(), + "expected an openemail canister id" + ); + + let stamped = + || contains("PUBLIC_CANISTER_ID:openemail:backend").and(contains(openemail_id.clone())); + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "settings", + "show", + "app", + "--environment", + "random-environment", + ]) + .assert() + .success() + .stdout(stamped()); + + // Drop the dependency from the id store this run reads — the state a service + // deployed from its own project root, or a fresh checkout, is in. + let store = ids_store_path(&project_dir, "random-environment"); + let mut ids: BTreeMap = + serde_json::from_str(&read_to_string(&store).expect("failed to read the id store")) + .expect("the id store should map canister names to principals"); + ids.remove("vendor/openemail:backend") + .expect("the dependency should be in the store"); + write_string( + &store, + &serde_json::to_string(&ids).expect("failed to serialize the id store"), + ) + .expect("failed to write the id store"); + + // Re-deploying just the app cannot recompute the id, and says so... + ctx.icp() + .current_dir(&project_dir) + .args(["deploy", "app", "--environment", "random-environment"]) + .assert() + .success() + .stderr( + contains("vendor/openemail:backend") + .and(contains("PUBLIC_CANISTER_ID:openemail:backend")), + ); + + // ...so it keeps the id already stamped, rather than dropping the variable. + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "settings", + "show", + "app", + "--environment", + "random-environment", + ]) + .assert() + .success() + .stdout(stamped()); +} diff --git a/crates/icp/src/operations/binding_env_vars.rs b/crates/icp/src/operations/binding_env_vars.rs index 8025a0b03..ed5b9f276 100644 --- a/crates/icp/src/operations/binding_env_vars.rs +++ b/crates/icp/src/operations/binding_env_vars.rs @@ -3,12 +3,14 @@ use std::collections::{BTreeMap, HashSet}; use crate::Canister; use futures::{StreamExt, stream::FuturesOrdered}; use ic_agent::{Agent, export::Principal}; -use ic_management_canister_types::{CanisterSettings, EnvironmentVariable, UpdateSettingsArgs}; +use ic_management_canister_types::{ + CanisterIdRecord, CanisterSettings, EnvironmentVariable, UpdateSettingsArgs, +}; use icp_events::TaskOutcome; use crate::operations::task::{Reporter, Task}; -use snafu::Snafu; -use tracing::error; +use snafu::{ResultExt, Snafu}; +use tracing::{error, warn}; use super::proxy::UpdateOrProxyError; use super::proxy_management; @@ -21,6 +23,12 @@ pub enum BindingEnvVarsOperationError { canister_names: Vec, }, + #[snafu(display("failed to fetch current canister settings for canister {canister}"))] + FetchCurrentSettings { + source: UpdateOrProxyError, + canister: Principal, + }, + #[snafu(transparent)] UpdateOrProxy { source: UpdateOrProxyError }, } @@ -31,12 +39,27 @@ pub struct SetBindingEnvVarsManyError { names: Vec, } +/// The environment-variable namespace the project's bindings are stamped into. +const BINDING_PREFIX: &str = "PUBLIC_CANISTER_ID:"; + +/// Write a canister's environment variables: the ones its manifest declares, +/// with the ids it is wired to stamped over them. +/// +/// `unresolved` names the bindings this run could not compute, as +/// `(variable, referenced canister)`. Their current values are carried over, +/// because the update below replaces the canister's whole variable list, and an +/// unresolved binding means "no id in *this* store" — the dependency was +/// deployed from another project root, or lies outside this command's scope — +/// not "this canister is not wired to it". Without that, a deploy scoped to one +/// service would delete the id a full workspace deploy had stamped, leaving the +/// canister to read an empty value with nothing said anywhere. pub async fn set_env_vars_for_canister( agent: &Agent, proxy: Option, canister_id: &Principal, canister_info: &Canister, binding_vars: &[(String, String)], + unresolved: &[(String, String)], ) -> Result<(), BindingEnvVarsOperationError> { let mut environment_variables = canister_info .settings @@ -44,6 +67,37 @@ pub async fn set_env_vars_for_canister( .to_owned() .unwrap_or_default(); + // Only an unresolved binding needs the canister's current state, so the + // common path still writes without reading first. + if !unresolved.is_empty() { + let status = proxy_management::canister_status( + agent, + proxy, + CanisterIdRecord { + canister_id: *canister_id, + }, + ) + .await + .context(FetchCurrentSettingsSnafu { + canister: *canister_id, + })?; + + for (variable, _) in unresolved { + let Some(current) = status + .settings + .environment_variables + .iter() + .find(|v| &v.name == variable) + else { + continue; + }; + // A value the manifest declares still outranks a stamped one. + environment_variables + .entry(variable.to_owned()) + .or_insert_with(|| current.value.clone()); + } + } + // inject the ids of the other canisters for (k, v) in binding_vars.iter() { environment_variables.insert(k.to_string(), v.to_string()); @@ -120,22 +174,35 @@ pub async fn set_binding_env_vars_many( // their aliases), resolved to the ids that exist in this environment. // A project without dependencies wires every canister to every sibling, // reproducing the previous flat behavior. - let binding_vars: Vec<(String, String)> = info - .bindings - .iter() - .filter_map(|(env_name, referenced_key)| { - canister_list.get(referenced_key).map(|principal| { - ( - format!("PUBLIC_CANISTER_ID:{env_name}"), - principal.to_text(), - ) - }) - }) - .collect(); + // + // A binding whose target has no id in this environment is kept aside + // rather than dropped: the canister may already carry the value, and + // silently writing it away is how a scoped deploy used to unwire a + // canister from its dependency. + let mut binding_vars: Vec<(String, String)> = Vec::new(); + let mut unresolved: Vec<(String, String)> = Vec::new(); + for (env_name, referenced_key) in &info.bindings { + let variable = format!("{BINDING_PREFIX}{env_name}"); + match canister_list.get(referenced_key) { + Some(principal) => binding_vars.push((variable, principal.to_text())), + None => unresolved.push((variable, referenced_key.to_owned())), + } + } let agent = agent.clone(); futs.push_back(async move { - let result = set_env_vars_for_canister(&agent, proxy, &cid, &info, &binding_vars).await; + for (variable, referenced_key) in &unresolved { + warn!( + "Canister '{}' is wired to '{referenced_key}', which has no id in environment \ + '{environment_name}'; leaving '{variable}' as it is. Deploy \ + '{referenced_key}' to stamp its id.", + info.name + ); + } + + let result = + set_env_vars_for_canister(&agent, proxy, &cid, &info, &binding_vars, &unresolved) + .await; match &result { Ok(()) => task.finish(TaskOutcome::succeeded()),