diff --git a/CHANGELOG.md b/CHANGELOG.md index fe715a0..d8e9bde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ If you're looking for changes from before this, refer to the project's git logs & PR history. # [Unreleased](https://github.com/puppetlabs/vmpooler/compare/0.3.0...master) +### Fixed + - Improve support for configuration via environment variables (POOLER-137) # [0.3.0](https://github.com/puppetlabs/vmpooler/compare/0.2.2...0.3.0) diff --git a/Gemfile b/Gemfile index 1ce5e86..dfa75dd 100644 --- a/Gemfile +++ b/Gemfile @@ -26,6 +26,7 @@ group :test do gem 'rspec', '>= 3.2' gem 'simplecov', '>= 0.11.2' gem 'yarjuf', '>= 2.0' + gem 'climate_control', '>= 0.2.0' # Rubocop would be ok jruby but for now we only use it on # MRI or Windows platforms gem "rubocop", :platforms => [:ruby, :x64_mingw] diff --git a/docs/configuration.md b/docs/configuration.md index d692f32..16ba8a9 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -19,7 +19,7 @@ Provide the entire configuration as a blob of yaml. Individual parameters passed Path to a the file to use when loading the vmpooler configuration. This is only evaluated if `VMPOOLER_CONFIG` has not been specified. -### DOMAIN\_NAME +### DOMAIN If set, returns a top-level 'domain' JSON key in POST requests (optional) @@ -103,7 +103,7 @@ Same as `vm_lifetime`, but applied if a valid authentication token is included during the request. (required) -### VM\_PREFIX +### PREFIX If set, prefixes all created VMs with this string. This should include a separator. (optional; default: '') diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb index 4a01310..eac072e 100644 --- a/lib/vmpooler.rb +++ b/lib/vmpooler.rb @@ -47,21 +47,21 @@ module Vmpooler end # Set some configuration defaults - parsed_config[:config]['task_limit'] = ENV['TASK_LIMIT'] || parsed_config[:config]['task_limit'] || 10 - parsed_config[:config]['migration_limit'] = ENV['MIGRATION_LIMIT'] if ENV['MIGRATION_LIMIT'] - parsed_config[:config]['vm_checktime'] = ENV['VM_CHECKTIME'] || parsed_config[:config]['vm_checktime'] || 1 - parsed_config[:config]['vm_lifetime'] = ENV['VM_LIFETIME'] || parsed_config[:config]['vm_lifetime'] || 24 - parsed_config[:config]['prefix'] = ENV['VM_PREFIX'] || parsed_config[:config]['prefix'] || '' + parsed_config[:config]['task_limit'] = string_to_int(ENV['TASK_LIMIT']) || parsed_config[:config]['task_limit'] || 10 + parsed_config[:config]['migration_limit'] = string_to_int(ENV['MIGRATION_LIMIT']) if ENV['MIGRATION_LIMIT'] + parsed_config[:config]['vm_checktime'] = string_to_int(ENV['VM_CHECKTIME']) || parsed_config[:config]['vm_checktime'] || 1 + parsed_config[:config]['vm_lifetime'] = string_to_int(ENV['VM_LIFETIME']) || parsed_config[:config]['vm_lifetime'] || 24 + parsed_config[:config]['prefix'] = ENV['PREFIX'] || parsed_config[:config]['prefix'] || '' parsed_config[:config]['logfile'] = ENV['LOGFILE'] if ENV['LOGFILE'] parsed_config[:config]['site_name'] = ENV['SITE_NAME'] if ENV['SITE_NAME'] - parsed_config[:config]['domain'] = ENV['DOMAIN_NAME'] if ENV['DOMAIN_NAME'] + parsed_config[:config]['domain'] = ENV['DOMAIN'] if ENV['DOMAIN'] parsed_config[:config]['clone_target'] = ENV['CLONE_TARGET'] if ENV['CLONE_TARGET'] - parsed_config[:config]['timeout'] = ENV['TIMEOUT'] if ENV['TIMEOUT'] - parsed_config[:config]['vm_lifetime_auth'] = ENV['VM_LIFETIME_AUTH'] if ENV['VM_LIFETIME_AUTH'] - parsed_config[:config]['max_tries'] = ENV['MAX_TRIES'] if ENV['MAX_TRIES'] - parsed_config[:config]['retry_factor'] = ENV['RETRY_FACTOR'] if ENV['RETRY_FACTOR'] + parsed_config[:config]['timeout'] = string_to_int(ENV['TIMEOUT']) if ENV['TIMEOUT'] + parsed_config[:config]['vm_lifetime_auth'] = string_to_int(ENV['VM_LIFETIME_AUTH']) if ENV['VM_LIFETIME_AUTH'] + parsed_config[:config]['max_tries'] = string_to_int(ENV['MAX_TRIES']) if ENV['MAX_TRIES'] + parsed_config[:config]['retry_factor'] = string_to_int(ENV['RETRY_FACTOR']) if ENV['RETRY_FACTOR'] parsed_config[:config]['create_folders'] = ENV['CREATE_FOLDERS'] if ENV['CREATE_FOLDERS'] parsed_config[:config]['create_template_delta_disks'] = ENV['CREATE_TEMPLATE_DELTA_DISKS'] if ENV['CREATE_TEMPLATE_DELTA_DISKS'] parsed_config[:config]['experimental_features'] = ENV['EXPERIMENTAL_FEATURES'] if ENV['EXPERIMENTAL_FEATURES'] @@ -70,26 +70,26 @@ module Vmpooler parsed_config[:redis] = parsed_config[:redis] || {} parsed_config[:redis]['server'] = ENV['REDIS_SERVER'] || parsed_config[:redis]['server'] || 'localhost' - parsed_config[:redis]['port'] = ENV['REDIS_PORT'] if ENV['REDIS_PORT'] + parsed_config[:redis]['port'] = string_to_int(ENV['REDIS_PORT']) if ENV['REDIS_PORT'] parsed_config[:redis]['password'] = ENV['REDIS_PASSWORD'] if ENV['REDIS_PASSWORD'] - parsed_config[:redis]['data_ttl'] = ENV['REDIS_DATA_TTL'] || parsed_config[:redis]['data_ttl'] || 168 + parsed_config[:redis]['data_ttl'] = string_to_int(ENV['REDIS_DATA_TTL']) || parsed_config[:redis]['data_ttl'] || 168 parsed_config[:statsd] = parsed_config[:statsd] || {} if ENV['STATSD_SERVER'] parsed_config[:statsd]['server'] = ENV['STATSD_SERVER'] if ENV['STATSD_SERVER'] parsed_config[:statsd]['prefix'] = ENV['STATSD_PREFIX'] if ENV['STATSD_PREFIX'] - parsed_config[:statsd]['port'] = ENV['STATSD_PORT'] if ENV['STATSD_PORT'] + parsed_config[:statsd]['port'] = string_to_int(ENV['STATSD_PORT']) if ENV['STATSD_PORT'] parsed_config[:graphite] = parsed_config[:graphite] || {} if ENV['GRAPHITE_SERVER'] parsed_config[:graphite]['server'] = ENV['GRAPHITE_SERVER'] if ENV['GRAPHITE_SERVER'] parsed_config[:graphite]['prefix'] = ENV['GRAPHITE_PREFIX'] if ENV['GRAPHITE_PREFIX'] - parsed_config[:graphite]['port'] = ENV['GRAPHITE_PORT'] if ENV['GRAPHITE_PORT'] + parsed_config[:graphite]['port'] = string_to_int(ENV['GRAPHITE_PORT']) if ENV['GRAPHITE_PORT'] parsed_config[:auth] = parsed_config[:auth] || {} if ENV['AUTH_PROVIDER'] if parsed_config.has_key? :auth parsed_config[:auth]['provider'] = ENV['AUTH_PROVIDER'] if ENV['AUTH_PROVIDER'] parsed_config[:auth][:ldap] = parsed_config[:auth][:ldap] || {} if parsed_config[:auth]['provider'] == 'ldap' parsed_config[:auth][:ldap]['host'] = ENV['LDAP_HOST'] if ENV['LDAP_HOST'] - parsed_config[:auth][:ldap]['port'] = ENV['LDAP_PORT'] if ENV['LDAP_PORT'] + parsed_config[:auth][:ldap]['port'] = string_to_int(ENV['LDAP_PORT']) if ENV['LDAP_PORT'] parsed_config[:auth][:ldap]['base'] = ENV['LDAP_BASE'] if ENV['LDAP_BASE'] parsed_config[:auth][:ldap]['user_object'] = ENV['LDAP_USER_OBJECT'] if ENV['LDAP_USER_OBJECT'] end @@ -175,4 +175,11 @@ module Vmpooler end pools_hash end + + def self.string_to_int(s) + # Returns a integer if input is a string + return if s.nil? + return unless s =~ /\d/ + return Integer(s) + end end diff --git a/spec/unit/env_config.rb b/spec/unit/env_config.rb new file mode 100644 index 0000000..b152b3c --- /dev/null +++ b/spec/unit/env_config.rb @@ -0,0 +1,222 @@ +require 'vmpooler' +require 'climate_control' +require 'mock_redis' + +describe 'Vmpooler' do + describe 'config' do + + describe 'environment variables' do + + test_integer = '5' + test_string = 'test_string' + test_bool = 'true' + test_cases = [ + ['migration_limit', test_integer, nil], + ['task_limit', test_integer, 10], + ['vm_checktime', test_integer, 1], + ['vm_lifetime', test_integer, 24], + ['timeout', test_integer, nil], + ['vm_lifetime_auth', test_integer, nil], + ['max_tries', test_integer, nil], + ['retry_factor', test_integer, nil], + ['prefix', test_string, ""], + ['logfile', test_string, nil], + ['site_name', test_string, nil], + ['domain', test_string, nil], + ['clone_target', test_string, nil], + ['create_folders', test_bool, nil], + ['create_template_delta_disks', test_bool, nil], + ['experimental_features', test_bool, nil], + ['purge_unconfigured_folders', test_bool, nil], + ['usage_stats', test_bool, nil], + ] + + test_cases.each do |key, value, default| + it "should set a value for #{key}" do + with_modified_env "#{key.upcase}": value do + test_value = value + test_value = Integer(value) if value =~ /\d/ + config = Vmpooler.config + + expect(config[:config][key]).to eq(test_value) + end + end + + it "should set a default value for each #{key}" do + config = Vmpooler.config + + expect(config[:config][key]).to eq(default) + end + + if value =~ /\d/ + it "should not set bad_data as a value for #{key}" do + with_modified_env "#{key.upcase}": 'bad_data' do + config = Vmpooler.config + + expect(config[:config][key]).to eq(default) + end + end + end + end + end + + describe 'redis environment variables' do + let(:redis) { MockRedis.new } + + test_cases = [ + ['server', 'redis', 'localhost'], + ['port', '4567', nil], + ['password', 'testpass', nil], + ['data_ttl', '500', 168], + ] + + test_cases.each do |key, value, default| + it "should set a value for #{key}" do + allow(Vmpooler).to receive(:new_redis).and_return(redis) + with_modified_env "REDIS_#{key.upcase}": value do + test_value = value + test_value = Integer(value) if value =~ /\d/ + config = Vmpooler.config + + expect(config[:redis][key]).to eq(test_value) + end + end + + it "should set a default value for each #{key}" do + config = Vmpooler.config + + expect(config[:redis][key]).to eq(default) + end + + if value =~ /\d/ + it "should not set bad_data as a value for #{key}" do + with_modified_env "#{key.upcase}": 'bad_data' do + config = Vmpooler.config + + expect(config[:redis][key]).to eq(default) + end + end + end + end + end + + describe 'statsd environment variables' do + + test_cases = [ + ['prefix', 'vmpooler', nil], + ['port', '4567', nil], + ] + + test_cases.each do |key, value, default| + it "should set a value for #{key}" do + with_modified_env STATSD_SERVER: 'test', "STATSD_#{key.upcase}": value do + test_value = value + test_value = Integer(value) if value =~ /\d/ + config = Vmpooler.config + + expect(config[:statsd][key]).to eq(test_value) end + end + + it "should set a default value for each #{key}" do + with_modified_env STATSD_SERVER: 'test' do + config = Vmpooler.config + + expect(config[:statsd][key]).to eq(default) + end + end + + if value =~ /\d/ + it "should not set bad_data as a value for #{key}" do + with_modified_env STATSD_SERVER: 'test', "STATSD_#{key.upcase}": 'bad_data' do + config = Vmpooler.config + + expect(config[:statsd][key]).to eq(default) + end + end + end + end + end + + describe 'graphite environment variables' do + + test_cases = [ + ['prefix', 'vmpooler', nil], + ['port', '4567', nil], + ] + + test_cases.each do |key, value, default| + it "should set a value for #{key}" do + with_modified_env GRAPHITE_SERVER: 'test', "GRAPHITE_#{key.upcase}": value do + test_value = value + test_value = Integer(value) if value =~ /\d/ + config = Vmpooler.config + + expect(config[:graphite][key]).to eq(test_value) + end + end + + it "should set a default value for each #{key}" do + with_modified_env GRAPHITE_SERVER: 'test' do + config = Vmpooler.config + + expect(config[:graphite][key]).to eq(default) + end + end + + if value =~ /\d/ + it "should not set bad_data as a value for #{key}" do + with_modified_env GRAPHITE_SERVER: 'test', "GRAPHITE_#{key.upcase}": 'bad_data' do + config = Vmpooler.config + + expect(config[:graphite][key]).to eq(default) + end + end + end + end + end + + describe 'ldap environment variables' do + + test_cases = [ + ['host', 'test', nil], + ['port', '4567', nil], + ['base', 'dc=example,dc=com', nil], + ['user_object', 'uid', nil], + ] + + test_cases.each do |key, value, default| + it "should set a value for #{key}" do + with_modified_env AUTH_PROVIDER: 'ldap', "LDAP_#{key.upcase}": value do + test_value = value + test_value = Integer(value) if value =~ /\d/ + config = Vmpooler.config + + expect(config[:auth][:ldap][key]).to eq(test_value) + end + end + + it "should set a default value for each #{key}" do + with_modified_env AUTH_PROVIDER: 'ldap' do + config = Vmpooler.config + + expect(config[:auth][:ldap][key]).to eq(default) + end + end + + if value =~ /\d/ + it "should not set bad_data as a value for #{key}" do + with_modified_env AUTH_PROVIDER: 'ldap', "LDAP_#{key.upcase}": 'bad_data' do + config = Vmpooler.config + + expect(config[:auth][:ldap][key]).to eq(default) + end + end + end + end + end + end + + def with_modified_env(options, &block) + ClimateControl.modify(options, &block) + end +end