Enable setting a description for EBS Volume Snapshots #471

saxonww · 2024-03-25T22:18:18Z

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 · 2024-03-25T22:18:23Z

All committers have signed the CLA.

saxonww · 2024-03-25T22:23:17Z

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 · 2024-03-25T22:27:01Z

builder/ebsvolume/block_device.go

+	// The description for the snapshot.
+	SnapshotDescription string `mapstructure:"snapshot_description" required:"false"`
+


I chose to add this here instead of builder/common/snapshot_config.go because it doesn't look like SnapshotDescription can be applied to the underlying volume snapshot when building an AMI with the CreateImage() call. You also already support setting an AMI Description using the ami_description field. I can move this there if that would be better, but it seemed like it would be confusing, to me.

saxonww · 2024-03-26T01:27:39Z

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 · 2024-03-26T15:29:18Z

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 · 2024-03-26T15:52:11Z

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 · 2024-03-26T16:16:29Z

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!

lbajolet-hashicorp

LGTM @saxonww,

Just left a small nit regarding the option, I'd like if possible to warn/error when the option isn't used in conjunction with snapshot_volume, but aside from that LGTM!

lbajolet-hashicorp · 2024-03-27T14:24:45Z

builder/ebsvolume/block_device.go

@@ -27,6 +27,9 @@ type BlockDevice struct {
 	// Create a Snapshot of this Volume.
 	SnapshotVolume bool `mapstructure:"snapshot_volume" required:"false"`

+	// The description for the snapshot.
+	SnapshotDescription string `mapstructure:"snapshot_description" required:"false"`


I need to delve a bit into the code for understanding it better, but I believe we should add a check that if snapshot_description is set, snapshot_volume must be true, and error if that's not the case.
Probably the kind of thing to do in the Prepare function for the builder.

Alternatively if you don't want to error, maybe at least warn that it'll be ignored as no snapshot will be created for the block device?

I've implemented something and the output looks like this:

$ packer build example.pkr.hcl Warning: snapshot_description is ignored when snapshot_volume is not set to true. on example.pkr.hcl line 12: (source code not available)

This only shows up if snapshot_description is set but snapshot_volume is not (or is set to false).

Do I need to do something to cause the code line to be printed here? The line number is also not quite correct, it's the line number of the source definition.

saxonww · 2024-03-27T22:11:18Z

builder/ebsvolume/builder.go

@@ -161,6 +161,13 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, []string, error) {
 			"Packer, inclusion of enable_t2_unlimited will error your builds.")
 	}

+	for _, configVolumeMapping := range b.config.VolumeMappings {
+		if configVolumeMapping.SnapshotDescription != "" && configVolumeMapping.SnapshotVolume != true {
+			warns = append(warns, "snapshot_description is ignored when "+


@lbajolet-hashicorp it looks like all of the fields in ebs_volumes are optional, so I don't know of a good way to make this more specific. Do you have any suggestions, or is this good enough?

Also I just realized if someone has multiple volumes they set a snapshot_description for, and more than one is missing snapshot_volume, this message will be printed more than one time. If there's no nice way to make each message specific to an ebs_volumes entry, I would propose adding a break here so it's only printed once. What do you think?

Yeah I think it's good enough, if we're able to pinpoint which volume is the problem that would be even better, but not sure there's something we can rely on there.

If we cannot pinpoint which block is the problematic one, we can definitely break after the warning is present once yes! I would maybe suggest changing the phrasing a bit to signal that at least one block is faulty but there may be others.

Suggested change

warns = append(warns, "snapshot_description is ignored when "+

warns = append(warns, "at least one block's `snapshot_description` was ignored, as it needs `snapshot_volume` to be also set.")

Alternatively, this could also conceivably be an error, as they happen early in the process (before the build even starts unless I'm mistaken), this would force users to address the issue before continuing.

saxonww · 2024-04-08T19:46:38Z

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 · 2024-04-16T14:46:59Z

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

lbajolet-hashicorp · 2024-04-16T14:50:44Z

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!

lbajolet-hashicorp

Couple nits/suggestions, thanks for the reroll @saxonww !

builder/ebsvolume/builder.go

lbajolet-hashicorp · 2024-04-16T14:57:09Z

builder/ebsvolume/builder.go

@@ -161,6 +161,13 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, []string, error) {
 			"Packer, inclusion of enable_t2_unlimited will error your builds.")
 	}

+	for _, configVolumeMapping := range b.config.VolumeMappings {
+		if configVolumeMapping.SnapshotDescription != "" && configVolumeMapping.SnapshotVolume != true {
+			warns = append(warns, "snapshot_description is ignored when "+


Yeah I think it's good enough, if we're able to pinpoint which volume is the problem that would be even better, but not sure there's something we can rely on there.

If we cannot pinpoint which block is the problematic one, we can definitely break after the warning is present once yes! I would maybe suggest changing the phrasing a bit to signal that at least one block is faulty but there may be others.

Suggested change

warns = append(warns, "snapshot_description is ignored when "+

warns = append(warns, "at least one block's `snapshot_description` was ignored, as it needs `snapshot_volume` to be also set.")

lbajolet-hashicorp · 2024-04-16T14:58:03Z

builder/ebsvolume/builder.go

@@ -161,6 +161,13 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, []string, error) {
 			"Packer, inclusion of enable_t2_unlimited will error your builds.")
 	}

+	for _, configVolumeMapping := range b.config.VolumeMappings {
+		if configVolumeMapping.SnapshotDescription != "" && configVolumeMapping.SnapshotVolume != true {
+			warns = append(warns, "snapshot_description is ignored when "+


Alternatively, this could also conceivably be an error, as they happen early in the process (before the build even starts unless I'm mistaken), this would force users to address the issue before continuing.

saxonww · 2024-04-16T15:46:51Z

I like the error idea, will this work?

lbajolet-hashicorp

LGTM! Thanks for pushing this one through @saxonww

saxonww · 2024-04-16T17:44:48Z

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 · 2024-04-16T17:46:10Z

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

saxonww requested a review from a team as a code owner March 25, 2024 22:18

saxonww commented Mar 25, 2024

View reviewed changes

Enable setting a description for EBS Volume Snapshots

c0d841a

lbajolet-hashicorp approved these changes Mar 27, 2024

View reviewed changes

warn if snapshot_description is set but snapshot_volume is false

00b6451

saxonww commented Mar 27, 2024

View reviewed changes

lbajolet-hashicorp reviewed Apr 16, 2024

View reviewed changes

go fmt; replace warning with error

2a95551

lbajolet-hashicorp approved these changes Apr 16, 2024

View reviewed changes

lbajolet-hashicorp merged commit c9a383b into hashicorp:main Apr 16, 2024
12 checks passed

lbajolet-hashicorp added the enhancement label Apr 16, 2024

saxonww deleted the ebsvolume-add-description branch April 16, 2024 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable setting a description for EBS Volume Snapshots #471

Enable setting a description for EBS Volume Snapshots #471

saxonww commented Mar 25, 2024

hashicorp-cla commented Mar 25, 2024 •

edited

Loading

saxonww commented Mar 25, 2024

saxonww Mar 25, 2024 •

edited

Loading

saxonww commented Mar 26, 2024

lbajolet-hashicorp commented Mar 26, 2024

saxonww commented Mar 26, 2024

lbajolet-hashicorp commented Mar 26, 2024

lbajolet-hashicorp left a comment

lbajolet-hashicorp Mar 27, 2024

saxonww Mar 27, 2024

saxonww Mar 27, 2024

saxonww Mar 27, 2024

lbajolet-hashicorp Apr 16, 2024

lbajolet-hashicorp Apr 16, 2024

saxonww commented Apr 8, 2024

saxonww commented Apr 16, 2024

lbajolet-hashicorp commented Apr 16, 2024

lbajolet-hashicorp left a comment

lbajolet-hashicorp Apr 16, 2024

lbajolet-hashicorp Apr 16, 2024

saxonww commented Apr 16, 2024

lbajolet-hashicorp left a comment

saxonww commented Apr 16, 2024

lbajolet-hashicorp commented Apr 16, 2024

		// The description for the snapshot.
		SnapshotDescription string `mapstructure:"snapshot_description" required:"false"`

	warns = append(warns, "snapshot_description is ignored when "+
	warns = append(warns, "at least one block's `snapshot_description` was ignored, as it needs `snapshot_volume` to be also set.")

Enable setting a description for EBS Volume Snapshots #471

Enable setting a description for EBS Volume Snapshots #471

Conversation

saxonww commented Mar 25, 2024

hashicorp-cla commented Mar 25, 2024 • edited Loading

saxonww commented Mar 25, 2024

saxonww Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

saxonww commented Mar 26, 2024

lbajolet-hashicorp commented Mar 26, 2024

saxonww commented Mar 26, 2024

lbajolet-hashicorp commented Mar 26, 2024

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saxonww commented Apr 8, 2024

saxonww commented Apr 16, 2024

lbajolet-hashicorp commented Apr 16, 2024

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saxonww commented Apr 16, 2024

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

saxonww commented Apr 16, 2024

lbajolet-hashicorp commented Apr 16, 2024

hashicorp-cla commented Mar 25, 2024 •

edited

Loading

saxonww Mar 25, 2024 •

edited

Loading