Re: [pbs-devel] [PATCH v3 proxmox 1/4] http: client: make https connector generic over resolver

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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