[Pki-devel] CLI for editing profiles

[Endi Sukma Dewata]

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