On 02/19/2016 03:02 PM, Alexander Bokovoy wrote: > On Fri, 19 Feb 2016, Petr Vobornik wrote: >> On 02/19/2016 11:12 AM, Alexander Bokovoy wrote: >>> On Fri, 19 Feb 2016, Martin Basti wrote: >>>> WIP patch attached >>>> >>>> https://fedorahosted.org/freeipa/ticket/5665 >>>> >>> Comments inline. >>> >>>> + # we need to run sidgen task >>>> + sidgen_task_dn = DN("cn=sidgen,cn=ipa-sidgen-task,cn=tasks," >>>> + "cn=config") >>>> + sidgen_tasks_attr = { >>>> + "objectclass": ["top", "extensibleObject"], >>>> + "cn": ["sidgen"], >>>> + "delay": [0], >>>> + "nsslapd-basedn": [self.api.env.basedn], >>>> + } >>> May be you are better to name this task more uniquely? >>> Something like 'cn=generate domain sid,cn=...'? >>> >>>> + >>>> + task_entry = ldap.make_entry(sidgen_task_dn, >>>> + **sidgen_tasks_attr) >>>> + try: >>>> + ldap.add_entry(task_entry) >>>> + except errors.DuplicateEntry: >>>> + self.log.debug("sidgen task already created") >>>> + else: >>>> + self.log.debug("sidgen task has been created") >>> There could be multiple tasks running in parallel, that's why it could >>> be good to use a different and unique name. >>> >>>> + # we have to check all trusts domains which have been added >>>> after the >>>> + # upgrade that caused bug was done. >>>> + >>>> + base_dn = DN(self.api.env.container_adtrusts, >>>> self.api.env.basedn) >>>> + trust_domain_entries, truncated = ldap.find_entries( >>>> + base_dn=base_dn, >>>> + scope=ldap.SCOPE_ONELEVEL, >>>> + attrs_list=[attr_name, "cn"], >>>> + ) >>>> + >>>> + if truncated: >>>> + self.log.warning("update_sids: Search results were >>>> truncated") >>>> + >>>> + for entry in trust_domain_entries: >>>> + if entry.single_value[attr_name] is None: >>>> + domain = entry.single_value["cn"] >>>> + self.log.error( >>>> + "Your trust to %s is broken. Please re-create it >>>> by " >>>> + "running 'ipa trust-add' again", domain) >>>> + >>>> + sysupgrade.set_upgrade_state('sidgen', 'update_sids', False) >>>> + return False, () >>> This part looks fine. Basically, a similar check needs to be added to >>> trust_find, trust_show, and may be trust_add. >>> >> >> Why trust-add? >> >> I'm not a big fan of cluttering existing commands(find, show, mod) with logic >> to fix one upgrade bug. But I understand a need to communicate it somehow. >> >> Would it make sense to move such logic to a separate command, e.g. >> trust-check/trust-verify? The command can do additional check in a future. > No. What is the value of trust-add if it says you 'Trust established and > verified' while in reality it didn't? This specific issue is very easy > to catch.
I think the point was that we are proposing and doing a non-trivial engineering effort to do warning for one-off bug that will be fixed in next bugfix release. I would agree if the check is more systematic, but doing a special LDAP search (for example) from now on because of this one-off bug does not seem to me as ideal path. -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code