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

[Petr Vobornik]

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.
-- 
Petr Vobornik