PaloAltoNetworks / terraform-aws-vmseries-modules

Terraform Reusable Modules for VM-Series on AWS
https://registry.terraform.io/modules/PaloAltoNetworks/vmseries-modules/aws
MIT License
40 stars 49 forks source link

Ignore AMI changes for VM Series & panorama aws_instance resources #404

Closed elduds closed 11 months ago

elduds commented 1 year ago

The VM series and panorama modules always reference the latest AMI in the respective aws_ami data source.

This is fine for the initial apply but subsequent applies may trigger a destroy/replace if a newer AMI has since become available. The workaround is to lookup & supply the AMI ID as a variable however this is needlessly complicated, doesn't work across regions etc.

Suggest adding

  lifecycle {
    ignore_changes = [ami]
  }

This will pick the latest AMI at launch and keep the instance running. A newer AMI can be used to replace an existing instance via terraform apply -replace or terraform taint

https://github.com/PaloAltoNetworks/terraform-aws-vmseries-modules/blob/94102b824e2bdd8f043494c655be77fde6487b36/modules/vmseries/main.tf#L112

https://github.com/PaloAltoNetworks/terraform-aws-vmseries-modules/blob/94102b824e2bdd8f043494c655be77fde6487b36/modules/panorama/main.tf#L31

sebastianczech commented 1 year ago

Currently in ignore_changes for module vmseries we have defined:

    ignore_changes = [
      user_data,
    ]

because bootstrapping of VM-Series is only done via first lunch of the virtual machine and there is no need to recreate VM, if user data is changed.

The best approach would be to allow dynamically define ignored changes e.g. some of the customers could use [user_data, ami], other customers could use [user_data]. Unfortunately in ignore_changes only static list expression is required, so we cannot set it dynamically and we cannot use variable to define which changes can be ignored. More details, why it's not possible in Terraform, can be found in issue 3116 - Cannot use interpolations in lifecycle attributes.

Besides that fact about static expression, if vmseries_ami_id is null, then vmseries module is not using latest version of PAN-OS, but PAN-OS specified by vmseries_version: https://github.com/PaloAltoNetworks/terraform-aws-vmseries-modules/blob/5684f529cbb0b58ea832669b453869eba8490680/modules/vmseries/main.tf#L2-L18

so until there is no change in vmseries_version value, then AMI ID is also the same and there is no need to replace existing VM-Series instance.

Is that description enough ?

sebastianczech commented 12 months ago

@elduds can I close that issue ?

sebastianczech commented 11 months ago

I'm closing the issue. In case more information or other changes are needed, please open new issue.