> On Jan. 22, 2015, 3:48 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, > > line 1199 > > <https://reviews.apache.org/r/30181/diff/1/?file=830207#file830207line1199> > > > > 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.
This code need to be here for the inital kerberization process, else there will be no single point of entry to turn on Kerberos for the entire cluster at once. > On Jan. 22, 2015, 3:48 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, > > line 1223 > > <https://reviews.apache.org/r/30181/diff/1/?file=830207#file830207line1223> > > > > 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? if `desiredSecurityState` is null, then no attempt was made to change the security state (ie, update `cluster-env/security_enabled`). Therefore, there is no need to try to determine if the `cluster-env/security_enabled` value is was actually changed at all. It is possible to get into this block if some other value of the cluster-env configuration was changed, so checking to see if `security_enabled` was changed is needed. > On Jan. 22, 2015, 3:48 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, > > line 1238 > > <https://reviews.apache.org/r/30181/diff/1/?file=830207#file830207line1238> > > > > will result in a NPE if desiredSecurityState doesn't get set above. This line is surrounded by ``` if(desiredSecurityState != null) { ... } ``` Which should ensure that `desiredSecurityState` is not null and thus not produce a NPE. > On Jan. 22, 2015, 3:48 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, > > line 1375 > > <https://reviews.apache.org/r/30181/diff/1/?file=830207#file830207line1375> > > > > 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. This is simply untouched code. Before the check to determine if the security flag was updated, we needed a way to know whether we needed to attempt to toggle Kerberos or not. I will fix this. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30181/#review69255 ----------------------------------------------------------- On Jan. 22, 2015, 2: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, 2: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 > >
