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
