Updated behavior, features, and variable names

[Nuru]

Motivation

Cloud Posse is undertaking an effort to modernize and standardize its open source AWS Terraform modules with respect to handling AWS Security Groups, optional inputs, and certain other things. This module, terraform-aws-vpc is schedule for such modernization, and Cloud Posse is taking this opportunity to introduce breaking changes if needed. Changes that have been or would have been rejected earlier as nice but breaking are now being considered, and after this revision, this module will again go back to resisting breaking changes. This is the time to get those fixes in.

Please propose or comment on proposed breaking changes you would like to advocate for or against.

Changes (with decisions in [brackets])

Standardized input names, part 1: provider inputs.

Cloud Posse standard is that boolean inputs have names that end with _enabled. However, Cloud Posse seeks to use the same input names as the related Terraform AWS Resource to provide consistency with the documentation. The vpc resource provides a challenge in this regard. The resource, and the current version of this module, provide inputs named:

• enable_dns_hostnames

• enable_dns_support

• enable_classiclink

• enable_classiclink_dns_support

• assign_generated_ipv6_cidr_block

  1. Should we keep these variables with these name? [Yes, but mark them deprecated]

  2. Should rename these variables according to Cloud Posse naming conventions: [Yes]

  • enable_dns_hostnames -> dns_hostnames_enabled
  • enable_dns_support -> dns_support_enabled
  • enable_classiclink -> classiclink_enabled
  • enable_classiclink_dns_support -> classiclink_dns_support_enabled
  • assign_generated_ipv6_cidr_block -> ipv6_enabled
  3. Should we keep the old names and add the new names as aliases? [Yes. Create new file variables-deprecated.tf and put deprecated variables in it.]

Standardized input names, part 2: other inputs.

1. The input enable_internet_gateway is not a standard Terraform input, but follows the naming convention of the other inputs. Should we treat it like the above inputs and either leave it, rename it to internet_gateway_enabled, or alias it? [treat as above]

2. There is a Feature Request (Egress only internet gateway #93) and a PR (feat[#93]: Added aws_egress_only_internet_gateway #94) requesting that we add to this module the ability to create an Egress-only Internet Gateway. Such a gateway provides the same general security benefit for IPv6 that a NAT gateway provides for IPv4, but does not actually do Network Address Translation. This is because there is no such thing in an AWS VPC as a "private" IPv6 subnet; all IPv6 addresses are public. Because the IPv6 address space is big enough, there is no need for Network Address Translation, but since all the addresses are public, there is still a need for a way to block initiation of inbound connections: that is what the Egress-Only Gateway does. If we implement the feature, what should we name the input, given the decisions above. Should it be egress_only_internet_gateway_enabled? Or, given that it is a mistake to provision both a bidirectional internet gateway an an egress-only gateway at the same time, should we create a flag like internet_gateway_is_egress_only and have it toggle the kind of gateway created when internet_gateway_enabled is true?

[It is not necessarily a mistake to have both an internet gateway and an egress-only IPv6 network, since you can use routing tables and network ACLs to cordon off some servers to egress only. Add new flag ipv6_egress_only_internet_gateway_enabled]

Default Security Group

The input enable_default_security_group_with_custom_rules should be replaced as part of the security group upgrade. Probably the terraform-aws-security-group module should be enhanced to take the same rule sets and apply them to the default security group, so that we have consistent input formats for security group rules and no duplication of code parsing them. However, given the Bridegcrew recommendation that the default security group should prohibit all ingress and egress, perhaps we should just enforce that and encourage people to use non-default security groups.

If we are going to allow custom configuration of the default security group, then we should do it by accepting rules in the format of terraform-aws-security-group and using that module to manage them.

[Replace enable_default_security_group_with_custom_rules with default_security_group_deny_all. When true (the default), manage the default security group and remove all rules, disabling all ingress and egress. When false, do not manage the default security group, allowing it to be managed by another component.]

[max-lobur]

Standardized input names, part 1: provider inputs.

• rename these variables according to Cloud Posse naming conventions:

Standardized input names, part 2: other inputs.

• rename it to internet_gateway_enabled
  but use vars_deprecations.tf to alias to previous for some number of releases
• internet_gateway_enabled + ipv6_internet_gateway_type where the latter is "EgressOnly" (and string comparison inside the module)

Default Security Group

• encourage people to use non-default security groups. - I double this

[osterman]

Standardized input names, part 1: provider inputs.

I agree with all changes.

Should we keep these variables with these name?

Lets rename, but introduce the variables-deprecated.tf pattern and proxy with locals

Should rename these variables according to Cloud Posse naming conventions:
enable_dns_hostnames -> dns_hostnames_enabled
enable_dns_support -> dns_support_enabled
enable_classiclink -> classiclink_enabled
enable_classiclink_dns_support -> classiclink_dns_support_enabled
assign_generated_ipv6_cidr_block -> ipv6_enabled

It's painful, but yes. I think this long predated any of our conventions. The reason for the suffixed _enabled flag is so we can group similar parameters lexicographically.

Should we keep the old names and add the new names as aliases?

Yes, though we should make it clear that they are deprecated.

[osterman]

Standardized input names, part 2: other inputs.
The input enable_internet_gateway is not a standard Terraform input, but follows the naming convention of the other inputs. > Should we treat it like the above inputs and either leave it, rename it to internet_gateway_enabled, or alias it?

I think so. Let's rename, deprecate and alias.

There is a Feature Request (Egress only internet gateway #93) and a PR (feat[#93]: Added aws_egress_only_internet_gateway #94) requesting that we add to this module the ability to create an Egress-only Internet Gateway. Such a gateway provides the same general security benefit for IPv6 that a NAT gateway provides for IPv4, but does not actually do Network Address Translation. This is because there is no such thing in an AWS VPC as a "private" IPv6 subnet; all IPv6 addresses are public. Because the IPv6 address space is big enough, there is no need for Network Address Translation, but since all the addresses are public, there is still a need for a way to block initiation of inbound connections: that is what the Egress-Only Gateway does.

If we implement the feature, what should we name the input, given the decisions above. Should it be egress_only_internet_gateway_enabled?

I think it should be internet_gateway_<something> so we logically group related parameters.

internet_gateway_egress_only_enabled with a validator that expects ipv6_enabled to be true.

Or, given that it is a mistake to provision both a bidirectional internet gateway an an egress-only gateway at the same time,

I don't have enough experience to answer this.

should we create a flag like internet_gateway_is_egress_only and have it toggle the kind of gateway created when internet_gateway_enabled is true?

I hate boolean explosion. I'd prefer then we have a new variable called internet_gateway_type which can be evaluated in locals to boolean flags.

[osterman]

Default Security Group
The input enable_default_security_group_with_custom_rules should be replaced as part of the security group upgrade.

Agree

Probably the terraform-aws-security-group module should be enhanced to take the same rule sets and apply them to the default security group, so that we have consistent input formats for security group rules and no duplication of code parsing
them.

No, I don't think we should muck with the default security group.

However, maybe we add a feature flag that is delete_default_vpc (or I don't know what to call it) and it does this https://registry.terraform.io/providers/cloudposse/awsutils/latest/docs/resources/awsutils_default_vpc_deletion

However, given the Bridegcrew recommendation that the default security group should prohibit all ingress and egress, perhaps we should just enforce that and encourage people to use non-default security groups.

So I think this could be our defaults, for example, in the new VPC. Thus by deleting the default VPC and setting these secure defaults, we achieve the best practice.

If we are going to allow custom configuration of the default security group, then we should do it by accepting rules in the format of terraform-aws-security-group and using that module to manage them.

Can we change what security group is the "default" to one we manage?