[PATCH] pki-ftweedal-0015-Monitor-database-for-changes-to-LDAP-profiles.patch
This is the first cut of the LDAP profile change monitoring. It depends on patches 0004..0009 and 0014 (https://www.redhat.com/archives/pki-devel/2014-September/msg00052.html). To summarise the implementation: a separate thread carries out a persistent LDAP search and calls back into the ProfileSubsystem when changes occur. I haven't had much experience with persistent searches or multithreaded programming in Java, so eyeballs familiar with those areas are needed. I haven't yet tested with changes replicating between clones (a task for tomorrow) but I wanted to get the patch on list for feedback as early as possible. Cheers, Fraser
On Mon, Sep 22, 2014 at 05:36:26PM +1000, Fraser Tweedale wrote:
This is the first cut of the LDAP profile change monitoring. It depends on patches 0004..0009 and 0014 (https://www.redhat.com/archives/pki-devel/2014-September/msg00052.html).
To summarise the implementation: a separate thread carries out a persistent LDAP search and calls back into the ProfileSubsystem when changes occur. I haven't had much experience with persistent searches or multithreaded programming in Java, so eyeballs familiar with those areas are needed.
I haven't yet tested with changes replicating between clones (a task for tomorrow) but I wanted to get the patch on list for feedback as early as possible.
Further to earlier email, I tested profile change replication between clones and it is working well.
Cheers,
Fraser
From ff380d53abb1c12a1808c731b8542d4e4e9e65d1 Mon Sep 17 00:00:00 2001 From: Fraser Tweedale <ftweedal@redhat.com> Date: Mon, 22 Sep 2014 03:22:57 -0400 Subject: [PATCH] Monitor database for changes to LDAP profiles.
Use a persistent query to monitor the database for changes to LDAP profiles, and update the contents of the ProfileSubsystem according to the changes (Add/Modify/Delete) that occur.
The monitoring occurs within its own thread. --- .../netscape/cmscore/profile/ProfileSubsystem.java | 190 ++++++++++++++++++--- 1 file changed, 168 insertions(+), 22 deletions(-)
diff --git a/base/server/cmscore/src/com/netscape/cmscore/profile/ProfileSubsystem.java b/base/server/cmscore/src/com/netscape/cmscore/profile/ProfileSubsystem.java index ca0e09b785877da2a98fd2dd54977b8d3ebeaa24..8de92f5bb6c9bc3e9a5c430aa7c1269077f5148b 100644 --- a/base/server/cmscore/src/com/netscape/cmscore/profile/ProfileSubsystem.java +++ b/base/server/cmscore/src/com/netscape/cmscore/profile/ProfileSubsystem.java @@ -17,15 +17,20 @@ // --- END COPYRIGHT BLOCK --- package com.netscape.cmscore.profile;
+import java.lang.Thread; import java.util.Enumeration; import java.util.Hashtable; import java.util.Vector;
import netscape.ldap.LDAPAttribute; import netscape.ldap.LDAPConnection; +import netscape.ldap.LDAPControl; import netscape.ldap.LDAPEntry; import netscape.ldap.LDAPException; +import netscape.ldap.LDAPSearchConstraints; import netscape.ldap.LDAPSearchResults; +import netscape.ldap.controls.LDAPEntryChangeControl; +import netscape.ldap.controls.LDAPPersistSearchControl;
import com.netscape.certsrv.apps.CMS; import com.netscape.certsrv.base.EBaseException; @@ -55,6 +60,8 @@ public class ProfileSubsystem implements IProfileSubsystem {
private ILdapConnFactory dbFactory;
+ private Monitor monitor; + /** * Retrieves the name of this subsystem. */ @@ -81,14 +88,16 @@ public class ProfileSubsystem implements IProfileSubsystem { throws EBaseException { CMS.debug("ProfileSubsystem: start init");
+ if (monitor != null) { + monitor.stopMonitoring(); + monitor = null; + } + // (re)init member collections mProfileIds = new Vector<String>(); mProfiles = new Hashtable<String, IProfile>(); mProfileClassIds = new Hashtable<String, String>();
- IPluginRegistry registry = (IPluginRegistry) - CMS.getSubsystem(CMS.SUBSYSTEM_REGISTRY); - IConfigStore cs = CMS.getConfigStore(); IConfigStore dbCfg = cs.getSubStore("internaldb"); dbFactory = CMS.getLdapBoundConnFactory(); @@ -115,26 +124,10 @@ public class ProfileSubsystem implements IProfileSubsystem { dn, LDAPConnection.SCOPE_ONE, "(objectclass=*)", attrs, false);
while (ldapProfiles.hasMoreElements()) { - String id = "<unknown>"; try { - LDAPEntry ldapProfile = ldapProfiles.next(); - - id = (String) - ldapProfile.getAttribute("cn").getStringValues().nextElement(); - - String classid = (String) - ldapProfile.getAttribute("classId").getStringValues().nextElement(); - - IPluginInfo info = registry.getPluginInfo("profile", classid); - if (info == null) { - CMS.debug("Error loading profile: No plugins for type : profile, with id " + classid); - } else { - CMS.debug("Start Profile Creation - " + id + " " + classid + " " + info.getClassName()); - createProfile(id, classid, info.getClassName()); - CMS.debug("Done Profile Creation - " + id); - } + readProfile(ldapProfiles.next()); } catch (LDAPException e) { - CMS.debug("Error reading profile '" + id + "'; skipping."); + CMS.debug("Error retrieving profile; skipping."); } } } catch (LDAPException e) { @@ -154,13 +147,53 @@ public class ProfileSubsystem implements IProfileSubsystem {
CMS.debug("Registered Confirmation - " + id); } + + monitor = new Monitor(this, dn, dbFactory); + monitor.start(); + } + + /** + * Read the given LDAPEntry into the profile subsystem. + */ + private void readProfile(LDAPEntry ldapProfile) { + IPluginRegistry registry = (IPluginRegistry) + CMS.getSubsystem(CMS.SUBSYSTEM_REGISTRY); + + String profileId = "<unknown>"; + profileId = (String) + ldapProfile.getAttribute("cn").getStringValues().nextElement(); + + String classId = (String) + ldapProfile.getAttribute("classId").getStringValues().nextElement(); + + IPluginInfo info = registry.getPluginInfo("profile", classId); + if (info == null) { + CMS.debug("Error loading profile: No plugins for type : profile, with classId " + classId); + } else { + try { + CMS.debug("Start Profile Creation - " + profileId + " " + classId + " " + info.getClassName()); + createProfile(profileId, classId, info.getClassName()); + CMS.debug("Done Profile Creation - " + profileId); + } catch (EProfileException e) { + CMS.debug("Error creating profile '" + profileId + "'; skipping."); + } + } }
/** * Creates a profile instance. + * + * createProfile could theoretically be called simultaneously + * with the same profileId from Monitor and ProfileService. + * Therefore this method is synchronized and forgetProfile() is + * called prior to creating the profile, so that the profile + * will not appear twice. + * */ - public IProfile createProfile(String id, String classid, String className) + public synchronized IProfile createProfile(String id, String classid, String className) throws EProfileException { + forgetProfile(id); + try { String[] objectClasses = {"top", "certProfile"}; LDAPAttribute[] createAttrs = { @@ -211,11 +244,31 @@ public class ProfileSubsystem implements IProfileSubsystem { } }
+ forgetProfile(id); + } + + /** + * Forget a profile without deleting it from the database. + * + * This method is used when the profile change monitor receives + * notification that a profile was deleted. + */ + private void forgetProfile(String id) { mProfileIds.removeElement(id); mProfiles.remove(id); mProfileClassIds.remove(id); }
+ private void forgetProfile(LDAPEntry entry) { + String profileId = (String) + entry.getAttribute("cn").getStringValues().nextElement(); + if (profileId == null) { + CMS.debug("forgetProfile: error retrieving cn (profileId) from LDAPEntry"); + } else { + forgetProfile(profileId); + } + } + /** * Notifies this subsystem if owner is in running mode. */ @@ -229,6 +282,8 @@ public class ProfileSubsystem implements IProfileSubsystem { * <P> */ public void shutdown() { + monitor.stopMonitoring(); + monitor = null; mProfileIds.clear(); mProfiles.clear(); mProfileClassIds.clear(); @@ -348,4 +403,95 @@ public class ProfileSubsystem implements IProfileSubsystem { } return "cn=" + id + ",ou=certificateProfiles,ou=ca," + basedn; } + + private class Monitor extends Thread { + private volatile boolean stopped = false; + private ProfileSubsystem ps; + private String dn; + private ILdapConnFactory dbFactory; + + public Monitor(ProfileSubsystem ps, String dn, ILdapConnFactory dbFactory) { + setName("profileChangeMonitor"); + this.ps = ps; + this.dn = dn; + this.dbFactory = dbFactory; + } + + public void stopMonitoring() { + stopped = true; + } + + public void run() { + int op = LDAPPersistSearchControl.ADD + | LDAPPersistSearchControl.MODIFY + | LDAPPersistSearchControl.DELETE + | LDAPPersistSearchControl.MODDN; + LDAPPersistSearchControl persistCtrl = + new LDAPPersistSearchControl(op, true, true, true); + + LDAPConnection conn; + try { + conn = dbFactory.getConn(); + } catch (ELdapException e) { + CMS.debug("Profile change monitor: failed to get LDAPConnection"); + return; + } + try { + LDAPSearchConstraints cons = conn.getSearchConstraints(); + cons.setServerControls(persistCtrl); + cons.setBatchSize(1); + cons.setServerTimeLimit(0 /* seconds */); + LDAPSearchResults results = conn.search( + dn, LDAPConnection.SCOPE_ONE, "(objectclass=*)", + null, false, cons); + while (!stopped && results.hasMoreElements()) { + LDAPEntry entry = results.next(); + LDAPEntryChangeControl changeControl = null; + for (LDAPControl control : results.getResponseControls()) { + if (control instanceof LDAPEntryChangeControl) { + changeControl = (LDAPEntryChangeControl) control; + break; + } + } + CMS.debug("Profile change monitor: Processed change controls."); + if (changeControl != null) { + int changeType = changeControl.getChangeType(); + switch (changeType) { + case LDAPPersistSearchControl.ADD: + CMS.debug("Profile change monitor: ADD"); + readProfile(entry); + break; + case LDAPPersistSearchControl.DELETE: + CMS.debug("Profile change monitor: DELETE"); + forgetProfile(entry); + break; + case LDAPPersistSearchControl.MODIFY: + CMS.debug("Profile change monitor: MODIFY"); + forgetProfile(entry); + readProfile(entry); + break; + case LDAPPersistSearchControl.MODDN: + CMS.debug("Profile change monitor: MODDN"); + CMS.debug("Profile change monitor: MODDN shouldn't happen; ignoring."); + break; + default: + CMS.debug("Profile change monitor: unknown change type: " + changeType); + break; + } + } else { + CMS.debug("Profile change monitor: no LDAPEntryChangeControl in result."); + } + } + } catch (LDAPException e) { + CMS.debug("Profile change monitor: Caught exception: " + e.toString()); + } finally { + CMS.debug("Profile change monitor: stopping."); + try { + dbFactory.returnConn(conn); + } catch (Exception e) { + CMS.debug("Profile change monitor: Error releasing the LDAPConnection" + e.toString()); + } + } + } + } } -- 1.9.3
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
On 10/1/2014 1:42 AM, Fraser Tweedale wrote:
On Mon, Sep 22, 2014 at 05:36:26PM +1000, Fraser Tweedale wrote:
This is the first cut of the LDAP profile change monitoring. It depends on patches 0004..0009 and 0014 (https://www.redhat.com/archives/pki-devel/2014-September/msg00052.html).
To summarise the implementation: a separate thread carries out a persistent LDAP search and calls back into the ProfileSubsystem when changes occur. I haven't had much experience with persistent searches or multithreaded programming in Java, so eyeballs familiar with those areas are needed.
I haven't yet tested with changes replicating between clones (a task for tomorrow) but I wanted to get the patch on list for feedback as early as possible.
Further to earlier email, I tested profile change replication between clones and it is working well.
Some comments: 1. The MODDN operation can potentially happen if someone renames the profile in LDAP directly, and that's easier to do with LDAP-based profiles. I think we should handle this case too. 2. If the LDAP server is restarted the Monitor will stop working and subsequent changes are not detected. The Monitor should automatically reconnect in case the LDAP connection is interrupted. 3. Since the changesOnly is set to true, there's a possibility that changes that happens after initial profile loading and before the Monitor starts will not be detected. To avoid this problem, the changesOnly should be false, and the initial profile loading should be done in the Monitor as well. 4. The following assignment is redundant because the value is overwritten by the next line: String profileId = "<unknown>"; 5. When receiving a MODIFY operation the Monitor calls forgetProfile(). It's probably redundant because readProfile() calls createProfile() which calls forgetProfile() too. 6. Generally it's recommended to implement Runnable instead of extending Thread. The Monitor can be merged into ProfileSubsystem. I think #1, #2, #3 are important to fix because there is no mechanism to refresh the profiles in memory other than by restarting the server, so we cannot miss any changes in the database. -- Endi S. Dewata
Thanks Endi, you picked up several subtle problems in your review. I addressed all the issues in the new patch (attached). Fraser On Thu, Oct 09, 2014 at 09:47:29AM -0500, Endi Sukma Dewata wrote:
On 10/1/2014 1:42 AM, Fraser Tweedale wrote:
On Mon, Sep 22, 2014 at 05:36:26PM +1000, Fraser Tweedale wrote:
This is the first cut of the LDAP profile change monitoring. It depends on patches 0004..0009 and 0014 (https://www.redhat.com/archives/pki-devel/2014-September/msg00052.html).
To summarise the implementation: a separate thread carries out a persistent LDAP search and calls back into the ProfileSubsystem when changes occur. I haven't had much experience with persistent searches or multithreaded programming in Java, so eyeballs familiar with those areas are needed.
I haven't yet tested with changes replicating between clones (a task for tomorrow) but I wanted to get the patch on list for feedback as early as possible.
Further to earlier email, I tested profile change replication between clones and it is working well.
Some comments:
1. The MODDN operation can potentially happen if someone renames the profile in LDAP directly, and that's easier to do with LDAP-based profiles. I think we should handle this case too.
2. If the LDAP server is restarted the Monitor will stop working and subsequent changes are not detected. The Monitor should automatically reconnect in case the LDAP connection is interrupted.
3. Since the changesOnly is set to true, there's a possibility that changes that happens after initial profile loading and before the Monitor starts will not be detected. To avoid this problem, the changesOnly should be false, and the initial profile loading should be done in the Monitor as well.
4. The following assignment is redundant because the value is overwritten by the next line:
String profileId = "<unknown>";
5. When receiving a MODIFY operation the Monitor calls forgetProfile(). It's probably redundant because readProfile() calls createProfile() which calls forgetProfile() too.
6. Generally it's recommended to implement Runnable instead of extending Thread. The Monitor can be merged into ProfileSubsystem.
I think #1, #2, #3 are important to fix because there is no mechanism to refresh the profiles in memory other than by restarting the server, so we cannot miss any changes in the database.
-- Endi S. Dewata
On 11/20/2014 1:32 PM, Fraser Tweedale wrote:
Thanks Endi, you picked up several subtle problems in your review. I addressed all the issues in the new patch (attached).
A few more issues: 1. I have not actually tested this, but suppose the LDAP server is running on a separate machine, then the network is down, and during that time a profile is removed from the database. When the network is restored, the ProfileSubsystem will reload all remaining profiles, but it will still have the profile that was deleted in its memory. I think the ProfileSubsystem should clear its memory before running the persistent search. 2. In ProfileSubsystem.createProfileDN() the basedn is no longer used so it can be removed. 3. If the LDAP server is stopped the debug log says it's retrying in 5 seconds, but it's actually trying every second. I think #1 should be fixed, but the others are very trivial to fix too. -- Endi S. Dewata
On Wed, Dec 03, 2014 at 01:59:40PM +0700, Endi Sukma Dewata wrote:
On 11/20/2014 1:32 PM, Fraser Tweedale wrote:
Thanks Endi, you picked up several subtle problems in your review. I addressed all the issues in the new patch (attached).
A few more issues:
1. I have not actually tested this, but suppose the LDAP server is running on a separate machine, then the network is down, and during that time a profile is removed from the database. When the network is restored, the ProfileSubsystem will reload all remaining profiles, but it will still have the profile that was deleted in its memory. I think the ProfileSubsystem should clear its memory before running the persistent search.
2. In ProfileSubsystem.createProfileDN() the basedn is no longer used so it can be removed.
3. If the LDAP server is stopped the debug log says it's retrying in 5 seconds, but it's actually trying every second.
I think #1 should be fixed, but the others are very trivial to fix too.
-- Endi S. Dewata
New patch attached. All issues have been addressed.
On 3/30/2015 2:27 AM, Fraser Tweedale wrote:
1. I have not actually tested this, but suppose the LDAP server is running on a separate machine, then the network is down, and during that time a profile is removed from the database. When the network is restored, the ProfileSubsystem will reload all remaining profiles, but it will still have the profile that was deleted in its memory. I think the ProfileSubsystem should clear its memory before running the persistent search.
2. In ProfileSubsystem.createProfileDN() the basedn is no longer used so it can be removed.
3. If the LDAP server is stopped the debug log says it's retrying in 5 seconds, but it's actually trying every second.
I think #1 should be fixed, but the others are very trivial to fix too.
New patch attached. All issues have been addressed.
Let's say a profile is modified then deleted. These operations are replicated to a clone which is monitoring the changes using persistent search. When the clone receives the modify operation it will call readProfile(), which will call createProfile(), which will create an LDAPConfigStore for that profile. When LDAPConfigStore is created, it will read the profile from the database using a separate query. The problem is, at that point it's unclear whether the profile still exists. The current code might ignore the error if the entry doesn't exist, but in general I don't think the code should make a separate query to retrieve the profile info because it could lead to inconsistencies. It should have used the LDAP entry returned by the persistent search that corresponds to the operation being processed. -- Endi S. Dewata
On Mon, Apr 06, 2015 at 08:27:05PM -0500, Endi Sukma Dewata wrote:
On 3/30/2015 2:27 AM, Fraser Tweedale wrote:
1. I have not actually tested this, but suppose the LDAP server is running on a separate machine, then the network is down, and during that time a profile is removed from the database. When the network is restored, the ProfileSubsystem will reload all remaining profiles, but it will still have the profile that was deleted in its memory. I think the ProfileSubsystem should clear its memory before running the persistent search.
2. In ProfileSubsystem.createProfileDN() the basedn is no longer used so it can be removed.
3. If the LDAP server is stopped the debug log says it's retrying in 5 seconds, but it's actually trying every second.
I think #1 should be fixed, but the others are very trivial to fix too.
New patch attached. All issues have been addressed.
Let's say a profile is modified then deleted. These operations are replicated to a clone which is monitoring the changes using persistent search. When the clone receives the modify operation it will call readProfile(), which will call createProfile(), which will create an LDAPConfigStore for that profile. When LDAPConfigStore is created, it will read the profile from the database using a separate query. The problem is, at that point it's unclear whether the profile still exists.
The current code might ignore the error if the entry doesn't exist, but in general I don't think the code should make a separate query to retrieve the profile info because it could lead to inconsistencies. It should have used the LDAP entry returned by the persistent search that corresponds to the operation being processed.
Okey doke, new patch attached. The LDAPConfigStore no longer reads the entry on construction, and the LDAPProfileSubsystem reads the config from the incoming persistent search results and loads it into the LDAPConfigStore. Cheers! Fraser
-- Endi S. Dewata
On 4/8/2015 2:40 AM, Fraser Tweedale wrote:
On Mon, Apr 06, 2015 at 08:27:05PM -0500, Endi Sukma Dewata wrote:
On 3/30/2015 2:27 AM, Fraser Tweedale wrote: Let's say a profile is modified then deleted. These operations are replicated to a clone which is monitoring the changes using persistent search. When the clone receives the modify operation it will call readProfile(), which will call createProfile(), which will create an LDAPConfigStore for that profile. When LDAPConfigStore is created, it will read the profile from the database using a separate query. The problem is, at that point it's unclear whether the profile still exists.
The current code might ignore the error if the entry doesn't exist, but in general I don't think the code should make a separate query to retrieve the profile info because it could lead to inconsistencies. It should have used the LDAP entry returned by the persistent search that corresponds to the operation being processed.
Okey doke, new patch attached. The LDAPConfigStore no longer reads the entry on construction, and the LDAPProfileSubsystem reads the config from the incoming persistent search results and loads it into the LDAPConfigStore.
ACK. There are some minor issues, they can be addressed separately: 1. The try-catch blocks inside the while loop in run() probably can be combined. If an error occurs and it's still (!stopped) it will sleep for a while before continuing. 2. The profileId should be obtained from the cn in the DN instead of from the cn in the entry since the cn in the entry could have multiple values: String profileId = (String) ldapProfile.getAttribute("cn").getStringValues().nextElement(); 3. In LDAPProfileSubsystem.createProfile() the mProfileIds is guarded against duplicates: if (!mProfiles.containsKey(id)) mProfileIds.addElement(id); but in ProfileSubsystem.createProfile() it's not: mProfileIds.addElement(id); I think they should be consistent: 4. Related to #3, I think the mProfileIds can be replaced with mProfiles.keys() which contains unique profile IDs. -- Endi S. Dewata
On Thu, Apr 09, 2015 at 10:09:47AM -0500, Endi Sukma Dewata wrote:
On 4/8/2015 2:40 AM, Fraser Tweedale wrote:
On Mon, Apr 06, 2015 at 08:27:05PM -0500, Endi Sukma Dewata wrote:
On 3/30/2015 2:27 AM, Fraser Tweedale wrote: Let's say a profile is modified then deleted. These operations are replicated to a clone which is monitoring the changes using persistent search. When the clone receives the modify operation it will call readProfile(), which will call createProfile(), which will create an LDAPConfigStore for that profile. When LDAPConfigStore is created, it will read the profile from the database using a separate query. The problem is, at that point it's unclear whether the profile still exists.
The current code might ignore the error if the entry doesn't exist, but in general I don't think the code should make a separate query to retrieve the profile info because it could lead to inconsistencies. It should have used the LDAP entry returned by the persistent search that corresponds to the operation being processed.
Okey doke, new patch attached. The LDAPConfigStore no longer reads the entry on construction, and the LDAPProfileSubsystem reads the config from the incoming persistent search results and loads it into the LDAPConfigStore.
ACK.
There are some minor issues, they can be addressed separately:
1. The try-catch blocks inside the while loop in run() probably can be combined. If an error occurs and it's still (!stopped) it will sleep for a while before continuing.
2. The profileId should be obtained from the cn in the DN instead of from the cn in the entry since the cn in the entry could have multiple values:
String profileId = (String) ldapProfile.getAttribute("cn").getStringValues().nextElement();
3. In LDAPProfileSubsystem.createProfile() the mProfileIds is guarded against duplicates:
if (!mProfiles.containsKey(id)) mProfileIds.addElement(id);
but in ProfileSubsystem.createProfile() it's not:
mProfileIds.addElement(id);
I think they should be consistent:
4. Related to #3, I think the mProfileIds can be replaced with mProfiles.keys() which contains unique profile IDs.
Thanks! Pushed to master (1bfebdb). Patches for above issues are imminent. Cheers, Fraser
participants (2)
-
Endi Sukma Dewata -
Fraser Tweedale