Fixes #24708: Groups node ids list in API is still exhaustive even with restricted tenant access #5608
Conversation
.../rudder-core/src/main/scala/com/normation/rudder/configuration/ConfigurationRepository.scala
Outdated
Show resolved
Hide resolved
| .foreach(g.serverList)(nodeFactRepo.get _) | ||
| .map(_.flatMap(_.map(_.id))) | ||
| .map(nodeIds => g.copy(serverList = nodeIds)) | ||
| }) |
There was a problem hiding this comment.
I was wondering if it wasn't quicker to get nodeIds from nodeFact (keySet, already filtered for the tenant) and intersect with g.serverlist. My intuition is that it's better, but my perf intuition are generaly not good. WDYT?
There was a problem hiding this comment.
That is a clever idea ! I agree that it may be not so performant as it is : the ZIO.foreach on a set may be as slow as O(n)...
There was a problem hiding this comment.
✅ Fixed in 567f916 (I expect it to be also at most O(n) but better memory-wise, as we don't store complete NodeFact objects)
|
WOUAH, in retrospective, I'm so happy to have missed that case on the original code... That PR looks like it was a super thrilling piece of cake to do 😅 GG, appart for the perf question, looks awesome. |
|
PR updated with a new commit |
2 similar comments
|
PR updated with a new commit |
|
PR updated with a new commit |
clarktsiory
force-pushed
the
bug_24708/groups_node_ids_list_in_api_is_still_exhaustive_even_with_restricted_tenant_access
branch
from
21d9541
to
710a1fd
Compare
|
PR rebased |
|
This PR is not mergeable to upper versions. |
|
OK, squash merging this PR |
…th restricted tenant access
fanf
force-pushed
the
bug_24708/groups_node_ids_list_in_api_is_still_exhaustive_even_with_restricted_tenant_access
branch
from
710a1fd
to
7da1f63
Compare
https://issues.rudder.io/issues/24708
Everything that needs to "get a group" from LDAP now needs to have a
QueryContext.It propagates in many directions, up to plugins, which need to be merged along this PR :