[Pki-devel] CLI for editing profiles

Thu Nov 20 06:27:48 UTC 2014

Fourth cut of the feature.

Please review w.r.t. the issues below.

The big issue mentioned in the review but *not* dealt with in this
patchset is support for using file-based profiles OR LDAP-based
profiles.  This will involve copying the new ProfileSystem (as it
appears in the patches) to a new class ("LDAPProfileSubsystem" I
suppose), restoring the original ProfileSubsystem with the
file-based behaviour, providing config and mechanism for loading the
preferred variant of the subsystem, and reconciling any conflicts.
But the implementation of the LDAP-based version of ProfileSubsystem
won't be changed once these patches are ACKed.

(See also patch 0015-2 in the other thread)

Cheers,

Fraser

On Wed, Nov 12, 2014 at 03:52:47PM +1000, Fraser Tweedale wrote:
> 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
> 
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel