Improve internal server errors significantly

src/domain.rs

@@ -3,6 +3,7 @@ pub mod checker;
 pub mod config;
 pub mod manager;
 
+use std::fmt;
 use std::str::FromStr;
 
 use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
@@ -21,6 +22,12 @@ impl Name {
     }
 }
 
+impl fmt::Display for Name {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{}", self.utf8_str)
+    }
+}
+
 impl FromStr for Name {
     type Err = <trust_dns_rr::Name as FromStr>::Err;
 

src/domain/acme/manager.rs

@@ -14,7 +14,7 @@ pub trait Manager {
     fn sync_domain<'mgr>(
         &'mgr self,
         domain: domain::Name,
-    ) -> util::BoxedFuture<'mgr, Result<(), unexpected::Error>>;
+    ) -> util::BoxFuture<'mgr, Result<(), unexpected::Error>>;
 
     fn get_http01_challenge_key(&self, token: &str) -> Result<String, GetHttp01ChallengeKeyError>;
 
@@ -87,7 +87,7 @@ impl Manager for ManagerImpl {
     fn sync_domain<'mgr>(
         &'mgr self,
         domain: domain::Name,
-    ) -> util::BoxedFuture<'mgr, Result<(), unexpected::Error>> {
+    ) -> util::BoxFuture<'mgr, Result<(), unexpected::Error>> {
         Box::pin(async move {
             // if there's an existing cert, and its expiry (determined by the soonest value of
             // not_after amongst its parts) is later than 30 days from now, then we consider it to be

src/domain/manager.rs

@@ -3,8 +3,7 @@ use crate::error::unexpected::{self, Mappable};
 use crate::origin;
 use crate::util;
 
-use futures::stream;
-use std::{io, sync};
+use std::sync;
 use tokio_util::sync::CancellationToken;
 
 #[derive(thiserror::Error, Debug)]
@@ -58,8 +57,6 @@ impl From<origin::GetFileError> for GetFileError {
     }
 }
 
-pub type ReadFileIntoError = GetFileError;
-
 #[derive(thiserror::Error, Debug)]
 pub enum SyncError {
     #[error("not found")]
@@ -139,29 +136,22 @@ pub type GetAcmeHttp01ChallengeKeyError = acme::manager::GetHttp01ChallengeKeyEr
 pub trait Manager: Sync + Send + rustls::server::ResolvesServerCert {
     fn get_config(&self, domain: &domain::Name) -> Result<config::Config, GetConfigError>;
 
-    fn read_file_into(
-        &self,
-        domain: &domain::Name,
-        path: &str,
-        into: &mut dyn std::io::Write,
-    ) -> Result<(), ReadFileIntoError>;
-
     fn get_file<'store>(
         &'store self,
         domain: &domain::Name,
         path: &str,
-    ) -> Result<stream::BoxStream<'static, io::Result<Vec<u8>>>, GetFileError>;
+    ) -> Result<util::BoxByteStream, GetFileError>;
 
     fn sync_cert<'mgr>(
         &'mgr self,
         domain: domain::Name,
-    ) -> util::BoxedFuture<'mgr, Result<(), unexpected::Error>>;
+    ) -> util::BoxFuture<'mgr, Result<(), unexpected::Error>>;
 
     fn sync_with_config<'mgr>(
         &'mgr self,
         domain: domain::Name,
         config: config::Config,
-    ) -> util::BoxedFuture<'mgr, Result<(), SyncWithConfigError>>;
+    ) -> util::BoxFuture<'mgr, Result<(), SyncWithConfigError>>;
 
     fn get_acme_http01_challenge_key(
         &self,
@@ -238,23 +228,11 @@ impl Manager for ManagerImpl {
         Ok(self.domain_config_store.get(domain)?)
     }
 
-    fn read_file_into(
-        &self,
-        domain: &domain::Name,
-        path: &str,
-        into: &mut dyn std::io::Write,
-    ) -> Result<(), ReadFileIntoError> {
-        let config = self.domain_config_store.get(domain)?;
-        self.origin_store
-            .read_file_into(&config.origin_descr, path, into)?;
-        Ok(())
-    }
-
     fn get_file<'store>(
         &'store self,
         domain: &domain::Name,
         path: &str,
-    ) -> Result<stream::BoxStream<'static, io::Result<Vec<u8>>>, GetFileError> {
+    ) -> Result<util::BoxByteStream, GetFileError> {
         let config = self.domain_config_store.get(domain)?;
         let f = self.origin_store.get_file(&config.origin_descr, path)?;
         Ok(f)
@@ -263,7 +241,7 @@ impl Manager for ManagerImpl {
     fn sync_cert<'mgr>(
         &'mgr self,
         domain: domain::Name,
-    ) -> util::BoxedFuture<'mgr, Result<(), unexpected::Error>> {
+    ) -> util::BoxFuture<'mgr, Result<(), unexpected::Error>> {
         Box::pin(async move {
             if let Some(ref acme_manager) = self.acme_manager {
                 acme_manager.sync_domain(domain.clone()).await?;
@@ -277,7 +255,7 @@ impl Manager for ManagerImpl {
         &'mgr self,
         domain: domain::Name,
         config: config::Config,
-    ) -> util::BoxedFuture<'mgr, Result<(), SyncWithConfigError>> {
+    ) -> util::BoxFuture<'mgr, Result<(), SyncWithConfigError>> {
         Box::pin(async move {
             let config_hash = config
                 .hash()

src/origin.rs

@@ -1,6 +1,6 @@
 use crate::error::unexpected;
-use futures::stream;
-use std::{io, sync};
+use crate::util;
+use std::sync;
 
 pub mod git;
 pub mod mux;
@@ -41,8 +41,6 @@ pub enum GetFileError {
     Unexpected(#[from] unexpected::Error),
 }
 
-pub type ReadFileIntoError = GetFileError;
-
 #[mockall::automock]
 /// Describes a storage mechanism for Origins. Each Origin is uniquely identified by its Descr.
 pub trait Store {
@@ -52,18 +50,7 @@ pub trait Store {
 
     fn all_descrs(&self) -> Result<Vec<Descr>, AllDescrsError>;
 
-    fn read_file_into(
-        &self,
-        descr: &Descr,
-        path: &str,
-        into: &mut dyn std::io::Write,
-    ) -> Result<(), ReadFileIntoError>;
-
-    fn get_file(
-        &self,
-        descr: &Descr,
-        path: &str,
-    ) -> Result<stream::BoxStream<'static, io::Result<Vec<u8>>>, GetFileError>;
+    fn get_file(&self, descr: &Descr, path: &str) -> Result<util::BoxByteStream, GetFileError>;
 }
 
 pub fn new_mock() -> sync::Arc<sync::Mutex<MockStore>> {
@@ -79,21 +66,11 @@ impl Store for sync::Arc<sync::Mutex<MockStore>> {
         self.lock().unwrap().all_descrs()
     }
 
-    /// Deprecated, use get_file
-    fn read_file_into(
-        &self,
-        descr: &Descr,
-        path: &str,
-        into: &mut dyn std::io::Write,
-    ) -> Result<(), ReadFileIntoError> {
-        self.lock().unwrap().read_file_into(descr, path, into)
-    }
-
     fn get_file<'store>(
         &'store self,
         descr: &Descr,
         path: &str,
-    ) -> Result<stream::BoxStream<'static, io::Result<Vec<u8>>>, GetFileError> {
+    ) -> Result<util::BoxByteStream, GetFileError> {
         self.lock().unwrap().get_file(descr, path)
     }
 }

src/origin/git.rs

@@ -1,5 +1,5 @@
 use crate::error::unexpected::{self, Intoable, Mappable};
-use crate::origin;
+use crate::{origin, util};
 
 use std::path::{Path, PathBuf};
 use std::{collections, fs, io, sync};
@@ -290,56 +290,11 @@ impl super::Store for FSStore {
             ).try_collect()
     }
 
-    fn read_file_into(
-        &self,
-        descr: &origin::Descr,
-        path: &str,
-        into: &mut dyn std::io::Write,
-    ) -> Result<(), origin::ReadFileIntoError> {
-        let repo_snapshot = match self.get_repo_snapshot(descr) {
-            Ok(Some(repo_snapshot)) => repo_snapshot,
-            Ok(None) => return Err(origin::ReadFileIntoError::DescrNotSynced),
-            Err(e) => return Err(e.into()),
-        };
-
-        let mut clean_path = Path::new(path);
-        clean_path = clean_path.strip_prefix("/").unwrap_or(clean_path);
-
-        let repo = repo_snapshot.repo.to_thread_local();
-
-        let file_object = repo
-            .find_object(repo_snapshot.tree_object_id)
-            .map_unexpected_while(|| {
-                format!("finding tree object {}", repo_snapshot.tree_object_id)
-            })?
-            .peel_to_tree()
-            .map_unexpected_while(|| {
-                format!("peeling tree object {}", repo_snapshot.tree_object_id)
-            })?
-            .lookup_entry_by_path(clean_path)
-            .map_unexpected_while(|| {
-                format!(
-                    "looking up {} in tree object {}",
-                    clean_path.display(),
-                    repo_snapshot.tree_object_id
-                )
-            })?
-            .ok_or(origin::ReadFileIntoError::FileNotFound)?
-            .object()
-            .or_unexpected()?;
-
-        into.write_all(file_object.data.as_ref())
-            .or_unexpected_while("copying out file")?;
-
-        Ok(())
-    }
-
-    // TODO test this
     fn get_file<'store>(
         &'store self,
         descr: &origin::Descr,
         path: &str,
-    ) -> Result<stream::BoxStream<'static, io::Result<Vec<u8>>>, origin::GetFileError> {
+    ) -> Result<util::BoxByteStream, origin::GetFileError> {
         let repo_snapshot = match self.get_repo_snapshot(descr) {
             Ok(Some(repo_snapshot)) => repo_snapshot,
             Ok(None) => return Err(origin::GetFileError::DescrNotSynced),
@@ -382,10 +337,11 @@ impl super::Store for FSStore {
 #[cfg(test)]
 mod tests {
     use crate::origin::{self, Store};
+    use futures::StreamExt;
     use tempdir::TempDir;
 
-    #[test]
-    fn basic() {
+    #[tokio::test]
+    async fn basic() {
         let tmp_dir = TempDir::new("origin_store_git").unwrap();
 
         let curr_dir = format!("file://{}", std::env::current_dir().unwrap().display());
@@ -405,31 +361,29 @@ mod tests {
         store.sync(&descr).expect("sync should succeed");
         store.sync(&descr).expect("second sync should succeed");
 
-        let assert_write = |path: &str| {
-            let mut into: Vec<u8> = vec![];
-            store
-                .read_file_into(&descr, path, &mut into)
-                .expect("write should succeed");
-            assert!(into.len() > 0);
+        // RepoSnapshot doesn't exist
+        match store.get_file(&other_descr, "DNE") {
+            Err(origin::GetFileError::DescrNotSynced) => (),
+            _ => assert!(false, "descr should have not been found"),
         };
 
-        assert_write("src/lib.rs");
-        assert_write("/src/lib.rs");
+        let assert_file_dne = |path: &str| match store.get_file(&descr, path) {
+            Err(origin::GetFileError::FileNotFound) => (),
+            _ => assert!(false, "file should have not been found"),
+        };
 
-        let mut into: Vec<u8> = vec![];
+        let assert_file_not_empty = |path: &str| {
+            let f = store.get_file(&descr, path).expect("file not retrieved");
+            async move {
+                let body = f.map(|r| r.unwrap()).concat().await;
+                assert!(body.len() > 0);
+            }
+        };
 
-        // RepoSnapshot doesn't exist
-        assert!(matches!(
-            store.read_file_into(&other_descr, "DNE", &mut into),
-            Err::<_, origin::ReadFileIntoError>(origin::ReadFileIntoError::DescrNotSynced),
-        ));
-
-        // File doesn't exist
-        assert!(matches!(
-            store.read_file_into(&descr, "DNE", &mut into),
-            Err::<(), origin::ReadFileIntoError>(origin::ReadFileIntoError::FileNotFound),
-        ));
-        assert_eq!(into.len(), 0);
+        assert_file_not_empty("src/lib.rs").await;
+        assert_file_not_empty("/src/lib.rs").await;
+        assert_file_dne("DNE");
+        assert_file_dne("src/../src/lib.rs");
 
         let descrs = store.all_descrs().expect("all_descrs called");
 

src/origin/mux.rs

@@ -1,7 +1,5 @@
 use crate::error::unexpected::Mappable;
-use crate::origin;
-use futures::stream;
-use std::io;
+use crate::{origin, util};
 
 pub struct Store<F, S>
 where
@@ -43,22 +41,11 @@ where
         Ok(res)
     }
 
-    fn read_file_into(
-        &self,
-        descr: &origin::Descr,
-        path: &str,
-        into: &mut dyn std::io::Write,
-    ) -> Result<(), origin::ReadFileIntoError> {
-        (self.mapping_fn)(descr)
-            .or_unexpected_while(format!("mapping {:?} to store", &descr))?
-            .read_file_into(descr, path, into)
-    }
-
     fn get_file<'store>(
         &'store self,
         descr: &origin::Descr,
         path: &str,
-    ) -> Result<stream::BoxStream<'static, io::Result<Vec<u8>>>, origin::GetFileError> {
+    ) -> Result<util::BoxByteStream, origin::GetFileError> {
         (self.mapping_fn)(descr)
             .or_unexpected_while(format!("mapping {:?} to store", &descr))?
             .get_file(descr, path)

src/service/http.rs

@@ -4,15 +4,12 @@ mod tpl;
 use hyper::{Body, Method, Request, Response};
 use serde::{Deserialize, Serialize};
 
-use std::convert::Infallible;
 use std::str::FromStr;
 use std::{future, net, sync};
 
 use crate::error::unexpected;
 use crate::{domain, service, util};
 
-type SvcResponse = Result<Response<hyper::body::Body>, String>;
-
 pub struct Service {
     domain_manager: sync::Arc<dyn domain::manager::Manager>,
     target_a: net::Ipv4Addr,
@@ -94,7 +91,7 @@ struct DomainSyncArgs {
 }
 
 impl<'svc> Service {
-    fn serve_string(&self, status_code: u16, path: &'_ str, body: Vec<u8>) -> SvcResponse {
+    fn serve(&self, status_code: u16, path: &str, body: Body) -> Response<Body> {
         let content_type = mime_guess::from_path(path)
             .first_or_octet_stream()
             .to_string();
@@ -102,15 +99,23 @@ impl<'svc> Service {
         match Response::builder()
             .status(status_code)
             .header("Content-Type", content_type)
-            .body(body.into())
+            .body(body)
         {
-            Ok(res) => Ok(res),
-            Err(err) => Err(format!("failed to build {}: {}", path, err)),
+            Ok(res) => res,
+            Err(err) => {
+                // if the status code was already a 500, don't try to render _another_ 500, it'll
+                // probably fail for the same reason. At this point something is incredibly wrong,
+                // just panic.
+                if status_code == 500 {
+                    panic!("failed to build {}: {}", path, err);
+                }
+
+                self.internal_error(format!("failed to build {}: {}", path, err).as_str())
+            }
         }
     }
 
-    //// TODO make this use an io::Write, rather than SvcResponse
-    fn render<T>(&self, status_code: u16, name: &'_ str, value: T) -> SvcResponse
+    fn render<T>(&self, status_code: u16, name: &str, value: T) -> Response<Body>
     where
         T: Serialize,
     {
@@ -125,10 +130,10 @@ impl<'svc> Service {
             }
         };
 
-        self.serve_string(status_code, name, rendered.into())
+        self.serve(status_code, name, rendered.into())
     }
 
-    fn render_error_page(&'svc self, status_code: u16, e: &'_ str) -> SvcResponse {
+    fn render_error_page(&self, status_code: u16, e: &str) -> Response<Body> {
         #[derive(Serialize)]
         struct Response<'a> {
             error_msg: &'a str,
@@ -144,7 +149,15 @@ impl<'svc> Service {
         )
     }
 
-    fn render_page<T>(&self, name: &'_ str, data: T) -> SvcResponse
+    fn internal_error(&self, e: &str) -> Response<Body> {
+        log::error!("Internal error: {e}");
+        self.render_error_page(
+            500,
+            "An unexpected error occurred. The server administrator will be able to help.",
+        )
+    }
+
+    fn render_page<T>(&self, name: &str, data: T) -> Response<Body>
     where
         T: Serialize,
     {
@@ -158,7 +171,7 @@ impl<'svc> Service {
         )
     }
 
-    fn serve_origin(&self, domain: domain::Name, path: &'_ str) -> SvcResponse {
+    fn serve_origin(&self, domain: domain::Name, path: &str) -> Response<Body> {
         let mut path_owned;
 
         let path = match path.ends_with('/') {
@@ -170,35 +183,36 @@ impl<'svc> Service {
             false => path,
         };
 
-        let mut buf = Vec::<u8>::new();
-        match self.domain_manager.read_file_into(&domain, path, &mut buf) {
-            Ok(_) => self.serve_string(200, path, buf),
-            Err(domain::manager::ReadFileIntoError::DomainNotFound) => {
+        match self.domain_manager.get_file(&domain, path) {
+            Ok(f) => self.serve(200, path, Body::wrap_stream(f)),
+            Err(domain::manager::GetFileError::DomainNotFound) => {
                 return self.render_error_page(404, "Domain not found")
             }
-            Err(domain::manager::ReadFileIntoError::FileNotFound) => {
+            Err(domain::manager::GetFileError::FileNotFound) => {
                 self.render_error_page(404, "File not found")
             }
-            Err(domain::manager::ReadFileIntoError::Unexpected(e)) => {
-                self.render_error_page(500, format!("failed to fetch file {path}: {e}").as_str())
+            Err(domain::manager::GetFileError::Unexpected(e)) => {
+                self.internal_error(format!("failed to fetch file {path}: {e}").as_str())
             }
         }
     }
 
-    async fn with_query_req<'a, F, In, Out>(&self, req: &'a Request<Body>, f: F) -> SvcResponse
+    async fn with_query_req<'a, F, In, Out>(&self, req: &'a Request<Body>, f: F) -> Response<Body>
     where
         In: Deserialize<'a>,
         F: FnOnce(In) -> Out,
-        Out: future::Future<Output = SvcResponse>,
+        Out: future::Future<Output = Response<Body>>,
     {
         let query = req.uri().query().unwrap_or("");
         match serde_urlencoded::from_str::<In>(query) {
             Ok(args) => f(args).await,
-            Err(err) => Err(format!("failed to parse query args: {}", err)),
+            Err(err) => {
+                self.render_error_page(400, format!("failed to parse query args: {}", err).as_str())
+            }
         }
     }
 
-    fn domain_get(&self, args: DomainGetArgs) -> SvcResponse {
+    fn domain_get(&self, args: DomainGetArgs) -> Response<Body> {
         #[derive(Serialize)]
         struct Response {
             domain: domain::Name,
@@ -209,8 +223,13 @@ impl<'svc> Service {
             Ok(config) => Some(config),
             Err(domain::manager::GetConfigError::NotFound) => None,
             Err(domain::manager::GetConfigError::Unexpected(e)) => {
-                return self
-                    .render_error_page(500, format!("retrieving configuration: {}", e).as_str());
+                return self.internal_error(
+                    format!(
+                        "retrieving configuration for domain {}: {}",
+                        &args.domain, e
+                    )
+                    .as_str(),
+                );
             }
         };
 
@@ -227,7 +246,7 @@ impl<'svc> Service {
         &self,
         args: DomainInitArgs,
         domain_config: service::util::FlatConfig,
-    ) -> SvcResponse {
+    ) -> Response<Body> {
         #[derive(Serialize)]
         struct Response {
             domain: domain::Name,
@@ -247,8 +266,9 @@ impl<'svc> Service {
         let config_hash = match config.hash() {
             Ok(hash) => hash,
             Err(e) => {
-                return self
-                    .render_error_page(500, format!("failed to hash domain config: {e}").as_str())
+                return self.internal_error(
+                    format!("failed to hash domain config {config:?}: {e}").as_str(),
+                )
             }
         };
 
@@ -267,7 +287,7 @@ impl<'svc> Service {
         &self,
         args: DomainSyncArgs,
         domain_config: service::util::FlatConfig,
-    ) -> SvcResponse {
+    ) -> Response<Body> {
         if args.passphrase != self.passphrase.as_str() {
             return self.render_error_page(401, "Incorrect passphrase");
         }
@@ -309,7 +329,7 @@ impl<'svc> Service {
         self.render_page("/domain_sync.html", response)
     }
 
-    fn domains(&self) -> SvcResponse {
+    fn domains(&self) -> Response<Body> {
         #[derive(Serialize)]
         struct Response {
             domains: Vec<String>,
@@ -317,9 +337,7 @@ impl<'svc> Service {
 
         let domains = match self.domain_manager.all_domains() {
             Ok(domains) => domains,
-            Err(e) => {
-                return self.render_error_page(500, format!("failed get all domains: {e}").as_str())
-            }
+            Err(e) => return self.internal_error(format!("failed get all domains: {e}").as_str()),
         };
 
         let mut domains: Vec<String> = domains
@@ -332,7 +350,7 @@ impl<'svc> Service {
         self.render_page("/domains.html", Response { domains })
     }
 
-    async fn handle_request_inner(&self, req: Request<Body>) -> SvcResponse {
+    async fn handle_request(&self, req: Request<Body>) -> Response<Body> {
         let maybe_host = match (
             req.headers()
                 .get("Host")
@@ -355,14 +373,7 @@ impl<'svc> Service {
             let token = path.trim_start_matches("/.well-known/acme-challenge/");
 
             if let Ok(key) = self.domain_manager.get_acme_http01_challenge_key(token) {
-                let body: hyper::Body = key.into();
-                return match Response::builder().status(200).body(body) {
-                    Ok(res) => Ok(res),
-                    Err(err) => Err(format!(
-                        "failed to write acme http-01 challenge key: {}",
-                        err
-                    )),
-                };
+                return self.serve(200, "token.txt", key.into());
             }
         }
 
@@ -404,14 +415,10 @@ impl<'svc> Service {
                 .await
             }
             (&Method::GET, "/domains.html") => self.domains(),
-            _ => self.render_error_page(404, "Page not found!"),
-        }
-    }
-
-    pub async fn handle_request(&self, req: Request<Body>) -> Result<Response<Body>, Infallible> {
-        match self.handle_request_inner(req).await {
-            Ok(res) => Ok(res),
-            Err(err) => panic!("unexpected error {err}"),
+            _ => self.render_error_page(
+                404,
+                "This is not the page you're looking for. This page doesn't even exist!",
+            ),
         }
     }
 }

src/service/http/tasks.rs

@@ -18,7 +18,7 @@ pub async fn listen_http(
         // Create a `Service` for responding to the request.
         let hyper_service = hyper::service::service_fn(move |req| {
             let service = service.clone();
-            async move { service.handle_request(req).await }
+            async move { Ok::<_, convert::Infallible>(service.handle_request(req).await) }
         });
 
         // Return the service to hyper.
@@ -48,7 +48,7 @@ pub async fn listen_https(
         // Create a `Service` for responding to the request.
         let hyper_service = hyper::service::service_fn(move |req| {
             let service = service.clone();
-            async move { service.handle_request(req).await }
+            async move { Ok::<_, convert::Infallible>(service.handle_request(req).await) }
         });
 
         // Return the service to hyper.

src/util.rs

@@ -12,13 +12,15 @@ pub fn open_file(path: &path::Path) -> io::Result<Option<fs::File>> {
     }
 }
 
-pub type BoxedFuture<'a, O> = pin::Pin<Box<dyn futures::Future<Output = O> + Send + 'a>>;
+pub type BoxByteStream = futures::stream::BoxStream<'static, io::Result<Vec<u8>>>;
+
+pub type BoxFuture<'a, O> = pin::Pin<Box<dyn futures::Future<Output = O> + Send + 'a>>;
 
 pub struct TaskStack<E>
 where
     E: error::Error + Send + 'static,
 {
-    wait_group: Vec<BoxedFuture<'static, Result<(), E>>>,
+    wait_group: Vec<BoxFuture<'static, Result<(), E>>>,
 }
 
 impl<E> TaskStack<E>