[AMBARI-23130] Retrieve cluster template as artifact with passwords filtered out. by benyoka · Pull Request #931 · apache/ambari

benyoka · 2018-04-06T21:38:39Z

What changes were proposed in this pull request?

Cluster templates are saved as artifacts during blueprint cluster creation and can be retrieved via the artifact resource provider API. Passwords are replaced with references during retrieval.

How was this patch tested?

Tested manually. Wrote new unit tests. Unit test results:
Tests run: 4834, Failures: 150, Errors: 301, Skipped: 73

…ids on server restart (benyoka)

…eature-AMBARI-14714

…nto AMBARI-23130-branch-feature-AMBARI-14714

…enyoka)

…ate (benyoka)

asfgit · 2018-04-07T03:47:02Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/1705/
Test FAILed.
Test FAILured.

rnettleton

This patch looks fine to me, just a few minor comments/questions below.

Can you please also ask @echekanskiy to review, since I think he implemented the original version of the SecretReference code?

Thanks.

rnettleton · 2018-04-09T14:07:59Z

+                                                                  Multimap<String, String> passwordProperties) {
+    return propertyMap.entrySet().stream().map(entry -> {
+      String propertyType = entry.getKey();
+      String newValue = passwordProperties.get(configType).contains(propertyType) ?


Minor issue: If a configuration type is listed in a Blueprint/Cluster Creation template that is not a match with any known configuration type, will this throw a NullPointerException?

This might be a problem in cases where the cluster creation template has an error (mis-spelled config type name) or if a custom config type is specified that the stacks do not include.

Maybe we should catch this error here and log appropriately, if the code doesn't already filter out "unknown" config types at some other level.

Guava multimaps return an empty collection if the key is not in the map, that's one of their cool features.

rnettleton · 2018-04-09T14:23:08Z

  public static final String CLUSTER_NAME_PROPERTY = RESPONSE_KEY + PropertyHelper.EXTERNAL_PATH_SEP + CLUSTER_NAME;
  public static final String SERVICE_NAME_PROPERTY = RESPONSE_KEY + PropertyHelper.EXTERNAL_PATH_SEP + SERVICE_NAME;

+  public static final String PROVISION_REQUEST_ARTIFACT_NAME = "provision_cluster_request";


Minor concern:

I think this change is fine the way it is, but we might want to consider refactoring this in the future.

It may make more sense to keep any details about specific types of artifacts out of this class, and have a more generic strategy defined to allow for modifications of retrieved artifacts before the artifacts are returned to the caller.

Even having a static list of registered "filtering" or "updating" interfaces in this class might be better, and allow the resource provider code to only interact with that strategy interface.

As I mentioned, this current change is fine, but we might want to consider refactoring this if we find that other artifact types need to be persisted, and then retrieved in some custom fashion.

adoroszlai · 2018-04-09T14:31:23Z

+        Configuration replacedConfiguration =
+          SecretReference.replacePasswordsInConfigurations(configuration, passwordProperties);
+        return Configurable.convertConfigToMap(replacedConfiguration);
+      }


Can you please extract this lambda expression to a function that can be referenced and tested more easily?

This method simply transforms a map with a lambda that is only 3 statements. I don't think further decomposition should be needed.

adoroszlai · 2018-04-09T14:31:37Z

+              entry.getKey(),
+              applyToAllConfigurations(entry.getValue(), transform));
+          }
+        }).collect(toMap(Map.Entry::getKey, Map.Entry::getValue));


Can you please extract this lambda expression to a function that can be referenced and tested more easily?

I broke it up to smaller methods. I don't think it improves testing (the algorithm should be tested as a whole), on the other hand it hopefully improves readability.

adoroszlai · 2018-04-09T14:33:55Z

+      catch (StackAccessException ex) {
+        throw new IllegalArgumentException(ex);
+      }
+    }).collect(toList());


Can you please extract this lambda expression to a function that can be referenced and tested more easily?

This is a very simple lambda (a simple method call within a try - catch - re-throw). I don't think it makes sense to decompose it further.

adoroszlai · 2018-04-09T14:37:50Z

+    return getAllPasswordPropertiesInternal(metaInfo.getStacks());
+  }
+
+  private static SetMultimap<String, String> getAllPasswordPropertiesInternal(Collection<StackInfo> stacks) {


Making this non-private would allow testing without the need for PowerMock.

Powermock is needed because of the static call to AmbariServer.getController(), not because this method is private.

I meant that this method could be tested instead of the one that calls AmbariServer.getController(). Most of the logic is in this one.

adoroszlai · 2018-04-09T14:41:08Z

+        Map<String, Map<String, String>> replacedConfigProps = configTypeEntry.getValue().entrySet().stream().collect(
+          toMap(Map.Entry::getKey, e -> replacePasswordsInPropertyMap(e.getValue(), configType, passwordProperties)));
+        return new SimpleEntry<>(configType, replacedConfigProps);
+      }).collect(toMap(Map.Entry::getKey, Map.Entry::getValue));


Can you please extract these lambda expressions to a function that can be referenced and tested more easily?

I am thinking on a simplification.

adoroszlai · 2018-04-09T14:41:15Z

+        SECRET_PREFIX + ":" + configType + ":" + propertyType :
+        entry.getValue();
+      return new SimpleEntry<>(propertyType, newValue);
+    }).collect(toMap(Map.Entry::getKey, Map.Entry::getValue));


Can you please extract this lambda expression to a function that can be referenced and tested more easily?

This is another a very simple map transformation written in java-8-lambda style rather as for-loop. I don't think further decomposition would add anything to readability or testability. I changed the indentation though to make it more readable.

asfgit · 2018-04-10T00:40:28Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/1730/
Test FAILed.
Test FAILured.

asfgit · 2018-04-10T11:57:12Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/1735/
Test FAILed.
Test FAILured.

asfgit · 2018-04-10T13:04:18Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/1736/
Test FAILed.
Test FAILured.

Balazs Bence Sari added 16 commits March 6, 2018 16:00

AMBARI-23130 persist raw cluster provision request and extract stack …

34cda14

…ids on server restart (benyoka)

Merge branch 'branch-feature-AMBARI-14714' into AMBARI-23130-branch-f…

9902c61

…eature-AMBARI-14714

AMBARI-23130 add columnt to other DDLs + fix DDLs (benyoka)

2e8c8bf

AMBARI-23130 fix review findings (benyoka)

386c582

Merge branch 'branch-feature-AMBARI-14714' into AMBARI-23130-branch-f…

210f390

…eature-AMBARI-14714

AMBARI-23130 persist only mpack instances instead of the full request

fd24060

AMBARI-23130 address review findings (benyoka)

153d56e

AMBARI-23130 topology request mpack information normalized (benyoka)

80f0de6

Merge branch 'branch-feature-AMBARI-14714' into AMBARI-23130-branch-f…

512823e

…eature-AMBARI-14714

AMBARI-23130 fix broken unit test (benyoka)

f1302f7

AMBARI-23130 fix import and review comments

d28589c

Merge remote-tracking branch 'upstream/branch-feature-AMBARI-14714' i…

df2c66d

…nto AMBARI-23130-branch-feature-AMBARI-14714

AMBARI-23130 persist cluster template as artifact WIP (benyoka)

8a0f928

Merge remote-tracking branch 'upstream/branch-feature-AMBARI-14714' i…

09aeb4b

…nto AMBARI-23130-branch-feature-AMBARI-14714

AMBARI-23130 cluster template passwords replaced - almost complete (b…

09ed645

…enyoka)

AMBARI-23130 save/retrieve cluster template as artifact review candid…

b72a584

…ate (benyoka)

benyoka requested review from adoroszlai, rlevas and rnettleton April 6, 2018 21:38

benyoka changed the title ~~[AMBARI-23130] branch feature ambari 14714~~ [AMBARI-23130] Retrieve cluster template as artifact with passwords filtered out. Apr 9, 2018

rnettleton approved these changes Apr 9, 2018

View reviewed changes

adoroszlai suggested changes Apr 9, 2018

View reviewed changes

benyoka requested a review from echekanskiy April 9, 2018 15:51

AMBARI-23130 fix review comments (benyoka)

1d3730d

AMBARI-23130 fix review comments apache#2 (benyoka)

143885a

adoroszlai approved these changes Apr 10, 2018

View reviewed changes

AMBARI-23130 fix import (benyoka)

6527627

benyoka merged commit 0e70cf2 into apache:branch-feature-AMBARI-14714 Apr 11, 2018

Conversation

benyoka commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

asfgit commented Apr 7, 2018

Uh oh!

rnettleton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Apr 10, 2018

Uh oh!

asfgit commented Apr 10, 2018

Uh oh!

asfgit commented Apr 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benyoka commented Apr 6, 2018 •

edited

Loading