Merge pull request #252 from geoffdutton/fix-env-var-factory

[laconia-config] Only call .convertMultiple if values object is not empty
nearst · Aug 27, 2019 · f0c69f9 · f0c69f9
2 parents cb770ab + f8ca7c4
commit f0c69f9
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 6 deletions.
diff --git a/packages/laconia-config/src/EnvVarConfigFactory.js b/packages/laconia-config/src/EnvVarConfigFactory.js
@@ -31,7 +31,10 @@ module.exports = class EnvVarConfigFactory extends EnvVarInstanceFactory {
     const conversionResults = await Promise.all(
       types.map(type => {
         const values = filterAndRemoveType(type, typedValues);
-        return this.converters[type].convertMultiple(values);
+        return (
+          Object.keys(values).length > 0 &&
+          this.converters[type].convertMultiple(values)
+        );
       })
     );
     return Object.assign({}, ...conversionResults);

diff --git a/packages/laconia-config/src/SecretsManagerConfigConverter.js b/packages/laconia-config/src/SecretsManagerConfigConverter.js
@@ -20,6 +20,9 @@ module.exports = class SecretsManagerConfigConverter {
       let secret;
       if ({}.hasOwnProperty.call(res, "SecretString")) {
         secret = res.SecretString;
+        try {
+          secret = JSON.parse(secret);
+        } catch (_) {}
       } else {
         const buff = Buffer.from(res.SecretBinary, "base64");
         secret = buff.toString("ascii");

diff --git a/packages/laconia-config/test/EnvVarConfigFactory.spec.js b/packages/laconia-config/test/EnvVarConfigFactory.spec.js
@@ -60,6 +60,7 @@ describe("EnvVarConfigFactory", () => {
   describe("when there are multiple env vars and converters", () => {
     let ssmConverter;
     let booleanConverter;
+    let secretsManagerConverter;
 
     beforeEach(() => {
       ssmConverter = {
@@ -72,13 +73,20 @@ describe("EnvVarConfigFactory", () => {
           .fn()
           .mockResolvedValue({ enableLogging: false, enableFeature: true })
       };
+      secretsManagerConverter = {
+        convertMultiple: jest.fn().mockResolvedValue({ apikey: "secretApiKey" })
+      };
       const env = {
         LACONIA_CONFIG_SECRET: "ssm:/path/to/secret",
         LACONIA_CONFIG_PASSWORD: "ssm:/path/to/password",
         LACONIA_CONFIG_ENABLE_LOGGING: "boolean:no",
         LACONIA_CONFIG_ENABLE_FEATURE: "boolean:yes"
       };
-      const converters = { ssm: ssmConverter, boolean: booleanConverter };
+      const converters = {
+        ssm: ssmConverter,
+        boolean: booleanConverter,
+        secretsManager: secretsManagerConverter
+      };
       configFactory = new EnvVarConfigFactory(env, converters);
     });
 
@@ -94,6 +102,24 @@ describe("EnvVarConfigFactory", () => {
       });
     });
 
+    it("should not call the configured converters when `values` is empty", async () => {
+      const converters = {
+        ssm: ssmConverter,
+        boolean: booleanConverter,
+        secretsManager: secretsManagerConverter
+      };
+      const env = {
+        LACONIA_CONFIG_API_SECRET: "secretsManager:path/to/api-key"
+      };
+      configFactory = new EnvVarConfigFactory(env, converters);
+      await configFactory.makeInstances();
+      expect(ssmConverter.convertMultiple).not.toHaveBeenCalled();
+      expect(booleanConverter.convertMultiple).not.toHaveBeenCalled();
+      expect(secretsManagerConverter.convertMultiple).toHaveBeenCalledWith({
+        apiSecret: "path/to/api-key"
+      });
+    });
+
     it("should merge the values returned by converters", async () => {
       const instances = await configFactory.makeInstances();
       expect(instances).toEqual({

diff --git a/packages/laconia-config/test/SecretsManagerConfigConverter.spec.js b/packages/laconia-config/test/SecretsManagerConfigConverter.spec.js
@@ -53,7 +53,8 @@ describe("SecretsManagerConfigConverter", () => {
       secretsStore = {
         myProdApiKey: "secret-api-key",
         myProductionDbPassword: "secret-db-password",
-        myProdBase64EncodedKey: Buffer.from("base64-encoded")
+        myProdBase64EncodedKey: Buffer.from("base64-encoded"),
+        myKeyValueSecrets: `{ "apiKey": "test-api-key", "apiSecret": "some-secret" }`
       };
       secretsManager.getSecretValue.mockImplementation((params, callback) => {
         const secret = secretsStore[params.SecretId];
@@ -87,13 +88,18 @@ describe("SecretsManagerConfigConverter", () => {
       const result = await configConverter.convertMultiple({
         apiKey: "myProdApiKey",
         dbPass: "myProductionDbPassword",
-        base64Encoded: "myProdBase64EncodedKey"
+        base64Encoded: "myProdBase64EncodedKey",
+        keyValPair: "myKeyValueSecrets"
       });
 
       expect(result).toEqual({
         apiKey: "secret-api-key",
         dbPass: "secret-db-password",
-        base64Encoded: secretsStore.myProdBase64EncodedKey.toString("ascii")
+        base64Encoded: secretsStore.myProdBase64EncodedKey.toString("ascii"),
+        keyValPair: {
+          apiKey: "test-api-key",
+          apiSecret: "some-secret"
+        }
       });
 
       expect(secretsManager.getSecretValue).toBeCalledWith(
@@ -111,7 +117,12 @@ describe("SecretsManagerConfigConverter", () => {
         expect.any(Function)
       );
 
-      expect(secretsManager.getSecretValue).toHaveBeenCalledTimes(3);
+      expect(secretsManager.getSecretValue).toBeCalledWith(
+        expect.objectContaining({ SecretId: "myKeyValueSecrets" }),
+        expect.any(Function)
+      );
+
+      expect(secretsManager.getSecretValue).toHaveBeenCalledTimes(4);
     });
   });
 });