From 65ab545b44c8b3908fd37b69986fe71b1f2d050a Mon Sep 17 00:00:00 2001 From: Olof Date: Wed, 20 Nov 2024 11:22:59 +0000 Subject: [PATCH] fix deadlock and add retry to LE thread rather than just breaking the loop --- src/api/controllers/settings.rs | 27 +++--- src/api/mod.rs | 9 +- src/http_proxy/websockets.rs | 6 +- src/letsencrypt.rs | 155 +++++++++++++++++--------------- src/main.rs | 5 -- 5 files changed, 107 insertions(+), 95 deletions(-) diff --git a/src/api/controllers/settings.rs b/src/api/controllers/settings.rs index 8b7eca8..fcf4beb 100644 --- a/src/api/controllers/settings.rs +++ b/src/api/controllers/settings.rs @@ -1,9 +1,10 @@ use std::time::Duration; +use crate::configuration::OddBoxConfiguration; + use super::*; use utoipa::ToSchema; -use crate::configuration::OddBoxConfiguration; #[derive(Debug,Serialize,Deserialize,Clone,ToSchema)] @@ -157,6 +158,7 @@ pub async fn get_settings_handler( (status = 500, description = "When something goes wrong", body = String), ) )] + pub async fn set_settings_handler( axum::extract::State(global_state): axum::extract::State>, Json(new_settings): Json @@ -166,6 +168,8 @@ pub async fn set_settings_handler( // then we write it to disk. the disk update will be picked up by the reload function // which will then apply the new settings. let mut config = { global_state.config.read().await.clone() }; + let old_config = config.clone(); + let nlea = if new_settings.lets_encrypt_account_email.len() > 0 { Some(new_settings.lets_encrypt_account_email.clone()) } else { @@ -207,22 +211,25 @@ pub async fn set_settings_handler( } else { config.root_dir = Some(new_settings.root_dir.clone()); } + + if config.eq(&old_config) { + tracing::warn!("Request to update settings thru api changed nothing."); + return Ok::<(),(StatusCode,String)>(()) + } config.write_to_disk().map_err(|e|(StatusCode::BAD_REQUEST,format!("{}",e.to_string())))?; + + let mut i = 0; tracing::info!("Configuration updated and written to disk. Waiting for reload to complete."); - loop { - if i > 1000 { // 10 seconds - return Err((StatusCode::INTERNAL_SERVER_ERROR,"Failed to update global settings".to_string())); - } - { - let active_config = global_state.config.read().await; - if active_config.internal_version > config.internal_version { - break; - } + while i < 1000 { + let active_config = global_state.config.read().await; + if active_config.internal_version > old_config.internal_version { + break; } + drop(active_config); i+=1; tokio::time::sleep(Duration::from_millis(10)).await; } diff --git a/src/api/mod.rs b/src/api/mod.rs index c326b0e..b50485e 100644 --- a/src/api/mod.rs +++ b/src/api/mod.rs @@ -283,10 +283,8 @@ impl OddBoxAPI { next: axum::middleware::Next, state: Arc ) -> Result { - let guard = state.config.read().await; - - let path = req.uri().path(); + let path = req.uri().path(); if req.method() == hyper::Method::GET { if path.starts_with("/api-docs/") { return Ok(next.run(req).await); @@ -301,7 +299,10 @@ impl OddBoxAPI { return Ok(next.run(req).await); } } - if let Some(pwd) = &guard.odd_box_password { + + let possibly_password = { state.config.read().await.odd_box_password.clone() }; + + if let Some(pwd) = &possibly_password { match req.headers().get("Authorization") { Some(value) if value == pwd => { Ok(next.run(req).await) diff --git a/src/http_proxy/websockets.rs b/src/http_proxy/websockets.rs index c345e00..0b046ee 100644 --- a/src/http_proxy/websockets.rs +++ b/src/http_proxy/websockets.rs @@ -26,17 +26,17 @@ pub async fn handle_ws(req:Request,service:ReverseProxyService,ws: tracing::trace!("Handling websocket request: {req_host_name:?} --> {req_path}"); - let read_guard = service.state.config.read().await; + let cfg = { service.state.config.read().await.clone() }; let target = { - if let Some(proc) = read_guard.hosted_process.iter().flatten().find(|p| { + if let Some(proc) = cfg.hosted_process.iter().flatten().find(|p| { req_host_name == p.host_name || p.capture_subdomains.unwrap_or_default() && req_host_name.ends_with(&format!(".{}",p.host_name)) }) { crate::http_proxy::utils::Target::Proc(proc.clone()) - } else if let Some(remsite) = read_guard.remote_target.iter().flatten().find(|x| { + } else if let Some(remsite) = cfg.remote_target.iter().flatten().find(|x| { req_host_name == x.host_name || x.capture_subdomains.unwrap_or_default() && req_host_name.ends_with(&format!(".{}",x.host_name)) }) { diff --git a/src/letsencrypt.rs b/src/letsencrypt.rs index ffe0669..62f9e80 100644 --- a/src/letsencrypt.rs +++ b/src/letsencrypt.rs @@ -707,97 +707,106 @@ pub async fn bg_worker_for_lets_encrypt_certs(state: Arc) { // NOTE 2: We generate these certificates in a loop and not OTF. This is to avoid concurrent requests to lets-encrypt. loop { + tokio::time::sleep(Duration::from_secs(5)).await; + { + let mail = { + if let Some(e) = state.config.read().await.lets_encrypt_account_email.clone() { + e + } else { + + crate::BG_WORKER_THREAD_MAP.insert("Lets Encrypt".into(), BgTaskInfo { + liveness_ptr: Arc::downgrade(&liveness_token), + status: format!("Disabled. lets_encrypt_account_email not set.") + }); + continue; + + } - let state_guard = state.config.read().await; - if state_guard.lets_encrypt_account_email.is_none() { - crate::BG_WORKER_THREAD_MAP.insert("Lets Encrypt".into(), BgTaskInfo { - liveness_ptr: Arc::downgrade(&liveness_token), - status: format!("Disabled. lets_encrypt_account_email not set.") - }); - drop(state_guard); - tokio::time::sleep(Duration::from_secs(10)).await; - continue; - } - - let mut lem_guard = state.cert_resolver.lets_encrypt_manager.write().await; - if lem_guard.is_none() { - let state_guard = state.config.read().await; - lem_guard.replace( - LECertManager::new(state_guard.lets_encrypt_account_email.as_ref().unwrap()).await.unwrap() - ); - } - drop(lem_guard); + }; + let mut lem_guard = state.cert_resolver.lets_encrypt_manager.write().await; + if lem_guard.is_none() { + match LECertManager::new(&mail).await { + Ok(v) => lem_guard.replace(v) , + Err(e) => { + tracing::warn!("Failed to create lets-encrypt manager: {e:?}"); + continue + } + }; + } + drop(lem_guard); - let active_challenges_count = crate::letsencrypt::DOMAIN_TO_CHALLENGE_TOKEN_MAP.len(); + let active_challenges_count = crate::letsencrypt::DOMAIN_TO_CHALLENGE_TOKEN_MAP.len(); - - - // TODO: should filter out local sites so we do not try to create certs for things like test.localhost or test.localtest.me etc. - // but instead write a warning about it. - let mut all_sites_with_lets_encrypt_enabled = - state_guard.remote_target - .iter() - .flatten() - .filter(|x|x.enable_lets_encrypt.unwrap_or(false)).map(|x|x.host_name.clone()) - .chain( - state_guard.hosted_process - .iter() - .flatten() - .filter(|x|x.enable_lets_encrypt.unwrap_or(false)).map(|x|x.host_name.clone()) - ).chain( - state_guard.dir_server + + + // TODO: should filter out local sites so we do not try to create certs for things like test.localhost or test.localtest.me etc. + // but instead write a warning about it. + let state_guard = state.config.read().await; + let mut all_sites_with_lets_encrypt_enabled = + state_guard.remote_target .iter() .flatten() .filter(|x|x.enable_lets_encrypt.unwrap_or(false)).map(|x|x.host_name.clone()) - ).collect::>(); - - if let Some(ourl) = state_guard.odd_box_url.as_ref() { - all_sites_with_lets_encrypt_enabled.push(ourl.clone()); - } - - drop(state_guard); + .chain( + state_guard.hosted_process + .iter() + .flatten() + .filter(|x|x.enable_lets_encrypt.unwrap_or(false)).map(|x|x.host_name.clone()) + ).chain( + state_guard.dir_server + .iter() + .flatten() + .filter(|x|x.enable_lets_encrypt.unwrap_or(false)).map(|x|x.host_name.clone()) + ).collect::>(); + + if let Some(ourl) = state_guard.odd_box_url.as_ref() { + all_sites_with_lets_encrypt_enabled.push(ourl.clone()); + } - + drop(state_guard); - all_sites_with_lets_encrypt_enabled.sort(); - all_sites_with_lets_encrypt_enabled.dedup(); + - let guard = state.cert_resolver.lets_encrypt_manager.read().await; + all_sites_with_lets_encrypt_enabled.sort(); + all_sites_with_lets_encrypt_enabled.dedup(); - if let Some(mgr) = guard.as_ref() { - for domain_name in all_sites_with_lets_encrypt_enabled { + let guard = state.cert_resolver.lets_encrypt_manager.read().await; - if let Some(_) = state.cert_resolver.get_lets_encrypt_signed_cert_from_mem_cache(&domain_name) { - tracing::info!("LE CERT IS OK FOR: {}", domain_name); - continue; - } + if let Some(mgr) = guard.as_ref() { + for domain_name in all_sites_with_lets_encrypt_enabled { - match mgr.get_or_create_cert(&domain_name).await.context(format!("generating lets-encrypt cert for site {}",domain_name)) { - Ok(v) => { - state.cert_resolver.add_lets_encrypt_signed_cert_to_mem_cache(&domain_name, v); - generated_count += 1; + if let Some(_) = state.cert_resolver.get_lets_encrypt_signed_cert_from_mem_cache(&domain_name) { + tracing::info!("LE CERT IS OK FOR: {}", domain_name); + continue; } - Err(e) => { - tracing::error!("Failed to generate certificate for domain: {}. {e:?}", domain_name); - } - } - + match mgr.get_or_create_cert(&domain_name).await.context(format!("generating lets-encrypt cert for site {}",domain_name)) { + Ok(v) => { + state.cert_resolver.add_lets_encrypt_signed_cert_to_mem_cache(&domain_name, v); + generated_count += 1; + } + Err(e) => { + tracing::error!("Failed to generate certificate for domain: {}. {e:?}", domain_name); + } + } + + + } + } else { + tracing::error!("LE Manager not available.. will retry in 10 seconds."); } - } else { - tracing::error!("LE Manager not available.. will retry in 10 seconds."); - } + - - crate::BG_WORKER_THREAD_MAP.insert("Lets Encrypt".into(), BgTaskInfo { - liveness_ptr: Arc::downgrade(&liveness_token), - status: format!("Generated: {generated_count} - Pending: {active_challenges_count}.") - }); // we dont need to clean this up if we exit, there is a cleanup task that will do it. - - - tokio::time::sleep(Duration::from_secs(320)).await; + crate::BG_WORKER_THREAD_MAP.insert("Lets Encrypt".into(), BgTaskInfo { + liveness_ptr: Arc::downgrade(&liveness_token), + status: format!("Generated: {generated_count} - Pending: {active_challenges_count}.") + }); // we dont need to clean this up if we exit, there is a cleanup task that will do it. + + drop(guard); + } + tokio::time::sleep(Duration::from_secs(315)).await; } } \ No newline at end of file diff --git a/src/main.rs b/src/main.rs index 281c68c..c406244 100644 --- a/src/main.rs +++ b/src/main.rs @@ -675,18 +675,13 @@ async fn main() -> anyhow::Result<()> { match self_update::current_is_latest().await { Err(e) => { tracing::warn!("It was not possible to retrieve information regarding the latest available version of odd-box: {e:?}"); - std::thread::sleep(Duration::from_secs(5)); }, Ok(Some(v)) => { tracing::info!("There is a newer version of odd-box available - please consider upgrading to {v:?}. For unmanaged installations you can run 'odd-box --update' otherwise see your package manager for upgrade instructions."); - std::thread::sleep(Duration::from_secs(3)); }, Ok(None) => { tracing::info!("You are running the latest version of odd-box :D"); - std::thread::sleep(Duration::from_secs(1)); } - - }