<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 22.02.2016 17:05, Martin Basti
wrote:<br>
</div>
<blockquote cite="mid:56CB31E3.7030002@redhat.com" type="cite">
<br>
<br>
On 19.02.2016 15:02, Alexander Bokovoy wrote:
<br>
<blockquote type="cite">On Fri, 19 Feb 2016, Petr Vobornik wrote:
<br>
<blockquote type="cite">On 02/19/2016 11:12 AM, Alexander
Bokovoy wrote:
<br>
<blockquote type="cite">On Fri, 19 Feb 2016, Martin Basti
wrote:
<br>
<blockquote type="cite">WIP patch attached
<br>
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/5665">https://fedorahosted.org/freeipa/ticket/5665</a>
<br>
<br>
</blockquote>
Comments inline.
<br>
<br>
<blockquote type="cite">+ # we need to run sidgen
task
<br>
+ sidgen_task_dn =
DN("cn=sidgen,cn=ipa-sidgen-task,cn=tasks,"
<br>
+ "cn=config")
<br>
+ sidgen_tasks_attr = {
<br>
+ "objectclass": ["top", "extensibleObject"],
<br>
+ "cn": ["sidgen"],
<br>
+ "delay": [0],
<br>
+ "nsslapd-basedn": [self.api.env.basedn],
<br>
+ }
<br>
</blockquote>
May be you are better to name this task more uniquely?
<br>
Something like 'cn=generate domain sid,cn=...'?
<br>
<br>
<blockquote type="cite">+
<br>
+ task_entry = ldap.make_entry(sidgen_task_dn,
<br>
+ **sidgen_tasks_attr)
<br>
+ try:
<br>
+ ldap.add_entry(task_entry)
<br>
+ except errors.DuplicateEntry:
<br>
+ self.log.debug("sidgen task already created")
<br>
+ else:
<br>
+ self.log.debug("sidgen task has been
created")
<br>
</blockquote>
There could be multiple tasks running in parallel, that's
why it could
<br>
be good to use a different and unique name.
<br>
<br>
<blockquote type="cite">+ # we have to check all
trusts domains which have been added
<br>
after the
<br>
+ # upgrade that caused bug was done.
<br>
+
<br>
+ base_dn = DN(self.api.env.container_adtrusts,
<br>
self.api.env.basedn)
<br>
+ trust_domain_entries, truncated =
ldap.find_entries(
<br>
+ base_dn=base_dn,
<br>
+ scope=ldap.SCOPE_ONELEVEL,
<br>
+ attrs_list=[attr_name, "cn"],
<br>
+ )
<br>
+
<br>
+ if truncated:
<br>
+ self.log.warning("update_sids: Search results
were
<br>
truncated")
<br>
+
<br>
+ for entry in trust_domain_entries:
<br>
+ if entry.single_value[attr_name] is None:
<br>
+ domain = entry.single_value["cn"]
<br>
+ self.log.error(
<br>
+ "Your trust to %s is broken. Please
re-create it
<br>
by "
<br>
+ "running 'ipa trust-add' again",
domain)
<br>
+
<br>
+ sysupgrade.set_upgrade_state('sidgen',
'update_sids', False)
<br>
+ return False, ()
<br>
</blockquote>
This part looks fine. Basically, a similar check needs to be
added to
<br>
trust_find, trust_show, and may be trust_add.
<br>
<br>
</blockquote>
<br>
Why trust-add?
<br>
<br>
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.
<br>
<br>
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.
<br>
</blockquote>
No. What is the value of trust-add if it says you 'Trust
established and
<br>
verified' while in reality it didn't? This specific issue is
very easy
<br>
to catch.
<br>
<br>
</blockquote>
Patch attached, patch with warning will land soon.
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
Updated patches attached<br>
</body>
</html>