[Freeipa-devel] [PATCH] 0100 Track lightweight CAs on replica installation

[Martin Babinsky]

On 08/23/2016 08:40 AM, Fraser Tweedale wrote:
> Hi folks,
>
> Please review attached patch which fixes
> https://fedorahosted.org/freeipa/ticket/6019.
>
> Thanks,
> Fraser
>
>
>
Hi Fraser,

I have couple of comments:

1.)
-            for entry in lwcas:
-                self.server_track_lightweight_ca(entry)
+            try:
+                from ipaserver.install import cainstance
+ 
cainstance.add_lightweight_ca_tracking_requests(self.log, lwcas)
+            except Exception as e:
+                self.log.exception(
+                    "Failed to add lightweight CA tracking requests")

You are importing a server-side module in a basically client-side 
command which I don't like very much. Isn't there a possibility to use 
shared client-server module for this?

2.)
+    def __add_lightweight_ca_tracking_requests(self):
+        server_id = installutils.realm_to_serverid(api.env.realm)
+        dogtag_uri = 'ldapi://%%2fvar%%2frun%%2fslapd-%s.socket' % 
server_id
+        conn = ldap2.ldap2(api, ldap_uri=dogtag_uri)
+        is_already_connected = conn.isconnected()

Why use these connection setup shenanigans when you can use either 
api.Backend.ldap2 (IIRC this runs in server context so LDAPI should be a 
given) or even the service's own 'admin_conn' member.

+
+        if not is_already_connected:
+            try:
+                conn.connect(autobind=True)
+            except errors.PublicError as e:
+                self.log.error(
+                    "Cannot connect to LDAP to add "
+                        "lightweight CA tracking requests: %s",
+                    e
+                )
PEP8 error here, the second line of the message is misformatted.

+                return
+
+        try:
+            lwcas = conn.get_entries(
+                base_dn=ipautil.realm_to_suffix(api.env.realm),
+                filter='(objectclass=ipaca)',
+                attrs_list=['cn', 'ipacaid'],
+            )
I would rather use the result of api.Command.ca_find to fetch sub-CAs. 
Also, ipautil.realm_to_suffix is superseded by api.env.basedn to fetch 
search base.

+            add_lightweight_ca_tracking_requests(self.log, lwcas)
+        except errors.NotFound:
+            pass  # shouldn't happen, but don't fail if it does
I would add at least some debug message here.

+        finally:
+            if not is_already_connected:
+                conn.disconnect()
+


-- 
Martin^3 Babinsky