remove check for existing ips on local.nat_gateways_count

[joe-niland]

what

• Removed check which caused NAT instances/gateways not to be created if existing Elastic IPs provided
• It's not clear why this check was added

why

• We need the ability to assign existing elastic IPs to NAT instances/gateways

references

• closes Nat instances not created when using pre existing eips #101

[joe-niland]

@etwillbefine I was wondering why this check was added: https://github.com/cloudposse/terraform-aws-dynamic-subnets/pull/86/files#diff-994b65bfe2a964a586187336675fc84a4099482f4444273426a95fc0a79f62c7R11

This PR removes it but it'd be good to know if it served a particular purpose, as I am not sure!

[joe-niland]

/test all

[maximmi]

@joe-niland I don't know why this check was implemented in first place, but as soon as this fix makes things works it LGTM

[Nuru]

@joe-niland @maximmi @osterman @aknysh I am worried this PR changes the existing behavior in an incompatible way. Despite the language in #42, which was closed by PR #86 which introduced "existing IPs", and the description of the existing_nat_ips, the current behavior is that if you provide existing IPs, they are assumed to be connected to existing NAT gateways or instances. No new gateways are created. "existing_nat_ips" is interpreted to mean the IPs of existing NATs.

That said, the existing behavior makes no sense. Nothing is done with the existing IPs, all that happens is that if you provide them, no NATs are created. The same effect could be achieved by setting both nat_gateway_enabled and nat_instance_enabled to false.

So I propose a breaking change that renames existing_nat_ips to anything else, maybe nat_elastic_ips, so that anyone using existing_nat_ips will be notified about the change in behavior, which otherwise could go unnoticed and be expensive.

[joe-niland]

@Nuru @maximmi sorry, I should have pinged you guys directly.

@Nuru great point. I agree that the original intention of this variable seems to be to support existing NAT devices. It seems that there was an attempt at https://github.com/cloudposse/terraform-aws-dynamic-subnets/blob/master/nat-gateway.tf#L35 but it's contradicted by the check above.

I didn't consider the important cost implication of this change!

I've made the suggested change.

This Pull Request has been updated, so we're dismissing all reviews.

[joe-niland]

/test all

[joe-niland]

/test all

[osterman]

I agree with @Nuru 's recommendations.

[osterman]

(I take responsiblity for #86 =) - I thought it was just reusing existing Elastic IPs and associating them with a new NAT gateway or instance)