(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

@@ -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)
 

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]

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: '')

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]['task_limit'] = string_to_int(ENV['TASK_LIMIT']) || parsed_config[:config]['task_limit'] || 10
-    parsed_config[:config]['migration_limit'] = ENV['MIGRATION_LIMIT'] if ENV['MIGRATION_LIMIT']
+    parsed_config[:config]['migration_limit'] = string_to_int(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_checktime'] = string_to_int(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]['vm_lifetime'] = string_to_int(ENV['VM_LIFETIME']) || parsed_config[:config]['vm_lifetime']  || 24
-    parsed_config[:config]['prefix'] = ENV['VM_PREFIX'] || parsed_config[:config]['prefix'] || ''
+    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]['timeout'] = string_to_int(ENV['TIMEOUT']) if ENV['TIMEOUT']
-    parsed_config[:config]['vm_lifetime_auth'] = ENV['VM_LIFETIME_AUTH'] if ENV['VM_LIFETIME_AUTH']
+    parsed_config[:config]['vm_lifetime_auth'] = string_to_int(ENV['VM_LIFETIME_AUTH']) if ENV['VM_LIFETIME_AUTH']
-    parsed_config[:config]['max_tries'] = ENV['MAX_TRIES'] if ENV['MAX_TRIES']
+    parsed_config[:config]['max_tries'] = string_to_int(ENV['MAX_TRIES']) if ENV['MAX_TRIES']
-    parsed_config[:config]['retry_factor'] = ENV['RETRY_FACTOR'] if ENV['RETRY_FACTOR']
+    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

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