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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to