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

[Tomas Babej]

On 02/22/2016 07:15 PM, Martin Basti wrote:
> 
> 
> On 22.02.2016 17:05, Martin Basti wrote:
>>
>>
>> On 19.02.2016 15:02, 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.
>>>
>> Patch attached, patch with warning will land soon.
>>
>>
> Updated patches attached

During the RPM upgrade, the ipa-server-upgrade times out and leaves
directory server stopped.

Issues seem to be fixed after manual DS start&upgrade, but we should get
the upgrade during RPM cleanup sorted out to have a seamless experience.

Tomas