From 9e89c25e1068351df0c24918bc4191d0f6dcae88 Mon Sep 17 00:00:00 2001 From: Emmanuel Bosquet Date: Wed, 12 Jul 2023 12:50:28 +0200 Subject: [PATCH] draft: add thiserror to ConfigState::dispatch --- Cargo.lock | 1 + command/Cargo.toml | 3 +- command/src/state.rs | 281 ++++++++++++++++++++++++++----------------- 3 files changed, 173 insertions(+), 112 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cac2f2bd5..b839717cd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1885,6 +1885,7 @@ dependencies = [ "serde", "serde_json", "sha2", + "thiserror", "time", "toml", "trailer", diff --git a/command/Cargo.toml b/command/Cargo.toml index 7bfdf53d0..a2b7d8bde 100644 --- a/command/Cargo.toml +++ b/command/Cargo.toml @@ -27,7 +27,7 @@ include = [ ] [dependencies] -anyhow = "^1.0.71" +anyhow = "^1.0.71" # this has to go, ultimately hex = "^0.4.3" libc = "^0.2.147" log = "^0.4.19" @@ -46,6 +46,7 @@ sha2 = "^0.10.7" trailer = "^0.1.2" pool = "^0.1.4" poule = "^0.3.2" +thiserror = "^1.0.43" [features] unstable = [] diff --git a/command/src/state.rs b/command/src/state.rs index 25ad2cfe9..4989d5d22 100644 --- a/command/src/state.rs +++ b/command/src/state.rs @@ -29,6 +29,42 @@ use crate::{ /// To use throughout Sōzu pub type ClusterId = String; +#[derive(thiserror::Error, Debug)] +pub enum StateError { + #[error("Request came in empty")] + EmptyRequest, + #[error("blabla: {0}")] + RedundantRequest(String), + #[error("State can not handle this request")] + UnusableRequest, + #[error("Did not find cluster {0}")] + ClusterNotFound(String), + #[error("Listener exists already on address {0}")] + ListenerExistsAlready(String), + #[error("Wrong request: {0}")] + WrongRequest(String), + #[error("No listener to remove at address {0}")] + NoListenerToRemove(String), + #[error("No listener to activate at address {0}")] + NoListenerToActivate(String), + #[error("No listener to deactivate at address {0}")] + NoListenerToDeactivate(String), + #[error("Can not add the frontend, it is already there: {0}")] + FrontendAlreadyThere(String), + #[error("Can not remove this frontend, it is not found: {0}")] + NoFrontendToRemove(String), + #[error("Could not add certificate: {0}")] + AddCertificateError(String), + #[error("Could not remove certificate: {0}")] + RemoveCertificateError(String), + #[error("Could not replace certificate: {0}")] + ReplaceCertificateError(String), + #[error("The provided socket address '{address}' is wrong: {error}")] + WrongSocketAddress { address: String, error: String }, + #[error("Certificate not found")] + CertificateNotFound, +} + /// The `ConfigState` represents the state of Sōzu's business, which is to forward traffic /// from frontends to backends. Hence, it contains all details about: /// @@ -63,21 +99,17 @@ impl ConfigState { Self::default() } - pub fn dispatch(&mut self, request: &Request) -> anyhow::Result<()> { + pub fn dispatch(&mut self, request: &Request) -> Result<(), StateError> { let request_type = match &request.request_type { Some(t) => t, - None => bail!("Empty request!"), + None => return Err(StateError::EmptyRequest), }; self.increment_request_count(request); match request_type { - RequestType::AddCluster(cluster) => self - .add_cluster(cluster) - .with_context(|| "Could not add cluster"), - RequestType::RemoveCluster(cluster_id) => self - .remove_cluster(cluster_id) - .with_context(|| "Could not remove cluster"), + RequestType::AddCluster(cluster) => self.add_cluster(cluster), + RequestType::RemoveCluster(cluster_id) => self.remove_cluster(cluster_id), RequestType::AddHttpListener(listener) => self .add_http_listener(listener) .with_context(|| "Could not add HTTP listener"), @@ -143,9 +175,7 @@ impl ConfigState { | &RequestType::ReturnListenSockets(_) | &RequestType::HardStop(_) => Ok(()), - other_request => { - bail!("state cannot handle request message: {:#?}", other_request); - } + other_request => return Err(StateError::UnusableRequest), } } @@ -166,80 +196,79 @@ impl ConfigState { } } - fn add_cluster(&mut self, cluster: &Cluster) -> anyhow::Result<()> { + fn add_cluster(&mut self, cluster: &Cluster) -> Result<(), StateError> { let cluster = cluster.clone(); self.clusters.insert(cluster.cluster_id.clone(), cluster); Ok(()) } - fn remove_cluster(&mut self, cluster_id: &str) -> anyhow::Result<()> { + fn remove_cluster(&mut self, cluster_id: &str) -> Result<(), StateError> { match self.clusters.remove(cluster_id) { Some(_) => Ok(()), - None => bail!("No cluster found with this id"), + None => Err(StateError::ClusterNotFound(cluster_id.to_owned())), } } - fn add_http_listener(&mut self, listener: &HttpListenerConfig) -> anyhow::Result<()> { - match self.http_listeners.entry(listener.address.to_string()) { + fn add_http_listener(&mut self, listener: &HttpListenerConfig) -> Result<(), StateError> { + let address = listener.address.to_string(); + match self.http_listeners.entry(address) { BTreeMapEntry::Vacant(vacant_entry) => vacant_entry.insert(listener.clone()), - BTreeMapEntry::Occupied(_) => { - bail!("The entry is occupied for address {}", listener.address) - } + BTreeMapEntry::Occupied(_) => return Err(StateError::ListenerExistsAlready(address)), }; Ok(()) } - fn add_https_listener(&mut self, listener: &HttpsListenerConfig) -> anyhow::Result<()> { - match self.https_listeners.entry(listener.address.clone()) { + fn add_https_listener(&mut self, listener: &HttpsListenerConfig) -> Result<(), StateError> { + let address = listener.address.to_string(); + match self.https_listeners.entry(address) { BTreeMapEntry::Vacant(vacant_entry) => vacant_entry.insert(listener.clone()), - BTreeMapEntry::Occupied(_) => { - bail!("The entry is occupied for address {}", listener.address) - } + BTreeMapEntry::Occupied(_) => return Err(StateError::ListenerExistsAlready(address)), }; Ok(()) } - fn add_tcp_listener(&mut self, listener: &TcpListenerConfig) -> anyhow::Result<()> { - match self.tcp_listeners.entry(listener.address.clone()) { + fn add_tcp_listener(&mut self, listener: &TcpListenerConfig) -> Result<(), StateError> { + let address = listener.address.to_string(); + match self.tcp_listeners.entry(address) { BTreeMapEntry::Vacant(vacant_entry) => vacant_entry.insert(listener.clone()), - BTreeMapEntry::Occupied(_) => { - bail!("The entry is occupied for address {}", listener.address) - } + BTreeMapEntry::Occupied(_) => return Err(StateError::ListenerExistsAlready(address)), }; Ok(()) } - fn remove_listener(&mut self, remove: &RemoveListener) -> anyhow::Result<()> { + fn remove_listener(&mut self, remove: &RemoveListener) -> Result<(), StateError> { match ListenerType::from_i32(remove.proxy) { Some(ListenerType::Http) => self.remove_http_listener(&remove.address), Some(ListenerType::Https) => self.remove_https_listener(&remove.address), Some(ListenerType::Tcp) => self.remove_tcp_listener(&remove.address), - None => bail!("Wrong ListenerType on RemoveListener request"), + None => Err(StateError::WrongRequest( + "Wrong ListenerType on RemoveListener request".to_string(), + )), } } - fn remove_http_listener(&mut self, address: &str) -> anyhow::Result<()> { + fn remove_http_listener(&mut self, address: &str) -> Result<(), StateError> { if self.http_listeners.remove(address).is_none() { - bail!("No http listener to remove at address {}", address); + return Err(StateError::NoListenerToRemove(address.to_owned())); } Ok(()) } - fn remove_https_listener(&mut self, address: &str) -> anyhow::Result<()> { + fn remove_https_listener(&mut self, address: &str) -> Result<(), StateError> { if self.https_listeners.remove(address).is_none() { - bail!("No https listener to remove at address {}", address); + return Err(StateError::NoListenerToRemove(address.to_owned())); } Ok(()) } - fn remove_tcp_listener(&mut self, address: &str) -> anyhow::Result<()> { + fn remove_tcp_listener(&mut self, address: &str) -> Result<(), StateError> { if self.tcp_listeners.remove(address).is_none() { - bail!("No tcp listener to remove at address {}", address); + return Err(StateError::NoListenerToRemove(address.to_owned())); } Ok(()) } - fn activate_listener(&mut self, activate: &ActivateListener) -> anyhow::Result<()> { + fn activate_listener(&mut self, activate: &ActivateListener) -> Result<(), StateError> { match ListenerType::from_i32(activate.proxy) { Some(ListenerType::Http) => { if self @@ -248,7 +277,9 @@ impl ConfigState { .map(|listener| listener.active = true) .is_none() { - bail!("No http listener found with address {}", activate.address) + return Err(StateError::NoListenerToActivate( + activate.address.to_owned(), + )); } } Some(ListenerType::Https) => { @@ -258,7 +289,9 @@ impl ConfigState { .map(|listener| listener.active = true) .is_none() { - bail!("No https listener found with address {}", activate.address) + return Err(StateError::NoListenerToActivate( + activate.address.to_owned(), + )); } } Some(ListenerType::Tcp) => { @@ -268,15 +301,21 @@ impl ConfigState { .map(|listener| listener.active = true) .is_none() { - bail!("No tcp listener found with address {}", activate.address) + return Err(StateError::NoListenerToActivate( + activate.address.to_owned(), + )); } } - None => bail!("Wrong variant for ListenerType on request"), + None => { + return Err(StateError::WrongRequest( + "Wrong variant for ListenerType on request".to_string(), + )) + } } Ok(()) } - fn deactivate_listener(&mut self, deactivate: &DeactivateListener) -> anyhow::Result<()> { + fn deactivate_listener(&mut self, deactivate: &DeactivateListener) -> Result<(), StateError> { match ListenerType::from_i32(deactivate.proxy) { Some(ListenerType::Http) => { if self @@ -285,7 +324,9 @@ impl ConfigState { .map(|listener| listener.active = false) .is_none() { - bail!("No http listener found with address {}", deactivate.address) + return Err(StateError::NoListenerToDeactivate( + deactivate.address.to_owned(), + )); } } Some(ListenerType::Https) => { @@ -295,10 +336,9 @@ impl ConfigState { .map(|listener| listener.active = false) .is_none() { - bail!( - "No https listener found with address {}", - deactivate.address - ) + return Err(StateError::NoListenerToDeactivate( + deactivate.address.to_owned(), + )); } } Some(ListenerType::Tcp) => { @@ -308,68 +348,67 @@ impl ConfigState { .map(|listener| listener.active = false) .is_none() { - bail!("No tcp listener found with address {}", deactivate.address) + return Err(StateError::NoListenerToDeactivate( + deactivate.address.to_owned(), + )); } } - None => bail!("Wrong variant for ListenerType on request"), + None => { + return Err(StateError::WrongRequest( + "Wrong variant for ListenerType on request".to_string(), + )) + } } Ok(()) } - fn add_http_frontend(&mut self, front: &RequestHttpFrontend) -> anyhow::Result<()> { + fn add_http_frontend(&mut self, front: &RequestHttpFrontend) -> Result<(), StateError> { match self.http_fronts.entry(front.to_string()) { BTreeMapEntry::Vacant(e) => e.insert(front.clone().to_frontend()?), - BTreeMapEntry::Occupied(_) => bail!("This frontend is already present: {:?}", front), + BTreeMapEntry::Occupied(_) => { + return Err(StateError::FrontendAlreadyThere(front.to_string())) + } }; Ok(()) } - fn add_https_frontend(&mut self, front: &RequestHttpFrontend) -> anyhow::Result<()> { + fn add_https_frontend(&mut self, front: &RequestHttpFrontend) -> Result<(), StateError> { match self.https_fronts.entry(front.to_string()) { BTreeMapEntry::Vacant(e) => e.insert(front.clone().to_frontend()?), - BTreeMapEntry::Occupied(_) => bail!("This frontend is already present: {:?}", front), + BTreeMapEntry::Occupied(_) => { + return Err(StateError::FrontendAlreadyThere(front.to_string())) + } }; Ok(()) } - fn remove_http_frontend(&mut self, front: &RequestHttpFrontend) -> anyhow::Result<()> { + fn remove_http_frontend(&mut self, front: &RequestHttpFrontend) -> Result<(), StateError> { if self.http_fronts.remove(&front.to_string()).is_none() { - let error_msg = match &front.cluster_id { - Some(cluster_id) => format!( - "No such frontend at {} for the cluster {}", - front.address, cluster_id - ), - None => format!("No such frontend at {}", front.address), - }; - bail!(error_msg); + return Err(StateError::NoFrontendToRemove(front.to_string())); } Ok(()) } - fn remove_https_frontend(&mut self, front: &RequestHttpFrontend) -> anyhow::Result<()> { + fn remove_https_frontend(&mut self, front: &RequestHttpFrontend) -> Result<(), StateError> { if self.https_fronts.remove(&front.to_string()).is_none() { - let error_msg = match &front.cluster_id { - Some(cluster_id) => format!( - "No such frontend at {} for the cluster {}", - front.address, cluster_id - ), - None => format!("No such frontend at {}", front.address), - }; - bail!(error_msg); + return Err(StateError::NoFrontendToRemove(front.to_string())); } Ok(()) } - fn add_certificate(&mut self, add: &AddCertificate) -> anyhow::Result<()> { + fn add_certificate(&mut self, add: &AddCertificate) -> Result<(), StateError> { let fingerprint = Fingerprint( - calculate_fingerprint(add.certificate.certificate.as_bytes()) - .with_context(|| "cannot calculate the certificate's fingerprint")?, + calculate_fingerprint(add.certificate.certificate.as_bytes()).map_err( + |fingerprint_err| StateError::AddCertificateError(format!("{:#}", fingerprint_err)), + )?, ); - let address = add - .address - .parse() - .with_context(|| "Could not parse socket address")?; + let address = add.address.parse::().map_err(|parse_error| { + StateError::WrongSocketAddress { + address: add.address, + error: parse_error.to_string(), + } + })?; let entry = self .certificates @@ -388,16 +427,19 @@ impl ConfigState { Ok(()) } - fn remove_certificate(&mut self, remove: &RemoveCertificate) -> anyhow::Result<()> { - let fingerprint = Fingerprint( - hex::decode(&remove.fingerprint) - .with_context(|| "Failed at decoding the string (expected hexadecimal data)")?, - ); + fn remove_certificate(&mut self, remove: &RemoveCertificate) -> Result<(), StateError> { + let fingerprint = + Fingerprint(hex::decode(&remove.fingerprint).map_err(|decode_error| { + StateError::RemoveCertificateError(decode_error.to_string()) + })?); let address = remove .address - .parse() - .with_context(|| "Could not parse socket address")?; + .parse::() + .map_err(|parse_error| StateError::WrongSocketAddress { + address: remove.address, + error: parse_error.to_string(), + })?; if let Some(index) = self.certificates.get_mut(&address) { index.remove(&fingerprint); @@ -410,25 +452,30 @@ impl ConfigState { /// - calculate the new fingerprint /// - insert the new certificate with the new fingerprint as key /// - check that the new entry is present in the certificates hashmap - fn replace_certificate(&mut self, replace: &ReplaceCertificate) -> anyhow::Result<()> { + fn replace_certificate(&mut self, replace: &ReplaceCertificate) -> Result<(), StateError> { let address = replace .address - .parse() - .with_context(|| "Could not parse socket address")?; + .parse::() + .map_err(|parse_error| StateError::WrongSocketAddress { + address: replace.address, + error: parse_error.to_string(), + })?; - let old_fingerprint = Fingerprint( - hex::decode(&replace.old_fingerprint) - .with_context(|| "Failed at decoding the string (expected hexadecimal data)")?, - ); + let old_fingerprint = Fingerprint(hex::decode(&replace.old_fingerprint).map_err( + |decode_error| StateError::RemoveCertificateError(decode_error.to_string()), + )?); self.certificates .get_mut(&address) - .with_context(|| format!("No certificate to replace for address {}", replace.address))? + .ok_or(StateError::CertificateNotFound)? .remove(&old_fingerprint); let new_fingerprint = Fingerprint( - calculate_fingerprint(replace.new_certificate.certificate.as_bytes()) - .with_context(|| "cannot obtain the certificate's fingerprint")?, + calculate_fingerprint(replace.new_certificate.certificate.as_bytes()).map_err( + |fingerprint_err| { + StateError::ReplaceCertificateError(format!("{:#}", fingerprint_err)) + }, + )?, ); self.certificates @@ -438,20 +485,21 @@ impl ConfigState { if !self .certificates .get(&address) - .with_context(|| { + .ok_or(StateError::ReplaceCertificateError( "Unlikely error. This entry in the certificate hashmap should be present" - })? + .to_string(), + ))? .contains_key(&new_fingerprint) { - bail!(format!( + return Err(StateError::ReplaceCertificateError(format!( "Failed to insert the new certificate for address {}", replace.address - )) + ))); } Ok(()) } - fn add_tcp_frontend(&mut self, front: &RequestTcpFrontend) -> anyhow::Result<()> { + fn add_tcp_frontend(&mut self, front: &RequestTcpFrontend) -> Result<(), StateError> { let tcp_frontends = self .tcp_fronts .entry(front.cluster_id.clone()) @@ -459,25 +507,36 @@ impl ConfigState { let tcp_frontend = TcpFrontend { cluster_id: front.cluster_id.clone(), - address: front - .address - .parse() - .with_context(|| "wrong socket address")?, + address: front.address.parse::().map_err(|parse_error| { + StateError::WrongSocketAddress { + address: front.address, + error: parse_error.to_string(), + } + })?, tags: front.tags.clone(), }; if tcp_frontends.contains(&tcp_frontend) { - bail!("This tcp frontend is already present: {:?}", tcp_frontend); + return Err(StateError::FrontendAlreadyThere(format!( + "This tcp frontend is already present: {:?}", + tcp_frontend + ))); } tcp_frontends.push(tcp_frontend); Ok(()) } - fn remove_tcp_frontend(&mut self, front_to_remove: &RequestTcpFrontend) -> anyhow::Result<()> { + fn remove_tcp_frontend( + &mut self, + front_to_remove: &RequestTcpFrontend, + ) -> Result<(), StateError> { let address = front_to_remove .address - .parse() - .with_context(|| "wrong socket address")?; + .parse::() + .map_err(|parse_error| StateError::WrongSocketAddress { + address: front_to_remove.address, + error: parse_error.to_string(), + })?; let tcp_frontends = self .tcp_fronts @@ -497,7 +556,7 @@ impl ConfigState { Ok(()) } - fn add_backend(&mut self, add_backend: &AddBackend) -> anyhow::Result<()> { + fn add_backend(&mut self, add_backend: &AddBackend) -> Result<(), StateError> { let backend = Backend { address: add_backend .address @@ -522,7 +581,7 @@ impl ConfigState { Ok(()) } - fn remove_backend(&mut self, backend: &RemoveBackend) -> anyhow::Result<()> { + fn remove_backend(&mut self, backend: &RemoveBackend) -> Result<(), StateError> { let backend_list = self .backends .get_mut(&backend.cluster_id)