[Pki-devel] CLI for editing profiles

[Ade Lee]

On Fri, 2014-08-29 at 22:37 -0500, Endi Sukma Dewata wrote:
> 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.
> 
Perhaps, it looks though like the code that is invoked in releaseConn()
etc. has more to do with accounting and returning connections to the
connection pool - which I'm not sure is in LDAPJDK.

All the LDAPConnectionFactory stuff is in Dogtag code, so unless we're
thinking about using some LDAPConnectionFactory from the ldapjdk - which
is a much more invasive change, I think we should try to handle this in
a better way in the dogtag code.

I imagine the wrapper code looking something like:

try(ManagedLDAPConnection mconn = new ManagedLDAPConnection(ldapConnFactory)) {
	LDAPConnection conn = mconn.getLDAPConnection();
        ... do stuff with conn ..
}

When we get out of scope, mconn.close() will be called and will call ldapConnFactory.releaseConn().

I think thats cleaner and less error prone than what we currently do with try .. catch .. finally blocks.
We've had bugs in the past where releaseConn() was not called, causing problems.

Ade


> >> 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.
>