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

provider/aws: Fix crash when reading VPC Peering Connection options. #8338

Closed
wants to merge 2 commits into from
Closed

provider/aws: Fix crash when reading VPC Peering Connection options. #8338

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a46f9f4
Fix crash when reading VPC Peering Connection options.
kwilczynski Aug 20, 2016
36055db
Do not de-reference values when using Set().
kwilczynski Aug 23, 2016
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
106 changes: 74 additions & 32 deletions builtin/providers/aws/resource_aws_vpc_peering_connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func resourceAwsVPCPeeringCreate(d *schema.ResourceData, meta interface{}) error

resp, err := conn.CreateVpcPeeringConnection(createOpts)
if err != nil {
return fmt.Errorf("Error creating vpc peering connection: %s", err)
return fmt.Errorf("Error creating VPC Peering Connection: %s", err)
}

// Get the ID and store it
Expand Down Expand Up @@ -124,20 +124,38 @@ func resourceAwsVPCPeeringRead(d *schema.ResourceData, meta interface{}) error {
return nil
}
}
log.Printf("[DEBUG] VPC Peering Connection response: %#v", pc)

d.Set("accept_status", *pc.Status.Code)
d.Set("peer_owner_id", *pc.AccepterVpcInfo.OwnerId)
d.Set("peer_vpc_id", *pc.AccepterVpcInfo.VpcId)
d.Set("vpc_id", *pc.RequesterVpcInfo.VpcId)

err = d.Set("accepter", flattenPeeringOptions(pc.AccepterVpcInfo.PeeringOptions))
if err != nil {
return err
// When the VPC Peering Connection is pending acceptance,
Copy link
Contributor

Choose a reason for hiding this comment

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

When using d.Set, it's recommend to simply reference the attribute and not dereference it.

So instead of:

d.Set("vpc_id", *pc.RequesterVpcInfo.VpcId)

use

d.Set("vpc_id", pc.RequesterVpcInfo.VpcId)

d.Set will safely dereference it if it's nil, so it's considered safer than dereferencing its yourself. Note that this does not work for d.SetId, which must be an value and not a pointer.

Don't feel you must make this change, but I wanted to pass that on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@catsby thank you, I will revise :) Good to know.

// the details about accepter and/or requester peering
// options would not be included in the response.
if pc.AccepterVpcInfo != nil {
d.Set("peer_owner_id", *pc.AccepterVpcInfo.OwnerId)
d.Set("peer_vpc_id", *pc.AccepterVpcInfo.VpcId)

if _, ok := d.GetOk("accepter"); ok {
if pc.AccepterVpcInfo.PeeringOptions != nil {
err = d.Set("accepter", flattenPeeringOptions(pc.AccepterVpcInfo.PeeringOptions))
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this will only read information on the accepter if the user has configured it locally, is that correct? That behavior seems counter to most of Terraform, in that it would prevent us from detecting configuration drift. We should read the accepter regardless, I think. Is there a reason for this behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the only one way I found which could stop the accepter from being set either to 0 (the count), or when the value is to be computed to <computed> every time.

This happens when blocks are either not provided or are empty and when the auto_accept is set to false (the default), which results in the missing fields in the response from AWS, and in turn empty value is persisted on our side. An empty value results in a change being detected, and then we are back to trying to set (empty) value again, etc, etc. To add, if we allow empty value to be passed when making a request to the AWS (which in its own right is fine) it would explode if the peering connection is not yet accepted (and some people choose to accept it outside of Terraform), thus it would explode every time apply is run (error bubbles from the AWS side).

I am not sure how to solve this in a different way. My aim was to ignore work involving the new blocks completely if they are not present. There is no concept of "undefined" value in the Schema and relying on empty values often leads to this (or similar; especially when boolean is concerned) behaviour.

if err != nil {
return err
}
}
}
}

err = d.Set("requester", flattenPeeringOptions(pc.RequesterVpcInfo.PeeringOptions))
if err != nil {
return err
if pc.RequesterVpcInfo != nil {
d.Set("vpc_id", *pc.RequesterVpcInfo.VpcId)

if _, ok := d.GetOk("requester"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment/concern as above, with regards to always reading this attribute

if pc.RequesterVpcInfo.PeeringOptions != nil {
err = d.Set("requester", flattenPeeringOptions(pc.RequesterVpcInfo.PeeringOptions))
if err != nil {
return err
}
}
}
}

err = d.Set("tags", tagsToMap(pc.Tags))
Expand Down Expand Up @@ -202,18 +220,17 @@ func resourceAwsVPCPeeringUpdate(d *schema.ResourceData, meta interface{}) error
d.SetPartial("tags")
}

if _, ok := d.GetOk("auto_accept"); ok {
pcRaw, _, err := resourceAwsVPCPeeringConnectionStateRefreshFunc(conn, d.Id())()
if err != nil {
return err
}

if pcRaw == nil {
d.SetId("")
return nil
}
pc := pcRaw.(*ec2.VpcPeeringConnection)
pcRaw, _, err := resourceAwsVPCPeeringConnectionStateRefreshFunc(conn, d.Id())()
if err != nil {
return err
}
if pcRaw == nil {
d.SetId("")
return nil
}
pc := pcRaw.(*ec2.VpcPeeringConnection)

if _, ok := d.GetOk("auto_accept"); ok {
if pc.Status != nil && *pc.Status.Code == "pending-acceptance" {
status, err := resourceVPCPeeringConnectionAccept(conn, d.Id())
if err != nil {
Expand All @@ -224,8 +241,15 @@ func resourceAwsVPCPeeringUpdate(d *schema.ResourceData, meta interface{}) error
}

if d.HasChange("accepter") || d.HasChange("requester") {
_, ok := d.GetOk("auto_accept")
if !ok && pc.Status != nil && *pc.Status.Code != "active" {
return fmt.Errorf("Unable to modify peering options. The VPC Peering Connection "+
"%q is not active. Please set `auto_accept` attribute to `true`, "+
"or activate VPC Peering Connection manually.", d.Id())
}

if err := resourceVPCPeeringConnectionOptionsModify(d, meta); err != nil {
return err
return fmt.Errorf("Error modifying VPC Peering Connection options: %s", err)
}
}

Expand Down Expand Up @@ -255,7 +279,7 @@ func resourceAwsVPCPeeringConnectionStateRefreshFunc(conn *ec2.EC2, id string) r
if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "InvalidVpcPeeringConnectionID.NotFound" {
resp = nil
} else {
log.Printf("Error on VPCPeeringConnectionStateRefresh: %s", err)
log.Printf("Error reading VPC Peering Connection details: %s", err)
return nil, "", err
}
}
Expand All @@ -276,7 +300,6 @@ func vpcPeeringConnectionOptionsSchema() *schema.Schema {
return &schema.Schema{
Type: schema.TypeList,
Optional: true,
Computed: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
Expand All @@ -301,19 +324,38 @@ func vpcPeeringConnectionOptionsSchema() *schema.Schema {
}

func flattenPeeringOptions(options *ec2.VpcPeeringConnectionOptionsDescription) (results []map[string]interface{}) {
m := map[string]interface{}{
"allow_remote_vpc_dns_resolution": *options.AllowDnsResolutionFromRemoteVpc,
"allow_classic_link_to_remote_vpc": *options.AllowEgressFromLocalClassicLinkToRemoteVpc,
"allow_vpc_to_remote_classic_link": *options.AllowEgressFromLocalVpcToRemoteClassicLink,
m := make(map[string]interface{})

if options.AllowDnsResolutionFromRemoteVpc != nil {
m["allow_remote_vpc_dns_resolution"] = *options.AllowDnsResolutionFromRemoteVpc
}

if options.AllowEgressFromLocalClassicLinkToRemoteVpc != nil {
m["allow_classic_link_to_remote_vpc"] = *options.AllowEgressFromLocalClassicLinkToRemoteVpc
}

if options.AllowEgressFromLocalVpcToRemoteClassicLink != nil {
m["allow_vpc_to_remote_classic_link"] = *options.AllowEgressFromLocalVpcToRemoteClassicLink
}

results = append(results, m)
return
}

func expandPeeringOptions(m map[string]interface{}) *ec2.PeeringConnectionOptionsRequest {
return &ec2.PeeringConnectionOptionsRequest{
AllowDnsResolutionFromRemoteVpc: aws.Bool(m["allow_remote_vpc_dns_resolution"].(bool)),
AllowEgressFromLocalClassicLinkToRemoteVpc: aws.Bool(m["allow_classic_link_to_remote_vpc"].(bool)),
AllowEgressFromLocalVpcToRemoteClassicLink: aws.Bool(m["allow_vpc_to_remote_classic_link"].(bool)),
r := &ec2.PeeringConnectionOptionsRequest{}

if v, ok := m["allow_remote_vpc_dns_resolution"]; ok {
r.AllowDnsResolutionFromRemoteVpc = aws.Bool(v.(bool))
}

if v, ok := m["allow_classic_link_to_remote_vpc"]; ok {
r.AllowEgressFromLocalClassicLinkToRemoteVpc = aws.Bool(v.(bool))
}

if v, ok := m["allow_vpc_to_remote_classic_link"]; ok {
r.AllowEgressFromLocalVpcToRemoteClassicLink = aws.Bool(v.(bool))
}

return r
}
21 changes: 14 additions & 7 deletions builtin/providers/aws/resource_aws_vpc_peering_connection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ func TestAccAWSVPCPeeringConnection_plan(t *testing.T) {
t.Fatal("AWS_ACCOUNT_ID must be set.")
}
},

IDRefreshIgnore: []string{"auto_accept"},

Providers: testAccProviders,
CheckDestroy: testAccCheckAWSVpcPeeringConnectionDestroy,
Steps: []resource.TestStep{
Expand All @@ -85,13 +88,14 @@ func TestAccAWSVPCPeeringConnection_plan(t *testing.T) {

func TestAccAWSVPCPeeringConnection_tags(t *testing.T) {
var connection ec2.VpcPeeringConnection
peerId := os.Getenv("TF_PEER_ID")
if peerId == "" {
t.Skip("Error: TestAccAWSVPCPeeringConnection_tags requires a peer ID to be set.")
}

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
PreCheck: func() {
testAccPreCheck(t)
if os.Getenv("AWS_ACCOUNT_ID") == "" {
t.Fatal("AWS_ACCOUNT_ID must be set.")
}
},

IDRefreshName: "aws_vpc_peering_connection.foo",
IDRefreshIgnore: []string{"auto_accept"},
Expand All @@ -100,7 +104,7 @@ func TestAccAWSVPCPeeringConnection_tags(t *testing.T) {
CheckDestroy: testAccCheckVpcDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: fmt.Sprintf(testAccVpcPeeringConfigTags, peerId),
Config: testAccVpcPeeringConfigTags,
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSVpcPeeringConnectionExists(
"aws_vpc_peering_connection.foo",
Expand Down Expand Up @@ -355,7 +359,7 @@ resource "aws_vpc" "bar" {
resource "aws_vpc_peering_connection" "foo" {
vpc_id = "${aws_vpc.foo.id}"
peer_vpc_id = "${aws_vpc.bar.id}"
peer_owner_id = "%s"
auto_accept = true
tags {
foo = "bar"
}
Expand All @@ -365,6 +369,9 @@ resource "aws_vpc_peering_connection" "foo" {
const testAccVpcPeeringConfigOptions = `
resource "aws_vpc" "foo" {
cidr_block = "10.0.0.0/16"
tags {
Name = "TestAccAWSVPCPeeringConnection_options"
}
}

resource "aws_vpc" "bar" {
Expand Down
5 changes: 5 additions & 0 deletions website/source/docs/providers/aws/r/vpc_peering.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ resource "aws_vpc" "bar" {

## Argument Reference

-> **Note:** Modifying the VPC Peering Connection options requires peering to be active. An automatic activation
can be done using the [`auto_accept`](vpc_peering.html#auto_accept) attribute. Alternatively, the VPC Peering
Connection has to be made active manually using other means. See [notes](vpc_peering.html#notes) below for
more information.

The following arguments are supported:

* `peer_owner_id` - (Required) The AWS account ID of the owner of the peer VPC.
Expand Down