search

puppetlabs-toy-chest / puppetlabs-aws

Puppet module for managing AWS resources to build out infrastructure
Apache License 2.0
188 stars 219 forks source link

ec2_vpc_routetable idempotence issue #346

Open btrump opened 8 years ago

btrump commented 8 years ago

I'm using the following simple manifest to create a VPC, internet gateway, and routetable:

# test.pp
ec2_vpc { 'test-id-vpc':
  ensure           => present,
  region           => 'us-west-2',
  cidr_block       => '10.0.0.0/24',
  instance_tenancy => 'default',
}
ec2_vpc_internet_gateway { 'test-id-vpc-igw':
  ensure => 'present',
  region => 'us-west-2',
  vpc => 'test-id-vpc',
  require => Ec2_vpc['test-id-vpc'],
}
ec2_vpc_routetable { 'test-id-vpc-routetable':
  ensure => 'present',
  region => 'us-west-2',
  routes => [
    {'destination_cidr_block' => '10.0.0.0/24', 'gateway' => 'local'},
    {'destination_cidr_block' => '0.0.0.0/0', 'gateway' => 'test-id-vpc-igw'},
  ],
  vpc    => 'test-id-vpc',
  require => Ec2_vpc_internet_gateway['test-id-vpc-igw'],
}

A one-time run works just fine:

# puppet apply test.pp 
Notice: Compiled catalog for puppet in environment production in 0.31 seconds
Notice: /Stage[main]/Main/Ec2_vpc[test-id-vpc]/ensure: created
Notice: /Stage[main]/Main/Ec2_vpc_internet_gateway[test-id-vpc-igw]/ensure: created
Notice: /Stage[main]/Main/Ec2_vpc_routetable[test-id-vpc-routetable]/ensure: created
Notice: Applied catalog in 39.24 seconds`

However, subsequent runs produce an error stating that the routes property is read-only once ec2_vpc_routetable is created:

# puppet apply test.pp 
Notice: Compiled catalog for puppet in environment production in 0.34 seconds
Error: routes property is read-only once ec2_vpc_routetable created.
Error: /Stage[main]/Main/Ec2_vpc_routetable[test-id-vpc-routetable]/routes: change from [{"destination_cidr_block"=>"10.0.0.0/24", "gateway"=>"local"}] to {"destination_cidr_block"=>"10.0.0.0/24", "gateway"=>"local"} {"destination_cidr_block"=>"0.0.0.0/0", "gateway"=>"test-id-vpc-igw"} failed: routes property is read-only once ec2_vpc_routetable created.
Notice: Applied catalog in 36.66 seconds

I understand that the property is currently read-only, but should it not check that the values passed are identical to the current route property, thereby allowing the code to run multiple times without error?

prozach commented 8 years ago

@hunner @DavidS Here is another case where a create_only method would be useful, or some means to ignore a property once the thing already exists.

This might also just be a bug in the in_sync? method on the property, but if we can't modify it, then whats the point of checking other than to report a warning.

@btrump I too don't think this should be an error. Would a warning suffice for you in the case where the desired state does not match the actual state?

btrump commented 8 years ago

@zleslie @zleswomp a warning would be great. I just don't want a compilation failure, because I'm only setting the state of routes at creation, but hitting the same resource declaration every run.

prozach commented 8 years ago

Yeah, I agree with that assessment @btrump. I've been chatting with Hunner and DavidS about this issue and filed a ticket to track this effort: https://tickets.puppetlabs.com/browse/MODULES-3703

btrump commented 8 years ago

Thank you! It would mean huge progress for our team.

Soprano74 commented 8 years ago

btrump: what is the settings of your route table currently after running Puppet? Did it perhaps switch to the CIDR of the AWS instance?

For example, my subnet for the VPC is 10.15.0.0/16. But I push the route table using 10.0.0.0/16 to 'local'. AWS changes this to 10.15.0.0/16, hence Puppet decides it needs to change it again, and thats not allowed. When I correct it to 10.15.0.0/16, then i can keep running the resource without issues.

nrvale0 commented 7 years ago

I understand the desire to come up with a pattern for handling create-once resources but I'm left wondering how hard it would be to have the Provider simply compare desired state with actual state and error out if they don't match as opposed to what appears to be happening now: "error out if resource has parameter/property (sorry, forget the diff right now) which cannot be mutated."

If it cannot be mutated but it also doesn't need to be there's no reason to generate an error. The Provider is being too generous with its errors, IMO.

prozach commented 7 years ago

The PR to replace the error with a warning has been merged here. https://github.com/puppetlabs/puppetlabs-aws/pull/334

This will only throw a warning now, and only in the case where the desired state doesn't match actual state. If no changes are made to the Puppet resource, and there is still a warning that is being thrown, than the provider is not correctly describing the actual state, and thats a bug in the provider.