From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 135A4991BB for ; Thu, 16 Nov 2023 08:36:04 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E522DF714 for ; Thu, 16 Nov 2023 08:35:33 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 16 Nov 2023 08:35:32 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C604B435DC for ; Thu, 16 Nov 2023 08:35:31 +0100 (CET) Date: Thu, 16 Nov 2023 08:35:30 +0100 From: Wolfgang Bumiller To: Max Carrara Cc: pbs-devel@lists.proxmox.com Message-ID: References: <20231031184705.1142244-1-m.carrara@proxmox.com> <20231031184705.1142244-4-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL 0.101 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 T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH v3 proxmox-backup 3/3] proxy: redirect HTTP requests to HTTPS X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Nov 2023 07:36:04 -0000 On Wed, Nov 15, 2023 at 04:22:45PM +0100, Max Carrara wrote: > On 11/3/23 11:24, Wolfgang Bumiller wrote: > > On Tue, Oct 31, 2023 at 07:47:05PM +0100, Max Carrara wrote: > >> Signed-off-by: Max Carrara > >> --- > >> Changes v1 --> v2: > >> * Incorporate changes of the previous two patches correspondingly > >> > >> Changes v2 --> v3: > >> * None > >> > >> src/bin/proxmox-backup-proxy.rs | 46 ++++++++++++++++++++++++++++----- > >> 1 file changed, 39 insertions(+), 7 deletions(-) > >> > >> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs > >> index f38a02bd..f69f5bfc 100644 > >> --- a/src/bin/proxmox-backup-proxy.rs > >> +++ b/src/bin/proxmox-backup-proxy.rs > >> @@ -23,8 +23,8 @@ use proxmox_sys::{task_log, task_warn}; > >> use pbs_datastore::DataStore; > >> > >> use proxmox_rest_server::{ > >> - cleanup_old_tasks, cookie_from_header, rotate_task_log_archive, ApiConfig, RestEnvironment, > >> - RestServer, WorkerTask, > >> + cleanup_old_tasks, cookie_from_header, rotate_task_log_archive, ApiConfig, Redirector, > >> + RestEnvironment, RestServer, WorkerTask, > >> }; > >> > >> use proxmox_backup::rrd_cache::{ > >> @@ -253,6 +253,7 @@ async fn run() -> Result<(), Error> { > >> )?; > >> > >> let rest_server = RestServer::new(config); > >> + let redirector = Redirector::new(); > >> proxmox_rest_server::init_worker_tasks( > >> pbs_buildcfg::PROXMOX_BACKUP_LOG_DIR_M!().into(), > >> file_opts.clone(), > >> @@ -288,23 +289,54 @@ async fn run() -> Result<(), Error> { > >> Ok(Value::Null) > >> })?; > >> > >> - let connections = proxmox_rest_server::connection::AcceptBuilder::with_acceptor(acceptor) > >> + let connections = proxmox_rest_server::connection::AcceptBuilder::new() > >> .debug(debug) > >> .rate_limiter_lookup(Arc::new(lookup_rate_limiter)) > >> .tcp_keepalive_time(PROXMOX_BACKUP_TCP_KEEPALIVE_TIME); > >> + > >> let server = daemon::create_daemon( > >> ([0, 0, 0, 0, 0, 0, 0, 0], 8007).into(), > >> move |listener| { > >> - let connections = connections.accept(listener); > >> + let (secure_connections, insecure_connections) = > >> + connections.accept_tls_optional(listener, acceptor); > >> > >> Ok(async { > >> daemon::systemd_notify(daemon::SystemdNotify::Ready)?; > >> > >> - hyper::Server::builder(connections) > >> + let secure_server = hyper::Server::builder(secure_connections) > >> .serve(rest_server) > >> .with_graceful_shutdown(proxmox_rest_server::shutdown_future()) > >> - .map_err(Error::from) > >> - .await > >> + .map_err(Error::from); > >> + > >> + let insecure_server = hyper::Server::builder(insecure_connections) > >> + .serve(redirector) > >> + .with_graceful_shutdown(proxmox_rest_server::shutdown_future()) > >> + .map_err(Error::from); > >> + > >> + let handles = vec![tokio::spawn(secure_server), tokio::spawn(insecure_server)]; > > > > Maybe we should just detach the redirection-handler and potentially give > > it a retry logic and finally fail it with a log message. > > > > Did you have anything in particular in mind for the retry logic? I agree with > detaching the redirection-handler, but I don't quite understand what needs > to be retried; if something goes wrong, the entire daemon probably would need > to be recreated, AFAIU. Since they come from the same underlying listener, you're probably right. The parts in between are more unlikely to fail. Considering this, it might be just fine this way as it'll probably behave the same (I'd expect socket errors to fail both anyway), but then this should at least use a *simple* 2-way join, eg. `futures::future::try_join()` - to join exactly 2 futures we really don't need to manage a Vec and iterate through 2 elements ;-)