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.

Sign up for GitHub

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

Fix errors in EC2 Transit Gateway VPC Attachment when TGW is shared from another account #7513

Merged
merged 2 commits into from
Mar 14, 2019
Merged

Fix errors in EC2 Transit Gateway VPC Attachment when TGW is shared from another account #7513

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5d9af9
Fix EC2 Transit Gateway VPC Attachment "Transit gateway route table w…
andrew-corbalt Feb 12, 2019
fb3a1b3
Fix failing unit test
andrewsuperbrilliant Feb 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 72 additions & 23 deletions aws/resource_aws_ec2_transit_gateway_vpc_attachment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,19 @@ func resourceAwsEc2TransitGatewayVpcAttachmentCreate(d *schema.ResourceData, met

transitGatewayID := d.Get("transit_gateway_id").(string)

resp, err := conn.DescribeVpcs(&ec2.DescribeVpcsInput{
VpcIds: []*string{aws.String(d.Get("vpc_id").(string))},
})

if err != nil {
return err
}

if resp.Vpcs == nil || len(resp.Vpcs) == 0 {
return fmt.Errorf("VPC not found.")
}
vpcOwnerID := *resp.Vpcs[0].OwnerId

input := &ec2.CreateTransitGatewayVpcAttachmentInput{
Options: &ec2.CreateTransitGatewayVpcAttachmentRequestOptions{
DnsSupport: aws.String(d.Get("dns_support").(string)),
Expand Down Expand Up @@ -117,12 +130,14 @@ func resourceAwsEc2TransitGatewayVpcAttachmentCreate(d *schema.ResourceData, met
return fmt.Errorf("error describing EC2 Transit Gateway (%s): missing options", transitGatewayID)
}

if err := ec2TransitGatewayRouteTableAssociationUpdate(conn, aws.StringValue(transitGateway.Options.AssociationDefaultRouteTableId), d.Id(), d.Get("transit_gateway_default_route_table_association").(bool)); err != nil {
return fmt.Errorf("error updating EC2 Transit Gateway Attachment (%s) Route Table (%s) association: %s", d.Id(), aws.StringValue(transitGateway.Options.AssociationDefaultRouteTableId), err)
}
if *transitGateway.OwnerId == vpcOwnerID {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the above DescribeVpcs API call by checking the VPC owner ID available from creating the VPC attachment here. We should also leave an inline comment for future code maintainers about why this check is here.

Suggested change
if *transitGateway.OwnerId == vpcOwnerID {
// We cannot modify Transit Gateway Route Tables for Resource Access Manager shared Transit Gateways
if aws.StringValue(transitGateway.OwnerId) == aws.StringValue(output.TransitGatewayVpcAttachment.VpcOwnerId) {

if err := ec2TransitGatewayRouteTableAssociationUpdate(conn, aws.StringValue(transitGateway.Options.AssociationDefaultRouteTableId), d.Id(), d.Get("transit_gateway_default_route_table_association").(bool)); err != nil {
return fmt.Errorf("error updating EC2 Transit Gateway Attachment (%s) Route Table (%s) association: %s", d.Id(), aws.StringValue(transitGateway.Options.AssociationDefaultRouteTableId), err)
}

if err := ec2TransitGatewayRouteTablePropagationUpdate(conn, aws.StringValue(transitGateway.Options.PropagationDefaultRouteTableId), d.Id(), d.Get("transit_gateway_default_route_table_propagation").(bool)); err != nil {
return fmt.Errorf("error updating EC2 Transit Gateway Attachment (%s) Route Table (%s) propagation: %s", d.Id(), aws.StringValue(transitGateway.Options.PropagationDefaultRouteTableId), err)
if err := ec2TransitGatewayRouteTablePropagationUpdate(conn, aws.StringValue(transitGateway.Options.PropagationDefaultRouteTableId), d.Id(), d.Get("transit_gateway_default_route_table_propagation").(bool)); err != nil {
return fmt.Errorf("error updating EC2 Transit Gateway Attachment (%s) Route Table (%s) propagation: %s", d.Id(), aws.StringValue(transitGateway.Options.PropagationDefaultRouteTableId), err)
}
}

return resourceAwsEc2TransitGatewayVpcAttachmentRead(d, meta)
Expand Down Expand Up @@ -155,6 +170,19 @@ func resourceAwsEc2TransitGatewayVpcAttachmentRead(d *schema.ResourceData, meta
return nil
}

resp, err := conn.DescribeVpcs(&ec2.DescribeVpcsInput{
VpcIds: []*string{transitGatewayVpcAttachment.VpcId},
})

if err != nil {
return err
}

if resp.Vpcs == nil || len(resp.Vpcs) == 0 {
return fmt.Errorf("VPC not found.")
}
vpcOwnerID := *resp.Vpcs[0].OwnerId

transitGatewayID := aws.StringValue(transitGatewayVpcAttachment.TransitGatewayId)
transitGateway, err := ec2DescribeTransitGateway(conn, transitGatewayID)
if err != nil {
Expand All @@ -165,16 +193,20 @@ func resourceAwsEc2TransitGatewayVpcAttachmentRead(d *schema.ResourceData, meta
return fmt.Errorf("error describing EC2 Transit Gateway (%s): missing options", transitGatewayID)
}

transitGatewayAssociationDefaultRouteTableID := aws.StringValue(transitGateway.Options.AssociationDefaultRouteTableId)
transitGatewayDefaultRouteTableAssociation, err := ec2DescribeTransitGatewayRouteTableAssociation(conn, transitGatewayAssociationDefaultRouteTableID, d.Id())
if err != nil {
return fmt.Errorf("error determining EC2 Transit Gateway Attachment (%s) association to Route Table (%s): %s", d.Id(), transitGatewayAssociationDefaultRouteTableID, err)
}
var transitGatewayDefaultRouteTableAssociation *ec2.TransitGatewayRouteTableAssociation
var transitGatewayDefaultRouteTablePropagation *ec2.TransitGatewayRouteTablePropagation
if *transitGateway.OwnerId == vpcOwnerID {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the above DescribeVpcs API call by using the Transit Gateway VPC Attachment information we already have available. We should also leave an inline comment here for future code maintainers about this conditional. Please note this whole comment details about the defaults should be included to go along with a change below.

	// We cannot read Transit Gateway Route Tables for Resource Access Manager shared Transit Gateways
	// Default these to a non-nil value so we can match the existing schema of Default: true
	transitGatewayDefaultRouteTableAssociation := &ec2.TransitGatewayRouteTableAssociation{}
	transitGatewayDefaultRouteTablePropagation := &ec2.TransitGatewayRouteTablePropagation{}
	if aws.StringValue(transitGateway.OwnerId) == aws.StringValue(transitGatewayVpcAttachment.VpcOwnerId) {

transitGatewayAssociationDefaultRouteTableID := aws.StringValue(transitGateway.Options.AssociationDefaultRouteTableId)
transitGatewayDefaultRouteTableAssociation, err = ec2DescribeTransitGatewayRouteTableAssociation(conn, transitGatewayAssociationDefaultRouteTableID, d.Id())
if err != nil {
return fmt.Errorf("error determining EC2 Transit Gateway Attachment (%s) association to Route Table (%s): %s", d.Id(), transitGatewayAssociationDefaultRouteTableID, err)
}

transitGatewayPropagationDefaultRouteTableID := aws.StringValue(transitGateway.Options.PropagationDefaultRouteTableId)
transitGatewayDefaultRouteTablePropagation, err := ec2DescribeTransitGatewayRouteTablePropagation(conn, transitGatewayPropagationDefaultRouteTableID, d.Id())
if err != nil {
return fmt.Errorf("error determining EC2 Transit Gateway Attachment (%s) propagation to Route Table (%s): %s", d.Id(), transitGatewayPropagationDefaultRouteTableID, err)
transitGatewayPropagationDefaultRouteTableID := aws.StringValue(transitGateway.Options.PropagationDefaultRouteTableId)
transitGatewayDefaultRouteTablePropagation, err = ec2DescribeTransitGatewayRouteTablePropagation(conn, transitGatewayPropagationDefaultRouteTableID, d.Id())
if err != nil {
return fmt.Errorf("error determining EC2 Transit Gateway Attachment (%s) propagation to Route Table (%s): %s", d.Id(), transitGatewayPropagationDefaultRouteTableID, err)
}
}

if transitGatewayVpcAttachment.Options == nil {
Expand All @@ -192,8 +224,10 @@ func resourceAwsEc2TransitGatewayVpcAttachmentRead(d *schema.ResourceData, meta
return fmt.Errorf("error setting tags: %s", err)
}

d.Set("transit_gateway_default_route_table_association", (transitGatewayDefaultRouteTableAssociation != nil))
d.Set("transit_gateway_default_route_table_propagation", (transitGatewayDefaultRouteTablePropagation != nil))
if *transitGateway.OwnerId == vpcOwnerID {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional should be removed to match with the above change. 👍 Without calling d.Set() for the below attributes in the RAM shared Transit Gateway scenario, operators will be presented a plan immediately after import similar to:

"transit_gateway_default_route_table_association": "" => "true"
"transit_gateway_default_route_table_propagation": "" => "true"

d.Set("transit_gateway_default_route_table_association", (transitGatewayDefaultRouteTableAssociation != nil))
d.Set("transit_gateway_default_route_table_propagation", (transitGatewayDefaultRouteTablePropagation != nil))
}
d.Set("transit_gateway_id", aws.StringValue(transitGatewayVpcAttachment.TransitGatewayId))
d.Set("vpc_id", aws.StringValue(transitGatewayVpcAttachment.VpcId))
d.Set("vpc_owner_id", aws.StringValue(transitGatewayVpcAttachment.VpcOwnerId))
Expand All @@ -204,6 +238,19 @@ func resourceAwsEc2TransitGatewayVpcAttachmentRead(d *schema.ResourceData, meta
func resourceAwsEc2TransitGatewayVpcAttachmentUpdate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ec2conn

resp, err := conn.DescribeVpcs(&ec2.DescribeVpcsInput{
VpcIds: []*string{aws.String(d.Get("vpc_id").(string))},
})

if err != nil {
return err
}

if resp.Vpcs == nil || len(resp.Vpcs) == 0 {
return fmt.Errorf("VPC not found.")
}
vpcOwnerID := *resp.Vpcs[0].OwnerId

if d.HasChange("dns_support") || d.HasChange("ipv6_support") || d.HasChange("subnet_ids") {
input := &ec2.ModifyTransitGatewayVpcAttachmentInput{
Options: &ec2.ModifyTransitGatewayVpcAttachmentRequestOptions{
Expand Down Expand Up @@ -246,15 +293,17 @@ func resourceAwsEc2TransitGatewayVpcAttachmentUpdate(d *schema.ResourceData, met
return fmt.Errorf("error describing EC2 Transit Gateway (%s): missing options", transitGatewayID)
}

if d.HasChange("transit_gateway_default_route_table_association") {
if err := ec2TransitGatewayRouteTableAssociationUpdate(conn, aws.StringValue(transitGateway.Options.AssociationDefaultRouteTableId), d.Id(), d.Get("transit_gateway_default_route_table_association").(bool)); err != nil {
return fmt.Errorf("error updating EC2 Transit Gateway Attachment (%s) Route Table (%s) association: %s", d.Id(), aws.StringValue(transitGateway.Options.AssociationDefaultRouteTableId), err)
if *transitGateway.OwnerId == vpcOwnerID {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that it would be invalid to attempt adjusting these configuration arguments since we cannot manage the EC2 Transit Gateway Default Route Table for a RAM shared Transit Gateway, we can either remove all these proposed resourceAwsEc2TransitGatewayVpcAttachmentUpdate changes and allow the API to return any errors (I personally think this is preferable in this situation, paired with updates to the resource documentation) or continue with these adjustments where Terraform will instead just return a perpetual plan difference, e.g. "true" => "false".

If we do keep these changes, we can remove the above DescribeVpcs API call above by using information present in the Terraform state here. 👍 We should also provide an inline comment for future code maintainers about this conditional.

Suggested change
if *transitGateway.OwnerId == vpcOwnerID {
// We cannot modify Transit Gateway Route Tables for Resource Access Manager shared Transit Gateways
if aws.StringValue(transitGateway.OwnerId) == d.Get("vpc_owner_id").(string) {

if d.HasChange("transit_gateway_default_route_table_association") {
if err := ec2TransitGatewayRouteTableAssociationUpdate(conn, aws.StringValue(transitGateway.Options.AssociationDefaultRouteTableId), d.Id(), d.Get("transit_gateway_default_route_table_association").(bool)); err != nil {
return fmt.Errorf("error updating EC2 Transit Gateway Attachment (%s) Route Table (%s) association: %s", d.Id(), aws.StringValue(transitGateway.Options.AssociationDefaultRouteTableId), err)
}
}
}

if d.HasChange("transit_gateway_default_route_table_propagation") {
if err := ec2TransitGatewayRouteTablePropagationUpdate(conn, aws.StringValue(transitGateway.Options.PropagationDefaultRouteTableId), d.Id(), d.Get("transit_gateway_default_route_table_propagation").(bool)); err != nil {
return fmt.Errorf("error updating EC2 Transit Gateway Attachment (%s) Route Table (%s) propagation: %s", d.Id(), aws.StringValue(transitGateway.Options.PropagationDefaultRouteTableId), err)
if d.HasChange("transit_gateway_default_route_table_propagation") {
if err := ec2TransitGatewayRouteTablePropagationUpdate(conn, aws.StringValue(transitGateway.Options.PropagationDefaultRouteTableId), d.Id(), d.Get("transit_gateway_default_route_table_propagation").(bool)); err != nil {
return fmt.Errorf("error updating EC2 Transit Gateway Attachment (%s) Route Table (%s) propagation: %s", d.Id(), aws.StringValue(transitGateway.Options.PropagationDefaultRouteTableId), err)
}
}
}
}
Expand Down