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

[kwilczynski]

This resolves the issue introduced in #8310.

Signed-off-by: Krzysztof Wilczynski [email protected]

[mitchellh]

Hm, it looked like in the original issue that AccepterVpcInfo (and Requester) were nil, not only the PeeringOptions. Can we add a check for that just for safety regardless?

[kwilczynski]

@mitchellh OK, leave it with me.

[kwilczynski]

@mitchellh done.

I hope that I was able to put guards around the values that we believe might be missing. I have also noticed that sometimes the CidrBlock would be missing from AccepterVpcInfo and/or RequesterVpcInfo, but we don`t use it, so that is fine.

I sincerely apologise for all the troubles.

[kwilczynski]

I have updated the test that checks for tags so that it actually runs.

Tests are passing:

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSVPCPeeringConnection'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/20 15:11:42 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSVPCPeeringConnection -timeout 120m
=== RUN   TestAccAWSVPCPeeringConnection_importBasic
--- PASS: TestAccAWSVPCPeeringConnection_importBasic (44.56s)
=== RUN   TestAccAWSVPCPeeringConnection_basic
--- PASS: TestAccAWSVPCPeeringConnection_basic (42.99s)
=== RUN   TestAccAWSVPCPeeringConnection_plan
--- PASS: TestAccAWSVPCPeeringConnection_plan (40.04s)
=== RUN   TestAccAWSVPCPeeringConnection_tags
--- PASS: TestAccAWSVPCPeeringConnection_tags (45.76s)
=== RUN   TestAccAWSVPCPeeringConnection_options
--- PASS: TestAccAWSVPCPeeringConnection_options (76.77s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    250.137s

I have manually tested with auto_accept disabled:

$ terraform plan
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but
will not be persisted to local or remote state storage.


The Terraform execution plan has been generated and is shown below.
Resources are shown in alphabetical order for quick scanning. Green resources
will be created (or destroyed and then created if an existing resource
exists), yellow resources are being changed in-place, and red resources
will be destroyed. Cyan entries are data sources to be read.

Note: You didn't specify an "-out" parameter to save this plan, so when
"apply" is called, Terraform can't guarantee this is what will execute.

+ aws_vpc.x
    cidr_block:                "10.1.0.0/16"
    default_network_acl_id:    "<computed>"
    default_security_group_id: "<computed>"
    dhcp_options_id:           "<computed>"
    enable_classiclink:        "<computed>"
    enable_dns_hostnames:      "true"
    enable_dns_support:        "<computed>"
    instance_tenancy:          "<computed>"
    main_route_table_id:       "<computed>"

+ aws_vpc.y
    cidr_block:                "10.2.0.0/16"
    default_network_acl_id:    "<computed>"
    default_security_group_id: "<computed>"
    dhcp_options_id:           "<computed>"
    enable_classiclink:        "<computed>"
    enable_dns_hostnames:      "true"
    enable_dns_support:        "<computed>"
    instance_tenancy:          "<computed>"
    main_route_table_id:       "<computed>"

+ aws_vpc_peering_connection.xyz
    accept_status:                                "<computed>"
    accepter.#:                                   "<computed>"
    peer_owner_id:                                "635543228030"
    peer_vpc_id:                                  "${aws_vpc.x.id}"
    requester.#:                                  "1"
    requester.0.allow_classic_link_to_remote_vpc: "<computed>"
    requester.0.allow_remote_vpc_dns_resolution:  "true"
    requester.0.allow_vpc_to_remote_classic_link: "<computed>"
    vpc_id:                                       "${aws_vpc.y.id}"


Plan: 3 to add, 0 to change, 0 to destroy.

$ terraform apply
aws_vpc.x: Creating...
  cidr_block:                "" => "10.1.0.0/16"
  default_network_acl_id:    "" => "<computed>"
  default_security_group_id: "" => "<computed>"
  dhcp_options_id:           "" => "<computed>"
  enable_classiclink:        "" => "<computed>"
  enable_dns_hostnames:      "" => "true"
  enable_dns_support:        "" => "<computed>"
  instance_tenancy:          "" => "<computed>"
  main_route_table_id:       "" => "<computed>"
aws_vpc.y: Creating...
  cidr_block:                "" => "10.2.0.0/16"
  default_network_acl_id:    "" => "<computed>"
  default_security_group_id: "" => "<computed>"
  dhcp_options_id:           "" => "<computed>"
  enable_classiclink:        "" => "<computed>"
  enable_dns_hostnames:      "" => "true"
  enable_dns_support:        "" => "<computed>"
  instance_tenancy:          "" => "<computed>"
  main_route_table_id:       "" => "<computed>"
aws_vpc.y: Creation complete
aws_vpc.x: Creation complete
aws_vpc_peering_connection.xyz: Creating...
  accept_status:                                "" => "<computed>"
  accepter.#:                                   "" => "<computed>"
  peer_owner_id:                                "" => "635543228030"
  peer_vpc_id:                                  "" => "vpc-3f770e58"
  requester.#:                                  "" => "1"
  requester.0.allow_classic_link_to_remote_vpc: "" => "<computed>"
  requester.0.allow_remote_vpc_dns_resolution:  "" => "true"
  requester.0.allow_vpc_to_remote_classic_link: "" => "<computed>"
  vpc_id:                                       "" => "vpc-3e770e59"
Error applying plan:

1 error(s) occurred:

* aws_vpc_peering_connection.xyz: OperationNotPermitted: Peering pcx-888e01e1 is not active. Peering options can be added only to active peerings.
        status code: 400, request id: bdfac582-c419-4ed7-a307-98f2a89dced8

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

$ aws ec2 accept-vpc-peering-connection --vpc-peering-connection-id pcx-888e01e1
{
    "VpcPeeringConnection": {
        "Status": {
            "Message": "Provisioning",
            "Code": "provisioning"
        },
        "Tags": [],
        "AccepterVpcInfo": {
            "PeeringOptions": {
                "AllowEgressFromLocalVpcToRemoteClassicLink": false,
                "AllowEgressFromLocalClassicLinkToRemoteVpc": false
            },
            "OwnerId": "635543228030",
            "VpcId": "vpc-3f770e58",
            "CidrBlock": "10.1.0.0/16"
        },
        "VpcPeeringConnectionId": "pcx-888e01e1",
        "RequesterVpcInfo": {
            "PeeringOptions": {
                "AllowEgressFromLocalVpcToRemoteClassicLink": false,
                "AllowEgressFromLocalClassicLinkToRemoteVpc": false
            },
            "OwnerId": "635543228030",
            "VpcId": "vpc-3e770e59",
            "CidrBlock": "10.2.0.0/16"
        }
    }
}

$ terraform apply
aws_vpc.x: Refreshing state... (ID: vpc-3f770e58)
aws_vpc.y: Refreshing state... (ID: vpc-3e770e59)
aws_vpc_peering_connection.xyz: Refreshing state... (ID: pcx-888e01e1)
aws_vpc_peering_connection.xyz: Modifying...
  requester.0.allow_remote_vpc_dns_resolution: "false" => "true"
aws_vpc_peering_connection.xyz: Modifications complete

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

The state of your infrastructure has been saved to the path
below. This state is required to modify and destroy your
infrastructure, so keep it safe. To inspect the complete state
use the `terraform show` command.

State path: terraform.tfstate

$ terraform show
aws_vpc.x:
  id = vpc-3f770e58
  cidr_block = 10.1.0.0/16
  default_network_acl_id = acl-7e3a1219
  default_security_group_id = sg-c21dc5b8
  dhcp_options_id = dopt-59716e3b
  enable_classiclink = false
  enable_dns_hostnames = true
  enable_dns_support = true
  instance_tenancy = default
  main_route_table_id = rtb-6a73a70c
  tags.% = 0
aws_vpc.y:
  id = vpc-3e770e59
  cidr_block = 10.2.0.0/16
  default_network_acl_id = acl-7d3a121a
  default_security_group_id = sg-c31dc5b9
  dhcp_options_id = dopt-59716e3b
  enable_classiclink = false
  enable_dns_hostnames = true
  enable_dns_support = true
  instance_tenancy = default
  main_route_table_id = rtb-6b73a70d
  tags.% = 0
aws_vpc_peering_connection.xyz:
  id = pcx-888e01e1
  accept_status = active
  accepter.# = 1
  accepter.0.allow_classic_link_to_remote_vpc = false
  accepter.0.allow_remote_vpc_dns_resolution = false
  accepter.0.allow_vpc_to_remote_classic_link = false
  peer_owner_id = 635543228030
  peer_vpc_id = vpc-3f770e58
  requester.# = 1
  requester.0.allow_classic_link_to_remote_vpc = false
  requester.0.allow_remote_vpc_dns_resolution = true
  requester.0.allow_vpc_to_remote_classic_link = false
  tags.% = 0
  vpc_id = vpc-3e770e59

[heimweh]

When running terraform plan for an already existing VPC peering connection with the newly implemented accepter & requester, the resource always end up in the plan as changed.

Just wanted to check if this the intended behaviour? otherwise this works fine for me :)

Side-note and not really related: It would be awesome to get something like auto_accept working when peering across accounts. I think there's a PR open for this: (#6843) That would solve the manual step of having to manually accept the VPC Peering request on the other end.

Sorry for going a little bit off topic :)

$ terraform plan
~ module.stack.vpc.aws_vpc_peering_connection.main
    accepter.#: "" => "<computed>"

resource "aws_vpc_peering_connection" "main" {
  peer_owner_id = "${var.peer_owner_id}"
  peer_vpc_id   = "${var.peer_vpc_id}"
  vpc_id        = "${aws_vpc.main.id}"
  auto_accept   = false

  tags {
    Name        = "${var.environment}-service VPC Peering Connection"
    Environment = "${var.environment}"
  }
}

[kwilczynski]

@heimweh sorry for troubles!

I am going to look into the plan never settling issues.

[heimweh]

@kwilczynski - no worries at all, just happy to help :)

[kwilczynski]

@heimweh you rock!

Both blocks are computed should there be a value to read they would trigger update in state. Thus, once you apply and state will be persisted, then it should stop.

What does the show says? Have you applied already?

[heimweh]

@kwilczynski - this is what terraform show shows after terraform apply. It seems like requester actually persisted but not accepter.

module.stack.vpc.aws_vpc_peering_connection.main:
  id = pcx-xxxxxxxx
  accept_status = active
  auto_accept = false
  peer_owner_id = 000000000000
  peer_vpc_id = vpc-xxxxxxxx
  requester.# = 1
  requester.0.allow_classic_link_to_remote_vpc = false
  requester.0.allow_remote_vpc_dns_resolution = false
  requester.0.allow_vpc_to_remote_classic_link = false
  tags.% = 2
  tags.Environment = test
  tags.Name = test-service VPC Peering Connection
  vpc_id = vpc-xxxxxxxx

[kwilczynski]

@heimweh thank you!

Can I ask a favour of you? If you have a minute, can you add TF_LOG=debug before terraform plan command and include output somewhere?

It would be very helpful to see what it does show for you and your setup.

[heimweh]

@kwilczynski - I managed to replicate it as well with newly created resources.

To really isolate it, I created a VPC with a peering connection to another account.

Feel free to check out this gist: https://gist.github.com/heimweh/b3fc6f08f38e00be79420c6ab445a84f

I added a comment section below the plan log describing the steps I took.
Hope this helps in some way :)

[kwilczynski]

@heimweh thank you again.

There are two issues to solve:

1. Empty values and computed attribute;
2. VPC Peering Connection is to be accepted externally.

The (2) complicates things as options can only be applied to a peering in an active state.

[kwilczynski]

@mitchellh and @stack72, I am going to remove ability to set option at the aws_vpc_peering_connection level and move them to a new resource called aws_vpc_peering_connection_options.

This is primarily due to the more apparent chicken-and-egg problem that arises with the use case where the VPC Peering Connection is to be manually accepted (especially, when people are accepting peering across different accounts). Options cannot be set when the peering is not active and the also responses seem to exclude elements related to the peering options in such case.

Would that be fine with you?

Update:

This is an interesting problem - people want to create peering, but not accept. In such case even aws_vpc_peering_connection_options would fail, since the requirement is an active peering. We could make it so that the resource would ignore not active peering, but I am not sure about this.

Unless we keep the new feature, but issue a warning or an error when user is not setting the auto_accept.

[kwilczynski]

@mitchellh @stack72 @heimweh apologies for the delay with the fix. I am in the middle of moving back from Tokyo to London this week.

I have resolved the issue with nil pointer dereference, plus made sure that both blocks are only populated as needed only if they are present. I have also made sure that the user case when user would create VPC Peering Connection but do not accept it immediately in the aws_vpc_peering_connection resource is covered - we throw an appropriate error (similarly to what AWS does, see: https://postimg.org/image/qlg5wr3j7/) should the modification to the VPC Peering Connection options be introduced without making the peering active first (documentation also reflects this as a requirement).

I sincerely apologise for the delay and for the troubles caused!

[kwilczynski]

Tests are passing:

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSVPCPeeringConnection'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/21 14:38:16 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSVPCPeeringConnection -timeout 120m
=== RUN   TestAccAWSVPCPeeringConnection_importBasic
--- PASS: TestAccAWSVPCPeeringConnection_importBasic (39.84s)
=== RUN   TestAccAWSVPCPeeringConnection_basic
--- PASS: TestAccAWSVPCPeeringConnection_basic (45.04s)
=== RUN   TestAccAWSVPCPeeringConnection_plan
--- PASS: TestAccAWSVPCPeeringConnection_plan (41.81s)
=== RUN   TestAccAWSVPCPeeringConnection_tags
--- PASS: TestAccAWSVPCPeeringConnection_tags (46.04s)
=== RUN   TestAccAWSVPCPeeringConnection_options
--- PASS: TestAccAWSVPCPeeringConnection_options (71.09s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    243.832s

Provider builds successfully:

$ make plugin-dev PLUGIN=provider-aws
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/21 15:18:55 Generated command/internal_plugin_list.go
go install github.com/hashicorp/terraform/builtin/bins/provider-aws
mv /Users/krzysztof/Development/Projects/Go/bin/provider-aws /Users/krzysztof/Development/Projects/Go/bin/terraform-provider-aws

[mitchellh]

At a high level this looks good to me now but I'm going to wait for @catsby or someone else that is more familiar these days with the AWS provider to double check.

[kwilczynski]

@mitchellh thank you!

I have rebased against current master.

@catsby @stack72 over to you 🚀

[catsby]

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

[kwilczynski]

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

[catsby]

I have some questions about why we choose to conditionally read those attributes that I would like to hear back on before merging this. Otherwise the code changes seem good 👍

[kwilczynski]

@catsby (re-posting reply, since it got eaten by a commit amoeba):

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 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.

[catsby]

Continued and merged in #8432

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.