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 ability to set peering options in aws_vpc_peering_connection. #8310

Conversation

kwilczynski
Copy link
Contributor

This commit adds two optional blocks called "accepter" and "requester" to the
resource allowing for setting desired VPC Peering Connection options for VPCs
that participate in the VPC peering.

Signed-off-by: Krzysztof Wilczynski krzysztof.wilczynski@linux.com

@kwilczynski
Copy link
Contributor Author

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/19 13:48: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 (40.25s)
=== RUN   TestAccAWSVPCPeeringConnection_basic
--- PASS: TestAccAWSVPCPeeringConnection_basic (41.43s)
=== RUN   TestAccAWSVPCPeeringConnection_plan
--- PASS: TestAccAWSVPCPeeringConnection_plan (42.65s)
=== RUN   TestAccAWSVPCPeeringConnection_tags
--- SKIP: TestAccAWSVPCPeeringConnection_tags (0.00s)
        resource_aws_vpc_peering_connection_test.go:90: Error: TestAccAWSVPCPeeringConnection_tags requires a peer ID to be set.
=== RUN   TestAccAWSVPCPeeringConnection_options
--- PASS: TestAccAWSVPCPeeringConnection_options (67.72s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    192.067s

@kwilczynski kwilczynski changed the title Add ability to set peering options in aws_vpc_peering_connection. [WIP] Add ability to set peering options in aws_vpc_peering_connection. Aug 19, 2016
@kwilczynski
Copy link
Contributor Author

kwilczynski commented Aug 19, 2016

Resolves #8169.

Things to do:

  • Add "accepter" and "requester" blocks to the resource schema.
  • Add acceptance tests.
  • Add documentation.

@kwilczynski kwilczynski force-pushed the feature/support-for-peering-options_aws_vpc_peering_connection branch from 14dc4af to 9ef8cbb Compare August 19, 2016 09:36
@kwilczynski
Copy link
Contributor Author

@stack72 over to you 🚀

@kwilczynski kwilczynski changed the title [WIP] Add ability to set peering options in aws_vpc_peering_connection. Add ability to set peering options in aws_vpc_peering_connection. Aug 19, 2016
@@ -47,7 +47,9 @@ func resourceAwsVpcPeeringConnection() *schema.Resource {
Type: schema.TypeString,
Computed: true,
},
"tags": tagsSchema(),
"accepter": vpcPeeringConnectionOptionsSchema(),
"requester": vpcPeeringConnectionOptionsSchema(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the reuse of schema block here!

This commit adds two optional blocks called "accepter" and "requester" to the
resource allowing for setting desired VPC Peering Connection options for VPCs
that participate in the VPC peering.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
@kwilczynski kwilczynski force-pushed the feature/support-for-peering-options_aws_vpc_peering_connection branch from 9ef8cbb to b435988 Compare August 19, 2016 10:04
@stack72
Copy link
Contributor

stack72 commented Aug 19, 2016

Hi @kwilczynski

This LGTM! Thanks for making the changes here :)

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSVPCPeeringConnection'
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/19 10:51:56 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 (52.66s)
=== RUN   TestAccAWSVPCPeeringConnection_basic
--- PASS: TestAccAWSVPCPeeringConnection_basic (51.41s)
=== RUN   TestAccAWSVPCPeeringConnection_plan
--- PASS: TestAccAWSVPCPeeringConnection_plan (49.56s)
=== RUN   TestAccAWSVPCPeeringConnection_tags
--- SKIP: TestAccAWSVPCPeeringConnection_tags (0.00s)
    resource_aws_vpc_peering_connection_test.go:90: Error: TestAccAWSVPCPeeringConnection_tags requires a peer ID to be set.
=== RUN   TestAccAWSVPCPeeringConnection_options
--- PASS: TestAccAWSVPCPeeringConnection_options (92.67s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    246.318s

P.

@BSick7
Copy link
Contributor

BSick7 commented Aug 19, 2016

This is causing a terraform crash.
There is a nil pointer somewhere in there.

panic: runtime error: invalid memory address or nil pointer dereference
2016/08/19 23:38:01 [DEBUG] plugin: terraform: [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x91abd0]
2016/08/19 23:38:01 [DEBUG] plugin: terraform: 
2016/08/19 23:38:01 [DEBUG] plugin: terraform: goroutine 856 [running]:
2016/08/19 23:38:01 [DEBUG] plugin: terraform: panic(0x27c6e40, 0xc42000e090)
2016/08/19 23:38:01 [DEBUG] plugin: terraform:  /opt/go/src/runtime/panic.go:500 +0x1a1
2016/08/19 23:38:01 [DEBUG] plugin: terraform: github.com/hashicorp/terraform/builtin/providers/aws.resourceAwsVPCPeeringRead(0xc42115afc0, 0x2534d60, 0xc420063b80, 0x0, 0x1a)
2016/08/19 23:38:01 [DEBUG] plugin: terraform:  /opt/gopath/src/github.com/hashicorp/terraform/builtin/providers/aws/resource_aws_vpc_peering_connection.go:133 +0x560
2016/08/19 23:38:01 [DEBUG] plugin: terraform: github.com/hashicorp/terraform/helper/schema.(*Resource).Refresh(0xc4203eca20, 0xc420d0ea40, 0x2534d60, 0xc420063b80, 0xc420061228, 0x1, 0x18)
2016/08/19 23:38:01 [DEBUG] plugin: terraform:  /opt/gopath/src/github.com/hashicorp/terraform/helper/schema/resource.go:259 +0x131
2016/08/19 23:38:01 [DEBUG] plugin: terraform: github.com/hashicorp/terraform/helper/schema.(*Provider).Refresh(0xc4205703f0, 0xc420d0ea00, 0xc420d0ea40, 0x0, 0x0, 0x10)
2016/08/19 23:38:01 [DEBUG] plugin: terraform:  /opt/gopath/src/github.com/hashicorp/terraform/helper/schema/provider.go:201 +0x91
2016/08/19 23:38:01 [DEBUG] plugin: terraform: github.com/hashicorp/terraform/plugin.(*ResourceProviderServer).Refresh(0xc4208fa7e0, 0xc4202da0a0, 0xc4202da5f0, 0x0, 0x0)
2016/08/19 23:38:01 [DEBUG] plugin: terraform:  /opt/gopath/src/github.com/hashicorp/terraform/plugin/resource_provider.go:482 +0x4e
2016/08/19 23:38:01 [DEBUG] plugin: terraform: reflect.Value.call(0xc4203836e0, 0xc4209c17a0, 0x13, 0x2d6c961, 0x4, 0xc4217fded0, 0x3, 0x3, 0x43b1fc0, 0xc4217bb240, ...)

@BSick7
Copy link
Contributor

BSick7 commented Aug 19, 2016

It appears like

err = d.Set("accepter", flattenPeeringOptions(pc.AccepterVpcInfo.PeeringOptions))

Perhaps the pc.AccepterVpcInfo can come back nil?

@kwilczynski
Copy link
Contributor Author

@BSick7 sincere apologies about this!

Let me have a look. When I was testing this, I saw that at least in us-east-1 it was always returning the whole structure, for example:

2016/08/20 09:36:24 [DEBUG] plugin: terraform: aws-provider (internal) 2016/08/20 09:36:24 []interface {}{} {
2016/08/20 09:36:24 [DEBUG] plugin: terraform:   AccepterVpcInfo: {
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     CidrBlock: "10.1.0.0/16",
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     OwnerId: "635543228030",
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     PeeringOptions: {
2016/08/20 09:36:24 [DEBUG] plugin: terraform:       AllowDnsResolutionFromRemoteVpc: false,
2016/08/20 09:36:24 [DEBUG] plugin: terraform:       AllowEgressFromLocalClassicLinkToRemoteVpc: false,
2016/08/20 09:36:24 [DEBUG] plugin: terraform:       AllowEgressFromLocalVpcToRemoteClassicLink: false
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     },
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     VpcId: "vpc-41b5c226"
2016/08/20 09:36:24 [DEBUG] plugin: terraform:   },
2016/08/20 09:36:24 [DEBUG] plugin: terraform:   RequesterVpcInfo: {
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     CidrBlock: "10.2.0.0/16",
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     OwnerId: "635543228030",
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     PeeringOptions: {
2016/08/20 09:36:24 [DEBUG] plugin: terraform:       AllowDnsResolutionFromRemoteVpc: false,
2016/08/20 09:36:24 [DEBUG] plugin: terraform:       AllowEgressFromLocalClassicLinkToRemoteVpc: false,
2016/08/20 09:36:24 [DEBUG] plugin: terraform:       AllowEgressFromLocalVpcToRemoteClassicLink: false
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     },
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     VpcId: "vpc-42b5c225"
2016/08/20 09:36:24 [DEBUG] plugin: terraform:   },
2016/08/20 09:36:24 [DEBUG] plugin: terraform:   Status: {
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     Code: "active",
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     Message: "Active"
2016/08/20 09:36:24 [DEBUG] plugin: terraform:   },
2016/08/20 09:36:24 [DEBUG] plugin: terraform:   Tags: [{
2016/08/20 09:36:24 [DEBUG] plugin: terraform:       Key: "Name",
2016/08/20 09:36:24 [DEBUG] plugin: terraform:       Value: "Test"
2016/08/20 09:36:24 [DEBUG] plugin: terraform:     }],
2016/08/20 09:36:24 [DEBUG] plugin: terraform:   VpcPeeringConnectionId: "pcx-c64cc0af"
2016/08/20 09:36:24 [DEBUG] plugin: terraform: }

What region have you tried? I am going to do my best to try to reproduce and fix this.

kwilczynski added a commit to kwilczynski/terraform that referenced this pull request Aug 20, 2016
This resolves the issue introduced in hashicorp#8310.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
@kwilczynski
Copy link
Contributor Author

kwilczynski commented Aug 20, 2016

Resolved in #8338.

The details about peering options would not be included if the connection is still pending acceptance when the read and/or refresh happens, or when the peering is to be manually accepted (which also means that the request is pending acceptance).

@BSick7 sincere apologies for the troubles!

@BSick7
Copy link
Contributor

BSick7 commented Aug 20, 2016

Fast fix! Thanks @kwilczynski

@kwilczynski
Copy link
Contributor Author

@BSick7 thank you! I am sorry that this has caused you some troubles.

kwilczynski added a commit to kwilczynski/terraform that referenced this pull request Aug 20, 2016
This resolves the issue introduced in hashicorp#8310.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
kwilczynski added a commit to kwilczynski/terraform that referenced this pull request Aug 20, 2016
This resolves the issue introduced in hashicorp#8310.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
kwilczynski added a commit to kwilczynski/terraform that referenced this pull request Aug 21, 2016
This resolves the issue introduced in hashicorp#8310.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
kwilczynski added a commit to kwilczynski/terraform that referenced this pull request Aug 22, 2016
This resolves the issue introduced in hashicorp#8310.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
kwilczynski added a commit to kwilczynski/terraform that referenced this pull request Aug 22, 2016
This resolves the issue introduced in hashicorp#8310.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
kwilczynski added a commit to kwilczynski/terraform that referenced this pull request Aug 22, 2016
This resolves the issue introduced in hashicorp#8310.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
catsby added a commit that referenced this pull request Aug 24, 2016
…upersedes #8338) (#8432)

* Fix crash when reading VPC Peering Connection options.

This resolves the issue introduced in #8310.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>

* Do not de-reference values when using Set().

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>

* provider/aws: Update VPC Peering connect accept/request attributes

* change from type list to type set

* provider/aws: Update VPC Peering accept/requst options, tests

* errwrap some things
richardbowden pushed a commit to richardbowden/terraform that referenced this pull request Aug 27, 2016
…upersedes hashicorp#8338) (hashicorp#8432)

* Fix crash when reading VPC Peering Connection options.

This resolves the issue introduced in hashicorp#8310.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>

* Do not de-reference values when using Set().

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>

* provider/aws: Update VPC Peering connect accept/request attributes

* change from type list to type set

* provider/aws: Update VPC Peering accept/requst options, tests

* errwrap some things
@ghost
Copy link

ghost commented Apr 23, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants