hashicorp / packer-plugin-amazon

Packer plugin for Amazon AMI Builder
https://www.packer.io/docs/builders/amazon
Mozilla Public License 2.0
72 stars 110 forks source link

Enable setting a description for EBS Volume Snapshots #471

Closed saxonww closed 5 months ago

saxonww commented 6 months ago

This PR adds a new snapshot_description field to the block device configuration for the amazon-ebsvolume builder. If the companion field snapshot_volume is set for the block device, the value of snapshot_description will be included as the Description when creating the snapshot.

See AWS Documentation for the Description request parameter.

Closes #470

hashicorp-cla commented 6 months ago

CLA assistant check
All committers have signed the CLA.

saxonww commented 6 months ago

Hello! I could use some pointers on how to write a good test for this. Looking at e.g. builder_test.go and step_snapshot_ebs_volumes_test.go, it's not obvious how to validate that the description was actually applied; it doesn't look like you do similar tests to make sure e.g. tags are applied. What would you like to see?

saxonww commented 6 months ago

I tested this using the following template:

data "amazon-ami" "example" {
  filters = {
    virtualization-type = "hvm"
    name                = "al2023-ami-*-x86_64"
    root-device-type    = "ebs"
  }
  owners      = ["137112412989"]
  most_recent = true
  region      = "us-west-2"
}

source "amazon-ebsvolume" "ssm-example" {
  region               = "us-west-2"
  ssh_username         = "ec2-user"
  instance_type        = "t3.nano"
  source_ami           = data.amazon-ami.example.id

  ebs_volumes {
    device_name = "/dev/xvda"
    volume_type = "gp3"
    delete_on_termination = true
    snapshot_volume = true
    snapshot_description = "so descriptive"
  }
}

build {
  sources = ["source.amazon-ebsvolume.ssm-example"]

  provisioner "shell" {
    inline = [
      "echo yay"
    ]
  }
}

It produced the attached output when run as:

PACKER_LOG=1 packer build example.pkr.hcl

I was able to confirm that the resulting snapshot had a description:

$ aws ec2 describe-snapshots --snapshot-ids snap-0656e1a30d0c3b147 --query 'Snapshots[].Description'
[
    "so descriptive"
]
lbajolet-hashicorp commented 6 months ago

I tested this using the following template:

data "amazon-ami" "example" {
  filters = {
    virtualization-type = "hvm"
    name                = "al2023-ami-*-x86_64"
    root-device-type    = "ebs"
  }
  owners      = ["137112412989"]
  most_recent = true
  region      = "us-west-2"
}

source "amazon-ebsvolume" "ssm-example" {
  region               = "us-west-2"
  ssh_username         = "ec2-user"
  instance_type        = "t3.nano"
  source_ami           = data.amazon-ami.example.id

  ebs_volumes {
    device_name = "/dev/xvda"
    volume_type = "gp3"
    delete_on_termination = true
    snapshot_volume = true
    snapshot_description = "so descriptive"
  }
}

build {
  sources = ["source.amazon-ebsvolume.ssm-example"]

  provisioner "shell" {
    inline = [
      "echo yay"
    ]
  }
}

It produced the attached output when run as:

PACKER_LOG=1 packer build example.pkr.hcl

I was able to confirm that the resulting snapshot had a description:

$ aws ec2 describe-snapshots --snapshot-ids snap-0656e1a30d0c3b147 --query 'Snapshots[].Description'
[
    "so descriptive"
]

Hi @saxonww,

Thanks for quickly addressing the related issue!

That test you wrote is essentially what I'd expect to see as an acceptance test quite honestly, if you're able to add it to the builder_acc_test.go file for the EBS builder for example that'd be perfect!

Not sure we have a convenience method to get the snapshots of an AMI, but I'm certain it's very feasible through the Go clients.

Let me know if you want to take a jab at this, otherwise I can take a look and push a commit on top of your branch so it gets rolled-in with the PR.

saxonww commented 6 months ago

Hi @saxonww,

Thanks for quickly addressing the related issue!

That test you wrote is essentially what I'd expect to see as an acceptance test quite honestly, if you're able to add it to the builder_acc_test.go file for the EBS builder for example that'd be perfect!

Would it go here, or in a new builder_acc_test.go for the ebsvolume builder?

I'm not opposed to giving this a shot, but it will take me longer to spin this up than it would someone familiar with the test framework. I can try to look at it this evening.

Not sure we have a convenience method to get the snapshots of an AMI, but I'm certain it's very feasible through the Go clients.

I agree, but my thinking is that it's not solving the same problem and probably should be in its own PR. At least from our perspective, we aren't blocked by lack of description on snapshots associated with an AMI. We're blocked because we can't set a description on standalone snapshots.

Let me know if you want to take a jab at this, otherwise I can take a look and push a commit on top of your branch so it gets rolled-in with the PR.

We're motivated to get the ebsvolume builder updated to allow setting the snapshot description. If we need to also update ebs to help make that happen then yeah we can take a look at that, but ideally this PR would stand on its own, I think.

lbajolet-hashicorp commented 6 months ago

Oh I see that's the ebsvolume builder that changed here, nevermind ebs then, we don't have any acceptance tests written for that builder yet, so if you tested locally and you're happy with the output, I believe we can move forward with it!

I'll do a quick final review, but this looked good to me, I believe we will be able to merge this PR soon, sorry for the misunderstanding!

saxonww commented 5 months ago

Hello @lbajolet-hashicorp, can you comment on next steps for this PR? I haven't seen any activity in a couple of weeks; I see that the PR is approved, but it looks like a handful of other workflow steps are required, and it's not clear to me how to get those handled.

At least for us, we're approaching a point where we really need this functionality. We know how to build this ourselves but not how to use it outside of a dev environment.

saxonww commented 5 months ago

Hello @lbajolet-hashicorp just checking in on this again.

lbajolet-hashicorp commented 5 months ago

Hey @saxonww,

Sorry about this, got sidetracked on other stuff.

I'll do a pass right now, I'll let you know what's the next steps (if any).

Thanks for the reminder!

saxonww commented 5 months ago

I like the error idea, will this work?

saxonww commented 5 months ago

LGTM! Thanks for pushing this one through @saxonww

Thank you! This will be a big help for us.

What is the release cadence for this plugin? Will we see this get released this month?

lbajolet-hashicorp commented 5 months ago

For sure, I'll try to make it happen by tomorrow, please ping me again if I forget :)