public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client
@ 2025-04-08 12:38 Christian Ebner
  2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox 1/4] http: client: make https connector generic over resolver Christian Ebner
                   ` (5 more replies)
  0 siblings, 6 replies; 10+ messages in thread
From: Christian Ebner @ 2025-04-08 12:38 UTC (permalink / raw)
  To: pbs-devel

As discussed in issue #4788 [0], statically linking of the
`proxmox-backup-client` still suffers from possible incompatible
dependencies on the NSS module libraries, further described in
[1].

This patch series provides the means to statically compile the client
with a workaround for the NSS lib limitation by relying on
`hickory-dns` [2], as suggested by Thomas.
The hickory-resolver is used for name resolution with the statically
compiled binary instead of the default `getaddrinfo` based
`GaiResolver`.

Since hickory-resolver depends on `ipconfig` for Windows targets when
the `system-config` feature is enabled (required to read nameservers
from `/etc/resolv.conf`), also the dependency on `ipconfig` is
required by cargo. To workaround this, a dummy crate is registered in
the cargo registry.

Changes since version 2:
- Use packaged version for hickory-resolver
- Drop unneeded `ipconfig` dependency workaround
- Rebased onto current master

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4788
[1] https://sourceware.org/glibc/wiki/FAQ#Even_statically_linked_programs_need_some_shared_libraries_which_is_not_acceptable_for_me.__What_can_I_do.3F
[2] https://github.com/hickory-dns/hickory-dns

proxmox:

Christian Ebner (1):
  http: client: make https connector generic over resolver

 proxmox-http/Cargo.toml              |  1 +
 proxmox-http/src/client/connector.rs | 17 ++++++++++++-----
 proxmox-http/src/client/simple.rs    |  3 ++-
 3 files changed, 15 insertions(+), 6 deletions(-)

proxmox-backup:

Christian Ebner (3):
  fix: 4788: Makefile: target for statically linked client binary
  Makefile: switch path based on build mode and target
  client: http: Use custom resolver for statically linked binary

 Cargo.toml                    |  1 +
 Makefile                      | 13 ++++++
 pbs-client/Cargo.toml         |  1 +
 pbs-client/src/http_client.rs | 81 +++++++++++++++++++++++++++++++++--
 4 files changed, 92 insertions(+), 4 deletions(-)

-- 
2.39.5



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [pbs-devel] [PATCH v3 proxmox 1/4] http: client: make https connector generic over resolver
  2025-04-08 12:38 [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client Christian Ebner
@ 2025-04-08 12:38 ` Christian Ebner
  2025-04-08 14:06   ` Wolfgang Bumiller
  2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox-backup 2/4] fix: 4788: Makefile: target for statically linked client binary Christian Ebner
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 10+ messages in thread
From: Christian Ebner @ 2025-04-08 12:38 UTC (permalink / raw)
  To: pbs-devel

Allow to instantiate a `HttpsConnector` not using the default
`getaddrinfo` based `GaiResolver` for domain name resolution, but
rather a custom resolver implementing the required traits.

The usecase for this is to swap out the DNS resolver for the
statically linked proxmox-backup-client binary, where the glibc
dependency is problematic because of possible ABI incompatibility.

Adds tower-service as cargo workspace dependency and build dependency
to debian/control.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- no changes

 proxmox-http/Cargo.toml              |  1 +
 proxmox-http/src/client/connector.rs | 17 ++++++++++++-----
 proxmox-http/src/client/simple.rs    |  3 ++-
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/proxmox-http/Cargo.toml b/proxmox-http/Cargo.toml
index c8c963f7..6bfb1413 100644
--- a/proxmox-http/Cargo.toml
+++ b/proxmox-http/Cargo.toml
@@ -22,6 +22,7 @@ openssl =  { version = "0.10", optional = true }
 serde_json = { workspace = true, optional = true }
 tokio = { workspace = true, features = [], optional = true }
 tokio-openssl = { workspace = true, optional = true }
+tower-service.workspace = true
 ureq = { version = "2.4", features = ["native-certs", "native-tls"], optional = true, default-features = false }
 url = { workspace = true, optional = true }
 
diff --git a/proxmox-http/src/client/connector.rs b/proxmox-http/src/client/connector.rs
index 63b9d10c..c0435c60 100644
--- a/proxmox-http/src/client/connector.rs
+++ b/proxmox-http/src/client/connector.rs
@@ -6,6 +6,7 @@ use std::task::{Context, Poll};
 
 use futures::*;
 use http::Uri;
+use hyper::client::connect::dns::Name;
 use hyper::client::HttpConnector;
 use openssl::ssl::SslConnector;
 use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt};
@@ -23,8 +24,8 @@ use crate::{RateLimitedStream, ShareableRateLimit};
 type SharedRateLimit = Arc<dyn ShareableRateLimit>;
 
 #[derive(Clone)]
-pub struct HttpsConnector {
-    connector: HttpConnector,
+pub struct HttpsConnector<T> {
+    connector: HttpConnector<T>,
     ssl_connector: Arc<SslConnector>,
     proxy: Option<ProxyConfig>,
     tcp_keepalive: u32,
@@ -32,9 +33,9 @@ pub struct HttpsConnector {
     write_limiter: Option<SharedRateLimit>,
 }
 
-impl HttpsConnector {
+impl<T> HttpsConnector<T> {
     pub fn with_connector(
-        mut connector: HttpConnector,
+        mut connector: HttpConnector<T>,
         ssl_connector: SslConnector,
         tcp_keepalive: u32,
     ) -> Self {
@@ -122,7 +123,13 @@ impl HttpsConnector {
     }
 }
 
-impl hyper::service::Service<Uri> for HttpsConnector {
+impl<T> hyper::service::Service<Uri> for HttpsConnector<T>
+where
+    T: tower_service::Service<Name> + Clone + Send + Sync + 'static,
+    T::Future: Send,
+    T::Error: Into<Box<(dyn std::error::Error + Send + Sync + 'static)>>,
+    T::Response: std::iter::Iterator<Item = std::net::SocketAddr>,
+{
     type Response = MaybeTlsStream<RateLimitedStream<TcpStream>>;
     type Error = Error;
     #[allow(clippy::type_complexity)]
diff --git a/proxmox-http/src/client/simple.rs b/proxmox-http/src/client/simple.rs
index 062889ac..cb8bb777 100644
--- a/proxmox-http/src/client/simple.rs
+++ b/proxmox-http/src/client/simple.rs
@@ -8,6 +8,7 @@ use futures::*;
 #[cfg(all(feature = "client-trait", feature = "proxmox-async"))]
 use http::header::HeaderName;
 use http::{HeaderValue, Request, Response};
+use hyper::client::connect::dns::GaiResolver;
 use hyper::client::Client as HyperClient;
 use hyper::client::HttpConnector;
 use hyper::Body;
@@ -18,7 +19,7 @@ use crate::HttpOptions;
 
 /// Asynchronous HTTP client implementation
 pub struct Client {
-    client: HyperClient<HttpsConnector, Body>,
+    client: HyperClient<HttpsConnector<GaiResolver>, Body>,
     options: HttpOptions,
 }
 
-- 
2.39.5



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [pbs-devel] [PATCH v3 proxmox-backup 2/4] fix: 4788: Makefile: target for statically linked client binary
  2025-04-08 12:38 [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client Christian Ebner
  2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox 1/4] http: client: make https connector generic over resolver Christian Ebner
@ 2025-04-08 12:38 ` Christian Ebner
  2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox-backup 3/4] Makefile: switch path based on build mode and target Christian Ebner
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Christian Ebner @ 2025-04-08 12:38 UTC (permalink / raw)
  To: pbs-devel

Adds the build target including workarounds to generate a statically
linked version of the proxmox-backup-client binary.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=4788
Suggested-by: Christoph Heiss <c.heiss@proxmox.com>
Originally-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- included fixes trailer

 Makefile | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Makefile b/Makefile
index dfbaacab4..00e7f7401 100644
--- a/Makefile
+++ b/Makefile
@@ -227,3 +227,12 @@ upload: $(SERVER_DEB) $(CLIENT_DEB) $(RESTORE_DEB) $(DOC_DEB)
 	  | ssh -X repoman@repo.proxmox.com upload --product pbs --dist $(UPLOAD_DIST)
 	tar cf - $(CLIENT_DEB) $(CLIENT_DBG_DEB) | ssh -X repoman@repo.proxmox.com upload --product "pve,pmg,pbs-client" --dist $(UPLOAD_DIST)
 	tar cf - $(RESTORE_DEB) $(RESTORE_DBG_DEB) | ssh -X repoman@repo.proxmox.com upload --product "pve" --dist $(UPLOAD_DIST)
+
+.PHONY: proxmox-backup-client-static
+proxmox-backup-client-static:
+	mkdir -p target/release/deps/ && \
+          echo '!<arch>' > target/release/deps/libsystemd.a # workaround for to greedy linkage and proxmox-systemd
+	RUSTFLAGS='-C target-feature=+crt-static -C strip=debuginfo' \
+        $(CARGO) build $(CARGO_BUILD_ARGS) \
+          --package proxmox-backup-client --bin proxmox-backup-client \
+          --target x86_64-unknown-linux-gnu
-- 
2.39.5



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [pbs-devel] [PATCH v3 proxmox-backup 3/4] Makefile: switch path based on build mode and target
  2025-04-08 12:38 [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client Christian Ebner
  2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox 1/4] http: client: make https connector generic over resolver Christian Ebner
  2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox-backup 2/4] fix: 4788: Makefile: target for statically linked client binary Christian Ebner
@ 2025-04-08 12:38 ` Christian Ebner
  2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox-backup 4/4] client: http: Use custom resolver for statically linked binary Christian Ebner
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Christian Ebner @ 2025-04-08 12:38 UTC (permalink / raw)
  To: pbs-devel

Define variables for the compile path and target and use these
instead of hard-coding the path. Allows to easily switch between
debug and release mode for compilation.

Also, place the libsystemd stub into its own subdirectory for cleaner
separation from the compiled artifacts.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- no changes

 Makefile | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 00e7f7401..428ef40b9 100644
--- a/Makefile
+++ b/Makefile
@@ -36,11 +36,15 @@ SUBCRATES != cargo metadata --no-deps --format-version=1 \
 	| grep "$$PWD/" \
 	| sed -e "s!.*$$PWD/!!g" -e 's/\#.*$$//g' -e 's/)$$//g'
 
+STATIC_TARGET ?= x86_64-unknown-linux-gnu
+
 ifeq ($(BUILD_MODE), release)
 CARGO_BUILD_ARGS += --release
 COMPILEDIR := target/$(DEB_HOST_RUST_TYPE)/release
+STATIC_COMPILEDIR := target/$(STATIC_TARGET)/release
 else
 COMPILEDIR := target/$(DEB_HOST_RUST_TYPE)/debug
+STATIC_COMPILEDIR := target/$(STATIC_TARGET)/debug
 endif
 
 ifeq ($(valgrind), yes)
@@ -230,9 +234,9 @@ upload: $(SERVER_DEB) $(CLIENT_DEB) $(RESTORE_DEB) $(DOC_DEB)
 
 .PHONY: proxmox-backup-client-static
 proxmox-backup-client-static:
-	mkdir -p target/release/deps/ && \
-          echo '!<arch>' > target/release/deps/libsystemd.a # workaround for to greedy linkage and proxmox-systemd
-	RUSTFLAGS='-C target-feature=+crt-static -C strip=debuginfo' \
+	mkdir -p $(STATIC_COMPILEDIR)/deps-stubs/ && \
+          echo '!<arch>' > $(STATIC_COMPILEDIR)/deps-stubs/libsystemd.a # workaround for to greedy linkage and proxmox-systemd
+	RUSTFLAGS='-C target-feature=+crt-static -C strip=debuginfo -L $(STATIC_COMPILEDIR)/deps-stubs/' \
         $(CARGO) build $(CARGO_BUILD_ARGS) \
           --package proxmox-backup-client --bin proxmox-backup-client \
-          --target x86_64-unknown-linux-gnu
+          --target $(STATIC_TARGET)
-- 
2.39.5



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [pbs-devel] [PATCH v3 proxmox-backup 4/4] client: http: Use custom resolver for statically linked binary
  2025-04-08 12:38 [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client Christian Ebner
                   ` (2 preceding siblings ...)
  2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox-backup 3/4] Makefile: switch path based on build mode and target Christian Ebner
@ 2025-04-08 12:38 ` Christian Ebner
  2025-04-08 13:14 ` [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client Lukas Wagner
  2025-04-08 15:06 ` [pbs-devel] superseded: " Christian Ebner
  5 siblings, 0 replies; 10+ messages in thread
From: Christian Ebner @ 2025-04-08 12:38 UTC (permalink / raw)
  To: pbs-devel

The dependency on the `getaddrinfo` based `GaiResolver` used by
default for the `HttpClient` is not suitable for the statically
linked binary of the `proxmox-backup-client`, because of the
dependency on glibc NSS libraries, as described in glibc's FAQs [0].

As a workaround, conditionally compile the binary using the `hickory-dns`
resolver.

[0] https://sourceware.org/glibc/wiki/FAQ#Even_statically_linked_programs_need_some_shared_libraries_which_is_not_acceptable_for_me.__What_can_I_do.3F

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- rebased onto current master
- depend on packaged hickory-resolver
- drop unneeded workaround for ipconfig dependency

 Cargo.toml                    |  1 +
 pbs-client/Cargo.toml         |  1 +
 pbs-client/src/http_client.rs | 81 +++++++++++++++++++++++++++++++++--
 3 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 2b9fef9ed..50ae81db1 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -127,6 +127,7 @@ futures = "0.3"
 h2 = { version = "0.4", features = [ "legacy", "stream" ] }
 handlebars = "3.0"
 hex = "0.4.3"
+hickory-resolver = { version = "0.24.1", default-features = false, features = [ "system-config", "tokio-runtime" ] }
 hyper = { version = "0.14", features = [ "backports", "deprecated", "full" ] }
 libc = "0.2"
 log = "0.4.17"
diff --git a/pbs-client/Cargo.toml b/pbs-client/Cargo.toml
index c28fe87ca..b4f78b5fd 100644
--- a/pbs-client/Cargo.toml
+++ b/pbs-client/Cargo.toml
@@ -27,6 +27,7 @@ tokio = { workspace = true, features = [ "fs", "signal" ] }
 tokio-stream.workspace = true
 tower-service.workspace = true
 xdg.workspace = true
+hickory-resolver.workspace = true
 
 pathpatterns.workspace = true
 
diff --git a/pbs-client/src/http_client.rs b/pbs-client/src/http_client.rs
index 8a89031c8..c95def07b 100644
--- a/pbs-client/src/http_client.rs
+++ b/pbs-client/src/http_client.rs
@@ -4,6 +4,8 @@ use std::time::Duration;
 
 use anyhow::{bail, format_err, Error};
 use futures::*;
+#[cfg(not(target_feature = "crt-static"))]
+use hyper::client::connect::dns::GaiResolver;
 use hyper::client::{Client, HttpConnector};
 use hyper::http::header::HeaderValue;
 use hyper::http::Uri;
@@ -33,6 +35,74 @@ use pbs_api_types::{Authid, RateLimitConfig, Userid};
 use super::pipe_to_stream::PipeToSendStream;
 use super::PROXMOX_BACKUP_TCP_KEEPALIVE_TIME;
 
+#[cfg(not(target_feature = "crt-static"))]
+type DnsResolver = GaiResolver;
+
+#[cfg(target_feature = "crt-static")]
+type DnsResolver = resolver::HickoryDnsResolver;
+
+#[cfg(target_feature = "crt-static")]
+mod resolver {
+    use std::net::SocketAddr;
+    use std::pin::Pin;
+    use std::sync::Arc;
+    use std::task::{Context, Poll};
+
+    use futures::Future;
+    use hickory_resolver::error::ResolveError;
+    use hickory_resolver::lookup_ip::LookupIpIntoIter;
+    use hickory_resolver::TokioAsyncResolver;
+    use hyper::client::connect::dns::Name;
+    use tower_service::Service;
+
+    pub(crate) struct SocketAddrIter {
+        inner: LookupIpIntoIter,
+    }
+
+    impl Iterator for SocketAddrIter {
+        type Item = SocketAddr;
+
+        fn next(&mut self) -> Option<Self::Item> {
+            self.inner.next().map(|ip_addr| SocketAddr::new(ip_addr, 0))
+        }
+    }
+
+    #[derive(Clone)]
+    pub(crate) struct HickoryDnsResolver {
+        inner: Arc<TokioAsyncResolver>,
+    }
+
+    impl HickoryDnsResolver {
+        pub(crate) fn new() -> Self {
+            Self {
+                inner: Arc::new(TokioAsyncResolver::tokio_from_system_conf().unwrap()),
+            }
+        }
+    }
+
+    impl Service<Name> for HickoryDnsResolver {
+        type Response = SocketAddrIter;
+        type Error = ResolveError;
+        type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send>>;
+
+        fn poll_ready(&mut self, _ctx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
+            Poll::Ready(Ok(()))
+        }
+
+        fn call(&mut self, name: Name) -> Self::Future {
+            let inner = self.inner.clone();
+            Box::pin(async move {
+                inner
+                    .lookup_ip(name.as_str())
+                    .await
+                    .map(|r| SocketAddrIter {
+                        inner: r.into_iter(),
+                    })
+            })
+        }
+    }
+}
+
 /// Timeout used for several HTTP operations that are expected to finish quickly but may block in
 /// certain error conditions. Keep it generous, to avoid false-positive under high load.
 const HTTP_TIMEOUT: Duration = Duration::from_secs(2 * 60);
@@ -134,7 +204,7 @@ impl Default for HttpClientOptions {
 
 /// HTTP(S) API client
 pub struct HttpClient {
-    client: Client<HttpsConnector>,
+    client: Client<HttpsConnector<DnsResolver>>,
     server: String,
     port: u16,
     fingerprint: Arc<Mutex<Option<String>>>,
@@ -365,7 +435,8 @@ impl HttpClient {
             ssl_connector_builder.set_verify(openssl::ssl::SslVerifyMode::NONE);
         }
 
-        let mut httpc = HttpConnector::new();
+        let resolver = DnsResolver::new();
+        let mut httpc = HttpConnector::new_with_resolver(resolver);
         httpc.set_nodelay(true); // important for h2 download performance!
         httpc.enforce_http(false); // we want https...
 
@@ -526,7 +597,9 @@ impl HttpClient {
             _options: options,
         })
     }
+}
 
+impl HttpClient {
     /// Login
     ///
     /// Login is done on demand, so this is only required if you need
@@ -814,7 +887,7 @@ impl HttpClient {
     }
 
     async fn credentials(
-        client: Client<HttpsConnector>,
+        client: Client<HttpsConnector<DnsResolver>>,
         server: String,
         port: u16,
         username: Userid,
@@ -859,7 +932,7 @@ impl HttpClient {
     }
 
     async fn api_request(
-        client: Client<HttpsConnector>,
+        client: Client<HttpsConnector<DnsResolver>>,
         req: Request<Body>,
     ) -> Result<Value, Error> {
         Self::api_response(
-- 
2.39.5



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client
  2025-04-08 12:38 [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client Christian Ebner
                   ` (3 preceding siblings ...)
  2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox-backup 4/4] client: http: Use custom resolver for statically linked binary Christian Ebner
@ 2025-04-08 13:14 ` Lukas Wagner
  2025-04-08 14:19   ` Christian Ebner
  2025-04-08 15:06 ` [pbs-devel] superseded: " Christian Ebner
  5 siblings, 1 reply; 10+ messages in thread
From: Lukas Wagner @ 2025-04-08 13:14 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Christian Ebner

Gave this a quick test on the latest master.

$ make proxmox-backup-client-static
[...]
$ ldd target/x86_64-unknown-linux-gnu/debug/proxmox-backup-client
        statically linked

Nice! :)

Tested a couple of commands against my local PBS instance. One thing that tripped me
up was that hickory-dns does not like plain hostnames when resolving IPs
from /etc/hosts:

This does NOT work:

  $ cat /etc/hosts
  192.168.xxx.xxx pbs

  $ export PBS_REPOSITORY=pbs:store
  $ ./proxmox-backup-client ...


This works:

  $ cat /etc/hosts
  192.168.xxx.xxx pbs.example.com

  $ export PBS_REPOSITORY=pbs.example.com:store
  $ ./proxmox-backup-client ...

I did not really dig any deeper, but any obvious differences in name
resolution which could trip up users should probably be documented in
some form.

Tested-by: Lukas Wagner <l.wagner@proxmox.com>

-- 
- Lukas




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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pbs-devel] [PATCH v3 proxmox 1/4] http: client: make https connector generic over resolver
  2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox 1/4] http: client: make https connector generic over resolver Christian Ebner
@ 2025-04-08 14:06   ` Wolfgang Bumiller
  2025-04-08 14:21     ` Christian Ebner
  0 siblings, 1 reply; 10+ messages in thread
From: Wolfgang Bumiller @ 2025-04-08 14:06 UTC (permalink / raw)
  To: Christian Ebner; +Cc: pbs-devel

One tiny nit...

On Tue, Apr 08, 2025 at 02:38:08PM +0200, Christian Ebner wrote:
> Allow to instantiate a `HttpsConnector` not using the default
> `getaddrinfo` based `GaiResolver` for domain name resolution, but
> rather a custom resolver implementing the required traits.
> 
> The usecase for this is to swap out the DNS resolver for the
> statically linked proxmox-backup-client binary, where the glibc
> dependency is problematic because of possible ABI incompatibility.
> 
> Adds tower-service as cargo workspace dependency and build dependency
> to debian/control.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 2:
> - no changes
> 
>  proxmox-http/Cargo.toml              |  1 +
>  proxmox-http/src/client/connector.rs | 17 ++++++++++++-----
>  proxmox-http/src/client/simple.rs    |  3 ++-
>  3 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/proxmox-http/Cargo.toml b/proxmox-http/Cargo.toml
> index c8c963f7..6bfb1413 100644
> --- a/proxmox-http/Cargo.toml
> +++ b/proxmox-http/Cargo.toml
> @@ -22,6 +22,7 @@ openssl =  { version = "0.10", optional = true }
>  serde_json = { workspace = true, optional = true }
>  tokio = { workspace = true, features = [], optional = true }
>  tokio-openssl = { workspace = true, optional = true }
> +tower-service.workspace = true
>  ureq = { version = "2.4", features = ["native-certs", "native-tls"], optional = true, default-features = false }
>  url = { workspace = true, optional = true }
>  
> diff --git a/proxmox-http/src/client/connector.rs b/proxmox-http/src/client/connector.rs
> index 63b9d10c..c0435c60 100644
> --- a/proxmox-http/src/client/connector.rs
> +++ b/proxmox-http/src/client/connector.rs
> @@ -6,6 +6,7 @@ use std::task::{Context, Poll};
>  
>  use futures::*;
>  use http::Uri;
> +use hyper::client::connect::dns::Name;
>  use hyper::client::HttpConnector;
>  use openssl::ssl::SslConnector;
>  use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt};
> @@ -23,8 +24,8 @@ use crate::{RateLimitedStream, ShareableRateLimit};
>  type SharedRateLimit = Arc<dyn ShareableRateLimit>;
>  
>  #[derive(Clone)]
> -pub struct HttpsConnector {
> -    connector: HttpConnector,
> +pub struct HttpsConnector<T> {

Generics on structs can have defaults, so if we use

    struct HttpsConnctor<T = GaiResolver> {}

then even the old pbs code builds.
While it is *still* technically a breaking change, other users of this
crate *likely* can do with just a dependency bump.

(But at least it *sort of* documents what the heck this `T` is supposed
to be on a type otherwise completely devoid of documentation ;-) )

> +    connector: HttpConnector<T>,
>      ssl_connector: Arc<SslConnector>,
>      proxy: Option<ProxyConfig>,
>      tcp_keepalive: u32,
> @@ -32,9 +33,9 @@ pub struct HttpsConnector {
>      write_limiter: Option<SharedRateLimit>,
>  }
>  
> -impl HttpsConnector {
> +impl<T> HttpsConnector<T> {
>      pub fn with_connector(
> -        mut connector: HttpConnector,
> +        mut connector: HttpConnector<T>,
>          ssl_connector: SslConnector,
>          tcp_keepalive: u32,
>      ) -> Self {
> @@ -122,7 +123,13 @@ impl HttpsConnector {
>      }
>  }
>  
> -impl hyper::service::Service<Uri> for HttpsConnector {
> +impl<T> hyper::service::Service<Uri> for HttpsConnector<T>
> +where
> +    T: tower_service::Service<Name> + Clone + Send + Sync + 'static,
> +    T::Future: Send,
> +    T::Error: Into<Box<(dyn std::error::Error + Send + Sync + 'static)>>,
> +    T::Response: std::iter::Iterator<Item = std::net::SocketAddr>,
> +{
>      type Response = MaybeTlsStream<RateLimitedStream<TcpStream>>;
>      type Error = Error;
>      #[allow(clippy::type_complexity)]
> diff --git a/proxmox-http/src/client/simple.rs b/proxmox-http/src/client/simple.rs
> index 062889ac..cb8bb777 100644
> --- a/proxmox-http/src/client/simple.rs
> +++ b/proxmox-http/src/client/simple.rs
> @@ -8,6 +8,7 @@ use futures::*;
>  #[cfg(all(feature = "client-trait", feature = "proxmox-async"))]
>  use http::header::HeaderName;
>  use http::{HeaderValue, Request, Response};
> +use hyper::client::connect::dns::GaiResolver;
>  use hyper::client::Client as HyperClient;
>  use hyper::client::HttpConnector;
>  use hyper::Body;
> @@ -18,7 +19,7 @@ use crate::HttpOptions;
>  
>  /// Asynchronous HTTP client implementation
>  pub struct Client {
> -    client: HyperClient<HttpsConnector, Body>,
> +    client: HyperClient<HttpsConnector<GaiResolver>, Body>,
>      options: HttpOptions,
>  }
>  
> -- 
> 2.39.5


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client
  2025-04-08 13:14 ` [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client Lukas Wagner
@ 2025-04-08 14:19   ` Christian Ebner
  0 siblings, 0 replies; 10+ messages in thread
From: Christian Ebner @ 2025-04-08 14:19 UTC (permalink / raw)
  To: Lukas Wagner, Proxmox Backup Server development discussion

On 4/8/25 15:14, Lukas Wagner wrote:
> Gave this a quick test on the latest master.
> 
> $ make proxmox-backup-client-static
> [...]
> $ ldd target/x86_64-unknown-linux-gnu/debug/proxmox-backup-client
>          statically linked
> 
> Nice! :)
> 
> Tested a couple of commands against my local PBS instance. One thing that tripped me
> up was that hickory-dns does not like plain hostnames when resolving IPs
> from /etc/hosts:
> 
> This does NOT work:
> 
>    $ cat /etc/hosts
>    192.168.xxx.xxx pbs
> 
>    $ export PBS_REPOSITORY=pbs:store
>    $ ./proxmox-backup-client ...
> 
> 
> This works:
> 
>    $ cat /etc/hosts
>    192.168.xxx.xxx pbs.example.com
> 
>    $ export PBS_REPOSITORY=pbs.example.com:store
>    $ ./proxmox-backup-client ...
> 
> I did not really dig any deeper, but any obvious differences in name
> resolution which could trip up users should probably be documented in
> some form.

Thanks for testing, already on it to include a note mentioning the 
different behavior in the docs!

> 
> Tested-by: Lukas Wagner <l.wagner@proxmox.com>
> 



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pbs-devel] [PATCH v3 proxmox 1/4] http: client: make https connector generic over resolver
  2025-04-08 14:06   ` Wolfgang Bumiller
@ 2025-04-08 14:21     ` Christian Ebner
  0 siblings, 0 replies; 10+ messages in thread
From: Christian Ebner @ 2025-04-08 14:21 UTC (permalink / raw)
  To: Wolfgang Bumiller; +Cc: pbs-devel

On 4/8/25 16:06, Wolfgang Bumiller wrote:
> One tiny nit...
> 
> On Tue, Apr 08, 2025 at 02:38:08PM +0200, Christian Ebner wrote:
>>   
>>   #[derive(Clone)]
>> -pub struct HttpsConnector {
>> -    connector: HttpConnector,
>> +pub struct HttpsConnector<T> {
> 
> Generics on structs can have defaults, so if we use
> 
>      struct HttpsConnctor<T = GaiResolver> {}
> 
> then even the old pbs code builds.
> While it is *still* technically a breaking change, other users of this
> crate *likely* can do with just a dependency bump.
> 
> (But at least it *sort of* documents what the heck this `T` is supposed
> to be on a type otherwise completely devoid of documentation ;-) )

A great! Will add the default type for the generic and add the docs as 
requested by Lukas in the other response.


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [pbs-devel] superseded: [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client
  2025-04-08 12:38 [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client Christian Ebner
                   ` (4 preceding siblings ...)
  2025-04-08 13:14 ` [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client Lukas Wagner
@ 2025-04-08 15:06 ` Christian Ebner
  5 siblings, 0 replies; 10+ messages in thread
From: Christian Ebner @ 2025-04-08 15:06 UTC (permalink / raw)
  To: pbs-devel

superseded-by version 4:
https://lore.proxmox.com/pbs-devel/20250408150418.292311-1-c.ebner@proxmox.com/T/


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-04-08 15:06 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-04-08 12:38 [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client Christian Ebner
2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox 1/4] http: client: make https connector generic over resolver Christian Ebner
2025-04-08 14:06   ` Wolfgang Bumiller
2025-04-08 14:21     ` Christian Ebner
2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox-backup 2/4] fix: 4788: Makefile: target for statically linked client binary Christian Ebner
2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox-backup 3/4] Makefile: switch path based on build mode and target Christian Ebner
2025-04-08 12:38 ` [pbs-devel] [PATCH v3 proxmox-backup 4/4] client: http: Use custom resolver for statically linked binary Christian Ebner
2025-04-08 13:14 ` [pbs-devel] [PATCH v3 proxmox proxmox-backup 0/4] fix 4788: statically linked proxmox-backup-client Lukas Wagner
2025-04-08 14:19   ` Christian Ebner
2025-04-08 15:06 ` [pbs-devel] superseded: " Christian Ebner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal