refactor: adjust new custom error

TODO.md

@@ -1,12 +1,12 @@
 # TODO
 
-- [ ] custom error for pingora error
 - [ ] authentication for admin page
 - [ ] support etcd or other storage for config
 - [ ] better error handler
 - [ ] log rotate
 - [ ] tls cert auto update
 - [ ] support validate config before save(web)
+- [x] custom error for pingora error
 - [x] support alpn for location
 - [x] support add header for location
 - [x] support x-forwarded-for

src/http_extra/http_response.rs

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use crate::utils;
+
 use super::{
     HttpHeader, HTTP_HEADER_CONTENT_JSON, HTTP_HEADER_NO_CACHE, HTTP_HEADER_NO_STORE,
     HTTP_HEADER_TRANSFER_CHUNKED,
@@ -119,7 +121,7 @@ impl HttpResponse {
     {
         let buf = serde_json::to_vec(value).map_err(|e| {
             error!("To json fail: {e}");
-            pingora::Error::new_str("To json fail")
+            utils::new_internal_error(400, e.to_string())
         })?;
         Ok(HttpResponse {
             status: StatusCode::OK,
@@ -220,7 +222,7 @@ where
         loop {
             let size = self.reader.read(&mut buffer).await.map_err(|e| {
                 error!("Read data fail: {e}");
-                pingora::Error::new_str("Read data fail")
+                utils::new_internal_error(400, e.to_string())
             })?;
             if size == 0 {
                 break;

src/proxy/location.rs

@@ -16,6 +16,7 @@ use super::{Limiter, Upstream};
 use crate::config::LocationConf;
 use crate::http_extra::{convert_headers, HttpHeader};
 use crate::state::State;
+use crate::utils;
 use http::header::HeaderValue;
 use log::error;
 use once_cell::sync::Lazy;
@@ -82,7 +83,6 @@ fn new_path_selector(path: &str) -> Result<PathSelector> {
 }
 
 pub struct Location {
-    // name: String,
     path: String,
     path_selector: PathSelector,
     hosts: Vec<String>,
@@ -95,6 +95,7 @@ pub struct Location {
     limiter: Option<Limiter>,
     pub support_compression: bool,
     pub upstream: Arc<Upstream>,
+    pub upstream_name: String,
 }
 
 fn format_headers(values: &Option<Vec<String>>) -> Result<Option<Vec<HttpHeader>>> {
@@ -154,7 +155,7 @@ impl Location {
             None
         };
         Ok(Location {
-            // name: conf.name.clone(),
+            upstream_name: conf.upstream.clone(),
             path_selector: new_path_selector(&path)?,
             path,
             hosts,
@@ -253,10 +254,9 @@ impl Location {
     /// Validate the request count less than max limit, and set the guard to ctx.
     pub fn validate_limit(&self, session: &Session, ctx: &mut State) -> pingora::Result<()> {
         if let Some(limiter) = &self.limiter {
-            return limiter.incr(session, ctx).map_err(|_e| {
-                let e = pingora::Error::new(pingora::ErrorType::InternalError);
-                pingora::Error::because(pingora::ErrorType::HTTPStatus(429), "Limit eceed", e)
-            });
+            return limiter
+                .incr(session, ctx)
+                .map_err(|e| utils::new_internal_error(429, e.to_string()));
         }
         Ok(())
     }
@@ -468,4 +468,52 @@ mod tests {
             format!("{resp_header:?}")
         );
     }
+
+    #[test]
+    fn test_modify_accept_encoding() {
+        let upstream_name = "charts";
+        let upstream = Arc::new(
+            Upstream::new(
+                upstream_name,
+                &UpstreamConf {
+                    addrs: vec!["127.0.0.1".to_string()],
+                    ..Default::default()
+                },
+            )
+            .unwrap(),
+        );
+        let lo = Location::new(
+            "",
+            &LocationConf {
+                upstream: upstream_name.to_string(),
+                gzip_level: Some(1),
+                br_level: Some(2),
+                zstd_level: Some(3),
+                ..Default::default()
+            },
+            vec![upstream.clone()],
+        )
+        .unwrap();
+        let mut req_header = RequestHeader::build_no_case(Method::GET, b"", None).unwrap();
+        let result = lo.modify_accept_encoding(&mut req_header);
+        assert_eq!(true, result.is_none());
+
+        let key = "Accept-Encoding";
+        req_header
+            .insert_header(key, "gzip, deflate, br, zstd")
+            .unwrap();
+        let result = lo.modify_accept_encoding(&mut req_header);
+        assert_eq!("zstd", req_header.headers.get(key).unwrap());
+        assert_eq!(3, result.unwrap());
+
+        req_header.insert_header(key, "gzip, deflate, br").unwrap();
+        let result = lo.modify_accept_encoding(&mut req_header);
+        assert_eq!("br", req_header.headers.get(key).unwrap());
+        assert_eq!(2, result.unwrap());
+
+        req_header.insert_header(key, "gzip, deflate").unwrap();
+        let result = lo.modify_accept_encoding(&mut req_header);
+        assert_eq!("gzip", req_header.headers.get(key).unwrap());
+        assert_eq!(1, result.unwrap());
+    }
 }

src/proxy/server.rs

@@ -396,7 +396,12 @@ impl ProxyHttp for Server {
             .iter()
             .enumerate()
             .find(|(_, item)| item.matched(host, path))
-            .ok_or_else(|| pingora::Error::new_str("Location not found"))?;
+            .ok_or_else(|| {
+                utils::new_internal_error(
+                    500,
+                    format!("Location not found, host:{host} path:{path}"),
+                )
+            })?;
         if let Some(mut new_path) = lo.rewrite(path) {
             if let Some(query) = header.uri.query() {
                 new_path = format!("{new_path}?{query}");
@@ -432,10 +437,10 @@ impl ProxyHttp for Server {
     ) -> pingora::Result<Box<HttpPeer>> {
         let lo = &self.locations[ctx.location_index.unwrap_or_default()];
 
-        let peer = lo
-            .upstream
-            .new_http_peer(ctx, session)
-            .ok_or(pingora::Error::new_str("No available upstream"))?;
+        // pingora::Error::new_str("No available upstream")
+        let peer = lo.upstream.new_http_peer(ctx, session).ok_or_else(|| {
+            utils::new_internal_error(503, format!("No available upstream({})", lo.upstream_name))
+        })?;
 
         Ok(Box::new(peer))
     }

src/serve/admin.rs

@@ -19,7 +19,7 @@ use crate::config::{PingapConf, CATEGORY_LOCATION, CATEGORY_SERVER, CATEGORY_UPS
 use crate::http_extra::HttpResponse;
 use crate::state::State;
 use crate::state::{get_start_time, restart};
-use crate::utils::get_pkg_version;
+use crate::utils::{self, get_pkg_version};
 use async_trait::async_trait;
 use bytes::{BufMut, BytesMut};
 use bytesize::ByteSize;
@@ -80,11 +80,11 @@ impl AdminServe {
     fn load_config(&self) -> pingora::Result<PingapConf> {
         let conf = config::load_config(&config::get_config_path(), true).map_err(|e| {
             error!("failed to load config: {e}");
-            pingora::Error::new_str("Load config fail")
+            utils::new_internal_error(400, e.to_string())
         })?;
         conf.validate().map_err(|e| {
             error!("failed to validate config: {e}");
-            pingora::Error::new_str("Validate config fail")
+            utils::new_internal_error(400, e.to_string())
         })?;
         Ok(conf)
     }
@@ -116,7 +116,7 @@ impl AdminServe {
         };
         save_config(&config::get_config_path(), &mut conf, category).map_err(|e| {
             error!("failed to save config: {e}");
-            pingora::Error::new_str("Save config fail")
+            utils::new_internal_error(400, e.to_string())
         })?;
         Ok(HttpResponse::no_content())
     }
@@ -136,28 +136,28 @@ impl AdminServe {
             CATEGORY_UPSTREAM => {
                 let upstream: UpstreamConf = serde_json::from_slice(&buf).map_err(|e| {
                     error!("failed to deserialize upstream: {e}");
-                    pingora::Error::new_str("Upstream config invalid")
+                    utils::new_internal_error(400, e.to_string())
                 })?;
                 conf.upstreams.insert(key, upstream);
             }
             CATEGORY_LOCATION => {
                 let location: LocationConf = serde_json::from_slice(&buf).map_err(|e| {
                     error!("failed to deserialize location: {e}");
-                    pingora::Error::new_str("Location config invalid")
+                    utils::new_internal_error(400, e.to_string())
                 })?;
                 conf.locations.insert(key, location);
             }
             CATEGORY_SERVER => {
                 let server: ServerConf = serde_json::from_slice(&buf).map_err(|e| {
                     error!("failed to deserialize server: {e}");
-                    pingora::Error::new_str("Server config invalid")
+                    utils::new_internal_error(400, e.to_string())
                 })?;
                 conf.servers.insert(key, server);
             }
             _ => {
                 let basic_conf: BasicConfParams = serde_json::from_slice(&buf).map_err(|e| {
                     error!("failed to basic info: {e}");
-                    pingora::Error::new_str("Basic config invalid")
+                    utils::new_internal_error(400, e.to_string())
                 })?;
                 conf.error_template = basic_conf.error_template.unwrap_or_default();
                 conf.pid_file = basic_conf.pid_file;
@@ -177,7 +177,7 @@ impl AdminServe {
         };
         save_config(&config::get_config_path(), &mut conf, category).map_err(|e| {
             error!("failed to save config: {e}");
-            pingora::Error::new_str("Save config fail")
+            utils::new_internal_error(400, e.to_string())
         })?;
         Ok(HttpResponse::no_content())
     }
@@ -252,7 +252,7 @@ impl Serve for AdminServe {
         } else if path == "/restart" {
             if let Err(e) = restart() {
                 error!("Restart fail: {e}");
-                return Err(pingora::Error::new_str("restart fail"));
+                return Err(utils::new_internal_error(400, e.to_string()));
             }
             HttpResponse::no_content()
         } else {

src/serve/directory.rs

@@ -151,7 +151,7 @@ impl Directory {
 
                     let _ = f.read(&mut buffer).await.map_err(|e| {
                         error!("Read data fail: {e}");
-                        pingora::Error::new_str("Read data fail")
+                        utils::new_internal_error(400, e.to_string())
                     })?;
                     HttpResponse {
                         status: StatusCode::OK,

src/utils/mod.rs

@@ -118,3 +118,11 @@ pub fn get_query_value<'a>(req_header: &'a RequestHeader, name: &str) -> Option<
     }
     None
 }
+
+pub fn new_internal_error(status: u16, message: String) -> pingora::BError {
+    pingora::Error::because(
+        pingora::ErrorType::HTTPStatus(status),
+        message,
+        pingora::Error::new(pingora::ErrorType::InternalError),
+    )
+}