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 #23935: API for node group compliance #5272

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Dec 19, 2023

https://issues.rudder.io/issues/23935

This adds two endpoints that return compliance computation by node group :

  • one /compliance/groups/{id} for global compliance by taking into account any rule that apply to nodes within the group
  • another /compliance/groups/{id}/target for targeted compliance by taking into account only rules that target the group (using include/exclude targeting)

In the yaml tests I used concrete examples (a simple one and a more complex one) and the result correspond to what is expected with a some caveats :

  • lists are not sorted, and this could cause some issues because sometimes we use the Set collection. Maybe we should sort by 'compliance', from the "worst" ones to best ones ? Or by the name of the node/rule ? I left them as is so there is no specific order
  • when there are 'skipped' reports, with overrides, we remap them to "No report" with the NoAnswer report type, which could be confusing (but may be better than omitting it completely). Later we could handle that case specifically to display which rule/directive have been overriden

val targetedNodeIds = targetedNodeIdsByRuleId(rule.id)
val isRuleTargetingGroup =
RuleTarget.merge(rule.targets).includes(GroupTarget(nodeGroup.id)) // targeted compliance
(isGlobalCompliance || isRuleTargetingGroup) && targetedNodeIds.exists(currentGroupNodeIds.contains(_))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(:exploding_head:)

This adds the check for isRuleTargetingGroup only when compliance is not global (i.e. targeted compliance)

There are multiple of occurences of :

(condA || condB) && condC 

in this PR and the way to read them is : "I always want to check condC", and "I only want to check condB when not condA"

@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

3 similar comments
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@VinceMacBuche
Copy link
Member

@clarktsiory Tests are not ok !

I wondering about the NoAnswer case. I need to look if this is the same behavior on nodes/rules/directives

@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@clarktsiory clarktsiory force-pushed the ust_23935/api_for_node_group_compliance branch from c61cb57 to 26468b4 Compare December 27, 2023 14:47
@clarktsiory
Copy link
Contributor Author

I rebased on master (some changes from the use of NodeFactRepository instead of NodeInfoService)

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/5272
-- Your faithful QA
Kant merge: "Happiness is not an ideal of reason, but of imagination."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/78069/console)

@VinceMacBuche
Copy link
Member

OK, squash merging this PR

@VinceMacBuche VinceMacBuche force-pushed the ust_23935/api_for_node_group_compliance branch from 26468b4 to eea787d Compare December 29, 2023 09:31
@VinceMacBuche VinceMacBuche merged commit eea787d into Normation:master Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants