Re: [Pki-devel] CLI for editing profiles

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(a)redhat.com > https://www.redhat.com/mailman/listinfo/pki-devel