Merge pull request #712 from fabriziosestito/fix/do-not-exit-on-policy-error

feat: do not exit on policy error

Author: fabriziosestito

src/api/service.rs

@@ -36,12 +36,23 @@ pub(crate) fn evaluate(
     let start_time = Instant::now();
 
     let policy_name = policy_id.to_owned();
+    let vanilla_validation_response =
+        match evaluation_environment.validate(policy_id, validate_request) {
+            Ok(validation_response) => validation_response,
+            Err(EvaluationError::PolicyInitialization(error)) => {
+                return Ok(AdmissionResponse::reject(
+                    validate_request.uid().to_owned(),
+                    error.to_string(),
+                    500,
+                ))
+            }
+
+            Err(error) => return Err(error),
+        };
+
     let policy_mode = evaluation_environment.get_policy_mode(policy_id)?;
     let allowed_to_mutate = evaluation_environment.get_policy_allowed_to_mutate(policy_id)?;
 
-    let vanilla_validation_response =
-        evaluation_environment.validate(policy_id, validate_request)?;
-
     let policy_evaluation_duration = start_time.elapsed();
     let accepted = vanilla_validation_response.allowed;
     let mutated = vanilla_validation_response.patch.is_some();

src/evaluation/errors.rs

@@ -4,6 +4,9 @@ pub type Result<T> = std::result::Result<T, EvaluationError>;
 
 #[derive(Debug, Error)]
 pub enum EvaluationError {
+    #[error("{0}")]
+    PolicyInitialization(String),
+
     #[error("unknown policy: {0}")]
     PolicyNotFound(String),
 

src/evaluation/evaluation_environment.rs

@@ -61,6 +61,11 @@ pub(crate) struct EvaluationEnvironment {
     /// Map a `policy_id` to the `PolicyEvaluationSettings` instance. This allows us to obtain
     /// the list of settings to be used when evaluating a given policy.
     policy_id_to_settings: HashMap<String, PolicyEvaluationSettings>,
+
+    /// A map with the policy ID as key, and the error message as value.
+    /// This is used to store the errors that occurred during policies initialization.
+    /// The errors can occur in the fetching of the policy, or in the validation of the settings.
+    policy_initialization_errors: HashMap<String, String>,
 }
 
 #[cfg_attr(test, automock)]
@@ -88,6 +93,16 @@ impl EvaluationEnvironment {
                 ))
             })?;
 
+            let precompiled_policy = match precompiled_policy.as_ref() {
+                Ok(precompiled_policy) => precompiled_policy,
+                Err(e) => {
+                    eval_env
+                        .policy_initialization_errors
+                        .insert(policy_id.clone(), e.to_string());
+                    continue;
+                }
+            };
+
             eval_env
                 .register(
                     engine,
@@ -98,6 +113,8 @@ impl EvaluationEnvironment {
                     policy_evaluation_limit_seconds,
                 )
                 .map_err(|e| EvaluationError::BootstrapFailure(e.to_string()))?;
+
+            eval_env.validate_settings(policy_id)?;
         }
 
         Ok(eval_env)
@@ -208,18 +225,41 @@ impl EvaluationEnvironment {
 
     /// Perform a request validation
     pub fn validate(&self, policy_id: &str, req: &ValidateRequest) -> Result<AdmissionResponse> {
+        if let Some(error) = self.policy_initialization_errors.get(policy_id) {
+            return Err(EvaluationError::PolicyInitialization(error.to_string()));
+        }
+
         let settings = self.get_policy_settings(policy_id)?;
         let mut evaluator = self.rehydrate(policy_id)?;
 
         Ok(evaluator.validate(req.clone(), &settings.settings))
     }
 
     /// Validate the settings the user provided for the given policy
-    pub fn validate_settings(&self, policy_id: &str) -> Result<SettingsValidationResponse> {
+    fn validate_settings(&mut self, policy_id: &str) -> Result<()> {
         let settings = self.get_policy_settings(policy_id)?;
         let mut evaluator = self.rehydrate(policy_id)?;
 
-        Ok(evaluator.validate_settings(&settings.settings))
+        match evaluator.validate_settings(&settings.settings) {
+            SettingsValidationResponse {
+                valid: true,
+                message: _,
+            } => return Ok(()),
+            SettingsValidationResponse {
+                valid: false,
+                message,
+            } => {
+                self.policy_initialization_errors.insert(
+                    policy_id.to_string(),
+                    format!(
+                        "Policy settings are invalid: {}",
+                        message.unwrap_or("no message".to_owned())
+                    ),
+                );
+            }
+        };
+
+        Ok(())
     }
 
     /// Internal method, create a `PolicyEvaluator` by using a pre-initialized instance
@@ -321,7 +361,7 @@ mod tests {
                     context_aware_resources: BTreeSet::new(),
                 },
             );
-            precompiled_policies.insert(policy_url, precompiled_policy.clone());
+            precompiled_policies.insert(policy_url, Ok(precompiled_policy.clone()));
         }
 
         EvaluationEnvironment::new(
@@ -386,4 +426,20 @@ mod tests {
             1
         );
     }
+
+    #[test]
+    fn validate_policy_with_initialization_error() {
+        let mut evaluation_environment = build_evaluation_environment().unwrap();
+        let policy_id = "policy_3";
+        evaluation_environment
+            .policy_initialization_errors
+            .insert(policy_id.to_string(), "error".to_string());
+
+        let validate_request =
+            ValidateRequest::AdmissionRequest(build_admission_review_request().request);
+        assert!(matches!(
+            evaluation_environment.validate(policy_id, &validate_request).unwrap_err(),
+            EvaluationError::PolicyInitialization(error) if error == "error"
+        ));
+    }
 }

src/evaluation/precompiled_policy.rs

@@ -66,8 +66,9 @@ impl PrecompiledPolicy {
 
 /// A dictionary with:
 /// * Key: the URL of the WebAssembly module
-/// * value: the PrecompiledPolicy
-pub(crate) type PrecompiledPolicies = HashMap<String, PrecompiledPolicy>;
+/// * Value: a Result containing the precompiled policy or an error.
+/// Errors are stored and will be reported to the user in the API response.
+pub(crate) type PrecompiledPolicies = HashMap<String, Result<PrecompiledPolicy>>;
 
 /// Check if policy server version is compatible with  minimum kubewarden
 /// version required by the policy

src/lib.rs

@@ -24,7 +24,6 @@ use policy_evaluator::policy_fetcher::verify::FulcioAndRekorData;
 use policy_evaluator::wasmtime;
 use policy_evaluator::{callback_handler::CallbackHandlerBuilder, kube};
 use rayon::prelude::*;
-use std::collections::HashMap;
 use std::net::SocketAddr;
 use std::sync::Arc;
 use tokio::sync::oneshot;
@@ -41,7 +40,7 @@ use crate::evaluation::{
     EvaluationEnvironment,
 };
 use crate::policy_downloader::{Downloader, FetchedPolicies};
-use config::{Config, Policy};
+use config::Config;
 
 pub struct PolicyServer {
     router: Router,
@@ -129,14 +128,14 @@ impl PolicyServer {
                 &config.policies_download_dir,
                 config.verification_config.as_ref(),
             )
-            .await?;
+            .await;
 
         let mut wasmtime_config = wasmtime::Config::new();
         if config.policy_evaluation_limit_seconds.is_some() {
             wasmtime_config.epoch_interruption(true);
         }
         let engine = wasmtime::Engine::new(&wasmtime_config)?;
-        let precompiled_policies = precompile_policies(&engine, &fetched_policies)?;
+        let precompiled_policies = precompile_policies(&engine, &fetched_policies);
 
         let evaluation_environment = EvaluationEnvironment::new(
             &engine,
@@ -165,8 +164,6 @@ impl PolicyServer {
             info!("policy timeout protection is disabled");
         }
 
-        verify_policy_settings(&config.policies, &evaluation_environment).await?;
-
         let state = Arc::new(ApiServerState {
             semaphore: Semaphore::new(config.pool_size),
             evaluation_environment,
@@ -245,66 +242,21 @@ impl PolicyServer {
 fn precompile_policies(
     engine: &wasmtime::Engine,
     fetched_policies: &FetchedPolicies,
-) -> Result<PrecompiledPolicies> {
+) -> PrecompiledPolicies {
     debug!(
         wasm_modules_count = fetched_policies.len(),
         "instantiating wasmtime::Module objects"
     );
 
-    let precompiled_policies: HashMap<String, Result<PrecompiledPolicy>> = fetched_policies
+    fetched_policies
         .par_iter()
-        .map(|(policy_url, wasm_module_path)| {
-            let precompiled_policy = PrecompiledPolicy::new(engine, wasm_module_path);
-            debug!(?policy_url, "module compiled");
-            (policy_url.clone(), precompiled_policy)
-        })
-        .collect();
-
-    let errors: Vec<String> = precompiled_policies
-        .iter()
-        .filter_map(|(url, result)| match result {
-            Ok(_) => None,
-            Err(e) => Some(format!(
-                "[{url}] policy cannot be compiled to WebAssembly module: {e:?}"
-            )),
-        })
-        .collect();
-    if !errors.is_empty() {
-        return Err(anyhow!(
-            "workers pool bootstrap: cannot instantiate `wasmtime::Module` objects: {:?}",
-            errors.join(", ")
-        ));
-    }
-
-    Ok(precompiled_policies
-        .iter()
-        .filter_map(|(url, result)| match result {
-            Ok(p) => Some((url.clone(), p.clone())),
-            Err(_) => None,
+        .map(|(policy_url, fetched_policy)| match fetched_policy {
+            Ok(policy) => {
+                let precompiled_policy = PrecompiledPolicy::new(engine, policy);
+                debug!(?policy_url, "module compiled");
+                (policy_url.clone(), precompiled_policy)
+            }
+            Err(error) => (policy_url.clone(), Err(anyhow!(error.to_string()))),
         })
-        .collect())
-}
-
-/// Ensure the user provided valid settings for all the policies
-async fn verify_policy_settings(
-    policies: &HashMap<String, Policy>,
-    evaluation_environment: &EvaluationEnvironment,
-) -> Result<()> {
-    let mut errors = vec![];
-    for (policy_id, _policy) in policies.iter() {
-        let set_val_rep = evaluation_environment.validate_settings(policy_id)?;
-        if !set_val_rep.valid {
-            errors.push(format!(
-                "[{}] settings are not valid: {:?}",
-                policy_id, set_val_rep.message
-            ));
-            continue;
-        }
-    }
-
-    if errors.is_empty() {
-        Ok(())
-    } else {
-        Err(anyhow!("{}", errors.join(", ")))
-    }
+        .collect()
 }