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



I've taken a quick look at this, and I have a few concerns with this patch:

1. I'm not sure there is that much utility in replacing the secret tokens with 
the "default_password" value specified in the Cluster Creation Template.  It 
should be noted that the "default_password" was never meant as a 
production-level feature, and is there mainly for the convenience of developers 
building new Blueprints.  With that in mind, I'm not sure it makes sense to 
introduce this kind of change. In most production scenarios, it is likely that 
most of the passwords will differ, and will need to be handled manually anyway. 
 In those cases, it is usually best to configure the passwords in a Cluster 
Creation Template, keeping the Blueprint as portable as possible.
2. I've mentioned this in my comments, but I'm a little concerned about 
removing the Password property filter, for the reasons I list below.  We've hit 
many situations in which a new set of stack configurations cause a "password" 
to show up in clear text, and this filter is very useful in keeping much of 
that from being a problem.  If a change like the one proposed goes through, at 
the very least this filter should be re-introduced, and modified to account for 
any changes made.

Thanks.


ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
 (line 158)
<https://reviews.apache.org/r/45507/#comment193049>

    I'm a little concerned about removing this filter.  
    
    Many times in the past, we've run into situations where a component that 
has just been integrated with Ambari will have password properties that are not 
marked as such in the Stack metadata.  
    
    This can end up causing a given password to show up in clear text in an 
exported Blueprint.
    
    It does make sense that this filter would need to be modified for a change 
like the one proposed here, but I think it should be updated, rather than 
removed, to still catch the cases it was intended for (password properties that 
are not annotated as passwords in the stacks).



ambari-server/src/main/java/org/apache/ambari/server/topology/BlueprintValidatorImpl.java
 
<https://reviews.apache.org/r/45507/#comment193050>

    If the default top-level "default_password" property is not set, I believe 
this validation code would still be useful, so I'm inclined to think that this 
block should stay, perhaps in some modified form.



ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
 
<https://reviews.apache.org/r/45507/#comment193051>

    As above, I'd recommend re-introducing this test once the PasswordFilter 
has been re-introduced.


- Robert Nettleton


On March 30, 2016, 10:19 p.m., Amruta Borkar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45507/
> -----------------------------------------------------------
> 
> (Updated March 30, 2016, 10:19 p.m.)
> 
> 
> Review request for Ambari and Robert Nettleton.
> 
> 
> Bugs: AMBARI-15395 and AMBARI-15406
>     https://issues.apache.org/jira/browse/AMBARI-15395
>     https://issues.apache.org/jira/browse/AMBARI-15406
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Enhance blueprints to export placeholder/token for password properties. This 
> is to avoid user from having tochange the password once the cluster is formed 
> if a user wishes to do so.
> Secret References acting as tokens to password properties would be replaced 
> by user with appropriate passwords,
> If any Secret references are found during cluster deployment with blueprint, 
> those will be replaced by default_password provided in blueprint template. 
> Need more comments to make this feature more portable.
> 
> 
> Diffs
> -----
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  9cc7b13 
>   
> ambari-server/src/main/java/org/apache/ambari/server/topology/BlueprintValidatorImpl.java
>  432c6f8 
>   
> ambari-server/src/main/java/org/apache/ambari/server/topology/RequiredPasswordValidator.java
>  98eaa40 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  14a718d 
>   
> ambari-server/src/test/java/org/apache/ambari/server/topology/BlueprintImplTest.java
>  0b06eb8 
>   
> ambari-server/src/test/java/org/apache/ambari/server/topology/RequiredPasswordValidatorTest.java
>  e8a2ff5 
> 
> Diff: https://reviews.apache.org/r/45507/diff/
> 
> 
> Testing
> -------
> 
> Tested the patch by trying out blueprint export and creating a cluster from 
> the exported blueprint. Result was: the password tokens were replaced by 
> default password and cluster was created successfully.
> 
> 
> File Attachments
> ----------------
> 
> AMBARI-15406.patch
>   
> https://reviews.apache.org/media/uploaded/files/2016/03/30/8daa09ee-82e5-4cd8-98d3-b1c00f1269b7__AMBARI-15406.patch
> 
> 
> Thanks,
> 
> Amruta Borkar
> 
>

Reply via email to