F5Networks / terraform-aws-bigip-module

Terraform module for Deploying BIG-IP in AWS
Apache License 2.0
10 stars 21 forks source link

Error! AMI Data Query Failing #57

Closed deutmeyerbrianpfg closed 7 months ago

deutmeyerbrianpfg commented 10 months ago

We've been successfully running our pipeline until recently. Our last successful run was November 15, at 08:00 UTC. The next time we ran the pipeline at November 16, at 08:00 UTC, we started seeing this AMI issue.

We are searching for "F5 BIGIP-15.1.6.1-0.0.10 BYOL-All Modules 2Boot Loc-220701172124".

Error: Your query returned no results. Please change your search criteria and try again.
Thu, 16 Nov 2023 08:04:20 GMT
│ 
Thu, 16 Nov 2023 08:04:20 GMT
│   with module.bigip_1.data.aws_ami.f5_ami,
Thu, 16 Nov 2023 08:04:20 GMT
│   on .terraform/modules/bigip_1/data.tf line 17, in data "aws_ami" "f5_ami":
Thu, 16 Nov 2023 08:04:20 GMT
│   17: data "aws_ami" "f5_ami" {

image

If I modify the data lookup, it will change the value of the ami being passed into the instance here: https://github.com/F5Networks/terraform-aws-bigip-module/blob/main/main.tf#L193

This will result in a complete ec2 replacement as defined here: https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/service/ec2/ec2_instance.go#L67

I'd like to propose one of the following changes to choose from: OPTION 1 (PREFERRED): Update the data lookup to include deprecated values. https://github.com/F5Networks/terraform-aws-bigip-module/blob/main/data.tf#L17

data "aws_ami" "f5_ami" {
  most_recent             = true
  include_deprecated = true
  owners                     = ["aws-marketplace"]

  filter {
    name   = "description"
    values = [var.f5_ami_search_name]
  }
}

OPTION 2: Allow for hardcoding the AMI ID:

  1. Add a new NOT required variable allowing for the AMI ID to passed in:
    variable "f5_ami_id" {
    description = "BIG-IP AMI ID
    type        = string
    default     = null
    }
  2. Then update the ami input of the aws_instance resource at https://github.com/F5Networks/terraform-aws-bigip-module/blob/main/main.tf#L191 to: ami = var.f5_ami_id != null ? var.f5_ami_id : data.aws_ami.f5_ami.id

OPTION 3: Ignore changes to the AMI input on the instance:

  1. Add a lifecycle block to the aws_instance resource:

    resource "aws_instance" "f5_bigip" {
    <settings>
    
    lifecycle {
    ignore_changes = [ami]
    }
    }

SUMMARY With option 1, this is the preferred and easiest option, things should just continue to work With option 2, you can just bypass the lookup by hard coding the id and let it go back to the default lookup without it recreating instances. With option 3, you'd need a new module when you want a new deployment (which I see as good) and after the initial deployment, you can just let the search default and the deployed instance will stay the same.

Please help provide a path forward as we are stuck until a fix is presented here!

pgouband commented 10 months ago

Hi @deutmeyerbrianpfg,

Have you followed one of the examples from https://github.com/F5Networks/terraform-aws-bigip-module/tree/main/examples ?

deutmeyerbrianpfg commented 10 months ago

Hi @deutmeyerbrianpfg,

Have you followed one of the examples from https://github.com/F5Networks/terraform-aws-bigip-module/tree/main/examples ?

@pgouband - Yes. We already have this deployed. I'm needing to maintain an existing deployment. The issue is this module doesn't support deprecated images!

Lookup as coded today: image

Proposed change with option 1: image

deutmeyerbrianpfg commented 10 months ago

Our SE pointed us to the following doc, which calls out the need for inlcude_deprecated.... https://community.f5.com/t5/technical-articles/the-case-of-the-missing-f5-ami-f5-big-ip-ami-lifecycle-events/ta-p/324315

Take a look at the F5 BIG-IP Terraform Module section.

pgouband commented 10 months ago

Hi @deutmeyerbrianpfg,

So adding include_deprecated to your version of the module solves the issue, no?

deutmeyerbrianpfg commented 10 months ago

Hi @deutmeyerbrianpfg,

So adding include_deprecated to your version of the module solves the issue, no?

@pgouband - Yes, adding this input to the code in the module will solve this. I would do it, but I have to fill out the contributor agreement, so I need someone from F5 to do it. Once the input is added to the data lookup, it will fix it for us and anyone else encountering this when using this module.

pgouband commented 10 months ago

Hi @deutmeyerbrianpfg,

I wasn't thinking on changing the repo but creating your own module based on this one. We will review if we want to add include_deprecated in the module.

deutmeyerbrianpfg commented 10 months ago

@pgouband - We want to keep pulling in updates provided by F5 and we just download the required version directly from the GitHub pipeline. It would be better for long term use to have the feature in the main code. If you are worried about impacts for others, you can just variablize the input like this:

Keep the default the same while allowing for others to adjust:

variable "include_deprecated_amis" {
  description = "Whether or not to include deprecated AMIs when performing the AMI search"
  type = bool
  default = false
}

data "aws_ami" "f5_ami" {
  most_recent = true
  // owners      = ["679593333241"]
  owners = ["aws-marketplace"]
  include_deprecated = var.include_deprecated_amis

  filter {
    name   = "description"
    values = [var.f5_ami_search_name]
  }
}
pgouband commented 10 months ago

Hi,

Thanks for reporting. Added to the backlog and internal tracking ID for this request is: INFRAANO-1383.

RavinderReddyF5 commented 7 months ago

fixed in v1.2.2