* [pdm-devel] [PATCH datacenter-manager 1/3] api: remote shell: do not rely on first node being reachable
2025-12-11 13:07 [pdm-devel] [PATCH datacenter-manager 0/3] improve node shell Fabian Grünbichler
@ 2025-12-11 13:07 ` Fabian Grünbichler
2025-12-11 13:07 ` [pdm-devel] [PATCH datacenter-manager 2/3] api: remote shell: improve error message Fabian Grünbichler
2025-12-11 13:07 ` [pdm-devel] [PATCH datacenter-manager 3/3] api: remote shell: make websocket proxy a worker task Fabian Grünbichler
2 siblings, 0 replies; 4+ messages in thread
From: Fabian Grünbichler @ 2025-12-11 13:07 UTC (permalink / raw)
To: pdm-devel
currently, there is no way to get to the raw, underlying HTTP or API client
from a PVEClient, and the PVEClient does not support doing the Websocket
upgrade.
by extending the "raw_client" to allow picking a specific endpoint, the remote
shell no longer relies on the first node of a remote being available, if the
target node is directly reachable from the PDM instance. this also fixes an
issue where the `termproxy` request via a regular PVEClient, and the
`vncwebsocket` request via the raw client ended up talking to two different
nodes, which does not work, since neither of those endpoints are proxied to the
target node on the PVE side.
this is just a stop gap measure, ideally we find a way to re-use the
MultiClient for PVE targets for the remote shell.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
server/src/api/remote_shell.rs | 37 +++++++++++++++----
server/src/connection.rs | 10 ++---
.../src/metric_collection/collection_task.rs | 6 ++-
3 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/server/src/api/remote_shell.rs b/server/src/api/remote_shell.rs
index c617b4d..d5e43e9 100644
--- a/server/src/api/remote_shell.rs
+++ b/server/src/api/remote_shell.rs
@@ -7,6 +7,7 @@ use http::{
};
use hyper::upgrade::Upgraded;
use hyper_util::rt::TokioIo;
+use pve_api_types::client::{PveClient, PveClientImpl};
use serde_json::{json, Value};
use proxmox_auth_api::{
@@ -169,9 +170,17 @@ fn upgrade_to_websocket(
let (remotes, _digest) = pdm_config::remotes::config()?;
let remote = get_remote(&remotes, &remote)?;
- let (ticket, port) = match remote.ty {
+ let (ticket, port, endpoint, first_api_url) = match remote.ty {
RemoteType::Pve => {
- let pve = crate::connection::make_pve_client(&remote)?;
+ let cache = crate::remote_cache::RemoteMappingCache::get();
+ let endpoint = match cache.info_by_node_name(&remote.id, &node) {
+ Some(info) if info.reachable => Some(info.hostname.clone()),
+ _ => None,
+ };
+ let raw_client =
+ crate::connection::make_raw_client(remote, endpoint.as_deref())?;
+ let api_url = raw_client.api_url().clone();
+ let pve = PveClientImpl(*raw_client);
let pve_term_ticket = pve
.node_shell_termproxy(
&node,
@@ -181,21 +190,35 @@ fn upgrade_to_websocket(
},
)
.await?;
- (pve_term_ticket.ticket, pve_term_ticket.port)
+ (
+ pve_term_ticket.ticket,
+ pve_term_ticket.port,
+ endpoint,
+ Some(api_url),
+ )
}
RemoteType::Pbs => {
let pbs = crate::connection::make_pbs_client(&remote)?;
let pbs_term_ticket = pbs.node_shell_termproxy().await?;
- (pbs_term_ticket.ticket, pbs_term_ticket.port as i64)
+ (
+ pbs_term_ticket.ticket,
+ pbs_term_ticket.port as i64,
+ None,
+ None,
+ )
}
};
- let raw_client = crate::connection::make_raw_client(remote)?;
-
+ let raw_client = crate::connection::make_raw_client(remote, endpoint.as_deref())?;
let ws_key = proxmox_sys::linux::random_data(16)?;
let ws_key = proxmox_base64::encode(&ws_key);
- let api_url = raw_client.api_url().clone().into_parts();
+ // ensure request above and below end up at the same node
+ let api_url = raw_client.api_url().clone();
+ if first_api_url.is_some() && first_api_url.as_ref() != Some(&api_url) {
+ bail!("termproxy and vncwebsocket API calls must be made to the same node..");
+ }
+ let api_url = api_url.into_parts();
let mut builder = http::uri::Builder::new();
if let Some(scheme) = api_url.scheme {
diff --git a/server/src/connection.rs b/server/src/connection.rs
index 7e36671..efb5a1d 100644
--- a/server/src/connection.rs
+++ b/server/src/connection.rs
@@ -258,7 +258,7 @@ pub trait ClientFactory {
async fn make_pbs_client_and_login(&self, remote: &Remote) -> Result<Box<PbsClient>, Error>;
/// Create a new API client for raw acess to the given remote
- fn make_raw_client(&self, remote: &Remote) -> Result<Box<Client>, Error>;
+ fn make_raw_client(&self, remote: &Remote, node: Option<&str>) -> Result<Box<Client>, Error>;
}
/// Default production client factory
@@ -353,8 +353,8 @@ impl ClientFactory for DefaultClientFactory {
ConnectionCache::get().make_pve_client(remote)
}
- fn make_raw_client(&self, remote: &Remote) -> Result<Box<Client>, Error> {
- Ok(Box::new(crate::connection::connect(remote, None)?))
+ fn make_raw_client(&self, remote: &Remote, node: Option<&str>) -> Result<Box<Client>, Error> {
+ Ok(Box::new(crate::connection::connect(remote, node)?))
}
fn make_pbs_client(&self, remote: &Remote) -> Result<Box<PbsClient>, Error> {
@@ -429,8 +429,8 @@ pub fn make_pbs_client(remote: &Remote) -> Result<Box<PbsClient>, Error> {
instance().make_pbs_client(remote)
}
-pub fn make_raw_client(remote: &Remote) -> Result<Box<Client>, Error> {
- instance().make_raw_client(remote)
+pub fn make_raw_client(remote: &Remote, node: Option<&str>) -> Result<Box<Client>, Error> {
+ instance().make_raw_client(remote, node)
}
/// Create a new API client for PVE remotes.
diff --git a/server/src/metric_collection/collection_task.rs b/server/src/metric_collection/collection_task.rs
index cc1a460..7da85de 100644
--- a/server/src/metric_collection/collection_task.rs
+++ b/server/src/metric_collection/collection_task.rs
@@ -431,7 +431,11 @@ pub(super) mod tests {
bail!("not implemented")
}
- fn make_raw_client(&self, _remote: &Remote) -> Result<Box<Client>, Error> {
+ fn make_raw_client(
+ &self,
+ _remote: &Remote,
+ _node: Option<&str>,
+ ) -> Result<Box<Client>, Error> {
bail!("not implemented")
}
--
2.47.3
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* [pdm-devel] [PATCH datacenter-manager 3/3] api: remote shell: make websocket proxy a worker task
2025-12-11 13:07 [pdm-devel] [PATCH datacenter-manager 0/3] improve node shell Fabian Grünbichler
2025-12-11 13:07 ` [pdm-devel] [PATCH datacenter-manager 1/3] api: remote shell: do not rely on first node being reachable Fabian Grünbichler
2025-12-11 13:07 ` [pdm-devel] [PATCH datacenter-manager 2/3] api: remote shell: improve error message Fabian Grünbichler
@ 2025-12-11 13:07 ` Fabian Grünbichler
2 siblings, 0 replies; 4+ messages in thread
From: Fabian Grünbichler @ 2025-12-11 13:07 UTC (permalink / raw)
To: pdm-devel
so that errors and warnings are visible to the user, at least in the task
list.. the request here is made by xterm.js as part of upgrading the connection
to a websocket, so returning the UPID in a meaningful fashion is hard..
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Notes:
best viewed with `-w`, the actual change is quite small..
server/src/api/remote_shell.rs | 202 +++++++++++++++++----------------
ui/src/tasks.rs | 1 +
2 files changed, 103 insertions(+), 100 deletions(-)
diff --git a/server/src/api/remote_shell.rs b/server/src/api/remote_shell.rs
index 5207095..f1e1cab 100644
--- a/server/src/api/remote_shell.rs
+++ b/server/src/api/remote_shell.rs
@@ -158,122 +158,124 @@ fn upgrade_to_websocket(
let (mut ws, response) = WebSocket::new(parts.headers.clone())?;
- proxmox_rest_server::spawn_internal_task(async move {
- let incoming_ws: Upgraded =
- match hyper::upgrade::on(Request::from_parts(parts, req_body)).await {
- Ok(upgraded) => upgraded,
- Err(err) => bail!("failed to process incoming Websocket upgrade: {err}"),
+ // Can't return UPID because the response is the Websocket upgrade..
+ let _upid = proxmox_rest_server::WorkerTask::spawn(
+ "remote_shell",
+ None,
+ auth_id.to_string(),
+ true,
+ async move |_worker| {
+ let incoming_ws: Upgraded =
+ match hyper::upgrade::on(Request::from_parts(parts, req_body)).await {
+ Ok(upgraded) => upgraded,
+ Err(err) => bail!("failed to process incoming Websocket upgrade: {err}"),
+ };
+
+ let (remotes, _digest) = pdm_config::remotes::config()?;
+ let remote = get_remote(&remotes, &remote)?;
+ let (ticket, port, endpoint, first_api_url) = match remote.ty {
+ RemoteType::Pve => {
+ let cache = crate::remote_cache::RemoteMappingCache::get();
+ let endpoint = match cache.info_by_node_name(&remote.id, &node) {
+ Some(info) if info.reachable => Some(info.hostname.clone()),
+ _ => None,
+ };
+ let raw_client =
+ crate::connection::make_raw_client(remote, endpoint.as_deref())?;
+ let api_url = raw_client.api_url().clone();
+ let pve = PveClientImpl(*raw_client);
+ let pve_term_ticket = pve
+ .node_shell_termproxy(
+ &node,
+ pve_api_types::NodeShellTermproxy {
+ cmd: None,
+ cmd_opts: None,
+ },
+ )
+ .await?;
+ (
+ pve_term_ticket.ticket,
+ pve_term_ticket.port,
+ endpoint,
+ Some(api_url),
+ )
+ }
+ RemoteType::Pbs => {
+ let pbs = crate::connection::make_pbs_client(&remote)?;
+ let pbs_term_ticket = pbs.node_shell_termproxy().await?;
+ (
+ pbs_term_ticket.ticket,
+ pbs_term_ticket.port as i64,
+ None,
+ None,
+ )
+ }
};
- let (remotes, _digest) = pdm_config::remotes::config()?;
- let remote = get_remote(&remotes, &remote)?;
- let (ticket, port, endpoint, first_api_url) = match remote.ty {
- RemoteType::Pve => {
- let cache = crate::remote_cache::RemoteMappingCache::get();
- let endpoint = match cache.info_by_node_name(&remote.id, &node) {
- Some(info) if info.reachable => Some(info.hostname.clone()),
- _ => None,
- };
- let raw_client =
- crate::connection::make_raw_client(remote, endpoint.as_deref())?;
- let api_url = raw_client.api_url().clone();
- let pve = PveClientImpl(*raw_client);
- let pve_term_ticket = pve
- .node_shell_termproxy(
- &node,
- pve_api_types::NodeShellTermproxy {
- cmd: None,
- cmd_opts: None,
- },
- )
- .await?;
- (
- pve_term_ticket.ticket,
- pve_term_ticket.port,
- endpoint,
- Some(api_url),
- )
+ let raw_client = crate::connection::make_raw_client(remote, endpoint.as_deref())?;
+ let ws_key = proxmox_sys::linux::random_data(16)?;
+ let ws_key = proxmox_base64::encode(&ws_key);
+
+ // ensure request above and below end up at the same node
+ let api_url = raw_client.api_url().clone();
+ if first_api_url.is_some() && first_api_url.as_ref() != Some(&api_url) {
+ bail!("termproxy and vncwebsocket API calls must be made to the same node..");
}
- RemoteType::Pbs => {
- let pbs = crate::connection::make_pbs_client(&remote)?;
- let pbs_term_ticket = pbs.node_shell_termproxy().await?;
- (
- pbs_term_ticket.ticket,
- pbs_term_ticket.port as i64,
- None,
- None,
- )
+ let api_url = api_url.into_parts();
+
+ let mut builder = http::uri::Builder::new();
+ if let Some(scheme) = api_url.scheme {
+ builder = builder.scheme(scheme);
}
- };
+ if let Some(authority) = api_url.authority {
+ builder = builder.authority(authority)
+ }
+ let api_path = ApiPathBuilder::new(format!("/api2/json/nodes/{node}/vncwebsocket"))
+ .arg("vncticket", ticket.clone())
+ .arg("port", port)
+ .build();
+ let uri = builder
+ .path_and_query(api_path)
+ .build()
+ .map_err(|err| format_err!("failed to build Uri - {err}"))?;
- let raw_client = crate::connection::make_raw_client(remote, endpoint.as_deref())?;
- let ws_key = proxmox_sys::linux::random_data(16)?;
- let ws_key = proxmox_base64::encode(&ws_key);
+ let auth = raw_client.login_auth()?;
+ let req = Request::builder()
+ .method(Method::GET)
+ .uri(uri)
+ .header(UPGRADE, "websocket")
+ .header(SEC_WEBSOCKET_VERSION, "13")
+ .header(SEC_WEBSOCKET_KEY, ws_key);
- // ensure request above and below end up at the same node
- let api_url = raw_client.api_url().clone();
- if first_api_url.is_some() && first_api_url.as_ref() != Some(&api_url) {
- bail!("termproxy and vncwebsocket API calls must be made to the same node..");
- }
- let api_url = api_url.into_parts();
+ let req = auth.set_auth_headers(req).body(Body::empty())?;
- let mut builder = http::uri::Builder::new();
- if let Some(scheme) = api_url.scheme {
- builder = builder.scheme(scheme);
- }
- if let Some(authority) = api_url.authority {
- builder = builder.authority(authority)
- }
- let api_path = ApiPathBuilder::new(format!("/api2/json/nodes/{node}/vncwebsocket"))
- .arg("vncticket", ticket.clone())
- .arg("port", port)
- .build();
- let uri = builder
- .path_and_query(api_path)
- .build()
- .map_err(|err| format_err!("failed to build Uri - {err}"))?;
+ let res = raw_client.http_client().request(req).await?;
+ if res.status() != StatusCode::SWITCHING_PROTOCOLS {
+ bail!("server didn't upgrade: {}", res.status());
+ }
- let auth = raw_client.login_auth()?;
- let req = Request::builder()
- .method(Method::GET)
- .uri(uri)
- .header(UPGRADE, "websocket")
- .header(SEC_WEBSOCKET_VERSION, "13")
- .header(SEC_WEBSOCKET_KEY, ws_key);
+ let pve_ws = hyper::upgrade::on(res)
+ .await
+ .map_err(|err| format_err!("failed to upgrade - {}", err))?;
- let req = auth.set_auth_headers(req).body(Body::empty())?;
+ let username = if let proxmox_client::AuthenticationKind::Token(ref token) = *auth {
+ token.userid.clone()
+ } else {
+ bail!("shell not supported with ticket-based authentication")
+ };
- let res = raw_client.http_client().request(req).await?;
- if res.status() != StatusCode::SWITCHING_PROTOCOLS {
- bail!("server didn't upgrade: {}", res.status());
- }
+ let preamble = format!("{username}:{ticket}\n", ticket = ticket);
+ ws.mask = Some([0, 0, 0, 0]);
- let pve_ws = hyper::upgrade::on(res)
- .await
- .map_err(|err| format_err!("failed to upgrade - {}", err))?;
-
- let username = if let proxmox_client::AuthenticationKind::Token(ref token) = *auth {
- token.userid.clone()
- } else {
- bail!("shell not supported with ticket-based authentication")
- };
-
- let preamble = format!("{username}:{ticket}\n", ticket = ticket);
- ws.mask = Some([0, 0, 0, 0]);
-
- if let Err(err) = ws
- .proxy_connection(
+ ws.proxy_connection(
TokioIo::new(incoming_ws),
TokioIo::new(pve_ws),
preamble.as_bytes(),
)
.await
- {
- log::warn!("error while copying between websockets: {err:?}");
- }
-
- Ok(())
- });
+ .map_err(|err| format_err!("error while copying between websockets: {err:?}"))
+ },
+ )?;
Ok(response)
}
diff --git a/ui/src/tasks.rs b/ui/src/tasks.rs
index 0f9a9aa..a644d5b 100644
--- a/ui/src/tasks.rs
+++ b/ui/src/tasks.rs
@@ -105,6 +105,7 @@ pub fn register_pve_tasks() {
register_task_description("vzumount", ("CT", tr!("Unmount")));
register_task_description("zfscreate", (tr!("ZFS Storage"), tr!("Create")));
register_task_description("zfsremove", ("ZFS Pool", tr!("Remove")));
+ register_task_description("remote_shell", tr!("Remote node shell"));
}
/// Format a UPID that is either [`RemoteUpid`] or a [`UPID`]
--
2.47.3
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 4+ messages in thread