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

tests/provider: Fixes for testing provider re-configuration #16018

Merged
merged 1 commit into from
Nov 13, 2020

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Nov 4, 2020

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Reference: #8316
Reference: #15737
Reference: #15791

Release note for CHANGELOG:

NONE

Applies two fixes for provider re-configuration issues:

  • The testAccProviderFactories map should always return a new provider instance every invocation, otherwise concurrent testing with differing provider configurations can overwrite each other since the testing framework can reuse the same underlying provider instance address across multiple gRPC plugin configurations.
  • The testAccPreCheck() function is invoked for every test and unnecessarily verifies and reconfigures the "main" provider instance every time to the same configuration.

Adds comments around all this functionality to hopefully make this section of the code clearer for future travelers.

Output from acceptance testing (provided as smoke testing with differing provider configurations in the same testing invocation):

--- PASS: TestAccAWSEIPAssociation_instance (94.34s)
--- PASS: TestAccAWSEIPAssociation_ec2Classic (218.07s)

@bflad bflad requested a review from a team as a code owner November 4, 2020 18:00
@ghost ghost added size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Nov 4, 2020
bflad added a commit that referenced this pull request Nov 4, 2020
…hardcoded us-east-1 environment variable handling in tests

Reference: #8316
Reference: #15737
Reference: #15791
Reference: #16018 (to run testing locally)

Changes:

```
* resource/aws_eip: In EC2-Classic, wait until Instance returns as associated during create or update
* resource/aws_eip_association: Retry on additional EC2 Address eventual consistency errors on creation
* resource/aws_eip_association: In EC2-Classic, wait until Instance returns as associated during creation
```

Previously in AWS Commercial:

```
=== RUN   TestAccAWSEIP_Ec2Classic
TestAccAWSEIP_Ec2Classic: resource_aws_eip_test.go:96: Step 1/2 error: After applying this test step, the plan was not empty.
stdout:
An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# aws_eip.test will be updated in-place
~ resource "aws_eip" "test" {
domain           = "standard"
id               = "52.0.143.158"
+ instance         = "i-0c0a6ad483e281c59"
public_dns       = "ec2-52-0-143-158.compute-1.amazonaws.com"
public_ip        = "52.0.143.158"
public_ipv4_pool = "amazon"
vpc              = false
}
Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccAWSEIP_Ec2Classic (153.41s)

=== CONT  TestAccAWSEIPAssociation_ec2Classic
    resource_aws_eip_association_test.go:97: Step 1/2 error: After applying this test step, the plan was not empty.
        stdout:

        An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
        -/+ destroy and then create replacement

        Terraform will perform the following actions:

          # aws_eip_association.test must be replaced
        -/+ resource "aws_eip_association" "test" {
              + allocation_id        = (known after apply)
              ~ id                   = "34.224.14.254" -> (known after apply)
              + instance_id          = "i-0ab9e7598ae44485f"
              + network_interface_id = (known after apply)
              + private_ip_address   = (known after apply)
                public_ip            = "34.224.14.254"
            }

        Plan: 1 to add, 0 to change, 1 to destroy.
--- FAIL: TestAccAWSEIPAssociation_ec2Classic (74.15s)

=== CONT  TestAccAWSEIPAssociation_ec2Classic
    resource_aws_eip_association_test.go:97: Step 1/2 error: Error running apply: 2020/11/03 09:35:32 [DEBUG] Using modified User-Agent: Terraform/0.12.29 HashiCorp-terraform-exec/0.10.0

        Error: Error associating EIP: AuthFailure: The address '34.239.37.205' does not belong to you.
          status code: 400, request id: d4163627-4987-4466-a297-aa2a48331dc9

=== CONT  TestAccAWSEIPAssociation_disappears
    resource_aws_eip_association_test.go:154: Step 1/1 error: Error running apply: 2020/11/03 09:35:33 [DEBUG] Using modified User-Agent: Terraform/0.12.29 HashiCorp-terraform-exec/0.10.0

        Error: Error associating EIP: InvalidAllocationID.NotFound: The allocation ID 'eipalloc-0780b47e4b04f970a' does not exist
          status code: 400, request id: 2c1a4d76-0ec2-45d7-9427-89d6e5de03ec

=== CONT  TestAccAWSEIPAssociation_networkInterface
    resource_aws_eip_association_test.go:43: Step 1/2 error: Error running apply: 2020/11/03 09:35:30 [DEBUG] Using modified User-Agent: Terraform/0.12.29 HashiCorp-terraform-exec/0.10.0

        Error: Error associating EIP: InvalidAllocationID.NotFound: The allocation ID 'eipalloc-071e65b698ca98f08' does not exist
          status code: 400, request id: 862d2320-e52a-45ef-854c-9cc90004bf77
```

Previously in AWS GovCloud (US):

```
=== RUN   TestAccAWSEIPAssociation_ec2Classic
TestAccAWSEIPAssociation_ec2Classic: provider_test.go:196: [{0 error configuring Terraform AWS Provider: error validating provider credentials: error calling sts:GetCallerIdentity: InvalidClientTokenId: The security token included in the request is invalid.
  status code: 403, request id: f2a9b7c4-2448-47a0-b5ea-87de84dd9b7a  []}]
--- FAIL: TestAccAWSEIPAssociation_ec2Classic (0.36s)
```

Output from acceptance testing in AWS Commercial:

```
--- PASS: TestAccAWSEIP_associated_user_private_ip (231.25s)
--- PASS: TestAccAWSEIP_basic (18.88s)
--- PASS: TestAccAWSEIP_disappears (12.62s)
--- PASS: TestAccAWSEIP_Ec2Classic (195.34s)
--- PASS: TestAccAWSEIP_instance (99.75s)
--- PASS: TestAccAWSEIP_Instance_Reassociate (126.92s)
--- PASS: TestAccAWSEIP_networkInterface (81.71s)
--- PASS: TestAccAWSEIP_notAssociated (144.46s)
--- PASS: TestAccAWSEIP_PublicIpv4Pool_default (18.84s)
--- PASS: TestAccAWSEIP_tags_Ec2Classic (7.61s)
--- PASS: TestAccAWSEIP_tags_Vpc (26.35s)
--- PASS: TestAccAWSEIP_twoEIPsOneNetworkInterface (82.93s)
--- SKIP: TestAccAWSEIP_CustomerOwnedIpv4Pool (2.47s)
--- SKIP: TestAccAWSEIP_PublicIpv4Pool_custom (0.00s)

--- PASS: TestAccAWSEIPAssociation_basic (159.35s)
--- PASS: TestAccAWSEIPAssociation_disappears (101.24s)
--- PASS: TestAccAWSEIPAssociation_ec2Classic (93.73s)
--- PASS: TestAccAWSEIPAssociation_instance (93.00s)
--- PASS: TestAccAWSEIPAssociation_networkInterface (71.25s)
--- PASS: TestAccAWSEIPAssociation_spotInstance (71.11s)
```

Output from acceptance testing in AWS GovCloud (US):

```
--- PASS: TestAccAWSEIP_associated_user_private_ip (245.88s)
--- PASS: TestAccAWSEIP_basic (23.76s)
--- PASS: TestAccAWSEIP_disappears (16.42s)
--- PASS: TestAccAWSEIP_instance (107.18s)
--- PASS: TestAccAWSEIP_Instance_Reassociate (165.24s)
--- PASS: TestAccAWSEIP_networkInterface (90.37s)
--- PASS: TestAccAWSEIP_notAssociated (146.54s)
--- PASS: TestAccAWSEIP_PublicIpv4Pool_default (24.04s)
--- PASS: TestAccAWSEIP_tags_Vpc (37.31s)
--- PASS: TestAccAWSEIP_twoEIPsOneNetworkInterface (90.85s)
--- SKIP: TestAccAWSEIP_CustomerOwnedIpv4Pool (2.86s)
--- SKIP: TestAccAWSEIP_Ec2Classic (2.89s)
--- SKIP: TestAccAWSEIP_PublicIpv4Pool_custom (0.00s)
--- SKIP: TestAccAWSEIP_tags_Ec2Classic (2.89s)

--- PASS: TestAccAWSEIPAssociation_basic (156.32s)
--- PASS: TestAccAWSEIPAssociation_disappears (130.31s)
--- PASS: TestAccAWSEIPAssociation_instance (89.29s)
--- PASS: TestAccAWSEIPAssociation_networkInterface (79.42s)
--- PASS: TestAccAWSEIPAssociation_spotInstance (68.02s)
--- SKIP: TestAccAWSEIPAssociation_ec2Classic (2.92s)
```
@YakDriver YakDriver self-assigned this Nov 10, 2020
@YakDriver
Copy link
Member

Is this expected on GovCloud for now?

    provider_test.go:213: [{0 error configuring Terraform AWS Provider: error validating provider credentials: error calling sts:GetCallerIdentity: InvalidClientTokenId: The security token included in the request is invalid.
        	status code: 403, request id: 987c445a-d887-4e7e-aacd-d98e84e3b7d6  []}]
--- FAIL: TestAccAWSEIPAssociation_ec2Classic (0.21s)
--- PASS: TestAccAWSEIPAssociation_instance (105.15s)

Copy link
Member

@YakDriver YakDriver left a comment

Choose a reason for hiding this comment

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

Locally, I'm consistently getting errors on commercials (same thing on 3 runs):

    resource_aws_eip_association_test.go:103: Step 1/2 error: After applying this test step, the plan was not empty.
        stdout:
        
        
        An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
        -/+ destroy and then create replacement
        
        Terraform will perform the following actions:
        
          # aws_eip_association.test must be replaced
        -/+ resource "aws_eip_association" "test" {
              + allocation_id        = (known after apply)
              ~ id                   = "107.20.142.12" -> (known after apply)
              + instance_id          = "i-04820728183b496a3"
              + network_interface_id = (known after apply)
              + private_ip_address   = (known after apply)
                public_ip            = "107.20.142.12"
            }
        
        Plan: 1 to add, 0 to change, 1 to destroy.
--- FAIL: TestAccAWSEIPAssociation_ec2Classic (81.04s)

@bflad
Copy link
Contributor Author

bflad commented Nov 11, 2020

I was probably testing with the fixes from #16032 too, really any testing working with this covers the changes since its critical path for every test.

@YakDriver YakDriver added the partition/aws-us-gov Pertains to the aws-us-gov partition. label Nov 11, 2020
bflad added a commit that referenced this pull request Nov 12, 2020
…hardcoded us-east-1 environment variable handling in tests (#16032)

Reference: #8316
Reference: #15737
Reference: #15791
Reference: #16018 (to run testing locally)

Changes:

```
* resource/aws_eip: In EC2-Classic, wait until Instance returns as associated during create or update
* resource/aws_eip_association: Retry on additional EC2 Address eventual consistency errors on creation
* resource/aws_eip_association: In EC2-Classic, wait until Instance returns as associated during creation
```

Previously in AWS Commercial:

```
=== RUN   TestAccAWSEIP_Ec2Classic
TestAccAWSEIP_Ec2Classic: resource_aws_eip_test.go:96: Step 1/2 error: After applying this test step, the plan was not empty.
stdout:
An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# aws_eip.test will be updated in-place
~ resource "aws_eip" "test" {
domain           = "standard"
id               = "52.0.143.158"
+ instance         = "i-0c0a6ad483e281c59"
public_dns       = "ec2-52-0-143-158.compute-1.amazonaws.com"
public_ip        = "52.0.143.158"
public_ipv4_pool = "amazon"
vpc              = false
}
Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccAWSEIP_Ec2Classic (153.41s)

=== CONT  TestAccAWSEIPAssociation_ec2Classic
    resource_aws_eip_association_test.go:97: Step 1/2 error: After applying this test step, the plan was not empty.
        stdout:

        An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
        -/+ destroy and then create replacement

        Terraform will perform the following actions:

          # aws_eip_association.test must be replaced
        -/+ resource "aws_eip_association" "test" {
              + allocation_id        = (known after apply)
              ~ id                   = "34.224.14.254" -> (known after apply)
              + instance_id          = "i-0ab9e7598ae44485f"
              + network_interface_id = (known after apply)
              + private_ip_address   = (known after apply)
                public_ip            = "34.224.14.254"
            }

        Plan: 1 to add, 0 to change, 1 to destroy.
--- FAIL: TestAccAWSEIPAssociation_ec2Classic (74.15s)

=== CONT  TestAccAWSEIPAssociation_ec2Classic
    resource_aws_eip_association_test.go:97: Step 1/2 error: Error running apply: 2020/11/03 09:35:32 [DEBUG] Using modified User-Agent: Terraform/0.12.29 HashiCorp-terraform-exec/0.10.0

        Error: Error associating EIP: AuthFailure: The address '34.239.37.205' does not belong to you.
          status code: 400, request id: d4163627-4987-4466-a297-aa2a48331dc9

=== CONT  TestAccAWSEIPAssociation_disappears
    resource_aws_eip_association_test.go:154: Step 1/1 error: Error running apply: 2020/11/03 09:35:33 [DEBUG] Using modified User-Agent: Terraform/0.12.29 HashiCorp-terraform-exec/0.10.0

        Error: Error associating EIP: InvalidAllocationID.NotFound: The allocation ID 'eipalloc-0780b47e4b04f970a' does not exist
          status code: 400, request id: 2c1a4d76-0ec2-45d7-9427-89d6e5de03ec

=== CONT  TestAccAWSEIPAssociation_networkInterface
    resource_aws_eip_association_test.go:43: Step 1/2 error: Error running apply: 2020/11/03 09:35:30 [DEBUG] Using modified User-Agent: Terraform/0.12.29 HashiCorp-terraform-exec/0.10.0

        Error: Error associating EIP: InvalidAllocationID.NotFound: The allocation ID 'eipalloc-071e65b698ca98f08' does not exist
          status code: 400, request id: 862d2320-e52a-45ef-854c-9cc90004bf77
```

Previously in AWS GovCloud (US):

```
=== RUN   TestAccAWSEIPAssociation_ec2Classic
TestAccAWSEIPAssociation_ec2Classic: provider_test.go:196: [{0 error configuring Terraform AWS Provider: error validating provider credentials: error calling sts:GetCallerIdentity: InvalidClientTokenId: The security token included in the request is invalid.
  status code: 403, request id: f2a9b7c4-2448-47a0-b5ea-87de84dd9b7a  []}]
--- FAIL: TestAccAWSEIPAssociation_ec2Classic (0.36s)
```

Output from acceptance testing in AWS Commercial:

```
--- PASS: TestAccAWSEIP_associated_user_private_ip (231.25s)
--- PASS: TestAccAWSEIP_basic (18.88s)
--- PASS: TestAccAWSEIP_disappears (12.62s)
--- PASS: TestAccAWSEIP_Ec2Classic (195.34s)
--- PASS: TestAccAWSEIP_instance (99.75s)
--- PASS: TestAccAWSEIP_Instance_Reassociate (126.92s)
--- PASS: TestAccAWSEIP_networkInterface (81.71s)
--- PASS: TestAccAWSEIP_notAssociated (144.46s)
--- PASS: TestAccAWSEIP_PublicIpv4Pool_default (18.84s)
--- PASS: TestAccAWSEIP_tags_Ec2Classic (7.61s)
--- PASS: TestAccAWSEIP_tags_Vpc (26.35s)
--- PASS: TestAccAWSEIP_twoEIPsOneNetworkInterface (82.93s)
--- SKIP: TestAccAWSEIP_CustomerOwnedIpv4Pool (2.47s)
--- SKIP: TestAccAWSEIP_PublicIpv4Pool_custom (0.00s)

--- PASS: TestAccAWSEIPAssociation_basic (159.35s)
--- PASS: TestAccAWSEIPAssociation_disappears (101.24s)
--- PASS: TestAccAWSEIPAssociation_ec2Classic (93.73s)
--- PASS: TestAccAWSEIPAssociation_instance (93.00s)
--- PASS: TestAccAWSEIPAssociation_networkInterface (71.25s)
--- PASS: TestAccAWSEIPAssociation_spotInstance (71.11s)
```

Output from acceptance testing in AWS GovCloud (US):

```
--- PASS: TestAccAWSEIP_associated_user_private_ip (245.88s)
--- PASS: TestAccAWSEIP_basic (23.76s)
--- PASS: TestAccAWSEIP_disappears (16.42s)
--- PASS: TestAccAWSEIP_instance (107.18s)
--- PASS: TestAccAWSEIP_Instance_Reassociate (165.24s)
--- PASS: TestAccAWSEIP_networkInterface (90.37s)
--- PASS: TestAccAWSEIP_notAssociated (146.54s)
--- PASS: TestAccAWSEIP_PublicIpv4Pool_default (24.04s)
--- PASS: TestAccAWSEIP_tags_Vpc (37.31s)
--- PASS: TestAccAWSEIP_twoEIPsOneNetworkInterface (90.85s)
--- SKIP: TestAccAWSEIP_CustomerOwnedIpv4Pool (2.86s)
--- SKIP: TestAccAWSEIP_Ec2Classic (2.89s)
--- SKIP: TestAccAWSEIP_PublicIpv4Pool_custom (0.00s)
--- SKIP: TestAccAWSEIP_tags_Ec2Classic (2.89s)

--- PASS: TestAccAWSEIPAssociation_basic (156.32s)
--- PASS: TestAccAWSEIPAssociation_disappears (130.31s)
--- PASS: TestAccAWSEIPAssociation_instance (89.29s)
--- PASS: TestAccAWSEIPAssociation_networkInterface (79.42s)
--- PASS: TestAccAWSEIPAssociation_spotInstance (68.02s)
--- SKIP: TestAccAWSEIPAssociation_ec2Classic (2.92s)
```
Applies two fixes for provider re-configuration issues:

* The `testAccProviderFactories` map should always return a new provider instance every invocation, otherwise concurrent testing with differing provider configurations can overwrite each other since the testing framework can reuse the same underlying  provider instance address across multiple gRPC plugin configurations.
* The `testAccPreCheck()` function is invoked for every test and unnecessarily verifies and reconfigures the "main" provider instance every time to the same configuration.

Adds comments around all this functionality to hopefully make this section of the code clearer for future travelers.
@YakDriver
Copy link
Member

@bflad Apologies for the force push, just a rebase. With it, this PR is now passing.

GovCloud:

    ec2_classic_test.go:54: this test can only run in EC2-Classic, platforms available in us-gov-west-1: ["VPC"]
--- SKIP: TestAccAWSEIPAssociation_ec2Classic (2.80s)
--- PASS: TestAccAWSEIPAssociation_instance (85.07s)

Commercial:

--- PASS: TestAccAWSEIPAssociation_ec2Classic (212.95s)
--- PASS: TestAccAWSEIPAssociation_instance (90.99s)

Copy link
Member

@YakDriver YakDriver left a comment

Choose a reason for hiding this comment

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

Utmärkt arbete! 💯

@bflad bflad added this to the v3.16.0 milestone Nov 13, 2020
@bflad bflad merged commit baefec3 into master Nov 13, 2020
@bflad bflad deleted the t-provider-configuration branch November 13, 2020 13:01
@ghost
Copy link

ghost commented Nov 18, 2020

This has been released in version 3.16.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 Dec 13, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
partition/aws-us-gov Pertains to the aws-us-gov partition. size/S 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.

2 participants