Skip to content

Commit fd0a250

Browse files
authored
Merge pull request #928 from jvanz/donot-emit-warnings
fix: remove warnings from policy groups.
2 parents 2b57879 + a28c54e commit fd0a250

File tree

2 files changed

+5
-59
lines changed

2 files changed

+5
-59
lines changed

src/evaluation/evaluation_environment.rs

+2-41
Original file line numberDiff line numberDiff line change
@@ -659,16 +659,6 @@ 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-
672662
// The details of each policy evaluation are returned as part of the
673663
// AdmissionResponse.status.details.causes
674664
let mut status_causes = vec![];
@@ -677,13 +667,6 @@ impl EvaluationEnvironment {
677667

678668
for policy_id in &policy_ids {
679669
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-
687670
if !result.allowed {
688671
let cause = admission_response::StatusCause {
689672
field: Some(format!("spec.policies.{}", policy_id)),
@@ -692,14 +675,11 @@ impl EvaluationEnvironment {
692675
};
693676
status_causes.push(cause);
694677
}
695-
} else {
696-
warnings.push(format!("{}: not evaluated", policy_id));
697678
}
698679
}
699680
debug!(
700681
?policy_id,
701682
?allowed,
702-
?warnings,
703683
?status_causes,
704684
"policy group evaluation result"
705685
);
@@ -727,7 +707,7 @@ impl EvaluationEnvironment {
727707
patch: None,
728708
status,
729709
audit_annotations: None,
730-
warnings: Some(warnings),
710+
warnings: None,
731711
})
732712
}
733713
}
@@ -1060,11 +1040,6 @@ mod tests {
10601040
#[case::all_policies_are_evaluated(
10611041
"group_policy_with_unhappy_or_bracket_happy_and_unhappy_bracket",
10621042
false,
1063-
vec![
1064-
"unhappy_policy_2: rejected",
1065-
"unhappy_policy_1: rejected",
1066-
"happy_policy_1: allowed",
1067-
],
10681043
vec![
10691044
admission_response::StatusCause {
10701045
field: Some("spec.policies.unhappy_policy_1".to_string()),
@@ -1081,17 +1056,11 @@ mod tests {
10811056
#[case::not_all_policies_are_evaluated(
10821057
"group_policy_with_unhappy_or_happy_or_unhappy",
10831058
true,
1084-
vec![
1085-
"unhappy_policy_1: rejected",
1086-
"unhappy_policy_2: not evaluated",
1087-
"happy_policy_1: allowed",
1088-
],
10891059
Vec::new(), // no expected causes, since the request is accepted
10901060
)]
10911061
fn group_policy_warning_assignments(
10921062
#[case] policy_id: &str,
10931063
#[case] admission_accepted: bool,
1094-
#[case] expected_warnings: Vec<&str>,
10951064
#[case] expected_status_causes: Vec<admission_response::StatusCause>,
10961065
) {
10971066
let policy_id = PolicyID::Policy(policy_id.to_string());
@@ -1111,15 +1080,7 @@ mod tests {
11111080
.validate(&policy_id, &validate_request)
11121081
.expect("should not have errored");
11131082
assert_eq!(response.allowed, admission_accepted);
1114-
1115-
let warnings = response.warnings.expect("should have warnings");
1116-
for expected in expected_warnings {
1117-
assert!(
1118-
warnings.iter().any(|w| w.contains(expected)),
1119-
"could not find warning {}",
1120-
expected
1121-
);
1122-
}
1083+
assert_eq!(response.warnings, None);
11231084

11241085
if admission_accepted {
11251086
assert!(response.status.is_none());

tests/integration_test.rs

+3-18
Original file line numberDiff line numberDiff line change
@@ -122,18 +122,9 @@ async fn test_validate_policy_group(#[case] payload: &str, #[case] expected_allo
122122
.message,
123123
);
124124
}
125+
assert_eq!(admission_review_response.response.warnings, None);
125126

126-
let warning_messages = &admission_review_response
127-
.response
128-
.warnings
129-
.expect("warning messages should always be filled by policy groups");
130-
assert_eq!(1, warning_messages.len());
131-
132-
let warning_msg = &warning_messages[0];
133-
if expected_allowed {
134-
assert!(warning_msg.contains("allowed"));
135-
} else {
136-
assert!(warning_msg.contains("rejected"));
127+
if !expected_allowed {
137128
let causes = admission_review_response
138129
.response
139130
.status
@@ -252,13 +243,7 @@ async fn test_validate_policy_group_does_not_do_mutation() {
252243
);
253244
assert!(admission_review_response.response.patch.is_none());
254245

255-
let warning_messages = &admission_review_response
256-
.response
257-
.warnings
258-
.expect("warning messages should always be filled by policy groups");
259-
assert_eq!(1, warning_messages.len());
260-
let warning_msg = &warning_messages[0];
261-
assert!(warning_msg.contains("rejected"));
246+
assert_eq!(admission_review_response.response.warnings, None);
262247

263248
let causes = admission_review_response
264249
.response

0 commit comments

Comments
 (0)