terraform-aws-modules / terraform-aws-autoscaling

Terraform module to create AWS Auto Scaling resources 🇺🇦
https://registry.terraform.io/modules/terraform-aws-modules/autoscaling/aws
Apache License 2.0
292 stars 556 forks source link

instance_refresh needs lt_version="$Latest" #143

Closed tx-kstav closed 3 years ago

tx-kstav commented 3 years ago

Description

The lt_version attribute of the ASG is optional and has default value 'null'. On the provider registry it has default value "$Default".

When not explicitly set in Terraform, it shows as '-' on AWS console. And when also using the update_default_version = true attribute, the instance_refresh is not triggered. The version and the default version of the LT are correctly updated, though. Is this a bug? (or maybe a lack of documentation?)

The instance refresh seems to be triggered if lt_version = "$Latest" is set, but this is not reflected in the example.

Also, the instance refresh is correctly triggered if using the tag trigger, we are using that as a workaround currently.

Thank you!

Versions

bryantbiggs commented 3 years ago

thanks for the issue @tx-kstav - I'm wondering if this should be filed with the upstream AWS provider. in theory, if we set the value to null it should fallback to the provider version and/or the underlying SDK version. we use null because we want to make as few assumptions as possible, but if using null here is not defaulting to $Defualt as its stated in the AWS provider docs, that seems like it might be a bug in the provider

tx-kstav commented 3 years ago

Thanks for the input @bryantbiggs . I will do some more tests and get some clean outputs to open an issue on the provider repo.

tx-kstav commented 3 years ago

I closed this and will move discussion to the provider repo.

antonbabenko commented 3 years ago

@tx-kstav What is the value of lt_version that works for you to do a refresh? lt_version = "$Latest" does not seem to trigger it because aws_autoscaling_group's block launch_template is not changing:

  ~ resource "aws_autoscaling_group" "this" {
        # ...

        launch_template {
            id      = "lt-07a470533f7bc0fa1"
            name    = "lucky-eagle-20210609152344872000000001"
            version = "$Latest"
        }
    }

@bryantbiggs Should we change version value to be taken from launch_template in aws_autoscaling_group?

dynamic "launch_template" {
    for_each = var.use_lt ? [1] : []

    content {
      name    = local.launch_template
      version = var.lt_version  # <= something like `aws_launch_template.this[0].version` ?
    }
  }
bryantbiggs commented 3 years ago

this sounds like a bug in the provider or AWS. version = "$Latest" should mean that the autoscaling group will internally (via AWS internal API methods) be notified when to update because of the $Latest value.

we could do something like you suggested @antonbabenko but I wonder if we have to do it like:

content {
  name    = local.launch_template
  version = var.create_lt ? aws_launch_template.this[0].version : var.lt_version
}

I am curious though, how does this relate to instance_refresh?

antonbabenko commented 3 years ago

If I understand correctly, version = "$Latest" means that ASG will use the latest version of the launch template when the next instance starts. It does not trigger an update on purpose.

I will implement it tomorrow.

bryantbiggs commented 3 years ago

Correct, that is my understanding as well. When using instance refresh, I think the instances will be replaced at the next scheduled interval, not at the time of launch template update (I'll have to check the AWS docs to see if there is anything on this though)

antonbabenko commented 3 years ago

@tx-kstav Please give version v4.3.0 a try.

Instance refresh works well for me now as soon as there are changes to a launch template.

tx-kstav commented 3 years ago

Awesome @antonbabenko! I will check this too and let you know! Once again thank you for your work :)

bryantbiggs commented 3 years ago

so I got around to looking this up:

  1. my assumptions were wrong - I was confusing instance_refresh with max_instance_lifetime
  2. I believe what @antonbabenko implemented is correct based on the example Hashicorp provides here and the configuration definition image
github-actions[bot] commented 1 year ago

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.