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



I have some general concerns about this patch, and the proposed overall 
approach, and I've listed out my concerns on the following JIRA:

https://issues.apache.org/jira/browse/AMBARI-15395

The issues listed below are related to the concerns I've posted in the link 
above. 

Thanks.


File Attachment: AMBARI-15406-v1 - AMBARI-15406.patch
<https://reviews.apache.org//r/45507/#fcomment95>

    I'm not sure this makes sense. 
    
    The method here should never rely on specify implementations of the Filter 
interface.  
    
    In general, I think that passwords should still be filtered out, since the 
Blueprint/Cluster Creation template validation code will run 
error-checks/validation checks, and notify the user/blueprint deployer that a 
password is missing.



File Attachment: AMBARI-15406-v1 - AMBARI-15406.patch
<https://reviews.apache.org//r/45507/#fcomment96>

    I'm still not sure I agree with this approach generally. 
    
    The "Secret Reference" format used by the Ambari REST API doesn't really 
translate well to be included within a Blueprint.  The reference syntax 
includes things like configuration type, property name, etc, which are 
generally already included in the Blueprint.
    
    As with my comment above, it's my opinion that passwords should be 
excluded.  The Blueprints processor already has checks to fail a deployment 
attempt if a password is missing, and already has the support for the "default 
password" feature in non-production environments.


- Robert Nettleton


On May 12, 2016, 9:40 p.m., Amruta Borkar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45507/
> -----------------------------------------------------------
> 
> (Updated May 12, 2016, 9:40 p.m.)
> 
> 
> Review request for Ambari, Jayush Luniya, Robert Nettleton, and Yusaku Sako.
> 
> 
> 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
> AMBARI-15406-v1
>   
> https://reviews.apache.org/media/uploaded/files/2016/05/12/7faf76cc-a42d-4017-b203-8db19448b09d__AMBARI-15406.patch
> 
> 
> Thanks,
> 
> Amruta Borkar
> 
>

Reply via email to