Do not try to stop proxies with invalid config.

lib/logger.js

@@ -7,7 +7,7 @@ var bunyan = require('bunyan');
 var default_config = {
   name: 'oauth_reverse_proxy',
   streams: [{
-    level: process.env.OAUTH_REVERSE_PROXY_LOG_LEVEL || "trace",
+    level: process.env.OAUTH_REVERSE_PROXY_LOG_LEVEL || "warn",
     stream: process.stdout
   }]
 };

lib/proxy_manager.js

@@ -110,9 +110,9 @@ function setProxyValue(proxy_config_file, value) {
  * required_calls times.  Any proxies in the stop_me object will be stopped and any in
  * start_me will be started.  Once this is done, cb is called.
  */
-function create_proxy_config_finalizer(required_calls, stop_me, start_me, cb) {
+function create_proxy_config_finalizer(stop_me, start_me, cb) {
 
-  return _.after(required_calls, function() {
+  return function() {
 
     logger.trace('stop_me:\n%s', require('util').inspect(stop_me));
     logger.trace('start_me:\n%s', require('util').inspect(start_me));
@@ -138,7 +138,7 @@ function create_proxy_config_finalizer(required_calls, stop_me, start_me, cb) {
         deferred_cb();
       });
     });
-  });
+  };
 }
 
 /**
@@ -164,9 +164,9 @@ function loadConfigFiles(cb) {
     /* istanbul ignore if */
     if (err) return cb(err);
 
-    // Fire a callback only once all config files have been processed.
-    var required_calls = 0;
-    var wrapped_cb = create_proxy_config_finalizer(files.length, stop_me, start_me, cb);
+    // Create a callback that must be invoked once per file before actually firing.  This callback
+    // is responsible for stopping and starting all proxies that have changed state.
+    var wrapped_cb = _.after(files.length, create_proxy_config_finalizer(stop_me, start_me, cb));
 
     files.forEach(function(file) {
       logger.info('Loading proxy configuration file %s', file);
@@ -187,35 +187,56 @@ function loadConfigFiles(cb) {
           if (proxy_error) {
             logger.error("Failed to load proxy %s due to %s", file, proxy_error);
             setProxyValue(file, proxy_error);
+
+            // If this configuration has failed with the same error as last time we synced, remove
+            // it from the list of proxies to stop.
+            if (module.exports.proxies[file] === proxy_error) delete stop_me[file];
+
             return wrapped_cb();
           }
         } catch(e) {
           logger.error("Failed to load proxy %s due to %s", file, e);
+
+          // If this configuration has failed with the same error as last time we synced, remove
+          // it from the list of proxies to stop.
+          if (module.exports.proxies[file] === e.message) delete stop_me[file]
+
           setProxyValue(file, e.message);
           return wrapped_cb();
         }
 
         try {
 
+          // Check for a previously loaded proxy for this configuration file.  If the file was
+          // already loaded, we need to decide whether the loaded configuration is still valid.
           var existing_proxy = module.exports.proxies[file];
           if (existing_proxy && existing_proxy.config) {
+
+            // If this proxy is already running with identical config, make no changes to the proxy
+            // delete it from stop_me so that it continues running, and return with the callback
+            // since we don't want to start a new proxy instance.
             if (existing_proxy.config.equals(proxy_config)) {
-              // If this proxy is already running with identical config, make no changes.
               delete stop_me[file];
               return wrapped_cb();
-            } else {
-              // If this proxy is already running with different config, stop it.
-              stop_me[file] = existing_proxy;
             }
           }
 
-          // Create and start a proxy around a validated config.
+          // If we got here, we know that we either have a proxy with new configuration or we have
+          // a brand new proxy.  Either way, we need to add the proxy to the collection of pending
+          // proxies to start.
           var proxy = new Proxy(proxy_config);
           start_me[file] = proxy;
           wrapped_cb();
         } catch(e) {
           /* istanbul ignore next */
-          setProxyValue(file, "Uncaught exception starting proxy " + proxy_config.service_name + ": " + e + "\n" + e.stack);
+
+          var msg = "Uncaught exception starting proxy " + proxy_config.service_name + ": " + e + "\n" + e.stack;
+
+          // If this configuration has failed with the same error as last time we synced, remove
+          // it from the list of proxies to stop.
+          if (module.exports.proxies[file] === msg) delete stop_me[file]
+
+          setProxyValue(file, msg);
           /* istanbul ignore next */
           wrapped_cb();
         }

test/auth_proxy_dynamic_config_test.js

@@ -30,6 +30,7 @@ var copy_file = function(source, target, cb) {
 
 describe('oauth_reverse_proxy config loader', function() {
 
+  // Test that new proxies can be added to the system without restarting oauth_reverse_proxy.
   it ('should support adding proxies dynamically', function(done) {
     copy_file('./test/resources/dynamic_config_service.orig.json', './test/config.d/dynamic_config_service.json', function(err) {
       if (err) return done(err);
@@ -49,6 +50,10 @@ describe('oauth_reverse_proxy config loader', function() {
     });
   });
 
+  // Test that we can make changes to the proxies added in the previous test.  We want to validate that changing a
+  // running proxy works, whether or not we are changing the port on which the proxy runs.  By testing that a reconfigured
+  // proxy obeys the new configuration in the case where the proxy port doesn't change helps us validate that our proxy
+  // manager is correctly shutting down old instances of proxies before starting new ones.
   it ('should support updating proxies dynamically', function(done) {
     copy_file('./test/resources/dynamic_config_service.next.json', './test/config.d/dynamic_config_service.json', function(err) {
       if (err) return done(err);
@@ -78,19 +83,23 @@ describe('oauth_reverse_proxy config loader', function() {
     });
   });
 
+  // Test that we can remove the dynamically added proxies and that they will be correctly shut down.
   it ('should support removing proxies dynamically', function(done) {
     fs.unlink('./test/config.d/dynamic_config_service.json', function(err) {
       if (err) done(err);
-      var check_config = function() {
-        if (auth_proxy_bootstrap_test.proxies['dynamic_config_service.json'] === undefined) {
-          request_sender.sendAuthenticatedRequest('GET', 'http://localhost:8011/job/12345', null, 500, function(err) {
-            err.message.should.equal('connect ECONNREFUSED');
-            done();
-          });
-        } else setTimeout(check_config, 50);
-      };
+      fs.unlink('./test/config.d/dynamic_whitelist_config_service.json', function(err) {
+        if (err) done(err);
+        var check_config = function() {
+          if (auth_proxy_bootstrap_test.proxies['dynamic_config_service.json'] === undefined) {
+            request_sender.sendAuthenticatedRequest('GET', 'http://localhost:8011/job/12345', null, 500, function(err) {
+              err.message.should.equal('connect ECONNREFUSED');
+              done();
+            });
+          } else setTimeout(check_config, 50);
+        };
 
-      check_config();
+        check_config();
+      });
     });
   });
 });