feat: deploy directly to cloud engines via engine operators - #683
feat: deploy directly to cloud engines via engine operators#683NikolaMilosa wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds CloudEngine deployment through per-subnet engine-operator canisters resolved from a configurable registry.
Changes:
- Adds engine registry interfaces and configuration.
- Routes CloudEngine creation through engine operators.
- Retains legacy management-canister fallback.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/icp-cli/src/operations/create.rs |
Implements operator-based CloudEngine creation. |
crates/icp-canister-interfaces/src/lib.rs |
Exports the new interface module. |
crates/icp-canister-interfaces/src/engine_canister.rs |
Defines registry types, constants, and resolution configuration. |
Comments suppressed due to low confidence (1)
crates/icp-canister-interfaces/src/engine_canister.rs:77
- This test is environment-dependent:
engine_canister_id()honorsENGINE_CANISTER_ID, so CI or a developer using a valid alternate registry makes the assertion fail even though the resolver is behaving correctly. Test the built-in constant directly; override behavior should be covered separately with controlled environment isolation.
assert_eq!(
engine_canister_id().unwrap(),
Principal::from_text(ENGINE_CANISTER_CID).unwrap()
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match self.create_on_cloud_engine(settings, selected_subnet).await { | ||
| Ok(cid) => cid, | ||
| Err(e) => { | ||
| warn!( | ||
| "engine-operator creation failed ({e}); \ | ||
| falling back to management-canister creation" | ||
| ); | ||
| self.create_mgmt(settings, selected_subnet).await? |
There was a problem hiding this comment.
I addressed this but I don't really know if it is valid. Would be nice to have a second opinion from the reviewer. Also to see if my addressing was correct.
|
Opencloud stack has had this deployed to production so now a cloud engine owner can link their identity and do something like this: (on omarchy) ~/Downloads ❯ ./icp-test identity list
* opencloud-prod-nikola 5rbsx-2blhm-7gpfb-ekmug-3jb64-4bzdx-kzpqo-yisej-3zgdu-ce2ce-pae
engine-admin-2 5rbsx-2blhm-7gpfb-ekmug-3jb64-4bzdx-kzpqo-yisej-3zgdu-ce2ce-pae
opencloud-dev begoe-c6xh2-q3pwn-dd6hb-sitwl-zw2o7-imsaq-q2npf-253kp-bwlar-aqe
anonymous 2vxsx-fae
# They would have to find their subnet id from settings
(on omarchy) ~/Downloads ❯ ./icp-test canister create --detached -e ic --subnet fj2ae-lldsj-bshv6-najg4-b7d4a-dba6x-xrf5n-qav4g-2en7f-w6in7-yqe
Created canister with ID vj246-waaaa-aaabn-aaaca-caiAfter that the canister is visible in the UI and is present on the engine |
| /// default: a configured-but-invalid value must never silently route to the | ||
| /// built-in registry (and thus a different environment). | ||
| pub fn engine_canister_id() -> Result<Principal, String> { | ||
| match env::var(ENGINE_CANISTER_ID_ENV) { |
There was a problem hiding this comment.
We usually collect all the known environment variables very early and avoid having deeply nested modules changes their behavior based on an environment variable.
Also, I assume you want an environment variable because for testnets you might have a different canister id - isn't it possible to force the canister id to be the same on testnets as it is on mainnet like we do for other well known canisters?
There was a problem hiding this comment.
There is a way, I can look into it and maybe we can have it, but still overriding would be useful. For example we may deploy a staging canister to a different id to do some testing ahead of time.
Don't you think?
| /// | ||
| /// Overridable at runtime via the [`ENGINE_CANISTER_ID_ENV`] environment | ||
| /// variable — see [`engine_canister_id`]. | ||
| pub const ENGINE_CANISTER_CID: &str = "q6cfj-fyaaa-aaaar-qb77q-cai"; |
There was a problem hiding this comment.
is there a chance this canister will move to the system subnet at some point and that we will be forced to change its id?
There was a problem hiding this comment.
The chance is at 100% but we don't know when and we don't know what canister id will it have.
There was a problem hiding this comment.
The concern is that when it does move, the error message won't be very informative and it will silently fallback on the fallback. After chatting with @lwshang our suggestion is:
- If there is a place where we can get the canister id (and perhaps cache it for some time) then we should do that, ortherwise
- We should have an error message if that canister id is not found anymore saying something along the lines of:
By default the icp-cli will lookup the cloud engine control canister in canister
qa6c...but it seems to be unavailable. You can set the env var to the correct canister or try to upgrade icp-cli
Motivation
Before this PR, to deploy to a cloud engine we had to directly call the mgmt canister. It would accept the call only if the caller is a subnet admin. Subnet admins are scars resources (we allow 10 per cloud engine) and they give way more power than just creating canisters. For example a subnet admin can go and delete any canister in the cloud engine. To tackle that problem the team added the engine operator canister. Each engine has a dedicated system engine operator canister which is meant to have its own dedicated ACL and do permission checking.
The engine canister, which is the central registry for engines, will provide the mapping from
subnet-idto a dedicatedengine-operator-idwhich the icp cli should use to reach the the correct engine operator for a subnet the user wants to deploy to.Since different environments may deploy the central canister at different locations it is possible to override the engine canister id (the registry canister for engines) by specifying the
ENGINE_CANISTER_ID=...variable.Example call:
With this, we can get rid of subnet admins.