[Pki-devel] CLI for editing profiles

[Endi Sukma Dewata]

On 8/28/2014 12:30 AM, Fraser Tweedale wrote:
>> Do any of these InputStreams and OutputStreams require close()?
>> I think we need to think about refactoring the DBManager/ LDAPConnection
>> code so that the LDAPConnection implements Closeable.  Then we can
>> manage all of these resources using try-with-resources constructions,
>> and not have to worry about finally and nested finally blocks.
>>
>> This should probably be a separate patch, but it would be nice to do
>> that refactoring first, so that we could use the new cleaner code
>> structure here.
>
> LDAPConnection is provided by ldapjdk so I don't think we can modify
> that class.
>
> We could define a "wrapper" class that implements the desired
> behaviour.  It could contain an LDAPConnection and the factory and
> in its close() method return the connection to the factory.  The
> code that uses it could then just get the LDAPConnection out of the
> wrapper, resulting in few changes, and ILdapConnFactory (and its
> implementations) would learn a new method that returns one of these
> "wrapped" connections instead of a "bare" LDAPConnection.
>
> If this sounds like a reasonable approach, I will prototype this
> design.

I think to be a proper wrapper it should wrap the LDAP operations too 
(e.g. search(), add()) so we don't have to unwrap the original 
LDAPConnection. I'm not sure if we should implement this in Dogtag 
though. This change really belongs in the LDAPJDK itself.

>> Is there any synchronization required for the commit() for
>> LDAPConfigStore()?
>>
> The PropConfigStore load() and save() methods are synchronised.  I
> hope that my expectation that the LDAP server will serialize
> concurrent modifications to an entry is reasonably held.  Let me
> know if I am wrong about that, and suggested approach(es) to
> addressing it.

The code is probably fine for now. Later we probably want to improve it.

Given that the certProfileConfig attribute contains the entire profile 
properties, the chance that people overwrite each other's modification 
can be higher. A better solution is to separate the properties into 
separate attributes and only update the attributes that have changed.

We can also use REST's ETag to make sure we're not making modifications 
based on outdated data. This will only work if all changes are coming 
through REST though.

Some more comments:

17. In LDAPConfigStore.save() the writer.close() might not be called if 
there's an exception. I don't know how likely this may happen, but to be 
safe we should put close() in a finally block, or use try-with-resources.

18. In LDAPConfigStore.commit() the nested try-catch block might not be 
necessary. If the LDAPException is something other than NO_SUCH_OBJECT
it could throw an EBaseException at that point.

19. In ProfileSubsystem.init() while loading the profiles if one of the 
class ID's is invalid (e.g. due to typo) the loop will break and the 
subsystem initialization will fail. I'm not sure about the implication 
if that happens or how likely, but we probably should just disable the 
failed profiles and continue loading the rest. People will find the 
error when they try to use the profile but it's not available.

20. Similar issue with CAInstallerService.importProfiles(), what happens 
if one profile fails to import? This is probably less likely to happen 
since it's loading the default profiles.

21. The ProfileSubsystem.profileDN() probably should be called 
getProfileDN() or createProfileDN() to follow Java conventions.

22. In new code like CMSEngine.getDynSubsystemNames() we should use the 
light-weight ArrayList or LinkedList instead of Vector.

23. Since WarningListener.mEnabled is new, it shouldn't use the 
Hungarian notation.

-- 
Endi S. Dewata