AMBARI-9261. Ensure enable/disable Kerberos logic should invoke only when state of security flag is changed (rlevas)

ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java

@@ -1196,7 +1196,51 @@ public class AmbariManagementControllerImpl implements AmbariManagementControlle
       cluster.setClusterName(request.getClusterName());
     }
 
-    // set or create configuration mapping (and optionally create the map of properties)
+    // ----------------------
+    // Check to see if the security state is being changed... if so, attempt to enable or disable
+    // Kerberos
+    boolean toggleKerberos = false;
+
+    String desiredSecurityState = null;
+    List<ConfigurationRequest> desiredConfig = request.getDesiredConfig();
+    if (desiredConfig != null) {
+      for (ConfigurationRequest configurationRequest : desiredConfig) {
+        if ("cluster-env".equals(configurationRequest.getType())) {
+          Map<String, String> properties = configurationRequest.getProperties();
+
+          if ((properties == null) || properties.isEmpty()) {
+            Config configClusterEnv = cluster.getConfig(configurationRequest.getType(), configurationRequest.getVersionTag());
+            if (configClusterEnv != null) {
+              properties = configClusterEnv.getProperties();
+            }
+          }
+
+          desiredSecurityState = (properties == null) ? null : properties.get("security_enabled");
+        }
+      }
+    }
+
+    if(desiredSecurityState != null) {
+      Config configClusterEnv = cluster.getDesiredConfigByType("cluster-env");
+      if (configClusterEnv == null) {
+        String message = "The 'cluster-env' configuration is not available";
+        LOG.error(message);
+        throw new AmbariException(message);
+      }
+
+      Map<String, String> clusterEnvProperties = configClusterEnv.getProperties();
+      if (clusterEnvProperties == null) {
+        String message = "The 'cluster-env' configuration properties are not available";
+        LOG.error(message);
+        throw new AmbariException(message);
+      }
+
+      toggleKerberos = !desiredSecurityState.equals(clusterEnvProperties.get("security_enabled"));
+    }
+    // ----------------------
+
+
+        // set or create configuration mapping (and optionally create the map of properties)
     if (null != request.getDesiredConfig()) {
       Set<Config> configs = new HashSet<Config>();
       String note = null;
@@ -1326,13 +1370,15 @@ public class AmbariManagementControllerImpl implements AmbariManagementControlle
     }
 
     RequestStageContainer requestStageContainer = null;
-    Map<String, Service> services = cluster.getServices();
-    if ((services != null) && services.containsKey("KERBEROS")) {
-      // Handle either adding or removing Kerberos from the cluster. This may generate multiple stages
-      // or not depending the current state of the cluster.  The main configuration used to determine
-      // whether Kerberos is to be added or removed is cluster-config/security_enabled.
-      requestStageContainer = kerberosHelper.toggleKerberos(cluster,
-          request.getKerberosDescriptor(), null);
+    if(toggleKerberos) {
+      Map<String, Service> services = cluster.getServices();
+      if ((services != null) && services.containsKey("KERBEROS")) {
+        // Handle either adding or removing Kerberos from the cluster. This may generate multiple stages
+        // or not depending the current state of the cluster.  The main configuration used to determine
+        // whether Kerberos is to be added or removed is cluster-config/security_enabled.
+        requestStageContainer = kerberosHelper.toggleKerberos(cluster,
+            request.getKerberosDescriptor(), null);
+      }
     }
 
     if (requestStageContainer != null) {

ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java

@@ -45,6 +45,7 @@ import org.apache.ambari.server.security.ldap.LdapBatchDto;
 import org.apache.ambari.server.state.Cluster;
 import org.apache.ambari.server.state.Clusters;
 import org.apache.ambari.server.state.ComponentInfo;
+import org.apache.ambari.server.state.Config;
 import org.apache.ambari.server.state.Host;
 import org.apache.ambari.server.state.MaintenanceState;
 import org.apache.ambari.server.state.Service;
@@ -73,18 +74,7 @@ import java.util.TreeMap;
 import static org.apache.ambari.server.agent.ExecutionCommand.KeyNames.DB_DRIVER_FILENAME;
 import static org.apache.ambari.server.agent.ExecutionCommand.KeyNames.STACK_NAME;
 import static org.apache.ambari.server.agent.ExecutionCommand.KeyNames.STACK_VERSION;
-import static org.easymock.EasyMock.anyObject;
-import static org.easymock.EasyMock.capture;
-import static org.easymock.EasyMock.createMock;
-import static org.easymock.EasyMock.createMockBuilder;
-import static org.easymock.EasyMock.createNiceMock;
-import static org.easymock.EasyMock.createStrictMock;
-import static org.easymock.EasyMock.eq;
-import static org.easymock.EasyMock.expect;
-import static org.easymock.EasyMock.expectLastCall;
-import static org.easymock.EasyMock.replay;
-import static org.easymock.EasyMock.reset;
-import static org.easymock.EasyMock.verify;
+import static org.easymock.EasyMock.*;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
@@ -507,11 +497,12 @@ public class AmbariManagementControllerImplTest {
     // requests
     Set<ClusterRequest> setRequests = Collections.singleton(clusterRequest);
 
+    KerberosHelper kerberosHelper = createStrictMock(KerberosHelper.class);
     // expectations
     injector.injectMembers(capture(controllerCapture));
     expect(injector.getInstance(Gson.class)).andReturn(null);
     expect(injector.getInstance(MaintenanceStateHelper.class)).andReturn(null);
-    expect(injector.getInstance(KerberosHelper.class)).andReturn(createNiceMock(KerberosHelper.class));
+    expect(injector.getInstance(KerberosHelper.class)).andReturn(kerberosHelper);
     expect(clusterRequest.getClusterName()).andReturn("clusterNew").times(4);
     expect(clusterRequest.getClusterId()).andReturn(1L).times(6);
     expect(clusters.getClusterById(1L)).andReturn(cluster).times(2);
@@ -535,6 +526,47 @@ public class AmbariManagementControllerImplTest {
     verify(actionManager, cluster, clusters, injector, clusterRequest, sessionManager);
   }
 
+  /**
+   * Ensure that when the cluster is updated KerberosHandler.toggleKerberos is not invoked unless
+   * the security state is altered
+   */
+  @Test
+  public void testUpdateClustersToggleKerberosNotInvoked() throws Exception {
+    // member state mocks
+    Capture<AmbariManagementController> controllerCapture = new Capture<AmbariManagementController>();
+    Injector injector = createStrictMock(Injector.class);
+    Cluster cluster = createNiceMock(Cluster.class);
+    ActionManager actionManager = createNiceMock(ActionManager.class);
+    ClusterRequest clusterRequest = createNiceMock(ClusterRequest.class);
+
+    // requests
+    Set<ClusterRequest> setRequests = Collections.singleton(clusterRequest);
+
+    KerberosHelper kerberosHelper = createStrictMock(KerberosHelper.class);
+    // expectations
+    injector.injectMembers(capture(controllerCapture));
+    expect(injector.getInstance(Gson.class)).andReturn(null);
+    expect(injector.getInstance(MaintenanceStateHelper.class)).andReturn(null);
+    expect(injector.getInstance(KerberosHelper.class)).andReturn(kerberosHelper);
+    expect(clusterRequest.getClusterId()).andReturn(1L).times(6);
+    expect(clusters.getClusterById(1L)).andReturn(cluster).times(2);
+    expect(cluster.getClusterName()).andReturn("cluster").times(2);
+
+    cluster.addSessionAttributes(anyObject(Map.class));
+    expectLastCall().once();
+
+    // replay mocks
+    replay(actionManager, cluster, clusters, injector, clusterRequest, sessionManager);
+
+    // test
+    AmbariManagementController controller = new AmbariManagementControllerImpl(actionManager, clusters, injector);
+    controller.updateClusters(setRequests, null);
+
+    // assert and verify
+    assertSame(controller, controllerCapture.getValue());
+    verify(actionManager, cluster, clusters, injector, clusterRequest, sessionManager);
+  }
+
   /**
    * Ensure that RollbackException is thrown outside the updateClusters method
    * when a unique constraint violation occurs.

ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java

@@ -9369,7 +9369,7 @@ public class AmbariManagementControllerTest {
     injector.injectMembers(capture(controllerCapture));
     expect(injector.getInstance(Gson.class)).andReturn(null);
     expect(injector.getInstance(MaintenanceStateHelper.class)).andReturn(maintHelper);
-    expect(injector.getInstance(KerberosHelper.class)).andReturn(createNiceMock(KerberosHelper.class));
+    expect(injector.getInstance(KerberosHelper.class)).andReturn(createStrictMock(KerberosHelper.class));
 
     // getServices
     expect(clusters.getCluster("cluster1")).andReturn(cluster);
@@ -9413,7 +9413,7 @@ public class AmbariManagementControllerTest {
     injector.injectMembers(capture(controllerCapture));
     expect(injector.getInstance(Gson.class)).andReturn(null);
     expect(injector.getInstance(MaintenanceStateHelper.class)).andReturn(maintHelper);
-    expect(injector.getInstance(KerberosHelper.class)).andReturn(createNiceMock(KerberosHelper.class));
+    expect(injector.getInstance(KerberosHelper.class)).andReturn(createStrictMock(KerberosHelper.class));
 
     // getServices
     expect(clusters.getCluster("cluster1")).andReturn(cluster);
@@ -9472,7 +9472,7 @@ public class AmbariManagementControllerTest {
     injector.injectMembers(capture(controllerCapture));
     expect(injector.getInstance(Gson.class)).andReturn(null);
     expect(injector.getInstance(MaintenanceStateHelper.class)).andReturn(maintHelper);
-    expect(injector.getInstance(KerberosHelper.class)).andReturn(createNiceMock(KerberosHelper.class));
+    expect(injector.getInstance(KerberosHelper.class)).andReturn(createStrictMock(KerberosHelper.class));
 
     // getServices
     expect(clusters.getCluster("cluster1")).andReturn(cluster).times(4);