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

DNS resolution for inter-region vpc peering #7627

Merged
merged 1 commit into from
Sep 27, 2019

Conversation

ewbankkit
Copy link
Contributor

@ewbankkit ewbankkit commented Feb 20, 2019

Fixes #6730.

The heart of this PR is the addition of a cross-region-peering flag to expandVpcPeeringConnectionOptions and some changes in callers to be able to set this.

I took the opportunity to add a whole bunch more acceptance tests for the aws_vpc_peering_connection, aws_vpc_peering_connection_accepter and aws_vpc_peering_connection_options resources including some cross-account tests that will only run iif the VPC_PEER_AWS_ACCESS_KEY_ID and VPC_PEER_AWS_SECRET_ACCESS_KEY environment variables are set to values for the AWS account that accepts the VPC peering.

@ghost ghost added size/XL Managed by automation to categorize the size of a PR. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. labels Feb 20, 2019
@@ -22,6 +22,7 @@ var testAccProvidersWithTLS map[string]terraform.ResourceProvider
var testAccProviderFactories func(providers *[]*schema.Provider) map[string]terraform.ResourceProviderFactory
var testAccProvider *schema.Provider
var testAccTemplateProvider *schema.Provider
var testAccProviderFunc func() *schema.Provider
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can also be used in other cross-region acceptance tests:

$ grep 'return testAccProvider' *.go
provider_test.go:	testAccProviderFunc = func() *schema.Provider { return testAccProvider }
resource_aws_docdb_cluster_test.go:	return testAccCheckDocDBClusterExistsWithProvider(n, v, func() *schema.Provider { return testAccProvider })
resource_aws_instance_test.go:	return testAccCheckInstanceExistsWithProvider(n, i, func() *schema.Provider { return testAccProvider })
resource_aws_neptune_cluster_test.go:	return testAccCheckAWSNeptuneClusterExistsWithProvider(n, v, func() *schema.Provider { return testAccProvider })
resource_aws_rds_cluster_test.go:	return testAccCheckAWSClusterExistsWithProvider(n, v, func() *schema.Provider { return testAccProvider })
resource_aws_route53_zone_association_test.go:	return testAccCheckRoute53ZoneAssociationExistsWithProvider(n, zone, func() *schema.Provider { return testAccProvider })
resource_aws_route53_zone_test.go:	return testAccCreateRandomRoute53RecordsInZoneIdWithProvider(func() *schema.Provider { return testAccProvider }, zone, recordsCount)
resource_aws_route53_zone_test.go:	return testAccCheckRoute53ZoneExistsWithProvider(n, zone, func() *schema.Provider { return testAccProvider })
resource_aws_s3_bucket_test.go:	return testAccCheckAWSS3BucketExistsWithProvider(n, func() *schema.Provider { return testAccProvider })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a Tech Debt issue to tidy these up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe many of the testAccCheck...ExistsWithProvider() functions are extraneous or adding unnecessary complexity. Its easier to setup the acceptance testing configurations to only need the "normal" testAccCheck...Exists() function with its implicit testAccProvider usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed the merge conflict with the removal of the template provider on merge.

@ewbankkit
Copy link
Contributor Author

ewbankkit commented Feb 24, 2019

Acceptance tests:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVPCPeeringConnection_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 3 -run=TestAccAWSVPCPeeringConnection_ -timeout 120m
=== RUN   TestAccAWSVPCPeeringConnection_importBasic
=== PAUSE TestAccAWSVPCPeeringConnection_importBasic
=== RUN   TestAccAWSVPCPeeringConnection_basic
=== PAUSE TestAccAWSVPCPeeringConnection_basic
=== RUN   TestAccAWSVPCPeeringConnection_accept
=== PAUSE TestAccAWSVPCPeeringConnection_accept
=== RUN   TestAccAWSVPCPeeringConnection_plan
=== PAUSE TestAccAWSVPCPeeringConnection_plan
=== RUN   TestAccAWSVPCPeeringConnection_tags
=== PAUSE TestAccAWSVPCPeeringConnection_tags
=== RUN   TestAccAWSVPCPeeringConnection_options
=== PAUSE TestAccAWSVPCPeeringConnection_options
=== RUN   TestAccAWSVPCPeeringConnection_optionsNoAccept
=== PAUSE TestAccAWSVPCPeeringConnection_optionsNoAccept
=== RUN   TestAccAWSVPCPeeringConnection_failedState
=== PAUSE TestAccAWSVPCPeeringConnection_failedState
=== RUN   TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept
=== PAUSE TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept
=== RUN   TestAccAWSVPCPeeringConnection_region
=== PAUSE TestAccAWSVPCPeeringConnection_region
=== CONT  TestAccAWSVPCPeeringConnection_importBasic
=== CONT  TestAccAWSVPCPeeringConnection_region
=== CONT  TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept
--- PASS: TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept (16.95s)
=== CONT  TestAccAWSVPCPeeringConnection_failedState
--- PASS: TestAccAWSVPCPeeringConnection_importBasic (31.08s)
=== CONT  TestAccAWSVPCPeeringConnection_optionsNoAccept
--- PASS: TestAccAWSVPCPeeringConnection_failedState (17.98s)
=== CONT  TestAccAWSVPCPeeringConnection_options
--- PASS: TestAccAWSVPCPeeringConnection_region (35.24s)
=== CONT  TestAccAWSVPCPeeringConnection_tags
--- PASS: TestAccAWSVPCPeeringConnection_optionsNoAccept (20.35s)
=== CONT  TestAccAWSVPCPeeringConnection_plan
--- PASS: TestAccAWSVPCPeeringConnection_plan (28.07s)
=== CONT  TestAccAWSVPCPeeringConnection_accept
--- PASS: TestAccAWSVPCPeeringConnection_tags (49.04s)
=== CONT  TestAccAWSVPCPeeringConnection_basic
--- PASS: TestAccAWSVPCPeeringConnection_options (50.04s)
--- PASS: TestAccAWSVPCPeeringConnection_basic (28.26s)
--- PASS: TestAccAWSVPCPeeringConnection_accept (62.39s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	141.928s
$ VPC_PEER_AWS_ACCESS_KEY_ID=xxxxxxxxxxxxxxxxxxxx\
  VPC_PEER_AWS_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\
  make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVPCPeeringConnectionAccepter_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 3 -run=TestAccAWSVPCPeeringConnectionAccepter_ -timeout 120m
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount (31.63s)
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount (35.22s)
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount (35.79s)
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount (38.98s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	70.631s
$ VPC_PEER_AWS_ACCESS_KEY_ID=xxxxxxxxxxxxxxxxxxxx\
  VPC_PEER_AWS_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\
  make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVpcPeeringConnectionOptions_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 3 -run=TestAccAWSVpcPeeringConnectionOptions_ -timeout 120m
=== RUN   TestAccAWSVpcPeeringConnectionOptions_importBasic
=== PAUSE TestAccAWSVpcPeeringConnectionOptions_importBasic
=== RUN   TestAccAWSVpcPeeringConnectionOptions_sameRegionSameAccount
=== PAUSE TestAccAWSVpcPeeringConnectionOptions_sameRegionSameAccount
=== RUN   TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount
=== PAUSE TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount
=== RUN   TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount
=== PAUSE TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount
=== CONT  TestAccAWSVpcPeeringConnectionOptions_importBasic
=== CONT  TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount
=== CONT  TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount
--- PASS: TestAccAWSVpcPeeringConnectionOptions_importBasic (34.14s)
=== CONT  TestAccAWSVpcPeeringConnectionOptions_sameRegionSameAccount
--- PASS: TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount (36.73s)
--- PASS: TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount (38.04s)
--- PASS: TestAccAWSVpcPeeringConnectionOptions_sameRegionSameAccount (32.49s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	66.643s

@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. and removed size/XL Managed by automation to categorize the size of a PR. labels Feb 24, 2019
if err != nil {
return fmt.Errorf("Error setting VPC Peering Connection accepter information: %s", err)
}
if err := d.Set("accepter", flattenVpcPeeringConnectionOptions(pc.AccepterVpcInfo.PeeringOptions)); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

flattenVpcPeeringConnectionOptions handles pc.AccepterVpcInfo.PeeringOptions == nil

if err != nil {
return fmt.Errorf("Error setting VPC Peering Connection requester information: %s", err)
}
if err := d.Set("requester", flattenVpcPeeringConnectionOptions(pc.RequesterVpcInfo.PeeringOptions)); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

flattenVpcPeeringConnectionOptions handles pc.RequesterVpcInfo.PeeringOptions == nil

if len(v) > 0 {
req.RequesterPeeringConnectionOptions = expandVpcPeeringConnectionOptions(v[0].(map[string]interface{}))
VpcPeeringConnectionId: aws.String(d.Id()),
AccepterPeeringConnectionOptions: expandVpcPeeringConnectionOptions(d.Get("accepter").(*schema.Set).List(), crossRegionPeering),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

expandVpcPeeringConnectionOptions handles empty set

}

if d.HasChange("accepter") || d.HasChange("requester") {
_, ok := d.GetOk("auto_accept")
if !ok && pc.Status != nil && *pc.Status.Code != "active" {
if statusCode == ec2.VpcPeeringConnectionStateReasonCodeActive || statusCode == ec2.VpcPeeringConnectionStateReasonCodeProvisioning {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

VPC peering connection is in provisioning state if it has just been created.
DNS options can be set if the connection is in this state.

}

return nil
}

func resourceAwsVpcPeeringConnectionOptionsUpdate(d *schema.ResourceData, meta interface{}) error {
if err := resourceAwsVpcPeeringConnectionModifyOptions(d, meta); err != nil {
conn := meta.(*AWSClient).ec2conn
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to get the peering connection info here to determine whether or not it's a cross-region peering.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in the future we can avoid logic like this by saving this information into a computed attribute.

@ewbankkit ewbankkit changed the title [WIP] DNS resolution for inter-region vpc peering DNS resolution for inter-region vpc peering Feb 24, 2019
@ewbankkit
Copy link
Contributor Author

Removing WIP.
Ready for review.

// peer_vpc_id - The ID of the requester VPC
// peer_owner_id - The AWS account ID of the owner of the requester VPC
// peer_region - The region of the accepter VPC
// ** TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add an issue to investigate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmitriy-lukyanchikov
Copy link

do you have any estimates when it can be merged and accessable in new terraform version?


func TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount(t *testing.T) {
key := "VPC_PEER_AWS_ACCESS_KEY_ID"
Copy link
Contributor

Choose a reason for hiding this comment

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

You're definitely on the right track here, but I would like to call out that for cross-account testing we are trying to standardize on a setup like the one that can be found in https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_guardduty_invite_accepter_test.go -- the gist being:

  • Test PreCheck includes testAccAlternateAccountPreCheck(t) to ensure a standardized AWS_ALTERNATE_ACCESS_KEY_ID + AWS_ALTERNATE_SECRET_ACCESS_KEY or AWS_ALTERNATE_PROFILE set of information is provided for cross-account testing credentials
  • ProviderFactories is used instead of Providers like it is here 👍
  • Test configurations pull in a standardized testAccAlternateAccountProviderConfig() and use the aws.alternate provider alias for the necessary cross-account resources. The resource that is being tested should not use the provider alias to simplify the testing setup.
  • Any import testing steps include the Config to match the previous step

That being said, these particular acceptance tests are also dealing with same account different region and different account different region testing. The framework mentioned above needs some extensions to support standardized region selection as well, the preference being an optional AWS_ALTERNATE_REGION environment variable in conjunction with two new provider configurations similar to testAccAlternateAccountProviderConfig(), e.g.

func testAccGetAlternateRegion() string {
	v := os.Getenv("AWS_ALTERNATE_REGION")
	if v == "" {
		return "us-east-1"
	}
	return v
}

func testAccAlternateAccountAlternateRegionProviderConfig() string {
	return fmt.Sprintf(`
provider "aws" {
  access_key = %[1]q
  alias      = "alternate"
  profile    = %[2]q
  region     = %[3]q
  secret_key = %[4]q
}
`, os.Getenv("AWS_ALTERNATE_ACCESS_KEY_ID"), os.Getenv("AWS_ALTERNATE_PROFILE"), testAccGetAlternateRegion(), os.Getenv("AWS_ALTERNATE_SECRET_ACCESS_KEY"))
}

func testAccAlternateRegionProviderConfig() string {
	return fmt.Sprintf(`
provider "aws" {
  alias  = "alternate"
  region = %[1]q
}
`, testAccGetAlternateRegion())
}

The above should cover most of our acceptance testing use cases while allowing us to use a standard framework for implementations across the provider. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bflad Yes, I only saw those recommendations after making these changes - I'll incorporate the recommendations and enhance where appropriate. Thanks.

@ewbankkit ewbankkit force-pushed the issue-6730 branch 2 times, most recently from a7b9f74 to 5a3eb57 Compare April 25, 2019 19:54
@ewbankkit
Copy link
Contributor Author

Changes made to the acceptance testing framework.
Still need to re-run the relevant acceptance tests.

@ewbankkit
Copy link
Contributor Author

OK, acceptance tests re-run:

aws_vpc_peering_connection

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVPCPeeringConnection_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 3 -run=TestAccAWSVPCPeeringConnection_ -timeout 120m
=== RUN   TestAccAWSVPCPeeringConnection_basic
=== PAUSE TestAccAWSVPCPeeringConnection_basic
=== RUN   TestAccAWSVPCPeeringConnection_accept
=== PAUSE TestAccAWSVPCPeeringConnection_accept
=== RUN   TestAccAWSVPCPeeringConnection_plan
=== PAUSE TestAccAWSVPCPeeringConnection_plan
=== RUN   TestAccAWSVPCPeeringConnection_tags
=== PAUSE TestAccAWSVPCPeeringConnection_tags
=== RUN   TestAccAWSVPCPeeringConnection_options
=== PAUSE TestAccAWSVPCPeeringConnection_options
=== RUN   TestAccAWSVPCPeeringConnection_optionsNoAccept
=== PAUSE TestAccAWSVPCPeeringConnection_optionsNoAccept
=== RUN   TestAccAWSVPCPeeringConnection_failedState
=== PAUSE TestAccAWSVPCPeeringConnection_failedState
=== RUN   TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept
=== PAUSE TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept
=== RUN   TestAccAWSVPCPeeringConnection_region
=== PAUSE TestAccAWSVPCPeeringConnection_region
=== CONT  TestAccAWSVPCPeeringConnection_basic
=== CONT  TestAccAWSVPCPeeringConnection_region
=== CONT  TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept
--- PASS: TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept (18.36s)
=== CONT  TestAccAWSVPCPeeringConnection_failedState
--- PASS: TestAccAWSVPCPeeringConnection_basic (34.00s)
=== CONT  TestAccAWSVPCPeeringConnection_optionsNoAccept
--- PASS: TestAccAWSVPCPeeringConnection_region (34.86s)
=== CONT  TestAccAWSVPCPeeringConnection_options
--- PASS: TestAccAWSVPCPeeringConnection_failedState (18.50s)
=== CONT  TestAccAWSVPCPeeringConnection_tags
--- PASS: TestAccAWSVPCPeeringConnection_optionsNoAccept (21.80s)
=== CONT  TestAccAWSVPCPeeringConnection_plan
--- PASS: TestAccAWSVPCPeeringConnection_plan (29.65s)
=== CONT  TestAccAWSVPCPeeringConnection_accept
--- PASS: TestAccAWSVPCPeeringConnection_tags (50.03s)
--- PASS: TestAccAWSVPCPeeringConnection_options (52.53s)
--- PASS: TestAccAWSVPCPeeringConnection_accept (65.37s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	150.876s

aws_vpc_peering_connection_options

$ AWS_ALTERNATE_ACCESS_KEY_ID=AAAAAAAAAAAAAAAAAAAA AWS_ALTERNATE_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVpcPeeringConnectionOptions_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 3 -run=TestAccAWSVpcPeeringConnectionOptions_ -timeout 120m
=== RUN   TestAccAWSVpcPeeringConnectionOptions_sameRegionSameAccount
=== PAUSE TestAccAWSVpcPeeringConnectionOptions_sameRegionSameAccount
=== RUN   TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount
=== PAUSE TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount
=== RUN   TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount
=== PAUSE TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount
=== CONT  TestAccAWSVpcPeeringConnectionOptions_sameRegionSameAccount
=== CONT  TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount
=== CONT  TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount
--- PASS: TestAccAWSVpcPeeringConnectionOptions_sameRegionSameAccount (36.27s)
--- PASS: TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount (40.59s)
--- PASS: TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount (44.88s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	44.946s

aws_vpc_peering_connection_accepter

$ AWS_ALTERNATE_ACCESS_KEY_ID=AAAAAAAAAAAAAAAAAAAA AWS_ALTERNATE_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVPCPeeringConnectionAccepter_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 3 -run=TestAccAWSVPCPeeringConnectionAccepter_ -timeout 120m
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount (34.58s)
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount (37.71s)
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount (45.33s)
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount (35.74s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	70.362s

@ewbankkit
Copy link
Contributor Author

Ready for re-review.

@@ -313,7 +314,7 @@ manually sourced values from documentation.
- [ ] Check [Regions and Endpoints S3 website endpoints](https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_website_region_endpoints) and add Route53 Hosted Zone ID if available to `aws/hosted_zones.go`
- [ ] Check [CloudTrail Supported Regions docs](https://docs.aws.amazon.com/awscloudtrail/latest/userguide/cloudtrail-supported-regions.html) and add AWS Account ID if available to `aws/data_source_aws_cloudtrail_service_account.go`
- [ ] Check [Elastic Load Balancing Access Logs docs](https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/enable-access-logs.html#attach-bucket-policy) and add Elastic Load Balancing Account ID if available to `aws/data_source_aws_elb_service_account.go`
- [ ] Check [Redshift Database Audit Logging docs](https://docs.aws.amazon.com/redshift/latest/mgmt/db-auditing.html) and add AWS Account ID if available to `aws/data_source_aws_redshift_service_account.go`
- [ ] Check [Redshift Database Audit Logging docs](https://docs.aws.amazon.com/redshift/latest/mgmt/db-auditing.html#rs-db-auditing-cloud-trail-rs-acct-ids) and add AWS Account ID if available to `aws/data_source_aws_redshift_service_account.go`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add anchor here.

@ewbankkit
Copy link
Contributor Author

Rebased to resolve merge conflict.

@ewbankkit
Copy link
Contributor Author

Rebased again to resolve merge conflict.

@ewbankkit
Copy link
Contributor Author

Rebased again to resolve merge conflict.

@ewbankkit
Copy link
Contributor Author

Rebased again to resolve merge conflicts.
Retested after merging changes from #9499:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVPCPeeringConnection_'==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 3 -run=TestAccAWSVPCPeeringConnection_ -timeout 120m
=== RUN   TestAccAWSVPCPeeringConnection_basic
=== PAUSE TestAccAWSVPCPeeringConnection_basic
=== RUN   TestAccAWSVPCPeeringConnection_accept
=== PAUSE TestAccAWSVPCPeeringConnection_accept
=== RUN   TestAccAWSVPCPeeringConnection_plan
=== PAUSE TestAccAWSVPCPeeringConnection_plan
=== RUN   TestAccAWSVPCPeeringConnection_tags
=== PAUSE TestAccAWSVPCPeeringConnection_tags
=== RUN   TestAccAWSVPCPeeringConnection_options
=== PAUSE TestAccAWSVPCPeeringConnection_options
=== RUN   TestAccAWSVPCPeeringConnection_optionsNoAccept
=== PAUSE TestAccAWSVPCPeeringConnection_optionsNoAccept
=== RUN   TestAccAWSVPCPeeringConnection_failedState
=== PAUSE TestAccAWSVPCPeeringConnection_failedState
=== RUN   TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept
=== PAUSE TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept
=== RUN   TestAccAWSVPCPeeringConnection_region
=== PAUSE TestAccAWSVPCPeeringConnection_region
=== CONT  TestAccAWSVPCPeeringConnection_basic
=== CONT  TestAccAWSVPCPeeringConnection_region
=== CONT  TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept
--- PASS: TestAccAWSVPCPeeringConnection_peerRegionAndAutoAccept (18.72s)
=== CONT  TestAccAWSVPCPeeringConnection_failedState
--- PASS: TestAccAWSVPCPeeringConnection_basic (34.23s)
=== CONT  TestAccAWSVPCPeeringConnection_optionsNoAccept
--- PASS: TestAccAWSVPCPeeringConnection_region (35.04s)
=== CONT  TestAccAWSVPCPeeringConnection_options
--- PASS: TestAccAWSVPCPeeringConnection_failedState (19.00s)
=== CONT  TestAccAWSVPCPeeringConnection_tags
--- PASS: TestAccAWSVPCPeeringConnection_optionsNoAccept (21.50s)
=== CONT  TestAccAWSVPCPeeringConnection_plan
--- PASS: TestAccAWSVPCPeeringConnection_plan (29.48s)
=== CONT  TestAccAWSVPCPeeringConnection_accept
--- PASS: TestAccAWSVPCPeeringConnection_options (52.59s)
--- PASS: TestAccAWSVPCPeeringConnection_tags (51.05s)
--- PASS: TestAccAWSVPCPeeringConnection_accept (66.18s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	151.439s
$ go test ./aws -v -sweep=us-east-1,us-west-2 -sweep-run=aws_vpc_peering_connection -timeout 10h
2019/08/02 16:17:41 [DEBUG] Running Sweepers for region (us-east-1):
2019/08/02 16:17:41 [INFO] Building AWS auth structure
2019/08/02 16:17:41 [INFO] Setting AWS metadata API timeout to 100ms
2019/08/02 16:17:42 [INFO] Ignoring AWS metadata API endpoint at default location as it doesn't return any instance-id
2019/08/02 16:17:42 [INFO] AWS Auth provider used: "EnvProvider"
2019/08/02 16:17:42 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2019/08/02 16:17:43 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2019/08/02 16:17:43 Sweeper Tests ran:
	- aws_vpc_peering_connection
2019/08/02 16:17:43 [DEBUG] Running Sweepers for region (us-west-2):
2019/08/02 16:17:43 [INFO] Building AWS auth structure
2019/08/02 16:17:43 [INFO] Setting AWS metadata API timeout to 100ms
2019/08/02 16:17:44 [INFO] Ignoring AWS metadata API endpoint at default location as it doesn't return any instance-id
2019/08/02 16:17:44 [INFO] AWS Auth provider used: "EnvProvider"
2019/08/02 16:17:44 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2019/08/02 16:17:44 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2019/08/02 16:17:45 Sweeper Tests ran:
	- aws_vpc_peering_connection
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3.191s

@ewbankkit
Copy link
Contributor Author

@bflad I think all the review issues you raised have been addressed.

@nywilken nywilken added the bug Addresses a defect in current functionality. label Aug 19, 2019
@nywilken
Copy link
Contributor

Hi @ewbankkit I was hoping to help push forward this PR. Is this ready to be reviewed again?

@ewbankkit
Copy link
Contributor Author

@nywilken Yes, ready for review. Thanks.

@ewbankkit
Copy link
Contributor Author

ewbankkit commented Sep 8, 2019

Rebased to resolve merge commits.
Re-ran acceptance tests:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVPCPeeringConnection_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 2 -run=TestAccAWSVPCPeeringConnection_ -timeout 120m
=== RUN   TestAccAWSVPCPeeringConnection_basic
=== PAUSE TestAccAWSVPCPeeringConnection_basic
=== RUN   TestAccAWSVPCPeeringConnection_plan
=== PAUSE TestAccAWSVPCPeeringConnection_plan
=== RUN   TestAccAWSVPCPeeringConnection_tags
=== PAUSE TestAccAWSVPCPeeringConnection_tags
=== RUN   TestAccAWSVPCPeeringConnection_options
=== PAUSE TestAccAWSVPCPeeringConnection_options
=== RUN   TestAccAWSVPCPeeringConnection_failedState
=== PAUSE TestAccAWSVPCPeeringConnection_failedState
=== RUN   TestAccAWSVPCPeeringConnection_peerRegionAutoAccept
=== PAUSE TestAccAWSVPCPeeringConnection_peerRegionAutoAccept
=== RUN   TestAccAWSVPCPeeringConnection_region
=== PAUSE TestAccAWSVPCPeeringConnection_region
=== RUN   TestAccAWSVPCPeeringConnection_accept
=== PAUSE TestAccAWSVPCPeeringConnection_accept
=== RUN   TestAccAWSVPCPeeringConnection_optionsNoAutoAccept
=== PAUSE TestAccAWSVPCPeeringConnection_optionsNoAutoAccept
=== CONT  TestAccAWSVPCPeeringConnection_basic
=== CONT  TestAccAWSVPCPeeringConnection_optionsNoAutoAccept
--- PASS: TestAccAWSVPCPeeringConnection_optionsNoAutoAccept (21.30s)
=== CONT  TestAccAWSVPCPeeringConnection_accept
--- PASS: TestAccAWSVPCPeeringConnection_basic (32.76s)
=== CONT  TestAccAWSVPCPeeringConnection_region
--- PASS: TestAccAWSVPCPeeringConnection_region (35.24s)
=== CONT  TestAccAWSVPCPeeringConnection_peerRegionAutoAccept
--- PASS: TestAccAWSVPCPeeringConnection_peerRegionAutoAccept (17.82s)
=== CONT  TestAccAWSVPCPeeringConnection_failedState
--- PASS: TestAccAWSVPCPeeringConnection_accept (71.87s)
=== CONT  TestAccAWSVPCPeeringConnection_options
--- PASS: TestAccAWSVPCPeeringConnection_failedState (18.57s)
=== CONT  TestAccAWSVPCPeeringConnection_tags
--- PASS: TestAccAWSVPCPeeringConnection_tags (32.87s)
=== CONT  TestAccAWSVPCPeeringConnection_plan
--- PASS: TestAccAWSVPCPeeringConnection_options (54.19s)
--- PASS: TestAccAWSVPCPeeringConnection_plan (29.31s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	166.597s
$ AWS_ALTERNATE_ACCESS_KEY_ID=AAAAAAAAAAAAAAAAAAAA AWS_ALTERNATE_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVpcPeeringConnectionOptions_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAWSVpcPeeringConnectionOptions_ -timeout 120m
=== RUN   TestAccAWSVpcPeeringConnectionOptions_basic
=== PAUSE TestAccAWSVpcPeeringConnectionOptions_basic
=== RUN   TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount
=== PAUSE TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount
=== RUN   TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount
=== PAUSE TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount
=== CONT  TestAccAWSVpcPeeringConnectionOptions_basic
=== CONT  TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount
=== CONT  TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount
--- PASS: TestAccAWSVpcPeeringConnectionOptions_basic (37.22s)
--- PASS: TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount (43.46s)
--- PASS: TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount (44.19s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	44.221s
$ AWS_ALTERNATE_ACCESS_KEY_ID=AAAAAAAAAAAAAAAAAAAA AWS_ALTERNATE_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVPCPeeringConnectionAccepter_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAWSVPCPeeringConnectionAccepter_ -timeout 120m
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount
=== RUN   TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount
=== PAUSE TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount
=== CONT  TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount (35.74s)
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount (39.81s)
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount (39.92s)
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount (42.08s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	42.123s
$ go test ./aws -v -sweep=us-east-1,us-west-2 -sweep-run=aws_vpc_peering_connection -timeout 10h
2019/09/08 17:52:17 [DEBUG] Running Sweepers for region (us-east-1):
2019/09/08 17:52:17 [INFO] Building AWS auth structure
2019/09/08 17:52:17 [INFO] Setting AWS metadata API timeout to 100ms
2019/09/08 17:52:17 [INFO] Ignoring AWS metadata API endpoint at default location as it doesn't return any instance-id
2019/09/08 17:52:17 [INFO] AWS Auth provider used: "EnvProvider"
2019/09/08 17:52:17 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2019/09/08 17:52:17 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2019/09/08 17:52:18 Sweeper Tests ran:
	- aws_vpc_peering_connection
2019/09/08 17:52:18 [DEBUG] Running Sweepers for region (us-west-2):
2019/09/08 17:52:18 [INFO] Building AWS auth structure
2019/09/08 17:52:18 [INFO] Setting AWS metadata API timeout to 100ms
2019/09/08 17:52:18 [INFO] Ignoring AWS metadata API endpoint at default location as it doesn't return any instance-id
2019/09/08 17:52:18 [INFO] AWS Auth provider used: "EnvProvider"
2019/09/08 17:52:18 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2019/09/08 17:52:19 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2019/09/08 17:52:20 Sweeper Tests ran:
	- aws_vpc_peering_connection
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3.083s

@krish7919
Copy link

Is there anything stopping this merge?

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

After rebasing and minor adjustments noted below, pulling this in. Thanks for the hard work here @ewbankkit and sorry for the slow review process.

--- PASS: TestAccAWSVPCPeeringConnection_accept (72.88s)
--- PASS: TestAccAWSVPCPeeringConnection_basic (43.19s)
--- PASS: TestAccAWSVPCPeeringConnection_failedState (30.05s)
--- PASS: TestAccAWSVPCPeeringConnection_options (61.69s)
--- PASS: TestAccAWSVPCPeeringConnection_optionsNoAutoAccept (41.69s)
--- PASS: TestAccAWSVPCPeeringConnection_peerRegionAutoAccept (20.44s)
--- PASS: TestAccAWSVPCPeeringConnection_plan (38.92s)
--- PASS: TestAccAWSVPCPeeringConnection_region (46.76s)
--- PASS: TestAccAWSVPCPeeringConnection_tags (46.62s)
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_differentRegionDifferentAccount (51.57s)
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_differentRegionSameAccount (46.22s)
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_sameRegionDifferentAccount (44.01s)
--- PASS: TestAccAWSVPCPeeringConnectionAccepter_sameRegionSameAccount (28.72s)
--- PASS: TestAccAWSVpcPeeringConnectionOptions_basic (47.93s)
--- PASS: TestAccAWSVpcPeeringConnectionOptions_differentRegionSameAccount (46.49s)
--- PASS: TestAccAWSVpcPeeringConnectionOptions_sameRegionDifferentAccount (36.81s)

export AWS_ALTERNATE_REGION=...
```

#### Acceptance Testing Guidelines
Copy link
Contributor

Choose a reason for hiding this comment

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

This was moved up in the file earlier this year, so fixing on merge.

@@ -22,6 +22,7 @@ var testAccProvidersWithTLS map[string]terraform.ResourceProvider
var testAccProviderFactories func(providers *[]*schema.Provider) map[string]terraform.ResourceProviderFactory
var testAccProvider *schema.Provider
var testAccTemplateProvider *schema.Provider
var testAccProviderFunc func() *schema.Provider
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed the merge conflict with the removal of the template provider on merge.

}

return nil
}

func resourceAwsVpcPeeringConnectionOptionsUpdate(d *schema.ResourceData, meta interface{}) error {
if err := resourceAwsVpcPeeringConnectionModifyOptions(d, meta); err != nil {
conn := meta.(*AWSClient).ec2conn
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in the future we can avoid logic like this by saving this information into a computed attribute.

@ghost
Copy link

ghost commented Oct 3, 2019

This has been released in version 2.31.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Nov 1, 2019

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. documentation Introduces or discusses updates to documentation. service/ec2 Issues and PRs that pertain to the ec2 service. size/XXL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support of dns resolution on inter-region vpc peering
5 participants