fix: allow importing public subnets with no MapPublicIPOnLaunch by iamhopaul123 · Pull Request #2457 · aws/copilot-cli

iamhopaul123 · 2021-06-14T20:07:43Z

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

efekarakus · 2021-06-14T20:19:23Z

-type ListVPCSubnetsOpts func([]*ec2.Subnet) []*ec2.Subnet
-
-// FilterForPublicSubnets is used to filter to get public subnets.
-func FilterForPublicSubnets() ListVPCSubnetsOpts {


Should we not be figuring out if a subnet has an internet gateway? so that we filter out the ones that are not relevant

It would be very difficult to tell since we need to check

if the VPC has an internet gateway attached to it

describe the route table check if it has route to the internet gateway and check if the route table is associated with the subnet (we would need to do it for every subnet within the VPC)

Is the drawback that this is too fragile? If we can provide a good experience by making these API calls and if there are no valid subnets, prompt customers what changes they'd need to make to their subnet in order for it to be useful to Copilot?

It's not too bad I think. We can describe the route tables for a given VPC from the user:

$ aws ec2 describe-route-tables --filters Name=vpc-id,Values=vpc-03e4614a3d0217256

In the response we'll look for SubnetId under Associations and for Routes to have a GatewayId that starts with igw-:

View JSON response

"Associations": [ { "Main": false, "RouteTableAssociationId": "rtbassoc-096f8559b5e31d9d3", "RouteTableId": "rtb-073d682766628c37f", "SubnetId": "subnet-018ccb78d353cec9b", "AssociationState": { "State": "associated" } }, { "Main": false, "RouteTableAssociationId": "rtbassoc-0ebf8e431225c1b20", "RouteTableId": "rtb-073d682766628c37f", "SubnetId": "subnet-005dc83a6473feee7", "AssociationState": { "State": "associated" } } ], "PropagatingVgws": [], "RouteTableId": "rtb-073d682766628c37f", "Routes": [ { "DestinationCidrBlock": "10.0.0.0/16", "GatewayId": "local", "Origin": "CreateRouteTable", "State": "active" }, { "DestinationCidrBlock": "0.0.0.0/0", "GatewayId": "igw-058421418aeacb2f0", "Origin": "CreateRoute", "State": "active" } ]

The only downside is we won't have tags associated with the subnets, but we can make two network calls to get that info.

ok addressed!

Lou1415926 · 2021-06-14T20:57:24Z

-				log.Errorf(`No existing private subnets were found in VPC %s. You can either:
- Create new private subnets and then import them.
+				log.Errorf(`No existing subnets were found in VPC %s. You can either:
+- Create new subnets and then import them as private subnets.


I see it a bit confusing to say 'import them as private subnets', since whether they're private subnets doesn't depend on how they import them 🤔

bvtujo · 2021-06-15T23:50:55Z

-type ListVPCSubnetsOpts func([]*ec2.Subnet) []*ec2.Subnet
-
-// FilterForPublicSubnets is used to filter to get public subnets.
-func FilterForPublicSubnets() ListVPCSubnetsOpts {


Is the drawback that this is too fragile? If we can provide a good experience by making these API calls and if there are no valid subnets, prompt customers what changes they'd need to make to their subnet in order for it to be useful to Copilot?

huanjani · 2021-06-17T20:40:57Z

+	Private []VPCResource
+}
+
+// ListVPCSubnets lists all subnets given a VPC ID. Note that public subnets


Suggested change

// ListVPCSubnets lists all subnets given a VPC ID. Note that public subnets

// ListVPCSubnets lists all subnets with a given VPC ID. Note that public subnets

huanjani · 2021-06-17T20:45:04Z

+}
+
+// ListVPCSubnets lists all subnets given a VPC ID. Note that public subnets
+// are subnets associated with a route table that routed by an internet gateway.


the wording of my fix is kinda weird but maybe a little clearer?

Suggested change

// are subnets associated with a route table that routed by an internet gateway.

// are subnets associated with an internet gateway through a route table.

huanjani · 2021-06-17T20:45:25Z

+
+// ListVPCSubnets lists all subnets given a VPC ID. Note that public subnets
+// are subnets associated with a route table that routed by an internet gateway.
+// And the rest subnets are private subnets.


Suggested change

// And the rest subnets are private subnets.

// And the rest of the subnets are private.

huanjani · 2021-06-17T20:50:37Z

-				log.Warningf(`No existing public subnets were found in VPC %s.
-If you proceed without public subnets, you will not be able to deploy Load Balanced Web Services in this environment.
+				log.Warningf(`No existing subnets were found in VPC %s.
+If you proceed without specifying public subnets, you will not be able to deploy Load Balanced Web Services in this environment.


Should we say "specifying two public subnets"?

yeah maybe "without at least two public subnets"

huanjani

Looks great! Just some tiny wording nits. Feel free to remove DNM label when those are addressed (or ignored) 😄 .

efekarakus · 2021-06-17T21:44:33Z

+// VPCResource contains the ID and name of a VPCResource.
+type VPCResource struct {


Why this rename? I liked reading ec2.VPC and ec2.Subnets better

Oh I see you wanted to use ID and Name across resources, what do you thikn of this alternative:

type resource struct { ID string Name string } type VPC resource

efekarakus · 2021-06-17T21:51:39Z

+// VPCSubnets are all subnets within a VPC.
+type VPCSubnets struct {
+	Public  []VPCResource
+	Private []VPCResource
+}


🤔 this is confusing, why is a subnet composed of VPC resources?

What do you think of refactoring to:

type Subnet struct { resource Public bool VPCID string } func (c *EC2) ListVPCSubnets(vpcID string) ([]Subnet, error) {

hmm following the logic here #2457 (comment) i think it might be clearer to just have

// VPCSubnets are all subnets within a VPC. type VPCSubnets struct { Public []Subnet Private []Subnet }

efekarakus · 2021-06-17T22:36:08Z

+	inputFilters := toEC2Filter(filters)
+	var routeTables []*ec2.RouteTable
+	input := &ec2.DescribeRouteTablesInput{
+		Filters: inputFilters,
+	}


nit:

Suggested change

inputFilters := toEC2Filter(filters)

var routeTables []*ec2.RouteTable

input := &ec2.DescribeRouteTablesInput{

Filters: inputFilters,

}

var routeTables []*ec2.RouteTable

input := &ec2.DescribeRouteTablesInput{

Filters: toEC2Filter(filters),

}

efekarakus · 2021-06-17T22:36:58Z

-				log.Warningf(`No existing public subnets were found in VPC %s.
-If you proceed without public subnets, you will not be able to deploy Load Balanced Web Services in this environment.
+				log.Warningf(`No existing subnets were found in VPC %s.
+If you proceed without specifying public subnets, you will not be able to deploy Load Balanced Web Services in this environment.


yeah maybe "without at least two public subnets"

efekarakus · 2021-06-17T22:44:39Z

 // PublicSubnets has the user multiselect public subnets given the VPC ID.
 func (s *EC2Select) PublicSubnets(prompt, help, vpcID string) ([]string, error) {
-	return s.subnet(prompt, help, vpcID, ec2.FilterForPublicSubnets())
+	return s.subnet(prompt, help, vpcID, true)


boolean params are usually a codesmell because it's not clear what the boolean stands for: https://martinfowler.com/bliki/FlagArgument.html

return s.selectPublicSubnets(prompt, help, vpcID) return s.selectPrivateSubnets(prompt, help, vpcID) func (s *EC2Select) selectPublicSubnets(prompt, help, vpcID string) ([]string, error) { subnets, err := s.ec2.ListVPCSubnets(vpcID) if err != nil { ... } publicSubnets := .... // for-loop and filter on .Public return s.selectSubnets(prompt, help, publicSubnets) // This method is common for s.selectPrivateSubnets }

efekarakus

Feel free to merge after the nit!

efekarakus · 2021-06-18T16:58:41Z

-// VPC contains the ID and name of a VPC.
-type VPC struct {
+// Resource contains the ID and name of a EC2 resource.
+type Resource struct {


does this need to be public?
You can still access the public fields even if the embedded struct is private so you should be able to do vpc.ID

It's for unit test in the other pkg https://github.com/aws/copilot-cli/pull/2457/files#diff-c870c9dd1b1575fbdd8a11486b23829bc2d95e6121202ea397c5f14c528fa53dR48-R50

…2457)  fixes aws#1724, fixes aws#2287  By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

iamhopaul123 requested a review from a team as a code owner June 14, 2021 20:07

iamhopaul123 requested a review from huanjani June 14, 2021 20:07

efekarakus changed the title ~~chore: allow importing public subnets with no MapPublicIPOnLaunch~~ fix: allow importing public subnets with no MapPublicIPOnLaunch Jun 14, 2021

efekarakus reviewed Jun 14, 2021

View reviewed changes

Lou1415926 reviewed Jun 14, 2021

View reviewed changes

iamhopaul123 force-pushed the better-public-subnet-import branch from 83cdd88 to 27e70aa Compare June 14, 2021 22:38

bvtujo approved these changes Jun 15, 2021

View reviewed changes

efekarakus added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 16, 2021

iamhopaul123 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 16, 2021

iamhopaul123 added 4 commits June 16, 2021 14:20

chore: allow importing public subnets with no MapPublicIPOnLaunch

eafc44d

Remove unused code

a048ca1

Fix error log message

388448b

Filter public subnets by checking if an internet gateway is associated

bd0c59e

iamhopaul123 force-pushed the better-public-subnet-import branch from 27e70aa to bd0c59e Compare June 16, 2021 23:31

huanjani reviewed Jun 17, 2021

View reviewed changes

huanjani added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 17, 2021

huanjani approved these changes Jun 17, 2021

View reviewed changes

efekarakus reviewed Jun 17, 2021

View reviewed changes

Address feedback

86be194

iamhopaul123 force-pushed the better-public-subnet-import branch from d18b6d1 to 86be194 Compare June 17, 2021 23:44

Split ExtractResource into VPC and subnet specific

635aa86

efekarakus approved these changes Jun 18, 2021

View reviewed changes

iamhopaul123 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 18, 2021

Merge branch 'mainline' into better-public-subnet-import

d1cc1f5

mergify Bot merged commit cc8ecfc into aws:mainline Jun 18, 2021

	// ListVPCSubnets lists all subnets given a VPC ID. Note that public subnets
	// ListVPCSubnets lists all subnets with a given VPC ID. Note that public subnets

	// are subnets associated with a route table that routed by an internet gateway.
	// are subnets associated with an internet gateway through a route table.

	// And the rest subnets are private subnets.
	// And the rest of the subnets are private.

		// VPCResource contains the ID and name of a VPCResource.
		type VPCResource struct {

Conversation

iamhopaul123 commented Jun 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamhopaul123 Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huanjani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efekarakus Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efekarakus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

iamhopaul123 Jun 14, 2021 •

edited

Loading

efekarakus Jun 17, 2021 •

edited

Loading