From d94b14a4d860300cc495a268f58ffff145bf2361 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Wed, 16 Jan 2019 16:44:14 -0800 Subject: [PATCH] (POOLER-137) Support integer environment variables This commit updates vmpooler to set configuration values received via environment variables to integer values when an integer value is expected. Without this change vmpooler does not support setting integer values via environment variables. Additionally, testing is added for configuring vmpooler via environment variables. To support this testing the gem climate_control is added, which allows for testing environment variables without those set variables leaking to other tests. --- CHANGELOG.md | 2 + Gemfile | 1 + docs/configuration.md | 4 +- lib/vmpooler.rb | 37 ++++--- spec/unit/env_config.rb | 222 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 249 insertions(+), 17 deletions(-) create mode 100644 spec/unit/env_config.rb 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