Avoid ambiguity when using the AWS_REGION environment var

garethr · MWilkinson · commit 788743cba153 · 2017-03-23T08:53:39.000Z
If using the AWS_REGION environment variable and a manifest where the region property of one or more resources doesn't match you can see unspecified behaviour. See puppetlabs-toy-chest#117 for discussions. This PR extracted from the above simply stops that from happening. If the env is present and doesn't match All of the region properties then we fail. This maintains the utility of the env var as an optimisation but stops us shooting ourselves in the foot.
diff --git a/lib/puppet/type/cloudwatch_alarm.rb b/lib/puppet/type/cloudwatch_alarm.rb
@@ -1,3 +1,5 @@
+require_relative '../../puppet_x/puppetlabs/property/region'
+
 Puppet::Type.newtype(:cloudwatch_alarm) do
   @doc = 'Type representing an AWS CloudWatch Alarm.'
 
@@ -73,13 +75,8 @@
     end
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to launch the instances.'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should not be blank' if value == ''
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:dimensions, :array_matching => :all) do
diff --git a/lib/puppet/type/ec2_autoscalinggroup.rb b/lib/puppet/type/ec2_autoscalinggroup.rb
@@ -87,13 +87,8 @@
     desc 'Indicates whether newly launched instances are protected from termination by Auto Scaling when scaling in.'
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to launch the instances.'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should not be blank' if value == ''
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:launch_configuration) do
diff --git a/lib/puppet/type/ec2_elastic_ip.rb b/lib/puppet/type/ec2_elastic_ip.rb
@@ -1,3 +1,5 @@
+require_relative '../../puppet_x/puppetlabs/property/region'
+
 Puppet::Type.newtype(:ec2_elastic_ip) do
   @doc = "Type representing an Elastic IP and it's association."
 
@@ -19,12 +21,8 @@
     end
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The name of the region in which the Elastic IP is found.'
-    validate do |value|
-      fail 'region should be a String' unless value.is_a?(String)
-      fail 'You must provide a region for Elastic IPs.' if value.nil? || value.empty?
-    end
   end
 
   newproperty(:instance) do
diff --git a/lib/puppet/type/ec2_launchconfiguration.rb b/lib/puppet/type/ec2_launchconfiguration.rb
@@ -1,3 +1,5 @@
+require_relative '../../puppet_x/puppetlabs/property/region'
+
 Puppet::Type.newtype(:ec2_launchconfiguration) do
   @doc = 'Type representing an EC2 launch configuration.'
 
@@ -33,13 +35,8 @@ def insync?(is)
     end
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to launch the instances.'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should not be blank' if value == ''
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:instance_type) do
diff --git a/lib/puppet/type/ec2_scalingpolicy.rb b/lib/puppet/type/ec2_scalingpolicy.rb
@@ -1,3 +1,5 @@
+require_relative '../../puppet_x/puppetlabs/property/region'
+
 Puppet::Type.newtype(:ec2_scalingpolicy) do
   @doc = 'Type representing an EC2 scaling policy.'
 
@@ -21,13 +23,8 @@
     end
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to launch the policy.'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should not be blank' if value == ''
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:adjustment_type) do
diff --git a/lib/puppet/type/ec2_securitygroup.rb b/lib/puppet/type/ec2_securitygroup.rb
@@ -1,4 +1,5 @@
-require_relative '../../puppet_x/puppetlabs/property/tag.rb'
+require_relative '../../puppet_x/puppetlabs/property/tag'
+require_relative '../../puppet_x/puppetlabs/property/region'
 require_relative '../../puppet_x/puppetlabs/aws_ingress_rules_parser'
 
 Puppet::Type.newtype(:ec2_securitygroup) do
@@ -14,12 +15,8 @@
     end
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'the region in which to launch the security group'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:ingress, :array_matching => :all) do
diff --git a/lib/puppet/type/ec2_vpc.rb b/lib/puppet/type/ec2_vpc.rb
@@ -1,4 +1,5 @@
-require_relative '../../puppet_x/puppetlabs/property/tag.rb'
+require_relative '../../puppet_x/puppetlabs/property/tag'
+require_relative '../../puppet_x/puppetlabs/property/region'
 
 Puppet::Type.newtype(:ec2_vpc) do
   @doc = 'A type representing an AWS VPC.'
@@ -13,12 +14,8 @@
     end
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to launch the VPC.'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:cidr_block) do
diff --git a/lib/puppet/type/ec2_vpc_customer_gateway.rb b/lib/puppet/type/ec2_vpc_customer_gateway.rb
@@ -1,4 +1,5 @@
-require_relative '../../puppet_x/puppetlabs/property/tag.rb'
+require_relative '../../puppet_x/puppetlabs/property/tag'
+require_relative '../../puppet_x/puppetlabs/property/region'
 
 Puppet::Type.newtype(:ec2_vpc_customer_gateway) do
   @doc = 'Type representing an AWS VPC customer gateways.'
@@ -31,12 +32,8 @@
     desc 'The tags for the customer gateway.'
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to launch the customer gateway.'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:type) do
diff --git a/lib/puppet/type/ec2_vpc_dhcp_options.rb b/lib/puppet/type/ec2_vpc_dhcp_options.rb
@@ -1,4 +1,5 @@
-require_relative '../../puppet_x/puppetlabs/property/tag.rb'
+require_relative '../../puppet_x/puppetlabs/property/tag'
+require_relative '../../puppet_x/puppetlabs/property/region'
 
 Puppet::Type.newtype(:ec2_vpc_dhcp_options) do
   @doc = 'Type representing a DHCP option set for AWS VPC.'
@@ -17,7 +18,7 @@
     desc 'Tags for the DHCP option set.'
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to assign the DHCP option set.'
     validate do |value|
       fail 'region should not contain spaces' if value =~ /\s/
diff --git a/lib/puppet/type/ec2_vpc_internet_gateway.rb b/lib/puppet/type/ec2_vpc_internet_gateway.rb
@@ -1,4 +1,5 @@
-require_relative '../../puppet_x/puppetlabs/property/tag.rb'
+require_relative '../../puppet_x/puppetlabs/property/tag'
+require_relative '../../puppet_x/puppetlabs/property/region'
 
 Puppet::Type.newtype(:ec2_vpc_internet_gateway) do
   @doc = 'Type representing an EC2 VPC Internet Gateway.'
@@ -17,12 +18,8 @@
     desc 'Tags to assign to the internet gateway.'
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to launch the internet gateway.'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:vpc) do
diff --git a/lib/puppet/type/ec2_vpc_routetable.rb b/lib/puppet/type/ec2_vpc_routetable.rb
@@ -1,4 +1,5 @@
-require_relative '../../puppet_x/puppetlabs/property/tag.rb'
+require_relative '../../puppet_x/puppetlabs/property/tag'
+require_relative '../../puppet_x/puppetlabs/property/region'
 
 Puppet::Type.newtype(:ec2_vpc_routetable) do
   @doc = 'Type representing a VPC route table.'
@@ -20,12 +21,8 @@
     end
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'Region in which to launch the route table.'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:routes, :array_matching => :all) do
diff --git a/lib/puppet/type/ec2_vpc_subnet.rb b/lib/puppet/type/ec2_vpc_subnet.rb
@@ -1,4 +1,5 @@
-require_relative '../../puppet_x/puppetlabs/property/tag.rb'
+require_relative '../../puppet_x/puppetlabs/property/tag'
+require_relative '../../puppet_x/puppetlabs/property/region'
 
 Puppet::Type.newtype(:ec2_vpc_subnet) do
   @doc = 'Type representing a VPC Subnet.'
@@ -20,12 +21,8 @@
     end
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to launch the subnet.'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:cidr_block) do
diff --git a/lib/puppet/type/ec2_vpc_vpn.rb b/lib/puppet/type/ec2_vpc_vpn.rb
@@ -1,4 +1,5 @@
-require_relative '../../puppet_x/puppetlabs/property/tag.rb'
+require_relative '../../puppet_x/puppetlabs/property/tag'
+require_relative '../../puppet_x/puppetlabs/property/region'
 
 Puppet::Type.newtype(:ec2_vpc_vpn) do
   @doc = 'Type representing an AWS Virtual Private Networks.'
@@ -53,12 +54,8 @@ def insync?(is)
     end
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to launch the VPN.'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:tags, :parent => PuppetX::Property::AwsTag) do
diff --git a/lib/puppet/type/ec2_vpc_vpn_gateway.rb b/lib/puppet/type/ec2_vpc_vpn_gateway.rb
@@ -1,4 +1,5 @@
-require_relative '../../puppet_x/puppetlabs/property/tag.rb'
+require_relative '../../puppet_x/puppetlabs/property/tag'
+require_relative '../../puppet_x/puppetlabs/property/region'
 
 Puppet::Type.newtype(:ec2_vpc_vpn_gateway) do
   @doc = 'A type representing a VPN gateway.'
@@ -24,12 +25,8 @@
     end
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to launch the VPN gateway.'
-    validate do |value|
-      fail 'region should not contain spaces' if value =~ /\s/
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newparam(:availability_zone) do
diff --git a/lib/puppet/type/elb_loadbalancer.rb b/lib/puppet/type/elb_loadbalancer.rb
@@ -1,4 +1,5 @@
-require_relative '../../puppet_x/puppetlabs/property/tag.rb'
+require_relative '../../puppet_x/puppetlabs/property/tag'
+require_relative '../../puppet_x/puppetlabs/property/region'
 
 Puppet::Type.newtype(:elb_loadbalancer) do
   @doc = 'Type representing an ELB load balancer.'
@@ -13,12 +14,8 @@
     end
   end
 
-  newproperty(:region) do
+  newproperty(:region, :parent => PuppetX::Property::AwsRegion) do
     desc 'The region in which to launch the load balancer.'
-    validate do |value|
-      fail 'region must not contain spaces' if value =~ /\s/
-      fail 'region should be a String' unless value.is_a?(String)
-    end
   end
 
   newproperty(:listeners, :array_matching => :all) do
diff --git a/lib/puppet_x/puppetlabs/property/region.rb b/lib/puppet_x/puppetlabs/property/region.rb
@@ -0,0 +1,15 @@
+module PuppetX
+  module Property
+    class AwsRegion < Puppet::Property
+      validate do |value|
+        name = resource[:name]
+        fail 'region should not contain spaces' if value =~ /\s/
+        fail 'region should not be blank' if value == ''
+        fail 'region should be a String' unless value.is_a?(String)
+        if !ENV['AWS_REGION'].nil? && ENV['AWS_REGION'] != value
+          fail "if using AWS_REGION environment variable it must match the specified region value for #{name}"
+        end
+      end
+    end
+  end
+end
diff --git a/spec/unit/type/ec2_elastic_ip_spec.rb b/spec/unit/type/ec2_elastic_ip_spec.rb
@@ -45,12 +45,12 @@
   it 'should require a region to be specified' do
     expect {
       type_class.new({ name: '10.0.0.1', region: '' })
-    }.to raise_error(Puppet::Error, /You must provide a region for Elastic IPs/)
+    }.to raise_error(Puppet::Error, /region should not be blank/)
   end
 
   it 'should require an instance to be specified' do
     expect {
-      type_class.new({ name: '10.0.0.1', region: 'us-east-1', instance: '' })
+      type_class.new({ name: '10.0.0.1', region: 'sa-east-1', instance: '' })
     }.to raise_error(Puppet::Error, /You must provide an instance for the Elastic IP association/)
   end
 
@@ -76,7 +76,7 @@
 
   context 'with valid properties' do
     it 'should create a valid elastic ip ' do
-      type_class.new({ name: '10.0.0.1', region: 'us-east-1', instance: 'web-1' })
+      type_class.new({ name: '10.0.0.1', region: 'sa-east-1', instance: 'web-1' })
     end
   end
 
