On 11/12/2012 04:02 PM, Simo Sorce wrote:
On Mon, 2012-11-12 at 13:30 +0100, Sumit Bose wrote:On Sat, Nov 10, 2012 at 10:05:36PM -0500, Simo Sorce wrote:This patch changes the way subdomain users are stored in the database.Thank you for the patch. I run couple of test and have not see an issue so far. But I have a couple of comments, see below.The reason for changing the way we do it is that the sysdb code, before the subdomain patches were added assumed a single domain per cache file. This assumption beled in many other interfaces including the way users are read and returned in the nss responder, as well as potentially how hbac and sudo handleAt least IPA HBAC is using the originalMemberOf attribute to find out about group memberships efficiently.YEah I know, in fact I had to add your patches (which I review and pushed) in order to make HBAC work.rules for checking if users are part of a rule. In order to make sure subdomain users are univocally recognized as such the safest way is to change how users are saved and always save subdomain users with sully qualified names.Is your plan to store the fully qualified name for users of the configured domain, too?No. My long term plan is to revisit this whole subdomain concept and see if we need to change anything. This patchset is a stopgap measure to avoid potential issues (and fix the actual group membership problem, but I am not entirely happy with as a final status. However we need a working fix for now, and this is the shortest path, any reconsideration of the database layout will require very careful consideration and design decisions that we'll need to discuss a bit.With this change we solve one of the most eveident issues we currently have where subdomain users are not listed fully wualified in group membership when they should.yes, this works in my tests with you patch now.The side effect of this change is that cache files need to be removed if the admin decides to change the formatting string for representing fully qualified users. An action like this has many other important consequences on the system so I think this limitation is perfectly reasonable.Wouldn't is be better to use a generic fully qualified name here and transform it to the configured style if needed?I thought about it, but we have 2 issues: 1) We do not have any reserve chars we can safely use to say: 'hey this is special do not return as is in fill_grent()' 2) the point of storing memberuid is to make fill_grent fast in the default case by not having to munge values. If we were to analyze every single entry there fill_grent() would be impacted more than I'd like to do on a short notice.I know you are afraid that it will cost too much performance in e.g. fill_members() but to evaluate this I've written a test for fill_members() to see where we are:It is just not performance it is correctness about all sysdb users. I wanted the least intrusive patch that made absolutely certain users were unique. Anything else I think should be deferred to a reconsideration of this whole business during 1.10 life cycle or later.Running suite(s): nss Testcase [0], members [1], fqdn [false], case-sensitive [false], body length [6], duration [0s 73us] Testcase [1], members [10], fqdn [false], case-sensitive [false], body length [60], duration [0s 12us] Testcase [2], members [100], fqdn [false], case-sensitive [false], body length [600], duration [0s 104us] Testcase [3], members [1000], fqdn [false], case-sensitive [false], body length [6000], duration [0s 512us] Testcase [4], members [10000], fqdn [false], case-sensitive [false], body length [60000], duration [0s 4713us] Testcase [5], members [100000], fqdn [false], case-sensitive [false], body length [949624], duration [0s 61037us] Testcase [6], members [1], fqdn [true], case-sensitive [false], body length [18], duration [0s 45us] Testcase [7], members [10], fqdn [true], case-sensitive [false], body length [180], duration [0s 22us] Testcase [8], members [100], fqdn [true], case-sensitive [false], body length [1800], duration [0s 120us] Testcase [9], members [1000], fqdn [true], case-sensitive [false], body length [18000], duration [0s 794us] Testcase [10], members [10000], fqdn [true], case-sensitive [false], body length [180000], duration [0s 7404us] Testcase [11], members [100000], fqdn [true], case-sensitive [false], body length [2149624], duration [0s 90789us] Testcase [12], members [1], fqdn [false], case-sensitive [true], body length [6], duration [0s 8us] Testcase [13], members [10], fqdn [false], case-sensitive [true], body length [60], duration [0s 12us] Testcase [14], members [100], fqdn [false], case-sensitive [true], body length [600], duration [0s 36us] Testcase [15], members [1000], fqdn [false], case-sensitive [true], body length [6000], duration [0s 149us] Testcase [16], members [10000], fqdn [false], case-sensitive [true], body length [60000], duration [0s 1321us] Testcase [17], members [100000], fqdn [false], case-sensitive [true], body length [949624], duration [0s 13968us] Testcase [18], members [1], fqdn [true], case-sensitive [true], body length [18], duration [0s 14us] Testcase [19], members [10], fqdn [true], case-sensitive [true], body length [180], duration [0s 29us] Testcase [20], members [100], fqdn [true], case-sensitive [true], body length [1800], duration [0s 86us] Testcase [21], members [1000], fqdn [true], case-sensitive [true], body length [18000], duration [0s 478us] Testcase [22], members [10000], fqdn [true], case-sensitive [true], body length [180000], duration [0s 4320us] Testcase [23], members [100000], fqdn [true], case-sensitive [true], body length [2149624], duration [0s 44809us] 100%: Checks: 1, Failures: 0, Errors: 0 I think the values are quite comfortable and would allow some extra processing in fill_members(). I plan to test if extracting name and domain from the DN stored in the member attribute would spoil those values of if it can be done efficiently.not just a performance issue, I would take correctness over performance every day given we have the mmap cache anyway. I am shooting for least intrusive patch at this time.This patch is in RFC status because I haven't dealt with database migrations to fix existing subdomain users. Would it be acceptable to simply remove all subdomain user entries on upgrade ?I would say yes. We only have to remember to give it a prominent place in the release notes to warn about the loss of cached password.Ok, I'll wait for more opinions on this specific point.
I'm OK with it given the circumstances. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
