[Freeipa-devel] [PATCH 0416][WIP] fix broken configuration of sidgen and extdom plugins
Martin Kosek
mkosek at redhat.com
Fri Feb 19 14:06:32 UTC 2016
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.
More information about the Freeipa-devel
mailing list