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.

Sign up for GitHub

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

Jump to bottom

Fixes #21924: Policy generation broken when defining a group with invert result of inclusion of another group #4703

Merged
merged 1 commit into from Mar 17, 2023
Merged

Fixes #21924: Policy generation broken when defining a group with invert result of inclusion of another group #4703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 35 additions & 26 deletions .../rudder-core/src/main/scala/com/normation/rudder/services/nodes/MergeNodeProperties.scala
View file
Open in desktop
Expand Up @@ -84,7 +84,7 @@ import scala.jdk.CollectionConverters._
* - if the overriding value is a simple value or an array, it replaces the previous one
* - if the overriding value is an object and the overridden value is a simple type or an array,
* the latter one is replaced by the former
* - if both overriding/overridden are arrays, overriding values REPLACE the overriden ones
* - if both overriding/overridden are arrays, overriding values REPLACE the overridden ones
* - if both overriding/overridden are objects, then each key is overridden recursively as explained,
* and new keys are added.
*/
Expand All @@ -93,11 +93,12 @@ import scala.jdk.CollectionConverters._
* Utility that represents a group with just the interesting things for us.
*/
final case class GroupProp(
properties: List[GroupProperty],
groupId: NodeGroupId,
condition: CriterionComposition,
isDynamic: Boolean,
parentGroups: List[String], // groupd ids - a list because order is important!
properties: List[GroupProperty],
groupId: NodeGroupId,
condition: CriterionComposition,
transformation: ResultTransformation,
isDynamic: Boolean,
parentGroups: List[String], // group ids - a list because order is important!

groupName: String // for error
)
Expand All @@ -106,13 +107,13 @@ object GroupProp {

/**
* This provider means that the property is purely inherited, ie it is not
* overriden for given node or group.
* overridden for given node or group.
*/
val INHERITANCE_PROVIDER = PropertyProvider("inherited")

/**
* This provider means that the property is inherited but also
* overriden for given node or group.
* overridden for given node or group.
*/
val OVERRIDE_PROVIDER = PropertyProvider("overridden")

Expand Down Expand Up @@ -140,6 +141,7 @@ object GroupProp {
group.properties,
group.id,
And,
ResultTransformation.Identity,
group.isDynamic,
Nil, // terminal leaf

Expand All @@ -152,6 +154,7 @@ object GroupProp {
group.properties,
group.id,
q.composition,
q.transform,
group.isDynamic,
q.criteria.flatMap {
// we are only interested in subgroup criterion with AND, and we want to
Expand All @@ -178,18 +181,15 @@ object GroupProp {
target.target match {
case FullCompositeRuleTarget(t) =>
Left(Unexpected(s"There is a composite target in group definition, it's likely a dev error: '${target.name}'"))
case FullOtherTarget(t) => // taget:all nodes, only root, only managed node, etc
case FullOtherTarget(t) => // target:all nodes, only root, only managed node, etc
Right(
GroupProp(
Nil, // they don't have properties for now

Nil, // they don't have properties for now
NodeGroupId(NodeGroupUid(t.target)),
And, // for simplification, since they don't have properties it doesn't matter

And, // for simplification, since they don't have properties it doesn't matter
ResultTransformation.Identity,
true, // these special targets behave as dynamic groups

Nil, // terminal leaf

Nil, // terminal leaf
target.name
)
)
Expand Down Expand Up @@ -430,7 +430,7 @@ object MergeNodeProperties {
merged <- mergeAll(flatten)
globals = globalParams.toList.map {
case (n, v) =>
// TODO: no verion in param for now
// TODO: no version in param for now
val config = GenericProperty.toConfig(
n,
GitVersion.DEFAULT_REV,
Expand Down Expand Up @@ -498,7 +498,7 @@ object MergeNodeProperties {
val groupList = groups.values.toList
for {
// we need to proceed in two pass because all vertices need to be added before edges
// add veritce
// add vertices
_ <- groupList.traverse { group =>
try {
Right(graph.addVertex(group))
Expand All @@ -517,19 +517,28 @@ object MergeNodeProperties {
for {
parents <- group.parentGroups.traverse { p =>
groups.get(p) match {
// the nominal case is to have the parent in the hierarchy, so groups.get(p) will return
// it and we can continue merging parent properties.
// I can happen that we don't have the parent in the hierarchy. This typically happens when the
// parent is part of an `invert` query (see https://issues.rudder.io/issues/21924). We don't
// want the parent to take part of property inheritance in that case, so we ignore it.
case None =>
Left(
Inconsistency(
s"Error when looking for parent group '${p}' of group '${group.groupName}' " +
s"[${group.groupId.serialize}]. Please check criterium for that group."
)
)
group.transformation match {
case ResultTransformation.Invert => Right(None)
case _ =>
Left(
Inconsistency(
s"Error when looking for parent group '${p}' of group '${group.groupName}' " +
s"[${group.groupId.serialize}]. Please check criterium for that group."
)
)
}
case Some(g) =>
Right(g)
Right(Some(g))
}
}
// reverse parents here, because we need to start from child and go up hierarchy
_ <- recAddEdges(graph, group, parents.reverse)
_ <- recAddEdges(graph, group, parents.flatten.reverse)
} yield ()
}
// get connected components
Expand Down
39 changes: 26 additions & 13 deletions ...er-core/src/test/scala/com/normation/rudder/services/nodes/TestMergeGroupProperties.scala
View file
Open in desktop
Expand Up @@ -121,7 +121,7 @@ class TestMergeGroupProperties extends Specification {
}

/*
* Hierartchy:
* Hierarchy:
* global
* |
* parent1 parent2
Expand Down Expand Up @@ -170,6 +170,18 @@ class TestMergeGroupProperties extends Specification {
merged must beLeft
}

"when the parent is in not in an inverted query and is missing, its an error" >> {
val merged = MergeNodeProperties.checkPropertyMerge(child.toTarget :: Nil, Map())
merged must beLeft
}

"when the parent is in an inverted query, its properties are not inherited" >> {
val ct2 = child.modify(_.query).setTo(Some(query.modify(_.transform).setTo(ResultTransformation.Invert))).toTarget
val merged = MergeNodeProperties.checkPropertyMerge(ct2 :: Nil, Map())
val expected = List(child).toH1("foo")
(merged must beRight(expected :: Nil)) and (merged.getOrElse(Nil).head.prop.valueAsString === "baz")
}

"override is done in the same order of line, the last wins" >> {
val q2 = query.modify(_.criteria).setTo(parent1.toCriterion :: parent2.toCriterion :: Nil)
val ct2 = child
Expand Down Expand Up @@ -217,7 +229,7 @@ class TestMergeGroupProperties extends Specification {
}

"be able to correct conflict" in {
val parent1 = NodeGroup(
val parent1 = NodeGroup(
NodeGroupId(NodeGroupUid("parent1")),
"parent1",
"",
Expand All @@ -227,7 +239,7 @@ class TestMergeGroupProperties extends Specification {
Set(),
true
)
val parent2 = NodeGroup(
val parent2 = NodeGroup(
NodeGroupId(NodeGroupUid("parent2")),
"parent2",
"",
Expand All @@ -237,7 +249,7 @@ class TestMergeGroupProperties extends Specification {
Set(),
true
)
val priorize = NodeGroup(
val prioritize = NodeGroup(
NodeGroupId(NodeGroupUid("parent3")),
"parent3",
"",
Expand All @@ -248,21 +260,22 @@ class TestMergeGroupProperties extends Specification {
true
)

val merged = MergeNodeProperties.checkPropertyMerge(parent1.toTarget :: parent2.toTarget :: priorize.toTarget :: Nil, Map())
val merged =
MergeNodeProperties.checkPropertyMerge(parent1.toTarget :: parent2.toTarget :: prioritize.toTarget :: Nil, Map())
val expected = List(parent2, parent1).toH1("dns") :: Nil
merged must beRight(expected)
}

/*
* Test case:
* p1: dns=1.1.1.1 p2: dns=9.9.9.9
* p3: p1 overriden by p2
* p3: p1 overridden by p2
* p4: only subgroup of p1
* ---------
* node in p4 and p3
*/
"one can solve conflicts at parent level" in {
val parent1 = NodeGroup(
val parent1 = NodeGroup(
NodeGroupId(NodeGroupUid("parent1")),
"parent1",
"",
Expand All @@ -272,7 +285,7 @@ class TestMergeGroupProperties extends Specification {
Set(),
true
)
val parent2 = NodeGroup(
val parent2 = NodeGroup(
NodeGroupId(NodeGroupUid("parent2")),
"parent2",
"",
Expand All @@ -282,7 +295,7 @@ class TestMergeGroupProperties extends Specification {
Set(),
true
)
val priorize = NodeGroup(
val prioritize = NodeGroup(
NodeGroupId(NodeGroupUid("parent3")),
"parent3",
"",
Expand All @@ -292,7 +305,7 @@ class TestMergeGroupProperties extends Specification {
Set(),
true
)
val parent4 = NodeGroup(
val parent4 = NodeGroup(
NodeGroupId(NodeGroupUid("parent4")),
"parent4",
"",
Expand All @@ -303,7 +316,7 @@ class TestMergeGroupProperties extends Specification {
true
)

val merged = MergeNodeProperties.checkPropertyMerge(List(parent1, parent2, priorize, parent4).map(_.toTarget), Map())
val merged = MergeNodeProperties.checkPropertyMerge(List(parent1, parent2, prioritize, parent4).map(_.toTarget), Map())
val expected = List(parent2, parent1).toH1("dns") :: Nil
merged must beRight(expected)
}
Expand All @@ -315,7 +328,7 @@ class TestMergeGroupProperties extends Specification {
merged must beRight(List(g.toG("foo")))
}

"global parameter are inherited and overriden by group and only one time" >> {
"global parameter are inherited and overridden by group and only one time" >> {
// empty properties, see if global is duplicated
val p2 = parent2.copy(properties = Nil)
val g = "bar".toConfigValue
Expand Down Expand Up @@ -397,7 +410,7 @@ class TestMergeGroupProperties extends Specification {
}
}

// checking that we get the overriden value for node and groups
// checking that we get the overridden value for node and groups
"preparing value for API" should {

"present only node value for override" in {
Expand Down