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

[Max Carrara <m.carrara@proxmox.com>]

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.

> Otherwise, this shouldn't need to be a Vec, a regular array should work,
> skips the extra allocation.
> 
>> +
>> +                let mut results: Vec<Result<(), Error>> = vec![];
>> +
>> +                for res_handle in futures::future::join_all(handles).await.into_iter() {
>> +                    let flattened_res = match res_handle {
>> +                        Ok(inner) => inner,
>> +                        Err(err) => Err(format_err!(err)),
>> +                    };
>> +
>> +                    results.push(flattened_res);
>> +                }
>> +
>> +                if results.iter().any(Result::is_err) {
>> +                    let cat_errors = results
>> +                        .into_iter()
>> +                        .filter_map(|res| res.err().map(|err| err.to_string()))
>> +                        .collect::<Vec<_>>()
>> +                        .join("\n");
>> +
>> +                    return Err(format_err!(cat_errors));
>> +                }
>> +
>> +                Ok(())
>>              })
>>          },
>>          Some(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN),
>> -- 
>> 2.39.2