Re: [Pki-devel] CLI for editing profiles

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?

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

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.

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.

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.

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();

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.

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

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.

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.

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.

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