Yair Zaslavsky has posted comments on this change. Change subject: aaa: Fix sync ......................................................................
Patch Set 4: (10 comments) http://gerrit.ovirt.org/#/c/28561/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java: Line 146 Line 147 Line 148 Line 149 Line 150 > if you do first datbase groups then user, you fetch more groups than first Actually I disagree. I will explain - Let's say in db you have g1,g2. Let's say u1 is member of g2, g4. g2 is member of g5. All are in namespace "xyz". Now, with my suggestion what happens is that dbGroups["xyz"] will contain at first g1,g2. This will also be the content of the cache. Then after you fetch for u1 (first level only), the content of the catch is g1,g2,g4. Then you fetch recursively for the 3 groups. Why do you fetch here more? You would have fetched recurisvely for g1 (that the user is not a member of) in your suggestion as well, no? Line 32: import org.ovirt.engine.core.utils.timer.OnTimerMethodAnnotation; Line 33: import org.ovirt.engine.core.utils.timer.SchedulerUtilQuartzImpl; Line 34: Line 35: public class DbUserCacheManager { Line 36: private static final String EVERYONE_GROUP_ID = "eee00000-0000-0000-0000-123456789eee"; > is this engine specific uuid? Yes, "EVERYONE" is not a real group in perspective of our extensions, therefore I had to diffrentiate. Line 37: private static final Log log = LogFactory.getLog(DbUserCacheManager.class); Line 38: private static final DbUserCacheManager _instance = new DbUserCacheManager(); Line 39: private boolean initialized = false; Line 40: private Map<String, List<DbGroup>> dbGroups = new HashMap<>(); Line 38: private static final DbUserCacheManager _instance = new DbUserCacheManager(); Line 39: private boolean initialized = false; Line 40: private Map<String, List<DbGroup>> dbGroups = new HashMap<>(); Line 41: Line 42: // private final Map<String, Map<String, DbGroup>> groupsMap = new HashMap<>(); > - Done Line 43: Line 44: public static DbUserCacheManager getInstance() { Line 45: return _instance; Line 46: } Line 85: DbUserDAO dao = DbFacade.getInstance().getDbUserDao(); Line 86: Line 87: // Retrieve all the users from the database: Line 88: List<DbUser> dbUsers = dao.getAll(); Line 89: dbGroups.clear(); > if you clear it then it probably need to be a parameter and not object stat Well, it is possible, I will pass it as parameters to the next to the next methods. Line 90: for (DbGroup dbGroup : DbFacade.getInstance().getDbGroupDao().getAll()) { Line 91: MultiValueMapUtils.addToMap(dbGroup.getNamespace(), dbGroup, dbGroups); Line 92: } Line 93: // Classify the users by directory. Note that the resulting map may have an entry with a null key, that Line 87: // Retrieve all the users from the database: Line 88: List<DbUser> dbUsers = dao.getAll(); Line 89: dbGroups.clear(); Line 90: for (DbGroup dbGroup : DbFacade.getInstance().getDbGroupDao().getAll()) { Line 91: MultiValueMapUtils.addToMap(dbGroup.getNamespace(), dbGroup, dbGroups); > I may miss something bug of dbGroups is shared among all, why do we need to No, at this point I construct the multi value map with all the users. I would like to have a map of users per namespaces. Line 92: } Line 93: // Classify the users by directory. Note that the resulting map may have an entry with a null key, that Line 94: // corresponds to the users whose directory has been removed from the configuration. Line 95: Map<String, List<DbUser>> authzToPrincipalMap = new HashMap<>(); Line 152: directoryUsers = Collections.emptyList(); Line 153: } Line 154: Line 155: Line 156: > - Done Line 157: // Build a map of users indexed by directory id to simplify the next step where we want to find the directory Line 158: // user corresponding to each database user: Line 159: Map<String, DirectoryUser> index = new HashMap<>(); Line 160: Set<DirectoryGroup> groupsToFetch = new HashSet<>(); Line 215: Map<String, DirectoryGroup> fetchedGroups, Line 216: Collection<DirectoryGroup> groupsToFetch Line 217: ) { Line 218: Line 219: if ((authz.getContext().<Long> get(Authz.ContextKeys.CAPABILITIES, 0L) & Authz.Capabilities.RECURSIVE_GROUPS_RESOLVING) != 0L) { > == 0? the condition is probably reversed as you need to flat only if it doe The condition is fine IMHO, please notice the block for the case the capability exists is shorter - the reason is that if the capability exist - you already have the "fetchedGroups" at hand - they are the "groupsToFetch" you passed as arguments. Maybe you don't like the name of the constant and it misled you? The meaning of the capablities constant , or at least this is what I had in mind is - "the provider performs recursive resolving to the groups, no need to pass the relevant flag to QUERY_FLAGS when querying for groups). Line 220: for (DirectoryGroup group : groupsToFetch) { Line 221: fetchedGroups.put(group.getId(), group); Line 222: } Line 223: } else { Line 223: } else { Line 224: for (DirectoryGroup group : groupsToFetch) { Line 225: if (!fetchedGroups.containsKey(group.getId())) { Line 226: Set<DirectoryGroup> flatGroups = new HashSet<>(); Line 227: DirectoryGroup fetchedGroup = AuthzUtils.findGroupById(authz, group.getNamespace(), group.getId(), true, true); > we do not need two flags now... only if fetch groups or not. Done Line 228: fetchedGroups.put(fetchedGroup.getId(), fetchedGroup); Line 229: DirectoryUtils.flatGroups(flatGroups, fetchedGroup.getGroups()); Line 230: for (DirectoryGroup fetchedMemberOf : flatGroups) { Line 231: fetchedGroups.put(fetchedMemberOf.getId(), fetchedMemberOf); Line 231: fetchedGroups.put(fetchedMemberOf.getId(), fetchedMemberOf); Line 232: } Line 233: } Line 234: } Line 235: } > I do not understand the algorithm.... either you do recursive or you do a l ok, please go over my comments again, if not clear, i will change, in the meanwhile i will change some of the variables names so it will be more clear, for example, fetchedGroups == cache. Line 236: Line 237: } Line 238: Line 239: /** http://gerrit.ovirt.org/#/c/28561/4/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthz.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthz.java: Line 123: Authz.PrincipalRecord.ID, Line 124: configuration.getProperty("config.authz.user.id") Line 125: ).mput( Line 126: Authz.PrincipalRecord.GROUPS, Line 127: Collections.emptyList() > no need, you should assume no groups if no key Ok. Line 128: ); Line 129: Line 130: } Line 131: -- To view, visit http://gerrit.ovirt.org/28561 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id49b51517a967c7a83e8e73f52181673baa31700 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
