Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #19114: Overridden directives in the same rule are missing (not even "skipped") #3570

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,16 @@ import zio._
object ReportingServiceUtils {

/*
* Build rule status reports from node reports, decide=ing which directive should be "skipped"
* Build rule status reports from node reports, deciding which directives should be "skipped"
*/
def buildRuleStatusReport(ruleId: RuleId, nodeReports: Map[NodeId, NodeStatusReport]): RuleStatusReport = {
val toKeep = nodeReports.values.flatMap( _.reports ).filter(_.ruleId == ruleId).toList
// we don't keep overrides for a directive which is already in "toKeep" or that don't target that rule
val toKeepDir = toKeep.map(_.directives.keySet).toSet.flatten
val overrides = nodeReports.values.flatMap( _.overrides.filterNot(r => r.policy.ruleId != ruleId || toKeepDir.contains(r.policy.directiveId))).toList.distinct
RuleStatusReport(ruleId, toKeep, overrides)
// and we must make overrides unique - ie, we don't keep overridden that are overridden by directive themselve in the overridden list
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, wasn't able to make it clearer :)

val overrides2 = overrides.filterNot(o => overrides.exists(_.policy == o.overridenBy))
RuleStatusReport(ruleId, toKeep, overrides2)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ class ReportingServiceUtilsTest extends Specification {
val rule3 = RuleId("rule3")
val dir1 = DirectiveId("dir1")
val dir2 = DirectiveId("dir2")
val dir3 = DirectiveId("dir3")

val expiration = new DateTime(0) // not used

Expand Down Expand Up @@ -184,4 +185,46 @@ class ReportingServiceUtilsTest extends Specification {
RuleStatusReport(rule2, List(rnReport(node2, rule2, dir2)), noOverrides)
)
}

/*
* More complexe for one node:
* - 3 directives: dir1 (most prioritary), dir2 and dir3 (less)
* - 3 rules: rule1 has dir2, dir3 (skipped), rule2 has all 3 (so dir1 ok, other skipped), rule3 has all 3 (skipped)
* There is no ducplication of reports.
*/
"a rule not overriden on all nodes is not written overriden" in {
val reports = List(
NodeStatusReport(node1, NoRunNoExpectedReport, RunComplianceInfo.OK
, List(
// on rule1, both dir2 and dir3 are overridden by rule2/dir1
thisOverrideThatOn2(rule2, dir1, rule1, dir2)
, thisOverrideThatOn2(rule2, dir1, rule1, dir3)
// on rule2, both dir2 and dir3 are overridden by rule2/dir1
, thisOverrideThatOn2(rule2, dir1, rule2, dir2)
, thisOverrideThatOn2(rule2, dir1, rule2, dir3)
// on rule3, dir2, dir2 and dir3 are overridden by rule2/dir1
, thisOverrideThatOn2(rule2, dir1, rule3, dir1)
, thisOverrideThatOn2(rule2, dir1, rule3, dir2)
, thisOverrideThatOn2(rule2, dir1, rule3, dir3)
// and these one are real but should not be kept to avoid having several time the same "skipped"
, thisOverrideThatOn2(rule1, dir2, rule1, dir3)
, thisOverrideThatOn2(rule2, dir2, rule2, dir3)
, thisOverrideThatOn2(rule3, dir1, rule3, dir2)
, thisOverrideThatOn2(rule3, dir1, rule3, dir3)
, thisOverrideThatOn2(rule3, dir2, rule3, dir3)
)
// only one expected report: dir1 on rule2
, Set(rnReport(node1, rule2, dir1))
)
).map(r => (r.nodeId, r)).toMap

ReportingServiceUtils.buildRuleStatusReport(rule1, reports).isSameReportAs(
RuleStatusReport(rule1, List(), List(thisOverrideThatOn2(rule2, dir1, rule1, dir2), thisOverrideThatOn2(rule2, dir1, rule1, dir3)))
) and ReportingServiceUtils.buildRuleStatusReport(rule2, reports).isSameReportAs(
RuleStatusReport(rule2, List(rnReport(node1, rule2, dir1)), List(thisOverrideThatOn2(rule2, dir1, rule2, dir2), thisOverrideThatOn2(rule2, dir1, rule2, dir3)))
) and ReportingServiceUtils.buildRuleStatusReport(rule3, reports).isSameReportAs(
RuleStatusReport(rule3, List(), List(thisOverrideThatOn2(rule2, dir1, rule3, dir1), thisOverrideThatOn2(rule2, dir1, rule3, dir2), thisOverrideThatOn2(rule2, dir1, rule3, dir3)))
)
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -552,9 +552,7 @@ object ComplianceData extends Loggable {
, explanation
, JsonTagSerialisation.serializeTags(rule.tags)
)

}

JsTableData(ruleComplianceLine.toList)
}

Expand All @@ -565,8 +563,7 @@ object ComplianceData extends Loggable {
, onRuleScreen: Option[Rule] // if we are on a rule, we want to adapt message
) : List[DirectiveComplianceLine] = {
val overridesData = for {
// we don't want to write an overriden directive several time for the same overriding rule/directive.
over <- overrides.groupBy(_.overridenBy).map(_._2.head)
over <- overrides
(overridenTech , overridenDir) <- directiveLib.allDirectives.get(over.policy.directiveId)
overridingRule <- rules.find( _.id == over.overridenBy.ruleId)
(overridingTech, overridingDir) <- directiveLib.allDirectives.get(over.overridenBy.directiveId)
Expand Down