[pdm-devel] [PATCH datacenter-manager 0/3] improve node shell

[pdm-devel] [PATCH datacenter-manager 0/3] improve node shell

[Fabian Grünbichler]

this series attempts to improve the remote node shell by no longer
always using the first endpoint for a given remote, and ensuring both
requests made for setting up the remote shell are talking to the same
PVE node.

Fabian Grünbichler (3):
  api: remote shell: do not rely on first node being reachable
  api: remote shell: improve error message
  api: remote shell: make websocket proxy a worker task

 server/src/api/remote_shell.rs                | 186 ++++++++++--------
 server/src/connection.rs                      |  10 +-
 .../src/metric_collection/collection_task.rs  |   6 +-
 ui/src/tasks.rs                               |   1 +
 4 files changed, 115 insertions(+), 88 deletions(-)

-- 
2.47.3



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

[pdm-devel] [PATCH datacenter-manager 1/3] api: remote shell: do not rely on first node being reachable

[Fabian Grünbichler]

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

[pdm-devel] [PATCH datacenter-manager 2/3] api: remote shell: improve error message

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 server/src/api/remote_shell.rs | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/server/src/api/remote_shell.rs b/server/src/api/remote_shell.rs
index d5e43e9..5207095 100644
--- a/server/src/api/remote_shell.rs
+++ b/server/src/api/remote_shell.rs
@@ -1,5 +1,5 @@
 use anyhow::{bail, format_err, Error};
-use futures::{FutureExt, TryFutureExt};
+use futures::FutureExt;
 use http::{
     header::{SEC_WEBSOCKET_KEY, SEC_WEBSOCKET_VERSION, UPGRADE},
     request::Parts,
@@ -160,12 +160,9 @@ fn upgrade_to_websocket(
 
         proxmox_rest_server::spawn_internal_task(async move {
             let incoming_ws: Upgraded =
-                match hyper::upgrade::on(Request::from_parts(parts, req_body))
-                    .map_err(Error::from)
-                    .await
-                {
+                match hyper::upgrade::on(Request::from_parts(parts, req_body)).await {
                     Ok(upgraded) => upgraded,
-                    _ => bail!("error"),
+                    Err(err) => bail!("failed to process incoming Websocket upgrade: {err}"),
                 };
 
             let (remotes, _digest) = pdm_config::remotes::config()?;
-- 
2.47.3



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

[pdm-devel] [PATCH datacenter-manager 3/3] api: remote shell: make websocket proxy a worker task

[Fabian Grünbichler]

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