Skip to content

Commit a28c54e

Browse files
committed
fix: remove warnings from policy groups.
While testing the group policy feature, it was realized that the warning messages returned by the policy groups can be confusing. This commit changes the code to stop adding warnings with the evaluation results from policy members. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
1 parent 2b57879 commit a28c54e

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)