fix(api): some api's logic and response status code (#92)

crates/provider/src/handlers/delete.rs

@@ -2,10 +2,14 @@ use crate::{
     consts,
     handlers::{function_get::get_function, utils::CustomError},
 };
-use actix_web::{HttpResponse, Responder, ResponseError, error, web};
+use actix_web::{HttpResponse, Responder, web};
 use serde::{Deserialize, Serialize};
 use service::containerd_manager::ContainerdManager;
 
+use super::function_list::Function;
+
+// 参考响应状态：https://github.com/openfaas/faas/blob/7803ea1861f2a22adcbcfa8c79ed539bc6506d5b/api-docs/spec.openapi.yml#L141C2-L162C45
+// 请求体反序列化失败，自动返回400错误
 pub async fn delete_handler(info: web::Json<DeleteContainerInfo>) -> impl Responder {
     let function_name = info.function_name.clone();
     let namespace = info
@@ -13,44 +17,44 @@ pub async fn delete_handler(info: web::Json<DeleteContainerInfo>) -> impl Respon
         .clone()
         .unwrap_or_else(|| consts::DEFAULT_FUNCTION_NAMESPACE.to_string());
 
-    match delete(&function_name, &namespace).await {
+    let namespaces = ContainerdManager::list_namespaces().await.unwrap();
+    if !namespaces.contains(&namespace.to_string()) {
+        return HttpResponse::NotFound().body(format!("Namespace '{}' does not exist", namespace));
+    }
+
+    let function = match get_function(&function_name, &namespace).await {
+        Ok(function) => function,
+        Err(e) => {
+            log::error!("Failed to get function: {}", e);
+            return HttpResponse::NotFound()
+                .body(format!("Function '{}' not found ", function_name));
+        }
+    };
+
+    match delete(&function, &namespace).await {
         Ok(()) => {
             HttpResponse::Ok().body(format!("Function {} deleted successfully.", function_name))
         }
-        Err(e) => e.error_response(),
+        Err(e) => {
+            HttpResponse::InternalServerError().body(format!("Failed to delete function: {}", e))
+        }
     }
 }
 
-async fn delete(function_name: &str, namespace: &str) -> Result<(), CustomError> {
+async fn delete(function: &Function, namespace: &str) -> Result<(), CustomError> {
-    let namespaces = ContainerdManager::list_namespaces().await.unwrap();
+    let function_name = function.name.clone();
-    if !namespaces.contains(&namespace.to_string()) {
-        return Err(CustomError::ActixError(error::ErrorBadRequest(format!(
-            "Namespace '{}' not valid or does not exist",
-            namespace
-        ))));
-    }
-    let function = get_function(function_name, namespace).await.map_err(|e| {
-        log::error!("Failed to get function: {}", e);
-        CustomError::ActixError(error::ErrorNotFound(format!(
-            "Function '{}' not found in namespace '{}'",
-            function_name, namespace
-        )))
-    })?;
     if function.replicas != 0 {
         log::info!("function.replicas: {:?}", function.replicas);
-        cni::delete_cni_network(namespace, function_name);
+        cni::delete_cni_network(namespace, &function_name);
         log::info!("delete_cni_network ok");
     } else {
         log::info!("function.replicas: {:?}", function.replicas);
     }
-    ContainerdManager::delete_container(function_name, namespace)
+    ContainerdManager::delete_container(&function_name, namespace)
         .await
         .map_err(|e| {
             log::error!("Failed to delete container: {}", e);
-            CustomError::ActixError(error::ErrorInternalServerError(format!(
+            CustomError::OtherError(format!("Failed to delete container: {}", e))
-                "Failed to delete container: {}",
-                e
-            )))
         })?;
     Ok(())
 }

crates/provider/src/handlers/deploy.rs

@@ -1,12 +1,10 @@
-use crate::{
+use crate::{consts, handlers::utils::CustomError, types::function_deployment::DeployFunctionInfo};
-    consts,
-    handlers::utils::CustomError,
-    types::function_deployment::{DeployFunctionInfo, FunctionDeployment},
-};
 use actix_web::{HttpResponse, Responder, web};
 
 use service::{containerd_manager::ContainerdManager, image_manager::ImageManager};
 
+// 参考响应状态 https://github.com/openfaas/faas/blob/7803ea1861f2a22adcbcfa8c79ed539bc6506d5b/api-docs/spec.openapi.yml#L121C1-L140C45
+// 请求体反序列化失败，自动返回400错误
 pub async fn deploy_handler(info: web::Json<DeployFunctionInfo>) -> impl Responder {
     let image = info.image.clone();
     let function_name = info.function_name.clone();
@@ -15,69 +13,64 @@ pub async fn deploy_handler(info: web::Json<DeployFunctionInfo>) -> impl Respond
         .clone()
         .unwrap_or(consts::DEFAULT_FUNCTION_NAMESPACE.to_string());
 
-    let config = FunctionDeployment {
+    log::info!("Namespace '{}' validated.", &namespace);
-        service: function_name,
+
-        image,
+    let container_list = match ContainerdManager::list_container_into_string(&namespace).await {
-        namespace: Some(namespace),
+        Ok(container_list) => container_list,
+        Err(e) => {
+            log::error!("Failed to list container: {}", e);
+            return HttpResponse::InternalServerError()
+                .body(format!("Failed to list container: {}", e));
+        }
     };
 
-    match deploy(&config).await {
+    if container_list.contains(&function_name) {
+        return HttpResponse::BadRequest().body(format!(
+            "Function '{}' already exists in namespace '{}'",
+            function_name, namespace
+        ));
+    }
+
+    match deploy(&function_name, &image, &namespace).await {
         Ok(()) => HttpResponse::Accepted().body(format!(
-            "Function {} deployment initiated successfully.",
+            "Function {} deployment initiated successfully .",
-            config.service
+            function_name
         )),
-        Err(e) => HttpResponse::InternalServerError().body(format!(
+        Err(e) => HttpResponse::BadRequest().body(format!(
             "failed to deploy function {}, because {}",
-            config.service, e
+            function_name, e
         )),
     }
 }
 
-async fn deploy(config: &FunctionDeployment) -> Result<(), CustomError> {
+async fn deploy(function_name: &str, image: &str, namespace: &str) -> Result<(), CustomError> {
-    let namespace = config.namespace.clone().unwrap();
+    ImageManager::prepare_image(image, namespace, true)
-
-    log::info!(
-        "Namespace '{}' validated.",
-        config.namespace.clone().unwrap()
-    );
-
-    let container_list = ContainerdManager::list_container_into_string(&namespace)
-        .await
-        .map_err(|e| CustomError::OtherError(format!("failed to list container:{}", e)))?;
-
-    if container_list.contains(&config.service) {
-        return Err(CustomError::OtherError(
-            "container has been existed".to_string(),
-        ));
-    }
-
-    ImageManager::prepare_image(&config.image, &namespace, true)
         .await
         .map_err(CustomError::from)?;
-    log::info!("Image '{}' validated ,", &config.image);
+    log::info!("Image '{}' validated ,", image);
 
-    ContainerdManager::create_container(&config.image, &config.service, &namespace)
+    ContainerdManager::create_container(image, function_name, namespace)
         .await
         .map_err(|e| CustomError::OtherError(format!("failed to create container:{}", e)))?;
 
     log::info!(
         "Container {} created using image {} in namespace {}",
-        &config.service,
+        function_name,
-        &config.image,
+        image,
         namespace
     );
 
-    ContainerdManager::new_task(&config.service, &namespace)
+    ContainerdManager::new_task(function_name, namespace)
         .await
         .map_err(|e| {
             CustomError::OtherError(format!(
                 "failed to start task for container {},{}",
-                &config.service, e
+                function_name, e
             ))
         })?;
     log::info!(
         "Task for container {} was created successfully",
-        &config.service
+        function_name
     );
 
     Ok(())

crates/provider/src/handlers/function_list.rs

@@ -23,43 +23,44 @@ pub struct Function {
     pub created_at: SystemTime,
 }
 
+// openfaas API文档和faasd源码的响应不能完全对齐，这里参考源码的响应码设置
+// 考虑到部分操作可能返回500错误，但是faasd并没有做internal server error的处理（可能上层有中间件捕获），这里应该需要做500的处理
 pub async fn function_list_handler(req: HttpRequest) -> impl Responder {
     let namespace = req.match_info().get("namespace").unwrap_or("");
     if namespace.is_empty() {
         return HttpResponse::BadRequest().body("provide namespace in path");
     }
-    match get_function_list(namespace).await {
-        Ok(functions) => HttpResponse::Ok().body(serde_json::to_string(&functions).unwrap()),
-        Err(e) => HttpResponse::from_error(e),
-    }
-}
-
-async fn get_function_list(namespace: &str) -> Result<Vec<Function>, CustomError> {
     let namespaces = match ContainerdManager::list_namespaces().await {
         Ok(namespace) => namespace,
         Err(e) => {
-            return Err(CustomError::OtherError(format!(
+            return HttpResponse::InternalServerError()
-                "Failed to list namespaces:{}",
+                .body(format!("Failed to list namespaces:{}", e));
-                e
-            )));
         }
     };
     if !namespaces.contains(&namespace.to_string()) {
-        return Err(CustomError::OtherError(format!(
+        return HttpResponse::BadRequest()
-            "Namespace '{}' not valid or does not exist",
+            .body(format!("Namespace '{}' does not exist", namespace));
-            namespace
-        )));
     }
+
     let container_list = match ContainerdManager::list_container_into_string(namespace).await {
         Ok(container_list) => container_list,
         Err(e) => {
-            return Err(CustomError::OtherError(format!(
+            return HttpResponse::InternalServerError()
-                "Failed to list container:{}",
+                .body(format!("Failed to list container:{}", e));
-                e
-            )));
         }
     };
     log::info!("container_list: {:?}", container_list);
+
+    match get_function_list(container_list, namespace).await {
+        Ok(functions) => HttpResponse::Ok().body(serde_json::to_string(&functions).unwrap()),
+        Err(e) => HttpResponse::BadRequest().body(format!("Failed to get function list: {}", e)),
+    }
+}
+
+async fn get_function_list(
+    container_list: Vec<String>,
+    namespace: &str,
+) -> Result<Vec<Function>, CustomError> {
     let mut functions: Vec<Function> = Vec::new();
     for cid in container_list {
         log::info!("cid: {}", cid);

crates/provider/src/handlers/invoke_resolver.rs

@@ -1,6 +1,6 @@
 use crate::consts::DEFAULT_FUNCTION_NAMESPACE;
 use crate::handlers::function_get::get_function;
-use actix_web::{Error, error::ErrorInternalServerError};
+use actix_web::{Error, error::ErrorInternalServerError, error::ErrorServiceUnavailable};
 use log;
 use url::Url;
 
@@ -23,7 +23,7 @@ impl InvokeResolver {
             Ok(function) => function,
             Err(e) => {
                 log::error!("Failed to get function:{}", e);
-                return Err(ErrorInternalServerError("Failed to get function"));
+                return Err(ErrorServiceUnavailable("Failed to get function"));
             }
         };
         log::info!("Function:{:?}", function);

crates/provider/src/proxy/proxy_handler.rs

@@ -2,13 +2,19 @@ use crate::handlers::invoke_resolver::InvokeResolver;
 use crate::proxy::builder::build_proxy_request;
 use crate::proxy::client::new_proxy_client_from_config;
 use crate::types::config::FaaSConfig;
-use actix_web::{Error, HttpRequest, HttpResponse, Responder, http::Method, web};
+use actix_web::{
+    Error, HttpRequest, HttpResponse,
+    error::{ErrorBadRequest, ErrorInternalServerError, ErrorMethodNotAllowed},
+    http::Method,
+    web,
+};
 
+// 主要参考源码的响应设置
 pub async fn proxy_handler(
     config: web::Data<FaaSConfig>,
     req: HttpRequest,
     payload: web::Payload,
-) -> impl Responder {
+) -> Result<HttpResponse, Error> {
     let proxy_client = new_proxy_client_from_config(config.as_ref()).await;
     log::info!("proxy_client : {:?}", proxy_client);
 
@@ -19,11 +25,8 @@ pub async fn proxy_handler(
         | Method::GET
         | Method::PATCH
         | Method::HEAD
-        | Method::OPTIONS => match proxy_request(&req, payload, &proxy_client).await {
+        | Method::OPTIONS => proxy_request(&req, payload, &proxy_client).await,
-            Ok(resp) => resp,
+        _ => Err(ErrorMethodNotAllowed("method not allowed")),
-            Err(e) => HttpResponse::from_error(e),
-        },
-        _ => HttpResponse::MethodNotAllowed().body("method not allowed"),
     }
 }
 
@@ -35,13 +38,10 @@ async fn proxy_request(
 ) -> Result<HttpResponse, Error> {
     let function_name = req.match_info().get("name").unwrap_or("");
     if function_name.is_empty() {
-        return Ok(HttpResponse::BadRequest().body("provide function name in path"));
+        return Err(ErrorBadRequest("function name is required"));
     }
 
-    let function_addr = match InvokeResolver::resolve_function_url(function_name).await {
+    let function_addr = InvokeResolver::resolve_function_url(function_name).await?;
-        Ok(function_addr) => function_addr,
-        Err(e) => return Ok(HttpResponse::BadRequest().body(e.to_string())),
-    };
 
     let proxy_req = build_proxy_request(req, &function_addr, proxy_client, payload).await?;
 
@@ -58,6 +58,6 @@ async fn proxy_request(
 
             Ok(client_resp.body(body))
         }
-        Err(e) => Ok(HttpResponse::BadGateway().body(e.to_string())),
+        Err(e) => Err(ErrorInternalServerError(e)),
     }
 }