+1 for #2. It does seem more correct
On 6/9/17 12:45, Kirk Lund 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.
