Skip to content

Commit

Permalink
Merge pull request #909 from flavio/group-policy-revisit-admission-re…
Browse files Browse the repository at this point in the history 
…sponse

fix: revisit AdmissionResponse of policy groups
  • Loading branch information
@flavio
flavio authored Sep 20, 2024
2 parents fd8b097 + 445a710 commit 9cd3c64
Show file tree
Hide file tree
Showing 5 changed files with 164 additions and 57 deletions.
6 changes: 3 additions & 3 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ opentelemetry = { version = "0.24.0", default-features = false, features = [
] }
opentelemetry_sdk = { version = "0.24.1", features = ["rt-tokio"] }
pprof = { version = "0.13", features = ["prost-codec"] }
policy-evaluator = { git = "https://github.com/kubewarden/policy-evaluator", tag = "v0.18.8" }
policy-evaluator = { git = "https://github.com/kubewarden/policy-evaluator", tag = "v0.19.0" }
rustls = { version = "0.23", default-features = false, features = [
"ring",
"logging",
Expand Down
27 changes: 18 additions & 9 deletions src/api/service.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ fn validation_response_with_constraints(
status: Some(AdmissionResponseStatus {
message: Some(format!("Request rejected by policy {policy_id}. The policy attempted to mutate the request, but it is currently configured to not allow mutations.")),
code: None,
..Default::default()
}),
// if `allowed_to_mutate` is false, we are in a validating webhook. If we send a patch, k8s will fail because validating webhook do not expect this field
patch: None,
Expand Down Expand Up @@ -197,12 +198,14 @@ fn validation_response_with_constraints(

#[cfg(test)]
mod tests {
use super::*;

use crate::test_utils::build_admission_review_request;

use lazy_static::lazy_static;
use policy_evaluator::admission_response;
use rstest::*;

use super::*;

lazy_static! {
static ref POLICY_ID: PolicyID = PolicyID::Policy("policy-id".to_string());
}
Expand Down Expand Up @@ -293,7 +296,7 @@ mod tests {
AdmissionResponse {
allowed: true,
patch: Some("patch".to_string()),
patch_type: Some("application/json-patch+json".to_string()),
patch_type: Some(admission_response::PatchType::JSONPatch),
..Default::default()
},
),
Expand All @@ -308,7 +311,7 @@ mod tests {
AdmissionResponse {
allowed: true,
patch: Some("patch".to_string()),
patch_type: Some("application/json-patch+json".to_string()),
patch_type: Some(admission_response::PatchType::JSONPatch),
..Default::default()
},
),
Expand All @@ -331,7 +334,7 @@ mod tests {
AdmissionResponse {
allowed: true,
patch: Some("patch".to_string()),
patch_type: Some("application/json-patch+json".to_string()),
patch_type: Some(admission_response::PatchType::JSONPatch),
..Default::default()
},
),
Expand All @@ -347,7 +350,7 @@ mod tests {
AdmissionResponse {
allowed: true,
patch: Some("patch".to_string()),
patch_type: Some("application/json-patch+json".to_string()),
patch_type: Some(admission_response::PatchType::JSONPatch),
..Default::default()
},
),
Expand Down Expand Up @@ -378,6 +381,7 @@ mod tests {
status: Some(AdmissionResponseStatus {
message: Some("some rejection message".to_string()),
code: Some(500),
..Default::default()
}),
..Default::default()
},
Expand Down Expand Up @@ -408,6 +412,7 @@ mod tests {
status: Some(AdmissionResponseStatus {
message: Some("some rejection message".to_string()),
code: Some(500),
..Default::default()
}),
..Default::default()
},
Expand All @@ -431,14 +436,14 @@ mod tests {
AdmissionResponse {
allowed: true,
patch: Some("patch".to_string()),
patch_type: Some("application/json-patch+json".to_string()),
patch_type: Some(admission_response::PatchType::JSONPatch),
..Default::default()
},
),
AdmissionResponse {
allowed: true,
patch: Some("patch".to_string()),
patch_type: Some("application/json-patch+json".to_string()),
patch_type: Some(admission_response::PatchType::JSONPatch),
..Default::default()
},
"Mutated request from a policy allowed to mutate should be accepted in protect mode"
Expand All @@ -452,7 +457,7 @@ mod tests {
AdmissionResponse {
allowed: true,
patch: Some("patch".to_string()),
patch_type: Some("application/json-patch+json".to_string()),
patch_type: Some(admission_response::PatchType::JSONPatch),
..Default::default()
},
),
Expand Down Expand Up @@ -493,6 +498,7 @@ mod tests {
status: Some(AdmissionResponseStatus {
message: Some("some rejection message".to_string()),
code: Some(500),
..Default::default()
}),
..Default::default()
},
Expand All @@ -502,6 +508,7 @@ mod tests {
status: Some(AdmissionResponseStatus {
message: Some("some rejection message".to_string()),
code: Some(500),
..Default::default()
}),
..Default::default()
}, "Not accepted request from a policy allowed to mutate should be rejected in protect mode"
Expand Down Expand Up @@ -530,6 +537,7 @@ mod tests {
status: Some(AdmissionResponseStatus {
message: Some("some rejection message".to_string()),
code: Some(500),
..Default::default()
}),
..Default::default()
},
Expand All @@ -539,6 +547,7 @@ mod tests {
status: Some(AdmissionResponseStatus {
message: Some("some rejection message".to_string()),
code: Some(500),
..Default::default()
}),
..Default::default()
}, "Not accepted request from a policy not allowed to mutate should be rejected in protect mode"
Expand Down
113 changes: 92 additions & 21 deletions src/evaluation/evaluation_environment.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use policy_evaluator::{
admission_response::{AdmissionResponse, AdmissionResponseStatus},
admission_response::{self, AdmissionResponse, AdmissionResponseStatus},
callback_requests::CallbackRequest,
evaluation_context::EvaluationContext,
kubewarden_policy_sdk::settings::SettingsValidationResponse,
Expand Down Expand Up @@ -659,28 +659,67 @@ impl EvaluationEnvironment {
// to define inside of the expression
let allowed = rhai_engine.eval_expression::<bool>(expression.as_str())?;

// The API Server puts some limitations on the warnings:
// - they cannot exceed 256 characters
// - the size of all the warnings cannot exceed 4096 characters
// - they are returned as HTTP headers, hence not all characters are allowed
//
// Because of these reasons, we use the warning struct only to
// tell the user whether a member policy was evaluated or not. When it was
// evaluated we just tell the outcome (allow/reject).
let mut warnings = vec![];

// The details of each policy evaluation are returned as part of the
// AdmissionResponse.status.details.causes
let mut status_causes = vec![];

let evaluation_results = policies_evaluation_results.lock().unwrap();

for policy_id in &policy_ids {
if let Some(result) = evaluation_results.get(policy_id) {
let outcome = if result.allowed {
"allowed"
} else {
"rejected"
};
warnings.push(format!("{policy_id}: {outcome}",));

if !result.allowed {
let cause = admission_response::StatusCause {
field: Some(format!("spec.policies.{}", policy_id)),
message: result.message.clone(),
..Default::default()
};
status_causes.push(cause);
}
} else {
warnings.push(format!("{}: not evaluated", policy_id));
}
}
debug!(
?policy_id,
?allowed,
?warnings,
?status_causes,
"policy group evaluation result"
);

let status = if allowed {
// The status field is discarded by the Kubernetes API server when the
// request is allowed.
None
} else {
Some(AdmissionResponseStatus {
message: Some(message),
code: None,
details: Some(admission_response::StatusDetails {
causes: status_causes,
..Default::default()
}),
..Default::default()
})
};

// Provide some feedback to the end user about the single policy evaluation results
let evaluation_results = policies_evaluation_results.lock().unwrap();
let warnings: Vec<String> = policy_ids
.iter()
.map(|policy_id| {
let result = evaluation_results
.get(policy_id)
.map(|result| result.to_string())
.unwrap_or_else(|| "[NOT EVALUATED]".to_string());
format!("{}: {}", policy_id, result)
})
.collect();

Ok(AdmissionResponse {
uid: req.uid().to_string(),
allowed,
Expand Down Expand Up @@ -1022,24 +1061,38 @@ mod tests {
"group_policy_with_unhappy_or_bracket_happy_and_unhappy_bracket",
false,
vec![
"unhappy_policy_2: [DENIED] - failing as expected",
"unhappy_policy_1: [DENIED] - failing as expected",
"happy_policy_1: [ALLOWED]",
"unhappy_policy_2: rejected",
"unhappy_policy_1: rejected",
"happy_policy_1: allowed",
],
vec![
admission_response::StatusCause {
field: Some("spec.policies.unhappy_policy_1".to_string()),
message: Some("failing as expected".to_string()),
..Default::default()
},
admission_response::StatusCause {
field: Some("spec.policies.unhappy_policy_2".to_string()),
message: Some("failing as expected".to_string()),
..Default::default()
},
]
)]
#[case::not_all_policies_are_evaluated(
"group_policy_with_unhappy_or_happy_or_unhappy",
true,
vec![
"unhappy_policy_1: [DENIED] - failing as expected",
"unhappy_policy_2: [NOT EVALUATED]",
"happy_policy_1: [ALLOWED]",
"unhappy_policy_1: rejected",
"unhappy_policy_2: not evaluated",
"happy_policy_1: allowed",
],
Vec::new(), // no expected causes, since the request is accepted
)]
fn group_policy_warning_assignments(
#[case] policy_id: &str,
#[case] admission_accepted: bool,
#[case] expected_warnings: Vec<&str>,
#[case] expected_status_causes: Vec<admission_response::StatusCause>,
) {
let policy_id = PolicyID::Policy(policy_id.to_string());
let evaluation_environment = Arc::new(build_evaluation_environment());
Expand All @@ -1063,10 +1116,28 @@ mod tests {
for expected in expected_warnings {
assert!(
warnings.iter().any(|w| w.contains(expected)),
"could not find {}",
"could not find warning {}",
expected
);
}

if admission_accepted {
assert!(response.status.is_none());
} else {
let causes = response
.status
.expect("should have status")
.details
.expect("should have details")
.causes;
for expected in expected_status_causes {
assert!(
causes.iter().any(|c| *c == expected),
"could not find cause {:?}",
expected
);
}
}
}

/// Given to identical wasm modules, only one instance of PolicyEvaluator is going to be
Expand Down
Loading

0 comments on commit 9cd3c64

Please sign in to comment.