Skip to content

Commit 9cd3c64

Browse files
flavioflavio
authored
Merge pull request #909 from flavio/group-policy-revisit-admission-response
fix: revisit AdmissionResponse of policy groups
2 parents fd8b097 + 445a710 commit 9cd3c64

File tree

5 files changed

+164
-57
lines changed

5 files changed

+164
-57
lines changed

‎Cargo.lock

+3-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ opentelemetry = { version = "0.24.0", default-features = false, features = [
2929
] }
3030
opentelemetry_sdk = { version = "0.24.1", features = ["rt-tokio"] }
3131
pprof = { version = "0.13", features = ["prost-codec"] }
32-
policy-evaluator = { git = "https://github.com/kubewarden/policy-evaluator", tag = "v0.18.8" }
32+
policy-evaluator = { git = "https://github.com/kubewarden/policy-evaluator", tag = "v0.19.0" }
3333
rustls = { version = "0.23", default-features = false, features = [
3434
"ring",
3535
"logging",

‎src/api/service.rs

+18-9
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,7 @@ fn validation_response_with_constraints(
160160
status: Some(AdmissionResponseStatus {
161161
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.")),
162162
code: None,
163+
..Default::default()
163164
}),
164165
// 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
165166
patch: None,
@@ -197,12 +198,14 @@ fn validation_response_with_constraints(
197198

198199
#[cfg(test)]
199200
mod tests {
201+
use super::*;
202+
200203
use crate::test_utils::build_admission_review_request;
204+
201205
use lazy_static::lazy_static;
206+
use policy_evaluator::admission_response;
202207
use rstest::*;
203208

204-
use super::*;
205-
206209
lazy_static! {
207210
static ref POLICY_ID: PolicyID = PolicyID::Policy("policy-id".to_string());
208211
}
@@ -293,7 +296,7 @@ mod tests {
293296
AdmissionResponse {
294297
allowed: true,
295298
patch: Some("patch".to_string()),
296-
patch_type: Some("application/json-patch+json".to_string()),
299+
patch_type: Some(admission_response::PatchType::JSONPatch),
297300
..Default::default()
298301
},
299302
),
@@ -308,7 +311,7 @@ mod tests {
308311
AdmissionResponse {
309312
allowed: true,
310313
patch: Some("patch".to_string()),
311-
patch_type: Some("application/json-patch+json".to_string()),
314+
patch_type: Some(admission_response::PatchType::JSONPatch),
312315
..Default::default()
313316
},
314317
),
@@ -331,7 +334,7 @@ mod tests {
331334
AdmissionResponse {
332335
allowed: true,
333336
patch: Some("patch".to_string()),
334-
patch_type: Some("application/json-patch+json".to_string()),
337+
patch_type: Some(admission_response::PatchType::JSONPatch),
335338
..Default::default()
336339
},
337340
),
@@ -347,7 +350,7 @@ mod tests {
347350
AdmissionResponse {
348351
allowed: true,
349352
patch: Some("patch".to_string()),
350-
patch_type: Some("application/json-patch+json".to_string()),
353+
patch_type: Some(admission_response::PatchType::JSONPatch),
351354
..Default::default()
352355
},
353356
),
@@ -378,6 +381,7 @@ mod tests {
378381
status: Some(AdmissionResponseStatus {
379382
message: Some("some rejection message".to_string()),
380383
code: Some(500),
384+
..Default::default()
381385
}),
382386
..Default::default()
383387
},
@@ -408,6 +412,7 @@ mod tests {
408412
status: Some(AdmissionResponseStatus {
409413
message: Some("some rejection message".to_string()),
410414
code: Some(500),
415+
..Default::default()
411416
}),
412417
..Default::default()
413418
},
@@ -431,14 +436,14 @@ mod tests {
431436
AdmissionResponse {
432437
allowed: true,
433438
patch: Some("patch".to_string()),
434-
patch_type: Some("application/json-patch+json".to_string()),
439+
patch_type: Some(admission_response::PatchType::JSONPatch),
435440
..Default::default()
436441
},
437442
),
438443
AdmissionResponse {
439444
allowed: true,
440445
patch: Some("patch".to_string()),
441-
patch_type: Some("application/json-patch+json".to_string()),
446+
patch_type: Some(admission_response::PatchType::JSONPatch),
442447
..Default::default()
443448
},
444449
"Mutated request from a policy allowed to mutate should be accepted in protect mode"
@@ -452,7 +457,7 @@ mod tests {
452457
AdmissionResponse {
453458
allowed: true,
454459
patch: Some("patch".to_string()),
455-
patch_type: Some("application/json-patch+json".to_string()),
460+
patch_type: Some(admission_response::PatchType::JSONPatch),
456461
..Default::default()
457462
},
458463
),
@@ -493,6 +498,7 @@ mod tests {
493498
status: Some(AdmissionResponseStatus {
494499
message: Some("some rejection message".to_string()),
495500
code: Some(500),
501+
..Default::default()
496502
}),
497503
..Default::default()
498504
},
@@ -502,6 +508,7 @@ mod tests {
502508
status: Some(AdmissionResponseStatus {
503509
message: Some("some rejection message".to_string()),
504510
code: Some(500),
511+
..Default::default()
505512
}),
506513
..Default::default()
507514
}, "Not accepted request from a policy allowed to mutate should be rejected in protect mode"
@@ -530,6 +537,7 @@ mod tests {
530537
status: Some(AdmissionResponseStatus {
531538
message: Some("some rejection message".to_string()),
532539
code: Some(500),
540+
..Default::default()
533541
}),
534542
..Default::default()
535543
},
@@ -539,6 +547,7 @@ mod tests {
539547
status: Some(AdmissionResponseStatus {
540548
message: Some("some rejection message".to_string()),
541549
code: Some(500),
550+
..Default::default()
542551
}),
543552
..Default::default()
544553
}, "Not accepted request from a policy not allowed to mutate should be rejected in protect mode"

‎src/evaluation/evaluation_environment.rs

+92-21
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use policy_evaluator::{
2-
admission_response::{AdmissionResponse, AdmissionResponseStatus},
2+
admission_response::{self, AdmissionResponse, AdmissionResponseStatus},
33
callback_requests::CallbackRequest,
44
evaluation_context::EvaluationContext,
55
kubewarden_policy_sdk::settings::SettingsValidationResponse,
@@ -659,28 +659,67 @@ impl EvaluationEnvironment {
659659
// to define inside of the expression
660660
let allowed = rhai_engine.eval_expression::<bool>(expression.as_str())?;
661661

662+
// The API Server puts some limitations on the warnings:
663+
// - they cannot exceed 256 characters
664+
// - the size of all the warnings cannot exceed 4096 characters
665+
// - they are returned as HTTP headers, hence not all characters are allowed
666+
//
667+
// Because of these reasons, we use the warning struct only to
668+
// tell the user whether a member policy was evaluated or not. When it was
669+
// evaluated we just tell the outcome (allow/reject).
670+
let mut warnings = vec![];
671+
672+
// The details of each policy evaluation are returned as part of the
673+
// AdmissionResponse.status.details.causes
674+
let mut status_causes = vec![];
675+
676+
let evaluation_results = policies_evaluation_results.lock().unwrap();
677+
678+
for policy_id in &policy_ids {
679+
if let Some(result) = evaluation_results.get(policy_id) {
680+
let outcome = if result.allowed {
681+
"allowed"
682+
} else {
683+
"rejected"
684+
};
685+
warnings.push(format!("{policy_id}: {outcome}",));
686+
687+
if !result.allowed {
688+
let cause = admission_response::StatusCause {
689+
field: Some(format!("spec.policies.{}", policy_id)),
690+
message: result.message.clone(),
691+
..Default::default()
692+
};
693+
status_causes.push(cause);
694+
}
695+
} else {
696+
warnings.push(format!("{}: not evaluated", policy_id));
697+
}
698+
}
699+
debug!(
700+
?policy_id,
701+
?allowed,
702+
?warnings,
703+
?status_causes,
704+
"policy group evaluation result"
705+
);
706+
662707
let status = if allowed {
708+
// The status field is discarded by the Kubernetes API server when the
709+
// request is allowed.
663710
None
664711
} else {
665712
Some(AdmissionResponseStatus {
666713
message: Some(message),
667714
code: None,
715+
details: Some(admission_response::StatusDetails {
716+
causes: status_causes,
717+
..Default::default()
718+
}),
719+
..Default::default()
668720
})
669721
};
670722

671-
// Provide some feedback to the end user about the single policy evaluation results
672-
let evaluation_results = policies_evaluation_results.lock().unwrap();
673-
let warnings: Vec<String> = policy_ids
674-
.iter()
675-
.map(|policy_id| {
676-
let result = evaluation_results
677-
.get(policy_id)
678-
.map(|result| result.to_string())
679-
.unwrap_or_else(|| "[NOT EVALUATED]".to_string());
680-
format!("{}: {}", policy_id, result)
681-
})
682-
.collect();
683-
684723
Ok(AdmissionResponse {
685724
uid: req.uid().to_string(),
686725
allowed,
@@ -1022,24 +1061,38 @@ mod tests {
10221061
"group_policy_with_unhappy_or_bracket_happy_and_unhappy_bracket",
10231062
false,
10241063
vec![
1025-
"unhappy_policy_2: [DENIED] - failing as expected",
1026-
"unhappy_policy_1: [DENIED] - failing as expected",
1027-
"happy_policy_1: [ALLOWED]",
1064+
"unhappy_policy_2: rejected",
1065+
"unhappy_policy_1: rejected",
1066+
"happy_policy_1: allowed",
10281067
],
1068+
vec![
1069+
admission_response::StatusCause {
1070+
field: Some("spec.policies.unhappy_policy_1".to_string()),
1071+
message: Some("failing as expected".to_string()),
1072+
..Default::default()
1073+
},
1074+
admission_response::StatusCause {
1075+
field: Some("spec.policies.unhappy_policy_2".to_string()),
1076+
message: Some("failing as expected".to_string()),
1077+
..Default::default()
1078+
},
1079+
]
10291080
)]
10301081
#[case::not_all_policies_are_evaluated(
10311082
"group_policy_with_unhappy_or_happy_or_unhappy",
10321083
true,
10331084
vec![
1034-
"unhappy_policy_1: [DENIED] - failing as expected",
1035-
"unhappy_policy_2: [NOT EVALUATED]",
1036-
"happy_policy_1: [ALLOWED]",
1085+
"unhappy_policy_1: rejected",
1086+
"unhappy_policy_2: not evaluated",
1087+
"happy_policy_1: allowed",
10371088
],
1089+
Vec::new(), // no expected causes, since the request is accepted
10381090
)]
10391091
fn group_policy_warning_assignments(
10401092
#[case] policy_id: &str,
10411093
#[case] admission_accepted: bool,
10421094
#[case] expected_warnings: Vec<&str>,
1095+
#[case] expected_status_causes: Vec<admission_response::StatusCause>,
10431096
) {
10441097
let policy_id = PolicyID::Policy(policy_id.to_string());
10451098
let evaluation_environment = Arc::new(build_evaluation_environment());
@@ -1063,10 +1116,28 @@ mod tests {
10631116
for expected in expected_warnings {
10641117
assert!(
10651118
warnings.iter().any(|w| w.contains(expected)),
1066-
"could not find {}",
1119+
"could not find warning {}",
10671120
expected
10681121
);
10691122
}
1123+
1124+
if admission_accepted {
1125+
assert!(response.status.is_none());
1126+
} else {
1127+
let causes = response
1128+
.status
1129+
.expect("should have status")
1130+
.details
1131+
.expect("should have details")
1132+
.causes;
1133+
for expected in expected_status_causes {
1134+
assert!(
1135+
causes.iter().any(|c| *c == expected),
1136+
"could not find cause {:?}",
1137+
expected
1138+
);
1139+
}
1140+
}
10701141
}
10711142

10721143
/// Given to identical wasm modules, only one instance of PolicyEvaluator is going to be

0 commit comments

Comments
 (0)