From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id B6A8C1FF146 for ; Tue, 12 May 2026 15:58:34 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B2281157B8; Tue, 12 May 2026 15:58:33 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 12 May 2026 15:57:54 +0200 Message-Id: To: "Thomas Lamprecht" , Subject: Re: [PATCH datacenter-manager v2 8/8] subscription: add Reissue Key action with pending-reissue queue From: "Lukas Wagner" X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260507082943.2749725-1-t.lamprecht@proxmox.com> <20260507082943.2749725-9-t.lamprecht@proxmox.com> In-Reply-To: <20260507082943.2749725-9-t.lamprecht@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778594162340 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [subscription.rs] Message-ID-Hash: KZPXYPDZBUNFTK3NWL3XFYLXVU4AQAN2 X-Message-ID-Hash: KZPXYPDZBUNFTK3NWL3XFYLXVU4AQAN2 X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu May 7, 2026 at 10:26 AM CEST, Thomas Lamprecht wrote: > Wire a new "Reissue Key" action on the Node Subscription Status panel > (previously titled Node Status) that queues the live subscription on a > remote node for removal at next Apply Pending. This mirrors how the > shop describes keys that may be re-bound to a different server ID, so > the panel uses the same vocabulary end to end. > > The pool entry bound to (remote, node) gets a pending-reissue flag. > Apply Pending issues a DELETE call on the remote and clears the > binding on success; the entry stays in the pool as a free key. Clear > Pending only resets the flag and leaves the binding intact, so an > operator can retry the queueing without losing context. > > Foreign-key adoption: when the live node runs a subscription the pool > has never seen, queueing a reissue imports that key into the pool with > the flag set. If the operator then runs Clear Pending, the imported > entry stays in the pool as a free key. This shortcut lets a legacy > node be brought under PDM management by queueing a reissue and then > immediately cancelling it, without re-typing the key. This feels a bit off to me. Maybe we should rather have an explicit 'Adopt Key' or 'Import Key' action, and actively disallow the 'Reissue Key' action if the key is not in the key pool? Would also be nice to have a visual distinction between 'foreign key' and 'key already in keypool' in the Node Subscription Status Panel, now that I think about it. > > The left-side Key Pool grid renames its action button to "Remove Key" > for clarity against the new Reissue Key action, and the confirmation > dialog now warns when the entry is still bound to a remote node so an > operator does not lose track of the live subscription by mistake. > > Note as of now this might be better called "Clear Key", it was split > out from some work where we can actually actively reissue here. > > Signed-off-by: Thomas Lamprecht [...] > +------------------------ > =20 > -The Node Status panel shows the live subscription state of every node be= hind a configured remote > -alongside any pending plan from the pool. Nodes that already hold a key = the registry assigned appear > -with the live level; nodes with a pending pool assignment show a clock i= con until the change is > -pushed to the remote. > +The Node Subscription Status panel shows the live subscription state of = every node behind a > +configured remote alongside any pending plan from the pool. Nodes that a= lready hold a key the > +registry assigned appear with the live level; nodes with a pending pool = assignment show a clock > +icon until the change is pushed to the remote. > =20 > -From this view an operator can clear a pending assignment or remove the = key from the pool entirely, > -which is convenient when a node is known to be wrong without first havin= g to find the matching entry > -on the key list. > +From this view an operator can clear a pending assignment or queue a rei= ssue on the selected > +node. Reissuing means freeing the live subscription key from this node s= o it can be reassigned > +elsewhere; this is the same notion the shop uses for keys that may be re= -bound to a new server > +ID. The action is queued, not immediate, so it can be confirmed or cance= lled together with > +other pending changes via Apply Pending and Clear Pending. The 'not immediate' reads a bit weird, I'd maybe phrase it as "The 'Reissue Key' action is queued until it is confirmed or cancelled via 'Apply Pending' or dismissed via 'Clear Pending', respectively". > =20 > -Assignment > ----------- > +Assignment and Reissue > +---------------------- > =20 > A key can be pinned to a single node manually. > =20 > The Auto-Assign action proposes a plan that fills unsubscribed nodes fro= m free pool keys. For > -Proxmox VE, the smallest covering key by socket count is chosen, so a 4-= socket key is not used on a > -2-socket host while a larger host stays unsubscribed. > +Proxmox VE, the smallest covering key by socket count is chosen, so a 4-= socket key is not used > +on a 2-socket host while a larger host stays unsubscribed. > =20 > -The proposed plan can be inspected before it is applied. Apply Pending p= ushes the queued keys to > -their target nodes; if a push fails the remaining queue is kept intact f= or retry. Clear Pending > +The Reissue Key action queues the live subscription on the selected node= for removal. Apply > +Pending later issues the removal on the remote and releases the pool bin= ding so the key becomes > +available for reassignment. Clear Pending drops the queued reissue witho= ut touching the remote; > +the binding stays intact and the operator can retry. > + > +When the live node runs a subscription that is not yet tracked by the po= ol, queueing a reissue > +also imports that key into the pool. If the operator then runs Clear Pen= ding, the imported > +entry stays as a free pool key, effectively adopting the previously fore= ign subscription for > +future PDM-side management. This shortcut lets a legacy node be brought = under the registry by > +queueing a reissue and then immediately cancelling it, without having to= re-enter the key by > +hand. > + > +The proposed plan can be inspected before it is applied. Apply Pending w= alks the queue in > +order; if any push or reissue fails the remaining queue is kept intact f= or retry. Clear Pending > drops the plan without touching any remote. > =20 > Permissions > diff --git a/lib/pdm-api-types/src/subscription.rs b/lib/pdm-api-types/sr= c/subscription.rs > index ead3c1b..9927d38 100644 > --- a/lib/pdm-api-types/src/subscription.rs > +++ b/lib/pdm-api-types/src/subscription.rs > @@ -317,6 +317,7 @@ pub enum SubscriptionKeySource { > "level": { optional: true }, > "status": { optional: true }, > "source": { optional: true }, > + "pending-reissue": { optional: true }, > }, > )] > #[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)] > @@ -346,6 +347,16 @@ pub struct SubscriptionKeyEntry { > #[serde(skip_serializing_if =3D "Option::is_none")] > pub node: Option, > =20 > + /// True when the operator queued a reissue for this entry's bound n= ode, that is, a request > + /// to free the key from `remote`/`node` so it can be reassigned to = a different node. > + /// > + /// Apply Pending will issue a DELETE on the remote and then clear `= remote`/`node` on > + /// success. Clear Pending only resets this flag and leaves the bind= ing untouched, so a > + /// foreign current_key that was newly imported to queue a reissue s= tays adopted in the > + /// pool. A bare flag is enough since the (remote, node) binding liv= es next to it. > + #[serde(default, rename =3D "pending-reissue")] > + pub pending_reissue: bool, > + We already have rename_all =3D "kebab-case" on the entire struct, I think the explicit rename here is not needed? Also, thinking about the representation in the config file and API, wouldn't a Option be nicer here? Otherwise pretty much all keys always have a 'pending-reissue false' in the section... > /// Server ID this key is bound to (from signed info, if available). > #[serde(skip_serializing_if =3D "Option::is_none")] > pub serverid: Option, > @@ -527,6 +538,9 @@ pub struct RemoteNodeStatus { > /// Current key on the node (from remote query). > #[serde(skip_serializing_if =3D "Option::is_none")] > pub current_key: Option, > + /// True when the pool has a reissue queued for this node. > + #[serde(default, rename =3D "pending-reissue")] > + pub pending_reissue: bool, > } > =20 [...] > +#[api( > + input: { > + properties: { > + remote: { schema: REMOTE_ID_SCHEMA }, > + // NODE_SCHEMA rejects path-traversal input before it ends u= p interpolated into > + // the remote URL `/api2/extjs/nodes/{node}/subscription`. > + node: { schema: NODE_SCHEMA }, > + digest: { > + type: ConfigDigest, > + optional: true, > + }, > + }, > + }, > + access: { > + permission: &Permission::Privilege(&["system"], PRIV_SYS_MODIFY,= false), > + }, > +)] > +/// Queue a reissue on a remote node, that is, mark its subscription for= removal so the key can > +/// be reassigned elsewhere. > +/// > +/// Sets `pending_reissue` on the pool entry currently bound to (remote,= node). Apply Pending > +/// later issues the DELETE on the remote and clears the binding on succ= ess; Clear Pending only > +/// resets the flag and leaves the binding intact so the operator can re= try. > +/// > +/// Adoption path: when the live node runs a key that is not yet in the = pool, queueing a reissue > +/// adds that key to the pool with the flag set. If the operator later r= uns Clear Pending, the > +/// newly-imported entry stays in the pool as a free key, effectively ad= opting a previously > +/// foreign subscription for future PDM-side management. > +/// > +/// Per-remote `PRIV_RESOURCE_MODIFY` is enforced inside the handler so = an operator with global > +/// system access alone cannot tear down subscriptions on remotes they h= ave no other authority on. > +async fn queue_reissue( > + remote: String, > + node: String, > + digest: Option, > + rpcenv: &mut dyn RpcEnvironment, > +) -> Result<(), Error> { > + let auth_id: Authid =3D rpcenv > + .get_auth_id() > + .context("no authid available")? > + .parse()?; > + let user_info =3D CachedUserInfo::new()?; > + user_info.check_privs( > + &auth_id, > + &["resource", &remote], > + PRIV_RESOURCE_MODIFY, > + false, > + )?; > + > + // Fetch live state before grabbing the config lock so the network c= all does not pin the lock > + // for the duration of a remote query. > + let (remotes_config, _) =3D pdm_config::remotes::config()?; > + let remote_entry =3D remotes_config > + .get(&remote) > + .ok_or_else(|| http_err!(NOT_FOUND, "remote '{remote}' not found= "))?; > + let live =3D get_subscription_info_for_remote(remote_entry, FRESH_NO= DE_STATUS_MAX_AGE) > + .await > + .map_err(|err| { > + http_err!( > + BAD_REQUEST, > + "could not read subscription on {remote}/{node}: {err}" > + ) > + })?; > + let live_current_key: Option =3D live > + .get(&node) > + .and_then(|info| info.as_ref()) > + .and_then(|info| info.key.clone()); > + > + let _lock =3D pdm_config::subscriptions::lock_config()?; This potentially locks up the async runtime, this must be inside a spawn_blocking > + let (mut config, config_digest) =3D pdm_config::subscriptions::confi= g()?; > + config_digest.detect_modification(digest.as_ref())?; > + > + let bound_id =3D config > + .iter() > + .find(|(_, e)| { > + e.remote.as_deref() =3D=3D Some(remote.as_str()) && e.node.a= s_deref() =3D=3D Some(node.as_str()) > + }) > + .map(|(id, _)| id.to_string()); > + > + if let Some(id) =3D bound_id { > + let entry =3D config.get_mut(&id).unwrap(); > + if entry.pending_reissue { > + http_bail!(BAD_REQUEST, "reissue already queued for {remote}= /{node}"); > + } > + entry.pending_reissue =3D true; > + } else { > + // No pool entry is bound to this (remote, node). Three sub-case= s for the live key: > + // - Already in the pool but unbound: rebind to target and flag.= This covers the user > + // path where Clear Assignment dropped the binding while the l= ive subscription stayed. > + // - Already in the pool but bound elsewhere: refuse, the operat= or has to reconcile. > + // - Not in the pool: adopt by inserting a fresh entry. If Clear= Pending fires later, > + // the entry stays as a now-managed free pool key. > + let current_key =3D live_current_key > + .ok_or_else(|| http_err!(NOT_FOUND, "no subscription on {rem= ote}/{node}"))?; > + > + if let Some(existing) =3D config.get_mut(¤t_key) { > + if existing.remote.is_some() || existing.node.is_some() { > + http_bail!( > + CONFLICT, > + "key '{current_key}' is in the pool but bound elsewh= ere; resolve manually first" > + ); > + } > + existing.remote =3D Some(remote.clone()); > + existing.node =3D Some(node.clone()); > + existing.pending_reissue =3D true; > + } else { > + SUBSCRIPTION_KEY_SCHEMA > + .parse_simple_value(¤t_key) > + .map_err(|err| http_err!(BAD_REQUEST, "key '{current_key= }' rejected: {err}"))?; > + let product_type =3D ProductType::from_key(¤t_key) > + .ok_or_else(|| format_err!("unrecognised key prefix: {cu= rrent_key}"))?; > + let entry =3D SubscriptionKeyEntry { > + key: current_key.clone(), > + product_type, > + level: SubscriptionLevel::from_key(Some(¤t_key)), > + source: SubscriptionKeySource::Manual, > + remote: Some(remote.clone()), > + node: Some(node.clone()), > + pending_reissue: true, > + ..Default::default() > + }; > + config.insert(current_key, entry); > + } > + } > + > + pdm_config::subscriptions::save_config(&config)?; > + Ok(()) > +} > + [...] > =20 > #[doc(hidden)] > @@ -332,8 +339,24 @@ fn key_cell(n: &RemoteNodeStatus) -> Html { > let assigned =3D n.assigned_key.as_deref(); > let current =3D n.current_key.as_deref(); > =20 > - // Pending =3D pool key assigned but the node doesn't have an active= subscription yet (the key > - // still needs to be pushed). > + if n.pending_reissue { > + // Reissue queued: surface the live key the operator is about to= free, with a recycle > + // icon in the warning colour so the row stands out next to ordi= nary pending pushes. > + let text =3D current.or(assigned).unwrap_or(""); > + return Tooltip::new( > + Row::new() > + .class(AlignItems::Baseline) > + .gap(2) > + .with_child(Fa::new("recycle").class(FontColor::Warning)= ) > + .with_child(text), > + ) > + .tip(tr!( > + "Pending Reissue - Apply Pending will remove this subscripti= on from the node." nit: I'd put Apply Pending in single quotes ('Apply Pending' will remove ..= .) > + )) > + .into(); > + } > + > + // Pending push =3D pool key assigned but the node doesn't have an a= ctive subscription yet. > let pending =3D > assigned.is_some() && n.status !=3D proxmox_subscription::Subscr= iptionStatus::Active; > =20 > @@ -449,7 +472,7 @@ impl LoadableComponent for SubscriptionRegistryComp { > }); > } > Msg::ClearSelectedNode =3D> { > - let Some(key) =3D self.selected_assigned_key() else { > + let Some(key) =3D self.clear_assignment_target_key() els= e { > return false; > }; > let link =3D ctx.link().clone(); > @@ -461,12 +484,16 @@ impl LoadableComponent for SubscriptionRegistryComp= { > link.send_reload(); > }); > } > - Msg::RemoveSelectedNodeKey =3D> { > - let Some(key) =3D self.selected_assigned_key() else { > + Msg::QueueReissueForSelectedNode =3D> { > + let Some((remote, node, current_key)) =3D self.selected_= node_for_reissue() else { > return false; > }; > ctx.link() > - .change_view(Some(ViewState::ConfirmRemoveSelectedNo= deKey(key))); > + .change_view(Some(ViewState::ConfirmQueueReissue { > + remote, > + node, > + current_key, > + })); > } > } > true > @@ -551,34 +578,56 @@ impl LoadableComponent for SubscriptionRegistryComp= { > ViewState::ConfirmAutoAssign(proposals) =3D> { > Some(self.render_auto_assign_dialog(ctx, proposals)) > } > - ViewState::ConfirmRemoveSelectedNodeKey(key) =3D> { > + ViewState::ConfirmQueueReissue { > + remote, > + node, > + current_key, > + } =3D> { > use pwt::widget::ConfirmDialog; > + let question =3D match current_key { > + Some(k) =3D> tr!( > + "Queue a reissue of {key} on {remote}/{node}?", > + key =3D k.clone(), > + remote =3D remote.clone(), > + node =3D node.clone(), > + ), > + None =3D> tr!( > + "Queue a reissue on {remote}/{node}?", > + remote =3D remote.clone(), > + node =3D node.clone(), > + ), > + }; > + let body =3D Column::new() > + .gap(2) > + .with_child(Container::from_tag("p").with_child(ques= tion)) > + .with_child(Container::from_tag("p").with_child(tr!( > + "Apply Pending will remove the subscription from= the node so the key can be reassigned elsewhere; Clear Pending undoes the = queueing without touching the remote." nit: I'd put Apply Pending in single quotes ('Apply Pending' will remove ..= .) > + ))); > + let remote_for_cb =3D remote.clone(); > + let node_for_cb =3D node.clone(); > let link =3D ctx.link().clone(); > - let key_for_callback =3D key.clone(); > Some( > - ConfirmDialog::new( > - tr!("Remove Key"), > - tr!( > - "Remove {key} from the key pool? This does n= ot revoke the subscription on the remote node.", > - key =3D key.clone(), > - ), > - ) > - .on_confirm(move |_| { > - let link =3D link.clone(); > - let key =3D key_for_callback.clone(); > - link.clone().spawn(async move { > - let url =3D format!( > - "/subscriptions/keys/{}", > - percent_encode_component(&key), > - ); > - if let Err(err) =3D http_delete(&url, None).= await { > - link.show_error(tr!("Remove Key"), err.t= o_string(), true); > - } > - link.change_view(None); > - link.send_reload(); > - }); > - }) > - .into(), > + ConfirmDialog::default() > + .title(tr!("Reissue Key")) > + .confirm_message(body) > + .on_confirm(move |_| { > + let link =3D link.clone(); > + let remote =3D remote_for_cb.clone(); > + let node =3D node_for_cb.clone(); > + link.clone().spawn(async move { > + let body =3D serde_json::json!({ > + "remote": remote, > + "node": node, > + }); > + if let Err(err) =3D http_post::<()>(QUEU= E_REISSUE_URL, Some(body)).await > + { > + link.show_error(tr!("Reissue Key"), = err.to_string(), true); > + } I'd use crate::pdm_client() here, since you added API bindings to lib/pdm_c= lient in this patch anyway? > + link.change_view(None); > + link.send_reload(); > + }); > + }) > + .into(), > ) > } > }