----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30181/#review69255 -----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java <https://reviews.apache.org/r/30181/#comment113895> This kerberos logic doesn't belong in this class. Because this logic is strictly related to kerberos, it should be encapsulated within a kerberos class and not leaked into other non-kerberos classes. I will also need to use this logic, or something very close, when invoking toggleKerberos as a result of host component state transitions. Probably should add the required arguments (Request?) to toggleKerberos(...) and handle the check there. I am ok with you adding this here for now and I will move it in my "add host" patch so that I can use it. ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java <https://reviews.apache.org/r/30181/#comment113893> Why are these checks necessary? You already attempted to get 'desiredSecurityState' above and don't try again based on these checks. Are these simply invariant assertions? ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java <https://reviews.apache.org/r/30181/#comment113894> will result in a NPE if desiredSecurityState doesn't get set above. ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java <https://reviews.apache.org/r/30181/#comment113896> What does it mean for security to be enabled and the KERBEROS service not being deployed? It seems that this should be an error? As mentioned above, this kerberos specific logic should be encapsulated in a kerberos class. Callers should simply call toggleKerberos(...) and assume that the correct thing is done without having to know these types of details. - John Speidel On Jan. 22, 2015, 7:51 p.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30181/ > ----------------------------------------------------------- > > (Updated Jan. 22, 2015, 7:51 p.m.) > > > Review request for Ambari, John Speidel, Robert Nettleton, and Tom Beerbower. > > > Bugs: AMBARI-9261 > https://issues.apache.org/jira/browse/AMBARI-9261 > > > Repository: ambari > > > Description > ------- > > The logic to enable or disable Kerberos is typically invoked when the Cluster > resource is updated. This occurs for several reasons, not all of them > indicate the state of Kerberos should be altered. > > By processing all updated to the Cluster resource, the enable/disable > Kerberos may get invoked when not necessary causing _noise_ on the task list > and potentially generating an error condition if the KDC administrator > credentials are not available. Certain states of the system will trigger the > enable/disable Kerberos logic to perform tasks requiring the KDC > administrator credentials. If not explicitly handing the security state > change, this behavior is not desired. > > To solve the issue, test the request on the update Cluster resource to see if > the security state property (`cluster-env/security_enabled`) has been > altered, if so invoke enable/disable Kerberos logic; else do not invoke > enable/disable Kerberos logic. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > dd18e8d > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java > e713d7f > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java > 805b498 > > Diff: https://reviews.apache.org/r/30181/diff/ > > > Testing > ------- > > Manually tested various cases on a test cluster > Added new unit tests > > > **Waiting for Jenkins tests to complete** > > > Thanks, > > Robert Levas > >
