-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51705/#review148212
-----------------------------------------------------------




ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py (line 103)
<https://reviews.apache.org/r/51705/#comment215671>

    This will cause problems since you're storing the obfuscated properties. If 
a command needed the property from the in-memory cache, it would not have the 
correct value anymore.
    
    Perhaps this approach is flawed in that there's no credential store being 
used for the in-memory passwords.



ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py (line 173)
<https://reviews.apache.org/r/51705/#comment215673>

    `i` is not a very good name here; maybe something a little clearer.



ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py (lines 178 - 
180)
<https://reviews.apache.org/r/51705/#comment215674>

    That's a lot of iterating - over the entire config dictionary per-key that 
needs replacing - also you never exit once you have a hit.
    
    This function needs to be cleaned up for better performance.



ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ExecutionCommandWrapper.java
 (lines 182 - 183)
<https://reviews.apache.org/r/51705/#comment215662>

    What about status commands - those send down configurations too?



ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java 
(lines 252 - 275)
<https://reviews.apache.org/r/51705/#comment215665>

    This seems like it should be cached once after calculation for the cluster 
so that it doesn't need to keep generating it over and over.



ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java 
(line 270)
<https://reviews.apache.org/r/51705/#comment215667>

    You call this property "password config types" and bind a name to a type, 
but then never check the type for PASSWORD in python. I'd say either make this 
a flat list of password-protected fields or enhance the python code to extract 
properties of type PASSWORD to check on.


- Jonathan Hurley


On Sept. 7, 2016, 5:14 p.m., Anita Jebaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51705/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2016, 5:14 p.m.)
> 
> 
> Review request for Ambari, Di Li, Jonathan Hurley, and Nate Cole.
> 
> 
> Bugs: AMBARI-18334
>     https://issues.apache.org/jira/browse/AMBARI-18334
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> The configurations.json file loaded in the ambari-agent cache located at 
> /var/lib/ambari-agent/cache/cluster_configuration contains password details 
> in plaintext (Ex: ssl.client.keystore.password,ssl.client.truststore.password 
> etc.). The values are loaded both in the memory cache and file cache, the 
> file seems to be used only for debugging purposes, so it would be a better 
> approach to mask the passwords in the file.
> 
> Approach:
> 
> The password_config_type is included in the heartbeat response for alert 
> definition command and execution command, for which the values are dumped 
> into the json file. The password_config_type contains the information on 
> which properties in the configurations has the propertyType password. Based 
> on the response, the json is parsed and the password values are masked before 
> dumping it into the configurations.json file.
> 
> 
> Diffs
> -----
> 
>   ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py 72b87be 
>   ambari-agent/src/test/python/ambari_agent/TestAlerts.py e114daa 
>   ambari-agent/src/test/python/ambari_agent/TestClusterConfigurationCache.py 
> a418f6d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ExecutionCommandWrapper.java
>  0562c15 
>   
> ambari-server/src/main/java/org/apache/ambari/server/agent/AlertDefinitionCommand.java
>  4d2e048 
>   
> ambari-server/src/main/java/org/apache/ambari/server/agent/ExecutionCommand.java
>  29737ee 
>   
> ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java 
> 70c24f9 
> 
> Diff: https://reviews.apache.org/r/51705/diff/
> 
> 
> Testing
> -------
> 
> Updated the test cases.
> Ran mvn test.
> 
> Manually tested by setting up a cluster, the password fields in the 
> configurations.json is masked. During testing, everytime the ambari agent is 
> restarted, it registers with the server and the memory cache and file cache 
> are updated, the alerts in turn uses the value from the memory cache.
> 
> 
> Thanks,
> 
> Anita Jebaraj
> 
>

Reply via email to