<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 19:15, Martin Basti
wrote:<br>
</div>
<blockquote cite="mid:56CB5052.5040803@redhat.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<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 moz-do-not-send="true" 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>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
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<br>
<br>
Patches attached.<br>
<br>
I will look on ldapsearch timeouts after restarting DS tomorrow.<br>
</body>
</html>