ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Speidel" <jspei...@hortonworks.com>
Subject Re: Review Request 30181: Ensure enable/disable Kerberos logic should invoke only when state of security flag is changed
Date Thu, 22 Jan 2015 20:48:53 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message