(CLOUD-184, CLOUD-205) Avoid ambiguity when using the AWS_REGION environment var #401
Conversation
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.
…/github.com/garethr/puppetlabs-aws into garethr-avoid-region-clash-between-env-and-property
ahenroid
changed the title
| @@ -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| | |||
There was a problem hiding this comment.
This validate can likely be removed now too.
There was a problem hiding this comment.
Ah! Good catch. I missed this one.
| module PuppetX | ||
| module Property | ||
| class AwsRegion < Puppet::Property | ||
| validate do |value| |
There was a problem hiding this comment.
I suspect the failure cases can be cleaned up here a bit. If we get a value, is it a string, else fail. If its a string, match a regex to ensure its a valid region.
| 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 |
There was a problem hiding this comment.
The logic is hard for me to read. Is there another way that this can be written to provide a bit more clarity about the intent?
| 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}" |
There was a problem hiding this comment.
For my own understanding, this is the case because we call *_client methods in the providers, and those use the default region, correct?
ahenroid
force-pushed
the
garethr-avoid-region-clash-between-env-and-property
branch
from
e177daf to
40fef22
Compare
|
Thanks for the feedback, @zleswomp! I've addressed each of your comments. |
| validate do |value| | ||
| name = resource[:name] | ||
| fail 'region should be a String' unless value.is_a?(String) | ||
| fail 'region should be a valid AWS region' unless value =~ /^(ap-(north|south)east-[12]|ap-south-1|ca-central-1|eu-west-[12]|eu-central-1|sa-east-1|us-(east|west)-[12])$/ |
There was a problem hiding this comment.
Every time AWS adds a region, this code will need to be changed. I feel a more generic regex would be better.
Ex [a-z]{2}-[a-z]+-\d
There was a problem hiding this comment.
Good point, @zabacad. I modified the regex
ahenroid
force-pushed
the
garethr-avoid-region-clash-between-env-and-property
branch
from
40fef22 to
4cc3c39
Compare
|
This looks good to me. Thanks for bringing this up to date! |
No description provided.