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

[Martin Basti]

On 22.02.2016 20:11, Martin Basti wrote:
>
>
> On 22.02.2016 19:15, 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
>>
>>
> I fixed except clause in 416, added patch with user warnings, IMO to 
> have separate search is the cleanest solution here, command is not 
> used often, I would like to avoid any hacks in find and show command
>
> Patches attached.
>
> I will look on ldapsearch timeouts after restarting DS tomorrow.
>
>
Updated patches attached + new patch fixing timeout error

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160223/f0742bbb/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0416.4-upgrade-fix-config-of-sidgen-and-extdom-plugins.patch
Type: text/x-patch
Size: 10846 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160223/f0742bbb/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0417.4-trusts-use-ipaNTTrustPartner-attribute-to-detect-tru.patch
Type: text/x-patch
Size: 2611 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160223/f0742bbb/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0418.4-Warn-user-if-trust-is-broken.patch
Type: text/x-patch
Size: 3832 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160223/f0742bbb/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0419.4-fix-upgrade-wait-for-proper-DS-socket-after-DS-resta.patch
Type: text/x-patch
Size: 1335 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160223/f0742bbb/attachment-0003.bin>