diff --git a/lib/logger.js b/lib/logger.js index d88ad95..c77312b 100644 --- a/lib/logger.js +++ b/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 }] }; diff --git a/lib/proxy_manager.js b/lib/proxy_manager.js index 487b240..e654af0 100644 --- a/lib/proxy_manager.js +++ b/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(); } diff --git a/test/auth_proxy_dynamic_config_test.js b/test/auth_proxy_dynamic_config_test.js index e45aa05..1000367 100644 --- a/test/auth_proxy_dynamic_config_test.js +++ b/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(); + }); }); }); });