Skip to content

Conversation

@h-kanazawa
Copy link
Contributor

@h-kanazawa h-kanazawa commented Aug 6, 2019

Which issue does this close?

Closes https://learningpool.atlassian.net/browse/LL-137

Describe your changes providing screenshots of UI changes if necessary.

@ryasmi ryasmi mentioned this pull request Aug 6, 2019
@WillSkates
Copy link

WillSkates commented Aug 13, 2019

I had a similar solution to this but credentials still get leaked into logs.

Instead of logging the object or string you are connecting with, try this:

diff --git a/lib/connections/redis.js b/lib/connections/redis.js
index 4a5f38ad..05f25c88 100644
--- a/lib/connections/redis.js
+++ b/lib/connections/redis.js
@@ -1,6 +1,7 @@
 import Redis from 'ioredis';
 import logger from 'lib/logger';
 import defaultTo from 'lodash/defaultTo';
+import url = require('url');
 
 const DEFAULT_REDIS_PORT = 6379;
 
@@ -19,16 +20,29 @@ export const getOptions = () => {
         const [host, port] = conn.split(':');
         return { host, port: defaultTo(Number(port), DEFAULT_REDIS_PORT) };
       });
+
+         logger.silly('REDIS: Connecting to Sentinel resource pool', {host: host, port: port, db: db, connections: connections});
+         
       return { db, password, name, sentinels };
     }
     default: case 'redis': {
       if (process.env.REDIS_URL) {
-        return process.env.REDIS_URL;
+                 var url = new URL(process.env.REDIS_URL);
+
+                 logger.silly(
+                         'REDIS: Creating Redis Client using URL',
+                         url.toString().replace(url.password, '').replace(url.username, '')
+                 );
+
+                 return process.env.REDIS_URL;
       }
+
       const db = defaultTo(Number(process.env.REDIS_DB), 0);
       const password = process.env.REDIS_PASSWORD;
       const host = process.env.REDIS_HOST;
       const port = defaultTo(Number(process.env.REDIS_PORT), DEFAULT_REDIS_PORT);
+
+         logger.silly('REDIS: Creating Redis Client from environment', {host: host, port: port, db: db});
       return { db, password, host, port };
     }
   }
@@ -37,8 +51,6 @@ export const getOptions = () => {
 export const createClient = () => {
   try {
     const options = getOptions();
-    logger.silly('Creating Redis client', options);
-    logger.error('Creating Redis client', options);
     return new Redis(options);
   } catch (e) {
     logger.error("Couldn't connect to redis", e);

@ryasmi ryasmi requested a review from HT2-QA August 19, 2019 09:00
@ryasmi ryasmi removed the request for review from HT2-QA September 6, 2019 08:16
@ryasmi ryasmi changed the title Add support for redis authentication feat(Redis): Adds support for Redis URL environment variable for more flexible configuration like the xapi-service. (LL-137) Sep 6, 2019
@ryasmi ryasmi merged commit 46213c6 into master Sep 6, 2019
@ryasmi ryasmi deleted the LL-137-add-support-for-Redis-authentication branch September 6, 2019 08:41
@HT2Bot
Copy link
Member

HT2Bot commented Sep 6, 2019

🎉 This PR is included in version 4.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@HT2Bot HT2Bot added the released label Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants