[Freeipa-devel] [PATCH] - Add DRM to IPA

[Petr Viktorin]

On 07/16/2014 02:55 PM, Petr Viktorin wrote:
> On 07/14/2014 11:45 AM, Ade Lee wrote:
>> Hi all,
>>
>> I have rebased all the previous patches against master, and have
>> squashed them all into a single patch.
>> Its a large patch, but as many folks have already reviewed the
>> constituent precursor patches, most if it
>> should be familiar and easier to review.
>
> I think it would be nice to have the fixes that aren't related to DRM
> (e.g. style issues) separate, so the patch can be reverted easily. But,
> what's done is done. Thanks for the cleanup!
>
>> The main difference with what was specified before is that the DRM
>> database is installed as a subtree
>> to o=ipaca.  This means that no new replication agreements will be
>> needed to replicate DRM data.
>> Replication agreements set up for the Dogtag CA will automatically
>> replicate DRM data.
>>
>> In order for this patch to work, a new 10.2 build of Dogtag 10.2 is
>> needed - with specific changes to
>> allow the ability to install a database as a subtree of an existing
>> tree.  At this time, these
>> changes have not yet been checked into the dogtag source.   You can
>> obtain such a build from:
>>
>> http://copr.fedoraproject.org/coprs/vakwetu/dogtag/build/21936/
>>
>> Please review,
>
>
> I've started reading the code; here are my thoughts on the first part:
>
> In ipa-ca-install and ipa-replica-install, you only ever set
> "REPLICA_INFO_TOP_DIR" to None. In Python, "global" has module scope,
> it's not for the entire process. (And it's bad practice anyway.)
> You'll need to pass it around explicitly, perhaps store it in
> ReplicaConfig.
>
> It would be nice to use the admintool framework for the new script,
> instead of copying the old boilerplate code again. See e.g.
> ipa-server-certinstall for an example.
>
> Logging functions interpolate their arguments, so you should use e.g.
>      root_logger.critical("failed to configure %s instance %s",
> subsystem, e)
> instead of using the % operator.

Also, in e.g.
     root_logger.debug(
         'Unable to determine PIN for the Dogtag instance: %s' % str(e))
the "s" in "%s" means "convert to string", so explicit str() isn't 
necessary.

> installutils.create_replica_config: Any time you call sys.exit(), please
> log a message so we know why the log file ends.
>
> In ipa-drm-install, why do you read /etc/ipa/default.conf manually? The
> information is available in api.env once api is finalized.
>
> In ipa-server-install, do we want to display the message about backing
> up /root/drmcert.p12 even if DRM is not installed?
>
> Please use path constants from ipaplatform.base.paths, or add new ones
> if necessary, instead of DogtagInstance.{AGENT_P12_PATH,AGENT_P12_PATH}.
> In ipa-upgradeconfig, you hardcoded '/etc/named.keytab'. Please change
> that back to paths.NAMED_KEYTAB.
> Same in CAInstance.uninstall with "/usr/bin/pkiremove".
>
> In ipa-upgradeconfig.find_subject_base, the docstring should mention to
> DsInstance.find_subject_base's docstring rather than being a copy; we
> dont' want them getting out of sync.
>
> In DsInstance.find_subject_base, don't use a bare `except:`, at least do
> `except Exception:`, so you don't e.g. disable Ctrl+C.
>
> In CADSInstance.uninstall, you don't need the _running and _user_exists
> variables at all, just call the functions.
> Same in CAInstance.__create_ra_agent_db for _stdout, _stderr, _returncode.
> Same in CAInstance.uninstall for _user_exists.
>
> In DogtagInstance, stop_tracking_certificates was moved here from
> cainstance, but it no longer stops tracking ipaCert in HTTPD_ALIAS_DIR.
> Is that intended?
>
> In CAInstance.__init__, when calling DogtagInstance.__init__, consider
> using super, and naming the arguments for clarity:
>      super(CAInstance, self).__init__(
>          realm=realm,
>          subsystem="CA",
>          service_desc="certificate server",
>          dogtag_constants=dogtag_constants,
>      )
Same in DogtagInstance, DRMInstance.

> Since CAInstance inherits from DogtagInstance, the `enable`,
> `start_instance`, `stop_instance`, `http_proxy` methods that just call
> the superclass are unnecessary.
> IMO the comment from CAInstance.__enable was important, it should be in
> DogtagInstance.enable.
>
> In DogtagInstance.spawn_instance, it's not nice to modify lists the
> caller pased in. Take a copy of the nolog first. Ideally combine that
> with the conversion to tuple, and allow any sequence to be passed in:
>      nolog_list = tuple(nolog_list) + (self.admin_password,
> self.dm_password)
>
> In CAInstance.__create_ca_user, it's better to use functions instead of
> staticmethods, unless you'll want to override them in subclasses (which
> you're explicitly discouraging here, by using the double underscore).
> Anyway I don't get why you want these to be staticmethods.
>

dogtaginstance.py, drminstance.py: Avoid star imports, especially in new 
code. AFAICS you're only using root_logger from ipapython.ipa_log_manager.
Also consider using an class-specific logger:
     class DogtagInstance(...):
         def __init__(...):
             ...
             self.log = ipa_log_manager.log_mgr.get_logger(self)

         later:
             self.log.error("Unable to determine PIN for the Dogtag 
instance:", e)

About the "# noinspection PyBroadException" comments -- we already use 
pylint, I don't think we want instructions for other linters in the code.

In DogtagInstance.get_admin_cert:
     admin_cert = entry_attrs.get('usercertificate')[0]
This relies on the order of values returned by LDAP. If the choice of 
certificate doesn't matter here, leave a comment.

drminstance.py: Please remove the `if __name__ == '__main__':` section 
at the end. If this needs to be tested, provide a real test; if it's 
example usage put it in the docs/docstring; if it's a personal tool just 
keep it to yourself.

In DRMInstance.__init__, the docstring is wrong; __init__ doesn't return 
anything.

In dogtag.py, this change breaks the formatting:
> -    |staus                    |string         |cert_request_status|unicode [1]_     |
> +    |status                    |string         |cert_request_status|unicode [1]_     |

In drm.__init__, use `os.path.join(api.env.dot_ipa, 'alias')` instead of 
joining manually. There are also hardcoded paths, put them in the 
platform paths module.

-- 
Petr³