Re: Review Request 45507: Enhance blueprint support for using references

[Robert Nettleton]

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


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


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

Re: Review Request 45507: Enhance blueprint support for using references

[Amruta Borkar]

> On April 19, 2016, 8:47 p.m., Robert Nettleton wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/topology/BlueprintValidatorImpl.java,
> >  line 87
> > 
> >
> > 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.

I have moved this method in another class ClusterTopologyImpl.java so that 
check will be performed to see any references left in the blueprint after 
default_password substitution is performed.


> On April 19, 2016, 8:47 p.m., Robert Nettleton wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java,
> >  line 158
> > 
> >
> > 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).
> 
> Amruta Borkar wrote:
> Please refer AMBARI-15406-v1

Reintegrated Password filter and added logic to treat password properties 
differently.


- Amruta


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


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

Re: Review Request 45507: Enhance blueprint support for using references

[Amruta Borkar]

> On April 19, 2016, 8:47 p.m., Robert Nettleton wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java,
> >  line 158
> > 
> >
> > 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).

Please refer AMBARI-15406-v1


- Amruta


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


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

Re: Review Request 45507: Enhance blueprint support for using references

[Amruta Borkar]

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


Changes
---

Hi Bob, Jayush,

I have addressed the issues marked below in this patch. Please review and 
provide your comments. 

Thank you,
Amruta


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 (updated)


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

Re: Review Request 45507: Enhance blueprint support for using references

[Shantanu Mundkur]

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



Bob, Jayush,

Thanks for reviewing this.

Your review comments for the code changes are valid and Amruta will make 
changes or respond to those.

I have prepared a document that has more details on the changes we are 
proposing. I hope it will give you the rationale behind these changes other 
than the fact that Bob had originally commented on the same lines while 
reviewing a defect fix in https://reviews.apache.org/r/43281/. Other than that 
these changes will go towards addressing some aspects that several users have 
complained about and improve the usability for Ambari blueprints. Please review 
and let us know if we can proceed with the work.


Thank you!

- Shantanu Mundkur


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

Re: Review Request 45507: Enhance blueprint support for using references

[Shantanu Mundkur]

> On April 28, 2016, 9:17 p.m., Shantanu Mundkur wrote:
> > Bob, Jayush,
> > 
> > Thanks for reviewing this.
> > 
> > Your review comments for the code changes are valid and Amruta will make 
> > changes or respond to those.
> > 
> > I have prepared a document that has more details on the changes we are 
> > proposing. I hope it will give you the rationale behind these changes other 
> > than the fact that Bob had originally commented on the same lines while 
> > reviewing a defect fix in https://reviews.apache.org/r/43281/. Other than 
> > that these changes will go towards addressing some aspects that several 
> > users have complained about and improve the usability for Ambari 
> > blueprints. Please review and let us know if we can proceed with the work.
> > 
> > 
> > Thank you!

The document is attached to https://issues.apache.org/jira/browse/AMBARI-15395


- Shantanu


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


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

Re: Review Request 45507: Enhance blueprint support for using references

[Amruta Borkar]

> On April 19, 2016, 8:47 p.m., Robert Nettleton wrote:
> > 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.

Hello Bob,

Thank you for your feedback, we are working on a document that would explain 
this and relevant functionality in more details. I also agree with some of the 
issues listed here and I am working on it. 

Thanks


- Amruta


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


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

Re: Review Request 45507: Enhance blueprint support for using references

[Jayush Luniya]

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



Agree with Bob's comments here.

- Jayush Luniya


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

Re: Review Request 45507: Enhance blueprint support for using references

[Robert Nettleton]

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


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
 


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
 


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 bl

Review Request 45507: Enhance blueprint support for using references

[Amruta Borkar]

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

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