[Pki-devel] CLI for editing profiles

Wed Nov 12 05:52:47 UTC 2014

On Thu, Oct 09, 2014 at 09:47:24AM -0500, Endi Sukma Dewata wrote:
> On 9/19/2014 2:46 AM, Fraser Tweedale wrote:
> >Third cut of the LDAP profiles feature.  All comments from previous
> >reviews have been addressed in this patchset.
> >
> >The switch from byte[] to Properties in the ProfileClient API (item
> >#13 from Endi's first review) is currently in a separte patch
> >(#0014) to make it clear what had to happen for this change, and to
> >make it easy to change or remove if there are problems.  If it's
> >preferred to squash it when acked, let me know (or it can stay
> >separate - I don't mind either way).
> >
> >Cheers,
> >
> >Fraser
> 
> 
> Sorry for the delay. I have some more comments below.
> 
> 
> Patch #4-3:
> 
> 1. What do you think about renaming classId into certProfileClassID? Is
> there a plan to use the classId for something other than profile?
> 
The classId represents a Java class.  It is not yet used elsewhere,
but if we move to finer-grained LDAP profile config schema it will
be used for the inputs/outputs/defaults also, therefore I will leave
it as-is.

> 2. Let's use real descriptions instead of generic 'CMS defined
> attribute/class', for example:
> * classId: Certificate profile class ID
> * certProfileConfig: Certificate profile configuration
> * certProfile: Certificate profile
> 
Will be adjusted in next patch revision.

> No major issue, but see comments about database upgrade below. ACK.
> 
> 
> Patch #5-3:
> 
> 1. The commit() parameter "createBackup" doesn't match the @param backup.
> 
Fixed the javadoc.

> Nothing major. ACK.
> 
> 
> Patch #6-3:
> 
> 1. The LDAP-based profile subsystem is fine, but it might not run on
> existing installation that still uses file-based profile. Are we going to
> (a) support both types and provide a migration tool that can be run at a
> later time, or (b) require the people to migrate to LDAP before they can use
> the new code?
> 
> Option (a) is more flexible, but we would need to keep both the old
> ProfileSubsystem and the new LDAPProfileSubsystem, and then switch the class
> when we migrate to LDAP. With (b) once the RPM is upgraded, the database has
> to be upgraded and the profiles have to be migrated before we can restart
> the server, otherwise the profile subsystem will not work. See also comments
> about profile migration tool below.
> 
Need to have a deeper discussion of the implications.  Will create a
new thread for this topic, or maybe we can discuss in next ipa-cs
team meeting.

> 2. If the default profile subsystem is changed to LDAP, we shouldn't need to
> copy the profile files anymore to the instance. Look for
> "pki_subsystem_profiles_path" in subsystem_layout.py.
> 
> 3. Are the profile.* properties in CS.cfg still used by any code (other than
> importProfiles()) if we use LDAP-based profile? If not, we probably should
> remove them from CS.cfg and move them into the corresponding profile files
> (just like the raw format), and use file list instead of profile.list during
> import.
> 
If we go with option b) above, this will clean things up a bit.  If
we go with option a) we either keep it or make more changes to move
the classid into the file-based representation.

> I'd suggest we use option (a), change the default to LDAP, and remove
> profile.* properties after we have the database upgrade and profile
> migration tool ready.
> 
> 
> Patch #7-3:
> 
> 1. By default the subsystems are enabled, so in setSubsystemEnabled() if
> enabled=true we can actually remove the PROP_ENABLED property to keep it
> consistent with other subsystems that are enabled by default.
> 
> Nothing major. ACK.
> 
> 
> Patch #8-3:
> 1. There's an unused variable:
> 
>     String className = info.getClassName();
> 
Removed it.

> Nothing major. ACK.
> 
> 
> Patch #9-3:
> 
> 1. The ca-profile-add --raw works, but the regular ca-profile-add throws an
> error. If this is an existing problem please open a ticket.
> 
This is a regression.  Will have it fixed in next patch revision.

> 2. The ca-profile-show --raw prepends a message to the raw output, for
> example:
> 
>   ---------------------
>   Profile "caAdminCert"
>   ---------------------
>   ...
> 
> I think the message should not be included in the raw output so it can be
> redirected into a file, for example:
> 
>   pki ca-profile-show caAdminCert --raw > caAdminCert.cfg
> 
Was included it for consistency with non-raw output.  Happy to see
it go.

> 3. On the server side we should use CMS.debug(e) to log exceptions instead
> of e.printStackTrace() so that the amount of logging can be controlled.
> 
OK

> 4. On the CLI side it would be better to throw an exception on execution
> error (not option error), or just don't catch the original exception, rather
> than printing an error message. The exception will be caught and displayed
> properly by the MainCLI, and we can see the full stack trace in verbose
> mode.
> 
> 5. The code that reads the raw profile data in ProfileAddCLI and
> ProfileModifyCLI probably can be refactored into
> ProfileData.loadRawProfile(). The ProfileCLI.readProfileFromFile() probably
> can also be moved into ProfileData.loadXMLProfile() for consistency.
> 
OK

> 6. The ca-profile-edit can also support both formats: XML and raw.
> 
> I think #1 and #2 should be fixed. The others can be addressed later.
> 
> 
> Patch #14-3:
> 
> 1. The ProfileResource interface still uses byte[]. I originally thought it
> will use Properties as well. Is that possible?
> 
> 2. Related to #1, the response content-type is application/xml, but we
> should be returning text/plain or application/octet-stream. I need to take a
> look at the REST framework to see how we can support it.
> 
> These issues require further investigation. The code itself is fine. ACK.
> 
I have found RESTeasy to be anything but.  Thanks for ACK; happy to
revisit later with someone who groks RESTeasy more than I.

> 
> Other comments:
> 
> 1. For database upgrade, we need to add the profile schema and the profile
> base entry into the database. We don't have a database upgrade framework
> yet, but I suppose we can prepare a script that will connect to the database
> and make the modifications. Later we can convert that script into an proper
> database upgrade scriptlet.
> 
> 2. For profile migration, we need to read profile files in the existing
> instance, add it to LDAP, and remove the profile files and the profile
> properties in CS.cfg. If it's a one-way migration the migration tool can be
> implemented as database upgrade scriptlet too. If it's two-way, it would
> have to be implemented as a stand-alone tool.
> 
> 3. Since these patches are targeted for 10.3, check with Matt/Ade to make
> sure the repo is branched properly before you push.
> 
> 4. The patches can be pushed separately or squashed, it doesn't really
> matter. Just make sure that each patch pushed contains a complete set of
> changes so the build doesn't break. This would be important if we need to
> cherry-pick or revert some patches later.
> 
> -- 
> Endi S. Dewata