Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add map_public_ip_on_launch variable #23

Merged
merged 1 commit into from
Oct 19, 2020

Conversation

alexjurkiewicz
Copy link
Contributor

what

  • Support map_public_ip_on_launch for public subnets

why

  • This is most useful when deploying public subnets with nat_enabled = false

@alexjurkiewicz alexjurkiewicz requested a review from a team as a code owner October 17, 2020 08:44
@alexjurkiewicz alexjurkiewicz requested review from Gowiem and aknysh and removed request for a team October 17, 2020 08:44
variables.tf Outdated Show resolved Hide resolved
@alexjurkiewicz
Copy link
Contributor Author

Thanks for the advice about README updating. I've extended this variable to work for both public & private subnets. The use-cases for private subnets with public IPs are pretty niche (self-managed network gateways, other?) but they do exist, and it was a special case to not support this anyway.

@Gowiem
Copy link
Member

Gowiem commented Oct 19, 2020

/test all

@Gowiem
Copy link
Member

Gowiem commented Oct 19, 2020

@alexjurkiewicz Ah sorry to do it to you after already asking for changes, but can also ask you to add separate variables for private vs public subnets? I worry that if folks go to enable this, they won't expect it to take effect on their private subnets, which could in turn create a security issue for them.

@alexjurkiewicz
Copy link
Contributor Author

alexjurkiewicz commented Oct 19, 2020

Since this module can only create public or private subnets (not both at the same time), what sort of scenario are you thinking of where people could enable this accidentally on private subnets?

I'm guessing you're thinking of some scenario with for_each. But it doesn't seem to be that tricky of a thing to deal with:

module "subnets" {
  for_each = toset(["public", "private"])
  source                  = "git::https://github.com/cloudposse/terraform-aws-named-subnets.git?ref=master"
  # other params skipped
  type                    = each.key
  map_public_ip_on_launch = each.key == "public"
}

You would be performing similar logic for other params like igw_id/ngw_id, cidr_block, etc.

@Gowiem
Copy link
Member

Gowiem commented Oct 19, 2020

@alexjurkiewicz Solid point! I haven't used this module personally (I typically use terraform-aws-dynamic-subnets), so I didn't know that it was meant to create one set of subnet type or another.

@Gowiem Gowiem merged commit 247776e into cloudposse:master Oct 19, 2020
@Gowiem
Copy link
Member

Gowiem commented Oct 19, 2020

@alexjurkiewicz
Thanks for the contribution! Released as 0.8.0.

@alexjurkiewicz alexjurkiewicz deleted the patch-1 branch October 19, 2020 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants