You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@ambari.apache.org by Robert Nettleton <rn...@hortonworks.com> on 2016/04/19 22:47:48 UTC
Re: Review Request 45507: Enhance blueprint support for using
references
-----------------------------------------------------------
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
>
>
Re: Review Request 45507: Enhance blueprint support for using
references
Posted by Amruta Borkar <ar...@us.ibm.com>.
> 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
>
>