I've reverted my change on release/1.2.0. I'm currently planning to work on #1 on develop in the short term.
On Fri, Jun 9, 2017 at 12:45 PM, Kirk Lund <kl...@pivotal.io> wrote: > The new changes for SecurityService don't work with Cluster Config. We > only had one test that would've failed but it was annotated with @Ignore. > > The problem is this: > > The security-manager is a gemfire property and is now used by the > InternalDistributedSystem because membership needs SecurityService to > handle peer-to-peer authentication. So with my changes, the > InternalDistributedSystem now creates SecurityService and passes it into > the constructor of GemFireCacheImpl. > > Next, GemFireCacheImpl#initialize requests Cluster Config. If this is a > new Server with no gemfire properties, it will have already joined the > Cluster (before and after my changes). It then gets Cluster Config that has > gemfire.properties including security-manager. But it's too late, the > immutable SecurityService has already been created by > InternalDistributedSystem. > > In theory, Cluster Config should be requested before the creation of > InternalDistributedSystem so that other gemfire.properties in Cluster > Config can be applied (such as member-timeout). In fact most of the > gemfire.properties control aspects of InternalDistributedSystem. Presently, > all gemfire.properties that would configure InternalDistributedSystem are > ignored because Cluster Config is loaded after InternalDistributedSystem > was created and the member has joined the cluster. > > We would need to make one of these changes: > > 1) GemFireCacheImpl creates its own SecurityService which is different > from the one used by membership. Locators are the only members that really > need membership to have a fully configured SecurityService (and I think we > already have a bug in which 2nd Locators need to be manually configured > rather than use Cluster Config). > > 2) Move Cluster Config request from GemFireCacheImpl to > InternalDistributedSystem. This is actually more correct, since Cluster > Config is potentially going to contain a lot of InternalDistributedSystem > configuration options that are currently ignored (in both Geode 1.x and > current develop). > > Unfortunately #2 is a fairly big refactoring change. > > We should probably revert my changes from develop and 1.2 until #1 or #2 > can be completed. Or we have to file a bug that says Cluster Config and > Security don't play nice together until we can complete one of these > options. I think the latter is probably not acceptable. > >
