Add support for Vault #42

aharonha · 2018-03-28T12:40:55Z

for issue #2

omerglam

please see my comments

omerglam · 2018-03-28T14:41:28Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+        throw new VaultException("Vault is sealed");
+      }
+    } catch (VaultException ignoredException) {
+      ignoredException.printStackTrace();


what will happen if the Vault instance will not be initiated here?

omerglam · 2018-03-28T14:43:59Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+    SECRET_DEFAULT_PATH = environmentLoader.loadVariable("VAULT_SECRETS_PATH");
+  }
+
+  public VaultConfigSource(Optional<VaultConfig> config) {


add javadocs for environment variables

omerglam · 2018-03-28T14:52:54Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+      return response.getData().entrySet().stream().map(LoadedConfigProperty::withNameAndValue).map(Builder::build)
+          .collect(Collectors.toMap(LoadedConfigProperty::name, Function.identity()));
+    } catch (VaultException ignoredException) {
+      return new HashMap<>();


how will the consumer behave in case of an empty collection is returned? will it try to re-fetch a config property in case the property is missing from the returned map? will propagating the exception will be a better solution here?

more Changes should me made

artem-v

test

artem-v · 2018-04-02T08:57:40Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+        throw new VaultException("Missing Vault token");
+      }
+      vault = new Vault(cfg.build());
+      checkVaultHealth();


There's no need to call checkVaultHealth() from constructor. Upon bootstrap, at first time, loadConfig() would be called from main thread anyway.

artem-v · 2018-04-02T08:58:49Z

config-vault/src/test/java/io/scalecube/config/vault/TestVaultConfigSource.java

+
+import static org.hamcrest.CoreMatchers.*;
+import static org.junit.Assert.*;
+


Could you please adjust eclipse settings to not to put those asterisks in imports.

artem-v · 2018-04-02T09:25:18Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+  public Map<String, ConfigProperty> loadConfig() {
+    try {
+      checkVaultHealth();
+    } catch (VaultException ignoredException) {


ignoredException is not that ignored, please rename

artem-v · 2018-04-02T09:28:28Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+ * This class is a {@link ConfigSource} implemented for Vault
+ * 
+ * @author AharonHa
+ * @see The Vault Project's site at {@link https://www.vaultproject.io/}


This @see is highlighted in read in IDE, looks like wrong @see format

please no @ author in code need to fix ide to omit this in javadocs

artem-v · 2018-04-02T09:48:33Z

config-vault/src/test/java/io/scalecube/config/vault/TestVaultConfigSource.java

+    } finally {
+      vaultContainer2.stop();
+    }
+


Please add test scenario for checking Vault 'polling' behaviour:

Vault container is up and running

Wait in the tests a time equal to ConfigRegistrySettings.reloadIntervalSec (by default it's 15 secs but can be overriden)

Check that after succesfull reload props are still at place
This is green scenario, everything should work fine, and successful reloads must not affect loaded props.

Please add test scenario for checking Vault 'connection loss' behavior:

Vault container is up and running

Wait in the tests a time equal to ConfigRegistrySettings.reloadIntervalSec (give it a chance to reload before going offline)

Shutdown Vault container

Wait in the tests a time equal to ConfigRegistrySettings.reloadIntervalSec

Check that loaded properties remained unaffected (not returning null, not returning default values )
This is red scenario, something we don't expect often, but want be prepared for; after disconnection to Vault properties which has been loaded and sitting in jvm memory should be unaffected by further disconnections of config source and any issues that config source might face.

artem-v · 2018-04-02T09:57:24Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+   */
+  VaultConfigSource(EnvironmentLoader environmentLoader) {
+    this(Optional.of(new VaultConfig().environmentLoader(environmentLoader)));
+    SECRET_DEFAULT_PATH = environmentLoader.loadVariable("VAULT_SECRETS_PATH");


Please move "VAULT_SECRETS_PATH" constant to some internal class constant

artem-v · 2018-04-02T10:12:33Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+
+  private static final Logger LOGGER = LoggerFactory.getLogger(VaultConfigSource.class);
+  private Vault vault;
+  private String SECRET_DEFAULT_PATH;


SECRET_DEFAULT_PATH is not constant, let's lower case it

artem-v · 2018-04-02T12:02:14Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+   * 
+   */
+
+  public VaultConfigSource(Optional<VaultConfig> config) {


Looks like we need only

token

address

and VAULT_SECRETS_PATH

+new builder class

(maybe because seal status is stronger?)

dmytro-lazebnyi · 2018-04-10T12:18:49Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+  }
+
+  /**
+   * Create a new {@link VaultConfigSource} with the given {@link VaultConfig}. <br>


This javadoc does not match with constructor signature.

There are still some outdated javadoc for constructor.
I mean this part "Default configurations can also be used by passing {@link Optional#empty() empty}." and so on.

dmytro-lazebnyi · 2018-04-10T12:19:01Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+   * 
+   */
+
+  VaultConfigSource(VaultConfigSource.Builder builder) {


I guess it's better to make constructor private.
Also you can write just Builder instead of VaultConfigSource.Builder.

The constructor is package protected because otherwise it would have this warning:
Access to enclosing constructor VaultConfigSource(VaultConfigSource.Builder) is emulated by a synthetic accessor method

fixed the param & javadocs

dmytro-lazebnyi · 2018-04-10T12:37:17Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+    this.secretsPath = builder.secretsPath();
+    try {
+      vault = new Vault(builder.config);
+      checkVaultStatus();


Do we actually need checkVaultStatus() in constructor?
It is called anyway from loadConfig() every time.

dmytro-lazebnyi · 2018-04-10T12:45:19Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+      checkVaultStatus();
+    } catch (VaultException exception) {
+      LOGGER.error("unable to build vault config source", exception);
+      vault = null;


I guess it is not good idea to set vault as null, because if VaultException happens in constructor we have instance of VaultConfigSource without correct vault (actually null) and we can't do anything with this.
I mean we can't reinitialize vault one more time in this particular instance of VaultConfigSource so we don't need to create that instance at all.

dmytro-lazebnyi · 2018-04-10T14:46:38Z

config-vault/src/main/java/io/scalecube/config/vault/VaultConfigSource.java

+    }
+  }
+
+  private void checkVaultStatus() throws VaultException {


Do we actually need to check status this way?
I mean we can just use Logical.read and Vault throws exceptions if something wrong.
Why do we need manually check health if we don't do anything only throw exception.

read() method does not tell you if vault is currently sealed or not initialized. it throws the REST 404 or 500 wrapped message.

artem-v · 2018-04-12T15:49:45Z

Everything is great, but mvn clean install doesn't pass. Enforcer plugin tells:

[INFO] --- maven-enforcer-plugin:1.4.1:enforce (enforce) @ config-vault ---
[WARNING] 
Dependency convergence error for commons-io:commons-io:2.5 paths to dependency are:
+-io.scalecube:config-vault:0.3.2-SNAPSHOT
  +-org.testcontainers:vault:1.6.0
    +-org.testcontainers:testcontainers:1.6.0
      +-commons-io:commons-io:2.5
and
+-io.scalecube:config-vault:0.3.2-SNAPSHOT
  +-org.testcontainers:vault:1.6.0
    +-org.testcontainers:testcontainers:1.6.0
      +-org.zeroturnaround:zt-exec:1.8
        +-commons-io:commons-io:1.4

[WARNING] Rule 0: org.apache.maven.plugins.enforcer.DependencyConvergence failed with message:
Failed while enforcing releasability the error(s) are [
Dependency convergence error for commons-io:commons-io:2.5 paths to dependency are:
+-io.scalecube:config-vault:0.3.2-SNAPSHOT
  +-org.testcontainers:vault:1.6.0
    +-org.testcontainers:testcontainers:1.6.0
      +-commons-io:commons-io:2.5
and
+-io.scalecube:config-vault:0.3.2-SNAPSHOT
  +-org.testcontainers:vault:1.6.0
    +-org.testcontainers:testcontainers:1.6.0
      +-org.zeroturnaround:zt-exec:1.8
        +-commons-io:commons-io:1.4
]

* Add support for Vault

Add support for Vault

cad87b6

for issue #2

aharonha requested a review from artem-v March 28, 2018 12:41

tests

72a48f8

omerglam requested changes Mar 28, 2018

View reviewed changes

aharonha added 2 commits March 29, 2018 10:33

logs and javadocs

25fb3a1

tests are now more readable

1a93e2e

aharonha requested a review from snripa March 29, 2018 08:19

aharonha added 2 commits March 29, 2018 11:50

rename test method

8cb7d79

changes (code review with @snripa)

ebfb4f5

more Changes should me made

artem-v reviewed Apr 2, 2018

View reviewed changes

aharonha added 3 commits April 10, 2018 08:36

Code review changes

91b1d87

waiting for vault to start

009e1b5

healthiness is tested after seal status

830e268

(maybe because seal status is stronger?)

dmytro-lazebnyi mentioned this pull request Apr 10, 2018

Add Vault support - refactoring #43

Closed

dmytro-lazebnyi requested changes Apr 10, 2018

View reviewed changes

aharonha added 3 commits April 11, 2018 10:10

PR requested changes

adfcbc6

+new test: seal and unseal auto-recovery.

0e9900b

CodeReview changes

5f69103

dmytro-lazebnyi approved these changes Apr 11, 2018

View reviewed changes

segabriel approved these changes Apr 11, 2018

View reviewed changes

omerglam approved these changes Apr 11, 2018

View reviewed changes

artem-v force-pushed the support_secured_properties_#2 branch from 6a548d9 to e6e9010 Compare April 13, 2018 11:59

enforcer.skip

c1bce27

aharonha force-pushed the support_secured_properties_#2 branch from f827842 to c1bce27 Compare April 15, 2018 06:00

aharonha changed the base branch from master to develop April 15, 2018 06:01

aharonha and others added 3 commits April 18, 2018 09:00

Merge branch 'develop' into support_secured_properties_#2

3a064be

Merge branch 'develop' into support_secured_properties_#2

41a53ed

Merge branch 'develop' into support_secured_properties_#2

80440f8

Merge branch 'develop' into support_secured_properties_#2

166b8d6

snripa approved these changes Apr 20, 2018

View reviewed changes

artem-v merged commit 71c6f9f into develop Apr 20, 2018

io-scalecube-ci pushed a commit that referenced this pull request Apr 23, 2018

Add support for Vault (#42)

6368c67

* Add support for Vault

aharonha deleted the support_secured_properties_#2 branch August 30, 2018 12:25


		import static org.hamcrest.CoreMatchers.*;
		import static org.junit.Assert.*;

Add support for Vault #42

Add support for Vault #42

Conversation

aharonha commented Mar 28, 2018

omerglam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-v left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-v commented Apr 12, 2018 • edited Loading

artem-v commented Apr 12, 2018 •

edited

Loading