Merge pull request #166 from Kuadrant/minor-refactor

Fix some lints and remove panic in auth action
Kuadrant · Feb 10, 2025 · abe70bb · abe70bb
2 parents 6b3fb69 + c920fa3
commit abe70bb
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 44 deletions.
diff --git a/src/auth_action.rs b/src/auth_action.rs
@@ -2,7 +2,7 @@ use crate::configuration::{Action, FailureMode, Service};
 use crate::data::{store_metadata, Predicate, PredicateVec};
 use crate::envoy::{CheckResponse, CheckResponse_oneof_http_response, HeaderValueOption};
 use crate::service::{GrpcErrResponse, GrpcService, HeaderKind, Headers};
-use log::debug;
+use log::{debug, warn};
 use std::rc::Rc;
 
 #[derive(Debug)]
@@ -42,6 +42,16 @@ impl AuthAction {
         self.grpc_service.get_failure_mode()
     }
 
+    pub fn resolve_failure_mode(&self) -> Result<HeaderKind, GrpcErrResponse> {
+        match self.get_failure_mode() {
+            FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()),
+            FailureMode::Allow => {
+                debug!("process_response(auth): continuing as FailureMode Allow");
+                Ok(HeaderKind::Request(Vec::default()))
+            }
+        }
+    }
+
     pub fn process_response(
         &self,
         check_response: CheckResponse,
@@ -54,13 +64,7 @@ impl AuthAction {
         match check_response.http_response {
             None => {
                 debug!("process_response(auth): received no http_response");
-                match self.get_failure_mode() {
-                    FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()),
-                    FailureMode::Allow => {
-                        debug!("process_response(auth): continuing as FailureMode Allow");
-                        Ok(HeaderKind::Request(Vec::default()))
-                    }
-                }
+                self.resolve_failure_mode()
             }
             Some(CheckResponse_oneof_http_response::denied_response(denied_response)) => {
                 debug!("process_response(auth): received DeniedHttpResponse");
@@ -76,16 +80,17 @@ impl AuthAction {
                 debug!("process_response(auth): received OkHttpResponse");
 
                 if !ok_response.get_response_headers_to_add().is_empty() {
-                    panic!("process_response(auth): response contained response_headers_to_add which is unsupported!")
-                }
-                if !ok_response.get_headers_to_remove().is_empty() {
-                    panic!("process_response(auth): response contained headers_to_remove which is unsupported!")
-                }
-                if !ok_response.get_query_parameters_to_set().is_empty() {
-                    panic!("process_response(auth): response contained query_parameters_to_set which is unsupported!")
-                }
-                if !ok_response.get_query_parameters_to_remove().is_empty() {
-                    panic!("process_response(auth): response contained query_parameters_to_remove which is unsupported!")
+                    warn!("process_response(auth): Unsupported field 'response_headers_to_add' in OkHttpResponse");
+                    return Err(GrpcErrResponse::new_internal_server_error());
+                } else if !ok_response.get_headers_to_remove().is_empty() {
+                    warn!("process_response(auth): Unsupported field 'headers_to_remove' in OkHttpResponse");
+                    return Err(GrpcErrResponse::new_internal_server_error());
+                } else if !ok_response.get_query_parameters_to_set().is_empty() {
+                    warn!("process_response(auth): Unsupported field 'query_parameters_to_set' in OkHttpResponse");
+                    return Err(GrpcErrResponse::new_internal_server_error());
+                } else if !ok_response.get_query_parameters_to_remove().is_empty() {
+                    warn!("process_response(auth): Unsupported field 'query_parameters_to_remove' in OkHttpResponse");
+                    return Err(GrpcErrResponse::new_internal_server_error());
                 }
                 Ok(HeaderKind::Request(Self::get_header_vec(
                     ok_response.get_headers(),

diff --git a/src/filter/operations.rs b/src/filter/operations.rs
@@ -1,5 +1,4 @@
 use crate::configuration::FailureMode;
-use crate::filter::operations::Operation::SendGrpcRequest;
 use crate::runtime_action_set::RuntimeActionSet;
 use crate::service::{GrpcErrResponse, GrpcRequest, HeaderKind, IndexedGrpcRequest};
 use std::rc::Rc;
@@ -64,10 +63,9 @@ impl GrpcMessageReceiverOperation {
                 }
                 operations.push(match next_msg {
                     None => Operation::Done(),
-                    Some(indexed_req) => SendGrpcRequest(GrpcMessageSenderOperation::new(
-                        self.runtime_action_set,
-                        indexed_req,
-                    )),
+                    Some(indexed_req) => Operation::SendGrpcRequest(
+                        GrpcMessageSenderOperation::new(self.runtime_action_set, indexed_req),
+                    ),
                 });
                 operations
             }

diff --git a/src/ratelimit_action.rs b/src/ratelimit_action.rs
@@ -154,6 +154,16 @@ impl RateLimitAction {
         self.grpc_service.get_failure_mode()
     }
 
+    pub fn resolve_failure_mode(&self) -> Result<HeaderKind, GrpcErrResponse> {
+        match self.get_failure_mode() {
+            FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()),
+            FailureMode::Allow => {
+                debug!("process_response(rl): continuing as FailureMode Allow");
+                Ok(HeaderKind::Response(Vec::default()))
+            }
+        }
+    }
+
     #[must_use]
     pub fn merge(&mut self, other: RateLimitAction) -> Option<RateLimitAction> {
         if self.scope == other.scope && self.service_name == other.service_name {
@@ -174,13 +184,7 @@ impl RateLimitAction {
                 ..
             } => {
                 debug!("process_response(rl): received UNKNOWN response");
-                match self.get_failure_mode() {
-                    FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()),
-                    FailureMode::Allow => {
-                        debug!("process_response(rl): continuing as FailureMode Allow");
-                        Ok(HeaderKind::Response(Vec::default()))
-                    }
-                }
+                self.resolve_failure_mode()
             }
             RateLimitResponse {
                 overall_code: RateLimitResponse_Code::OVER_LIMIT,

diff --git a/src/runtime_action.rs b/src/runtime_action.rs
@@ -48,6 +48,13 @@ impl RuntimeAction {
         }
     }
 
+    pub fn resolve_failure_mode(&self) -> Result<HeaderKind, GrpcErrResponse> {
+        match self {
+            Self::Auth(auth_action) => auth_action.resolve_failure_mode(),
+            Self::RateLimit(rl_action) => rl_action.resolve_failure_mode(),
+        }
+    }
+
     #[must_use]
     pub fn merge(&mut self, other: RuntimeAction) -> Option<RuntimeAction> {
         // only makes sense for rate limiting actions
@@ -73,26 +80,14 @@ impl RuntimeAction {
                 Ok(check_response) => auth_action.process_response(check_response),
                 Err(e) => {
                     debug!("process_response(auth): failed to parse response `{e:?}`");
-                    match self.get_failure_mode() {
-                        FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()),
-                        FailureMode::Allow => {
-                            debug!("process_response(auth): continuing as FailureMode Allow");
-                            Ok(HeaderKind::Request(Vec::default()))
-                        }
-                    }
+                    self.resolve_failure_mode()
                 }
             },
             Self::RateLimit(rl_action) => match Message::parse_from_bytes(msg) {
                 Ok(rate_limit_response) => rl_action.process_response(rate_limit_response),
                 Err(e) => {
                     debug!("process_response(rl): failed to parse response `{e:?}`");
-                    match self.get_failure_mode() {
-                        FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()),
-                        FailureMode::Allow => {
-                            debug!("process_response(rl): continuing as FailureMode Allow");
-                            Ok(HeaderKind::Response(Vec::default()))
-                        }
-                    }
+                    self.resolve_failure_mode()
                 }
             },
         }