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 longer timeouts to aws_ec2_client_vpn_route #40

Merged
merged 3 commits into from
Apr 21, 2022

Conversation

rpadovani
Copy link
Contributor

Default timeout for aws_ec2_client_vpn_route is 1 minute for all operations.

Trying to attach routes in eu-central-1 for a peered VPC, it constantly fails with:

│ Error: error waiting for EC2 Client VPN Route (cvpn-endpoint-xxx,subnet-yyy,10.100.0.0/16) create: timeout while waiting for state to become 'active' (last state: 'creating', timeout: 1m0s)
│ 
│   with module.ec2_client_vpn[0].aws_ec2_client_vpn_route.default[0],
│   on .terraform/modules/ec2_client_vpn/main.tf line 242, in resource "aws_ec2_client_vpn_route" "default":
│  242: resource "aws_ec2_client_vpn_route" "default" {

and

│ Error: error waiting for EC2 Client VPN Route (cvpn-endpoint-xxx,subnet-yyy,10.100.0.0/16) delete: timeout while waiting for resource to be gone (last state: 'deleting', timeout: 1m0s)

With this commit, we increment the timeout to 5 minutes. In my experiments, the route is always available in around 90 seconds. 5 minutes gives us plenty of time, and still is not too long to wait in case of problems.

Upstream issue: hashicorp/terraform-provider-aws#23787

I think fixing here is good, until we wait for an upstream improvement, because I am currently blocked

what

  • Increase Terraform timeouts for aws_ec2_client_vpn_route

why

  • Route creation fails constantly due to timeout, leaving the resources tainted

references

Default timeout for `aws_ec2_client_vpn_route` is 1 minute for all operations.

Trying to attach routes in `eu-central-1` for a peered VPC, it constantly fails with:

```
│ Error: error waiting for EC2 Client VPN Route (cvpn-endpoint-xxx,subnet-yyy,10.100.0.0/16) create: timeout while waiting for state to become 'active' (last state: 'creating', timeout: 1m0s)
│ 
│   with module.ec2_client_vpn[0].aws_ec2_client_vpn_route.default[0],
│   on .terraform/modules/ec2_client_vpn/main.tf line 242, in resource "aws_ec2_client_vpn_route" "default":
│  242: resource "aws_ec2_client_vpn_route" "default" {
```

and 

```
│ Error: error waiting for EC2 Client VPN Route (cvpn-endpoint-xxx,subnet-yyy,10.100.0.0/16) delete: timeout while waiting for resource to be gone (last state: 'deleting', timeout: 1m0s)
```

With this commit, we increment the timeout to 5 minutes. In my experiments, the route is always available in around 90 seconds. 5 minutes gives us plenty of time, and still is not too long to wait in case of problems.

Upstream issue: hashicorp/terraform-provider-aws#23787

I think fixing here is good, until we wait for an upstream improvement, because I am currently blocked
@rpadovani rpadovani requested review from a team as code owners April 21, 2022 12:31
@rpadovani
Copy link
Contributor Author

/cc @Gowiem

(sorry for the direct ping, but this is currently a blocker for me)

@Gowiem
Copy link
Member

Gowiem commented Apr 21, 2022

/test all

@Gowiem Gowiem self-requested a review April 21, 2022 15:35
Gowiem
Gowiem previously requested changes Apr 21, 2022
main.tf Outdated

timeouts {
create = "5m"
update = "5m"
Copy link
Member

Choose a reason for hiding this comment

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

@rpadovani it looks like update is not a timeouts config option on this resource. Please remove this single line and I'll re-run tests + review.

See https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ec2_client_vpn_route#timeouts for full info on the timeouts configuration for this resource.

Copy link
Contributor Author

@rpadovani rpadovani Apr 21, 2022

Choose a reason for hiding this comment

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

🤦 thanks for properly reviewing this @Gowiem, I added it without double-checking, I am sorry. Fixed.

There isn't such a timeout, remove it.
@mergify mergify bot dismissed Gowiem’s stale review April 21, 2022 15:54

This Pull Request has been updated, so we're dismissing all reviews.

@Gowiem
Copy link
Member

Gowiem commented Apr 21, 2022

/test all

Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

@rpadovani All good -- Happens to the best of us!

Before we merge this, can you triple confirm that adding timeouts in your own usage doesn't cause a rebuild of the resource or anything similar? It is a backwards compatible change? My assumption is yes, but I haven't added timeouts to a resource yet surprisingly so I'm not 100%.

@rpadovani
Copy link
Contributor Author

@Gowiem, I don't have a stable state, all my 3 routes are tainted, so I tested in this way:

  1. I removed one of the routes ('cause it is stale!), and imported it again:
aws-vault exec dock-vpn -- terragrunt state rm "module.ec2_client_vpn[0].aws_ec2_client_vpn_route.default[2]"
aws-vault exec dock-vpn -- terragrunt import "module.ec2_client_vpn[0].aws_ec2_client_vpn_route.default[2]" "cvpn-endpoint-04da04851fedf3b7b,subnet-0805d79cdca015578,10.100.0.0/16"
  1. I replaced the module with my fork:
➜ git --no-pager diff
diff --git a/client_vpn.tf b/client_vpn.tf
index 5e1e3ea..48206d2 100644
--- a/client_vpn.tf
+++ b/client_vpn.tf
@@ -22,8 +22,7 @@ locals {
 }
 
 module "ec2_client_vpn" {
-  source  = "cloudposse/ec2-client-vpn/aws"
-  version = "0.11.0"
+  source = "git::git@github.com:rpadovani/terraform-aws-ec2-client-vpn.git?ref=patch-1"
 
   count = var.client_vpn != null ? 1 : 0
  1. I ran plan:
aws-vault exec dock-vpn -- terragrunt plan -target="module.ec2_client_vpn[0]"                                                                                                       
Initializing modules...
Downloading git::ssh://git@github.com/rpadovani/terraform-aws-ec2-client-vpn.git?ref=patch-1 for ec2_client_vpn...
- ec2_client_vpn in .terraform/modules/ec2_client_vpn

[...useless part...]

Terraform will perform the following actions:

  # module.ec2_client_vpn[0].aws_ec2_client_vpn_route.default[0] is tainted, so must be replaced
-/+ resource "aws_ec2_client_vpn_route" "default" {
      ~ id                     = "cvpn-endpoint-04da04851fedf3b7b,subnet-0c9e828c615486624,10.100.0.0/16" -> (known after apply)
      ~ origin                 = "add-route" -> (known after apply)
      ~ type                   = "Nat" -> (known after apply)
        # (4 unchanged attributes hidden)

      + timeouts {
          + create = "5m"
          + delete = "5m"
        }
    }

  # module.ec2_client_vpn[0].aws_ec2_client_vpn_route.default[1] is tainted, so must be replaced
-/+ resource "aws_ec2_client_vpn_route" "default" {
      ~ id                     = "cvpn-endpoint-04da04851fedf3b7b,subnet-0477c31fa00337f2f,10.100.0.0/16" -> (known after apply)
      ~ origin                 = "add-route" -> (known after apply)
      ~ type                   = "Nat" -> (known after apply)
        # (4 unchanged attributes hidden)

      + timeouts {
          + create = "5m"
          + delete = "5m"
        }
    }

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

So, it seems to want to replace only the tainted routes, not the 3rd one I imported. I would say we are safe :-)

@Gowiem Gowiem merged commit f581b12 into cloudposse:master Apr 21, 2022
@Gowiem
Copy link
Member

Gowiem commented Apr 21, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants