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

r/aws_dx_gateway_association Handle missing dx_gateway_association_id attribute #8776

Merged
merged 4 commits into from
Oct 29, 2019

Conversation

ewbankkit
Copy link
Contributor

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" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #8773

Release note for CHANGELOG:

BUG FIXES:

* resource/aws_dx_gateway_association Fix backwards compatibility issue with missing dx_gateway_association_id attribute

@ghost ghost added size/S Managed by automation to categorize the size of a PR. service/directconnect Issues and PRs that pertain to the directconnect service. labels May 24, 2019
@ewbankkit
Copy link
Contributor Author

WIP as I need to work out if/how to add an acceptance test case for this.

@ewbankkit
Copy link
Contributor Author

I'm thinking that a schema migration is the way to go here.

@ewbankkit ewbankkit changed the title [WIP] r/aws_dx_gateway_association Handle missing dx_gateway_association_id attribute r/aws_dx_gateway_association Handle missing dx_gateway_association_id attribute May 27, 2019
return nil, err
}

if len(resp.DirectConnectGatewayAssociations) == 0 {
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 seems to be the only way of handling the fact that the resource no longer exists. Setting is.ID = "" didn't force removal from state and caused errors during the read phase.

@ghost ghost added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/M Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels May 27, 2019
@ewbankkit
Copy link
Contributor Author

Acceptance tests:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxGatewayAssociation_deprecatedSingleAccount'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsDxGatewayAssociation_deprecatedSingleAccount -timeout 120m
=== RUN   TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== CONT  TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
--- PASS: TestAccAwsDxGatewayAssociation_deprecatedSingleAccount (1642.68s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1642.718s

@JagdeepChahal
Copy link

Please prioritize this. Thanks in advance!

@maryelizbeth maryelizbeth added the bug Addresses a defect in current functionality. label Aug 27, 2019
@andrewbutman
Copy link

Are there still plans for this fix to go in for a missing dx_gateway_association_id attribute?

@ewbankkit
Copy link
Contributor Author

@andrewbutman I'll try and get the checks to go green...

@andrewbutman
Copy link

@ewbankkit Appreciate it! We experienced this issue yesterday Going from AWS provider 1.60.0 to 2.25.0 but aren't able to do the terraform apply workaround described in #8773.

@ewbankkit
Copy link
Contributor Author

ewbankkit commented Oct 3, 2019

Rebased and re-running acceptance test:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxGatewayAssociation_deprecatedSingleAccount'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccAwsDxGatewayAssociation_deprecatedSingleAccount -timeout 120m
=== RUN   TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== CONT  TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
--- FAIL: TestAccAwsDxGatewayAssociation_deprecatedSingleAccount (434.30s)
    testing.go:630: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: errors during apply: error deleting Direct Connect gateway association: DirectConnectServerException: Unable to process request
        	status code: 400, request id: 7f3167cd-e455-4634-a1ce-d621261ae696

I'll investigate.

I get the same error at the command line:

$ aws --region us-west-2 directconnect delete-direct-connect-gateway-association --association-id xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx

An error occurred (DirectConnectServerException) when calling the DeleteDirectConnectGatewayAssociation operation: Unable to process request

@bflad
Copy link
Contributor

bflad commented Oct 3, 2019

@ewbankkit we've been seeing the DirectConnectServerException error during Direct Connect Gateway Association deletions in our daily acceptance testing since September 20th. Unfortunately, I haven't had time to open an AWS Support case on the manner.

I believe when I briefly played with it a few days ago, the API was working when given the deprecated parameter (VirtualGatewayId). The console also seems to work fine. 🙁

@ewbankkit
Copy link
Contributor Author

@bflad Yes, AWS console worked fine for me also.
I can open an AWS Support case as well.

@ewbankkit
Copy link
Contributor Author

Rebased and did the Terraform Plugin SDK migration for the new source file.

@ewbankkit
Copy link
Contributor Author

@bflad I have heard back from AWS Support on the case:

We have fixed the issue and are currently rolling the fix to all regions. We expect to deploy the fix by Oct 18.

@ewbankkit
Copy link
Contributor Author

The acceptance tests are now passing again:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxGatewayAssociation_deprecatedSingleAccount'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccAwsDxGatewayAssociation_deprecatedSingleAccount -timeout 120m
go: finding github.com/terraform-providers/terraform-provider-tls v2.1.1+incompatible
go: finding github.com/terraform-providers/terraform-provider-tls v2.1.1+incompatible
=== RUN   TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== CONT  TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
--- PASS: TestAccAwsDxGatewayAssociation_deprecatedSingleAccount (929.30s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	929.325s

@andrewbutman
Copy link

@ewbankkit With the acceptance tests passing, once this is merged, will this go in a new aws provider version or will it be placed in a previous version?

@ewbankkit
Copy link
Contributor Author

@andrewbutman Yes, it will be in a new provider version.

@bflad bflad added the regression Pertains to a degraded workflow resulting from an upstream patch or internal enhancement. label Oct 28, 2019
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.

Hi @ewbankkit 👋 Thanks for submitting this. Unfortunately state migration with MigrateState does not help in Terraform 0.12 environments, which requires usage of StateUpgraders instead. Rather than using state migrations though since StateUpgraders are certainly more of a hassle to implement, we should be able to implement this logic directly in the Read and Delete functions instead. This will allow us to properly remove the resource from the state should it have been deleted as well. e.g. in Read (similar would apply for Delete)

func resourceAwsDxGatewayAssociationRead(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).dxconn

	var assoc *directconnect.GatewayAssociation
	associationId := d.Get("dx_gateway_association_id").(string)

	// Handle resources managed before version 2.8.0
	if associationId == "" {
		input := &directconnect.DescribeDirectConnectGatewayAssociationsInput{
			DirectConnectGatewayId: aws.String(d.Get("dx_gateway_id").(string)),
			VirtualGatewayId:       aws.String(d.Get("vpn_gateway_id").(string)),
		}

		output, err := conn.DescribeDirectConnectGatewayAssociations(input)

		if err != nil {
			return fmt.Errorf("error reading Direct Connect Gateway Association (%s): %s", d.Id(), err)
		}

		if len(output.DirectConnectGatewayAssociations) == 0 || output.DirectConnectGatewayAssociations[0] == nil {
			log.Printf("[WARN] Direct Connect Gateway Association (%s) not found, removing from state", d.Id())
			d.SetId("")
			return nil
		}

		assoc = output.DirectConnectGatewayAssociations[0]
	} else {
		assocRaw, state, err := dxGatewayAssociationStateRefresh(conn, associationId)()
		if err != nil {
			return fmt.Errorf("error reading Direct Connect gateway association (%s): %s", d.Id(), err)
		}
		if state == gatewayAssociationStateDeleted {
			log.Printf("[WARN] Direct Connect gateway association (%s) not found, removing from state", d.Id())
			d.SetId("")
			return nil
		}

		assoc = assocRaw.(*directconnect.GatewayAssociation)
	}

Please reach out with any questions or if you do not have time to implement the feedback, thanks.

@@ -27,6 +27,9 @@ func resourceAwsDxGatewayAssociation() *schema.Resource {
State: resourceAwsDxGatewayAssociationImport,
},

SchemaVersion: 1,
MigrateState: resourceAwsDxGatewayAssociationMigrateState,
Copy link
Contributor

Choose a reason for hiding this comment

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

State migrations in Terraform 0.12 require using StateUpgraders instead of MigrateState. Please see the Extending Terraform documentation for more information.

I verified this requirement with the following:

# Terraform 0.11 syntax for easy flipping between 0.11.14 and 0.12.12
terraform {
  required_version = "0.12.12"
}

provider "aws" {
  region  = "us-east-2"
  version = "2.7.0"
}

resource "aws_dx_gateway" "test" {
  name            = "bflad-testing"
  amazon_side_asn = 65534
}

resource "aws_vpc" "test" {
  cidr_block = "10.255.255.0/28"

  tags = {
    Name = "bflad-testing"
  }
}

resource "aws_vpn_gateway" "test" {
  tags = {
    Name = "bflad-testing"
  }
}

resource "aws_vpn_gateway_attachment" "test" {
  vpc_id         = "${aws_vpc.test.id}"
  vpn_gateway_id = "${aws_vpn_gateway.test.id}"
}

resource "aws_dx_gateway_association" "test" {
  dx_gateway_id  = "${aws_dx_gateway.test.id}"
  vpn_gateway_id = "${aws_vpn_gateway_attachment.test.vpn_gateway_id}"
}
$ terraform apply
# Update provider version constraint to 2.33.0
$ terraform init
$ terraform plan
# Note DirectConnectClientException
# Copy in provider from this branch (e.g. make build) and reinitialize
$ cp ~/go/bin/terraform-provider-aws .terraform/plugins/darwin_amd64/terraform-provider-aws_v2.33.0_x4
$ terraform init
$ 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.

aws_dx_gateway.test: Refreshing state... [id=1bf32e9e-0dfc-4782-b7cf-5d94c5f719ef]
aws_vpn_gateway.test: Refreshing state... [id=vgw-0c0dad070ebae48ff]
aws_vpc.test: Refreshing state... [id=vpc-0c15c4266f46c1c99]
aws_vpn_gateway_attachment.test: Refreshing state... [id=vpn-attachment-8b7e34d3]
aws_dx_gateway_association.test: Refreshing state... [id=ga-1bf32e9e-0dfc-4782-b7cf-5d94c5f719efvgw-0c0dad070ebae48ff]

Warning: "vpn_gateway_id": [DEPRECATED] use 'associated_gateway_id' argument instead

  on main.tf line 34, in resource "aws_dx_gateway_association" "test":
  34: resource "aws_dx_gateway_association" "test" {



Error: error reading Direct Connect gateway association (ga-1bf32e9e-0dfc-4782-b7cf-5d94c5f719efvgw-0c0dad070ebae48ff): DirectConnectClientException: Association ID  has an invalid format.
	status code: 400, request id: 7d6ed9db-6e00-44a7-9ade-28d84341a5cf

@ewbankkit
Copy link
Contributor Author

@bflad Converting from MigrateState to StateUpgraders didn't seem like too big a deal and prevents having that if statement all over the place (required for the Update method as well as Read and Delete).
Repeating your sample with the terraform plan using the provider built from this branch succeeded:

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

aws_vpc.test: Refreshing state... [id=vpc-0f88ee58ba5d64afe]
aws_dx_gateway.test: Refreshing state... [id=b8258145-98e6-44b6-9b76-758adece96ec]
aws_vpn_gateway.test: Refreshing state... [id=vgw-019db4cfba8530d77]
aws_vpn_gateway_attachment.test: Refreshing state... [id=vpn-attachment-e10fdf63]
aws_dx_gateway_association.test: Refreshing state... [id=ga-b8258145-98e6-44b6-9b76-758adece96ecvgw-019db4cfba8530d77]

------------------------------------------------------------------------

No changes. Infrastructure is up-to-date.

This means that Terraform did not detect any differences between your
configuration and real physical resources that exist. As a result, no
actions need to be performed.

And I can see that the schema was updated in the state file when I then terraform apply.
From

    {
      "mode": "managed",
      "type": "aws_dx_gateway_association",
      "name": "test",
      "provider": "provider.aws",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "dx_gateway_id": "b8258145-98e6-44b6-9b76-758adece96ec",
            "id": "ga-b8258145-98e6-44b6-9b76-758adece96ecvgw-019db4cfba8530d77",
            "timeouts": null,
            "vpn_gateway_id": "vgw-019db4cfba8530d77"
          },
          "private": "eyJlMmJmYjczMC1lY2FhLTExZTYtOGY4OC0zNDM2M2JjN2M0YzAiOnsiY3JlYXRlIjo5MDAwMDAwMDAwMDAsImRlbGV0ZSI6NjAwMDAwMDAwMDAwfX0=",
          "depends_on": [
            "aws_dx_gateway.test",
            "aws_vpn_gateway_attachment.test"
          ]
        }
      ]
    },

to

    {
      "mode": "managed",
      "type": "aws_dx_gateway_association",
      "name": "test",
      "provider": "provider.aws",
      "instances": [
        {
          "schema_version": 1,
          "attributes": {
            "allowed_prefixes": [
              "10.255.255.0/28"
            ],
            "associated_gateway_id": null,
            "associated_gateway_owner_account_id": "346386234494",
            "associated_gateway_type": "virtualPrivateGateway",
            "dx_gateway_association_id": "d84e32be-5ab1-452d-b42f-168fdb3168fa",
            "dx_gateway_id": "b8258145-98e6-44b6-9b76-758adece96ec",
            "dx_gateway_owner_account_id": "346386234494",
            "id": "ga-b8258145-98e6-44b6-9b76-758adece96ecvgw-019db4cfba8530d77",
            "proposal_id": null,
            "timeouts": null,
            "vpn_gateway_id": "vgw-019db4cfba8530d77"
          },
          "private": "eyJlMmJmYjczMC1lY2FhLTExZTYtOGY4OC0zNDM2M2JjN2M0YzAiOnsiY3JlYXRlIjo5MDAwMDAwMDAwMDAsImRlbGV0ZSI6NjAwMDAwMDAwMDAwfX0=",
          "depends_on": [
            "aws_dx_gateway.test",
            "aws_vpn_gateway_attachment.test"
          ]
        }
      ]
    },

log.Println("[INFO] Found Direct Connect gateway association state v0; migrating to v1")

// dx_gateway_association_id was introduced in v2.8.0. Handle the case where it's not yet present.
if _, ok := rawState["dx_gateway_association_id"]; !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why, but starting from a version 3 state (Terraform 0.11.14), it appears this attribute is coming across in rawState as a present key in the map with nil interface value: "dx_gateway_association_id":interface {}(nil), which prevented the lookup logic from occurring for me locally.

Changing this to the below allowed it to succeed in both Terraform 0.11.14 and Terraform 0.12.12 with this branch:

Suggested change
if _, ok := rawState["dx_gateway_association_id"]; !ok {
if v, ok := rawState["dx_gateway_association_id"]; !ok || v == nil {

@bflad
Copy link
Contributor

bflad commented Oct 29, 2019

One caveat to using state migrations instead of adding the logic directly to the CRUD functions is that state migrations are not called with -refresh=false during a destroy plan:

$ terraform0.11.14 destroy -refresh=false

Warning: aws_dx_gateway_association.test: "vpn_gateway_id": [DEPRECATED] use 'associated_gateway_id' argument instead




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

Terraform will perform the following actions:

  - aws_dx_gateway.test

  - aws_dx_gateway_association.test

  - aws_vpc.test

  - aws_vpn_gateway.test

  - aws_vpn_gateway_attachment.test


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

Do you really want to destroy all resources?
  Terraform will destroy all your managed infrastructure, as shown above.
  There is no undo. Only 'yes' will be accepted to confirm.

  Enter a value: yes

aws_dx_gateway_association.test: Destroying... (ID: ga-e188363b-e1c5-4034-9b0a-d68a3a99a765vgw-0d8faf2359729302c)

Error: Error applying plan:

1 error occurred:
	* aws_dx_gateway_association.test (destroy): 1 error occurred:
	* aws_dx_gateway_association.test: error deleting Direct Connect gateway association: DirectConnectClientException: Association ID  has an invalid format.
	status code: 400, request id: f85641e5-ff14-46e2-8b36-b4ded8e7cebe

EDIT: Even with refresh enabled, still returns the error.

$ terraform0.11.14 destroy

Warning: aws_dx_gateway_association.test: "vpn_gateway_id": [DEPRECATED] use 'associated_gateway_id' argument instead



aws_vpc.test: Refreshing state... (ID: vpc-09ddd282c5bab25c9)
aws_vpn_gateway.test: Refreshing state... (ID: vgw-0d8faf2359729302c)
aws_dx_gateway.test: Refreshing state... (ID: e188363b-e1c5-4034-9b0a-d68a3a99a765)
aws_vpn_gateway_attachment.test: Refreshing state... (ID: vpn-attachment-725b679e)
aws_dx_gateway_association.test: Refreshing state... (ID: ga-e188363b-e1c5-4034-9b0a-d68a3a99a765vgw-0d8faf2359729302c)

Error: Error refreshing state: 1 error occurred:
	* aws_dx_gateway_association.test: 1 error occurred:
	* aws_dx_gateway_association.test: aws_dx_gateway_association.test: error reading Direct Connect gateway association (ga-e188363b-e1c5-4034-9b0a-d68a3a99a765vgw-0d8faf2359729302c): DirectConnectClientException: Association ID  has an invalid format.
	status code: 400, request id: b6355c09-982d-4572-b0f3-039e46c97dab

$ terraform0.11.14 plan -destroy

Warning: aws_dx_gateway_association.test: "vpn_gateway_id": [DEPRECATED] use 'associated_gateway_id' argument instead



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.

aws_vpn_gateway.test: Refreshing state... (ID: vgw-0d8faf2359729302c)
aws_vpc.test: Refreshing state... (ID: vpc-09ddd282c5bab25c9)
aws_dx_gateway.test: Refreshing state... (ID: e188363b-e1c5-4034-9b0a-d68a3a99a765)
aws_vpn_gateway_attachment.test: Refreshing state... (ID: vpn-attachment-725b679e)
aws_dx_gateway_association.test: Refreshing state... (ID: ga-e188363b-e1c5-4034-9b0a-d68a3a99a765vgw-0d8faf2359729302c)

Error: Error refreshing state: 1 error occurred:
	* aws_dx_gateway_association.test: 1 error occurred:
	* aws_dx_gateway_association.test: aws_dx_gateway_association.test: error reading Direct Connect gateway association (ga-e188363b-e1c5-4034-9b0a-d68a3a99a765vgw-0d8faf2359729302c): DirectConnectClientException: Association ID  has an invalid format.
	status code: 400, request id: 2d02722f-4396-474c-b768-3e42637361e9

@bflad bflad added this to the v2.34.0 milestone Oct 29, 2019
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.

Hey again @ewbankkit 👋 While retesting this locally again from scratch, it looks like my last comment about funkiness with destroy plans was because somehow my aws_dx_gateway_association schema was upgraded with a bunch of nil values and saved with the higher version number 😦. I was able to confirm that the state upgrader function does need the suggested nil check though. With the nil check and replacing the acceptance testing to call the new state upgrader function instead of MigrateState, everything looked good in local testing and acceptance testing:

--- PASS: TestAccAwsDxGatewayAssociation_basicTransitGatewaySingleAccount (929.91s)
--- PASS: TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount (1000.41s)
--- PASS: TestAccAwsDxGatewayAssociation_basicTransitGatewayCrossAccount (1092.54s)
--- PASS: TestAccAwsDxGatewayAssociation_basicVpnGatewayCrossAccount (1122.39s)
--- PASS: TestAccAwsDxGatewayAssociation_deprecatedSingleAccount (1125.61s)
--- PASS: TestAccAwsDxGatewayAssociation_basicVpnGatewaySingleAccount (1223.48s)
--- PASS: TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewaySingleAccount (1324.25s)
--- PASS: TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewayCrossAccount (1325.74s)

Thanks as usual for your good work and apologies for the earlier confusion.

@bflad bflad merged commit 29c9b5e into hashicorp:master Oct 29, 2019
bflad added a commit that referenced this pull request Oct 29, 2019
@ewbankkit ewbankkit deleted the issue-8773 branch October 29, 2019 10:36
@bflad
Copy link
Contributor

bflad commented Oct 31, 2019

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

@ghost
Copy link

ghost commented Mar 29, 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 and limited conversation to collaborators Mar 29, 2020
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. regression Pertains to a degraded workflow resulting from an upstream patch or internal enhancement. service/directconnect Issues and PRs that pertain to the directconnect service. size/M 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.

Breaking change for aws_dx_gateway_association vpn_gateway_id parameter
5 participants