Repository navigation
Conversation
63e5b4c to
ba30d56
Compare
Signed-off-by: Leechael Yim <yanleech@gmail.com>
1dc17ba to
0574508
Compare
Signed-off-by: Leechael Yim <yanleech@gmail.com>
Signed-off-by: Leechael Yim <yanleech@gmail.com>
Signed-off-by: Leechael Yim <yanleech@gmail.com>
kvinwang
left a comment
kvinwang
left a comment
There was a problem hiding this comment.
Thanks, the GetSetupStatus approach looks right, and keeping Finish visible after a /finish error is a good fix.
The PR description is out of date. It says Onboard/Bootstrap return the stored keys instead of erroring and that the UI treats the already-provisioned error as success, but the final code still bails in both RPCs and adds a new Onboard.GetSetupStatus RPC instead. The Verification section lists stored_ and auto_bootstrap_ tests rather than the new setup_status_* ones. Please update it to describe the final approach.
The other comments are inline: one functional gap (recovering without a reload when the request succeeds server-side but fails on the client), one UX regression (the buttons now wait on GetAttestationInfo), and several simplifications.
| Ok(key.verifying_key().to_sec1_bytes().to_vec()) | ||
| } | ||
|
|
||
| fn setup_status(cfg: &KmsConfig) -> Result<SetupStatusResponse> { |
There was a problem hiding this comment.
This can be much shorter. Prost messages derive Default, and bootstrap_info is only display material, so a parse failure should degrade the same way the pubkey mismatch does instead of failing the whole RPC (which currently hides Finish even when keys are ready):
fn setup_status(cfg: &KmsConfig) -> Result<SetupStatusResponse> {
if !cfg.root_keys_exist() {
return Ok(Default::default());
}
let k256_pubkey = stored_k256_pubkey(cfg)?;
let (ca_pubkey, attestation) = fs::read(cfg.bootstrap_info())
.ok()
.and_then(|b| serde_json::from_slice::<BootstrapResponse>(&b).ok())
.filter(|info| info.k256_pubkey == k256_pubkey)
.map(|info| (info.ca_pubkey, info.attestation))
.unwrap_or_default();
Ok(SetupStatusResponse {
provisioned: true,
ready: cfg.keys_exists(),
k256_pubkey,
ca_pubkey,
attestation,
})
}| let info: BootstrapResponse = serde_json::from_slice( | ||
| &fs::read(cfg.bootstrap_info()).context("failed to read bootstrap info")?, | ||
| ) | ||
| .context("failed to parse bootstrap info")?; |
There was a problem hiding this comment.
An unparseable bootstrap_info makes the RPC fail, while a mismatching one is only warned and ignored. Both are auxiliary data; treat them the same (see suggestion above).
| } | ||
|
|
||
| #[rocket::async_test] | ||
| async fn setup_status_is_not_ready_when_certificates_are_missing() { |
There was a problem hiding this comment.
Five tests for a small status reader feel like a lot. After the simplification above, covering not-provisioned, matching bootstrap info and mismatching bootstrap info should be enough.
| bytes ca_pubkey = 3; | ||
| bytes attestation = 4; | ||
| // All key and certificate files are present, so Finish can start the KMS. | ||
| bool ready = 5; |
There was a problem hiding this comment.
ready implies provisioned. Please state that in the comment, or consider a single state enum.
| opacity: 0.7; | ||
| } | ||
|
|
||
| button:disabled:hover { |
There was a problem hiding this comment.
Changing the existing rule to button:not(:disabled):hover makes this override unnecessary.
| this.statusLoading = false; | ||
| }, | ||
| methods: { | ||
| applySetupStatus(data) { |
There was a problem hiding this comment.
The success/result rendering is now in three places (here, handleBootstrap, handleOnboard). BootstrapResponse/OnboardResponse use the same field names as SetupStatusResponse, so both handlers can just call this.applySetupStatus(data).
| if (data.attestation) { | ||
| this.success = 'Bootstrap successful!'; | ||
| this.result = JSON.stringify({ | ||
| caPubkey: '0x' + (data.ca_pubkey || ''), |
There was a problem hiding this comment.
provisioned guarantees a non-empty k256_pubkey, so the || '' fallbacks here and the empty-pubkey branch below are unnecessary.
| this.error = ''; | ||
| }, | ||
| async handleBootstrap() { | ||
| if (this.busy || this.statusLoading || this.success || this.broken) return; |
There was a problem hiding this comment.
This guard duplicates the template: success/broken already remove the form via v-if, and the disabled <fieldset> blocks both the submit button and Enter submission. Same for handleOnboard and handleFinish.
Separately, busy only covers double clicks. If the request succeeds server-side but the client sees a timeout or network error, the form stays up and the next click returns already bootstrapped, so the operator is stuck until they reload the page. Re-querying GetSetupStatus in the catch and calling applySetupStatus when provisioned && ready would close that gap with a few lines.
| } | ||
| }, | ||
| async handleOnboard() { | ||
| if (this.busy || this.statusLoading || this.success || this.broken) return; |
There was a problem hiding this comment.
Redundant guard, same as in handleBootstrap.
| } | ||
| }, | ||
| async handleFinish() { | ||
| if (this.busy) return; |
There was a problem hiding this comment.
Redundant: the Finish button is already :disabled="busy".
This branch has not been deployed
2 participants
Problem
Clicking Onboard or Bootstrap again after keys have been stored returns
KMS has already been onboarded(or bootstrapped). The page previously treated this as failure and hid Finish Setup. Reloading also lost the public keys from the first response, with no way to query the stored setup state.Fix
Onboard.GetSetupStatusto read the stored public key and report whether root keys and all required certificates are present.readyimpliesprovisioned. Bootstrap display material is optional: missing, malformed, or mismatchingbootstrap_infodoes not prevent setup recovery.OnboardandBootstraprejecting attempts to provision existing root keys; auto-bootstrap also refuses to replace them./finishfails.Verification
cargo test --manifest-path dstack/Cargo.toml -p dstack-kms --all-features: 60 passed, including 3setup_status_*tests covering unprovisioned, matching, and absent/invalid bootstrap information.cargo clippy --manifest-path dstack/Cargo.toml -p dstack-kms --all-features -- -D warnings -D clippy::expect_used -D clippy::unwrap_used --allow unused_variablespassed.prek run --files dstack/kms/src/onboard_service.rs dstack/kms/src/www/onboard.html dstack/kms/rpc/proto/kms_rpc.protopassed.