Re: [pbs-devel] [PATCH v3 proxmox-backup 3/3] proxy: redirect HTTP requests to HTTPS

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

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 <m.carrara@proxmox.com>
> >> ---
> >>  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 ;-)