[Freeipa-devel] [PATCH 0416][WIP] fix broken configuration of sidgen and extdom plugins

[Alexander Bokovoy]

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.

-- 
/ Alexander Bokovoy