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 #23856: Impact of RBAC node filtering on compliance #5268

Conversation

fanf
Copy link
Member

@fanf fanf commented Dec 15, 2023

https://issues.rudder.io/issues/23856

So this PR brings several things:

Where is the compliance computation update to handle node tenants?

Well, there is none, because we did the things correctly: compliance computation takes a list of nodes and everything is node-centric. So we are just passing the correct list based on the user tenant, and most of the changes are just about "correctly weaving from web context to compliance computation the QueryContext to be able to have the list of node based on tenants".

The main change is in RuleApplicationStatusService.isApplied and RuleValService.buildRuleVal and in ComplianceAPIService (reportingService.findDirectiveNodeStatusReports) that now uses the restricted list of nodes.

Evolving user authz availability in requests

That PR (finally) allows to have the user node permissions correctly threaded in all request contexts, be them in UI or in API.
It needed mainly two things:

  • (yet another) change in CurrentUser so that we correctly set its value from spring security result in all context (normal session UI, comet/async case, stateless API requests)
  • a change in what is the standard authzToken in API: in place of just getting the actor, it now holds a full QueryContext with also NodeSecurityContext. The actual change is trivial and happens in webapp/sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/ApiAuthorization.scala: we already had the user at end with correct info, so just use them.

Most of the remaining changes are boring declaration of implicit val qc: QueryContext = authzToken.qc in all API that needs them.

Using MapView in more places

Our CoreNodeFactRepository#getAll was changed to use MapView in place of a new Map or something else in a previous commit to avoid a too big refactoring and help with performance.

In that PR, we see that is seems to work very well: we have a lot of cases where we were filtering on nodeInfoService.getAll result to just map values to know the node's isPolicyServer or policyMode.
By having the MapView, we avoid a lot of intermediary copies (which were not really needed).
There was also a lot of places where MapView was used so we are just removing more collection creation.

This is a rather impacting change, it touches a lot of place, but it is rather trivial : now that Scala clearly differentiate between Map and MapView, the compiler tells us where we need to concretize the view.

Switching from NodeInfoService to CoreNodeFact

This is rather easy a normalized. CoreNodeFact is a super-set of NodeInfo, so we don't miss any data.
At several places, I was able to reduce the refactoring need by relaxing the parameter: we were asking for Map[NodeId, NodeInfo] but really using only nodeInfo.isPolicyServer for example. I think it used to be done like that to avoid more collection duplication/transformation.
With the previous change about MapView, I was able to pass MapView[NodeId, Boolean] without new collection copy, and without the need to switch from NodeInfo to CoreNodeFact (but yes, I had to switch from NodeInfo to Boolean ;) )

@fanf
Copy link
Member Author

fanf commented Dec 15, 2023

PR updated with a new commit

@fanf fanf force-pushed the arch_23856/impact_of_rbac_node_filtering_on_compliance branch from 0b91289 to 2fabb1a Compare December 17, 2023 10:57
@VinceMacBuche
Copy link
Member

There is a conflict on currentUser (i guess PR with spring/lift)

@fanf fanf force-pushed the arch_23856/impact_of_rbac_node_filtering_on_compliance branch from 2fabb1a to b55b985 Compare December 22, 2023 09:32
@fanf fanf force-pushed the arch_23856/impact_of_rbac_node_filtering_on_compliance branch from b55b985 to b8e08e6 Compare December 22, 2023 09:33
@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit b8e08e6 into Normation:master Dec 22, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants