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