On 09/05/2016 07:46 PM, Fraser Tweedale wrote:
On Mon, Aug 29, 2016 at 06:39:58PM +0200, Martin Babinsky wrote:
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:

Thanks for your review, Martin.  Updated patch attached.  Comments
inline.

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?

It's ugly.  It is an effect of my desire to keep LWCA tracking code
where IMO it belongs: in cainstance module.

If you know a nicer way to conditionally get at contents of
cainstance module I'm happy to do it differently.  Otherwise I don't
think it's a showstopper.

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.

I changed it to use admin_conn.

+
+        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.

Thanks, fixed.

+                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.

Updated to use api.env.basedn.  I hit problems using ca_find and
connecting the ldap2 backend so I'm sticking with admin_conn, which
is working.

+            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.

OK.

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


--
Martin^3 Babinsky

Thanks, ACK.

Rebased and pushed to:

master: 08b768313020c45bfa82d67cd214afabf605f4b3
ipa-4-4: 99b0db0ebf090c9f60078e9ca9bf2aba665635f5

--
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