> On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote:
> > ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py, line 103
> > <https://reviews.apache.org/r/51705/diff/1/?file=1493660#file1493660line103>
> >
> >     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.

Thanks for the catch, I updated the code to mask passwords only while dumping 
in the file cache, but missed to remove the code. Was not able to capture this 
in the testing. Thank you.


> On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote:
> > ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py, line 174
> > <https://reviews.apache.org/r/51705/diff/1/?file=1493660#file1493660line174>
> >
> >     `i` is not a very good name here; maybe something a little clearer.

Updated the code


> On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote:
> > ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py, lines 
> > 179-181
> > <https://reviews.apache.org/r/51705/diff/1/?file=1493660#file1493660line179>
> >
> >     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.

Updated the code with a different approach


> On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ExecutionCommandWrapper.java,
> >  lines 182-183
> > <https://reviews.apache.org/r/51705/diff/1/?file=1493663#file1493663line182>
> >
> >     What about status commands - those send down configurations too?

The configurations in the status commands are not dumped into the 
configurations.json file. The values are dumped only for alert definition and 
execution commands.


> On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java,
> >  lines 252-275
> > <https://reviews.apache.org/r/51705/diff/1/?file=1493666#file1493666line252>
> >
> >     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.

Updated the code with a different approach. Instead of getting the value from 
the Ambari Metainfo, I am getting the value from the config based on the 
desired Tags. Similar approach is being used for getting the required config 
properties for the response.


> On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java,
> >  line 270
> > <https://reviews.apache.org/r/51705/diff/1/?file=1493666#file1493666line270>
> >
> >     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.

Yes you are right, key-value pair is not required in this case. I have updated 
the code


- Anita


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


On Sept. 12, 2016, 8:20 p.m., Anita Jebaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51705/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2016, 8:20 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 2bddc43 
>   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