[WIP] aws_snapshot_create_volume_permission

[jeremy-asher]

Note: this is almost a direct copy of aws_ami_launch_permission since the API is almost identical for this functionality.

This adds the new resource aws_snapshot_create_volume_permission which manages the createVolumePermission attribute of snapshots. This allows granting an AWS account permissions to create a volume from a particular snapshot. This is often required to allow another account to copy a private AMI.

There's a few outstanding issues I could use some help with:

• there's a bug which is causing the AWS API call to succeed, but no create volume permission modifications to occur It turns out this is just really slow to update. Resolved with a wait.
• the resource is almost useless due to Can't index from schema.Set #9693, though there may be a workaround to get this to a half usable state
• the tests are non-functional until the above workaround is found

Here's some example code for testing things out. The snapshot_id has to be provided as an input after the copy completes.

variable "region" {
  type = "string"
}

variable "share_account_id" {
  type = "string"
}

variable "snapshot_id" {
  type = "string"
}

provider "aws" {
  region = "${var.region}"
}

data "aws_ami" "test" {
  name_regex  = "ubuntu/images/hvm-ssd/ubuntu-xenial-16.04-amd64-server-.*"
  owners      = ["099720109477"]
  most_recent = true
}

resource "aws_ami_copy" "test" {
  name = "create-volume-permission-test"
  description = "Create Volume Permission Test Copy"
  source_ami_id = "${data.aws_ami.test.id}"
  source_ami_region = "${var.region}"

  lifecycle {
    ignore_changes = ["source_ami_id"]
  }
}

# resource "aws_snapshot_create_volume_permission" "self-test" {
#   snapshot_id = "${var.snapshot_id}"
#   account_id = "${var.share_account_id}"
# }

[jeremy-asher]

Confirmed this works for both create and destroy, but it takes some time for the change to propagate AWS side.

[jeremy-asher]

I added a wait with a 5 minute timeout which resolves the propagation issue. The average wait seems to be around 2 minutes for my testing on us-west-2.

[catsby]

Hey @jeremy-asher – thanks for the contribution!

In #10017 we added a resource for EBS snapshots, aws_ebs_snapshot. It hasn't been included in any releases other than the v0.8 release candidates. Do you think that this permissions management you've proposed here can be rolled into that new resource, or do you think it's still something that should be it's own resource?

I was thinking we could bundle it with the ebs_snapshot resource, so something like this:

resource "aws_ebs_snapshot" "snap" {
    [...]
    permissions {
        users  = "1234567", "43245356"
        groups = "somegroup", "anothergroup"
    }
}

What do you think?

[jeremy-asher]

@catsby for my purposes, this needs to be its own resource, similar to aws_ami_launch_permission. My use case is to share AMI snapshots after an AMI is copied. The snapshot would already exist in that case. It could be nice to have a shortcut to share snapshots as they are created, but that would be a separate case.

[catsby]

I'd like to see the READ method patched up here, and I have a question about supporting multiple accounts here

[catsby]

We should probably call conn.DescribeSnapshotAttribute here and verify that the account ID we expect is still in the list of permissions. If not, we should remove this resource from state with d.SetId(""), that way if someone modifies the snapshot permissions outside of Terraform, we can detect that drift and offer to repair it.

[catsby]

ah, just noticed you implemented the Exists method, ignore this comment!

[catsby]

Do you think we should have this as account_ids and support a set of IDs, instead of a single 1:1 relationship?

[jeremy-asher]

I was thinking about that a bit. I agree that it would be nice to supply multiple IDs at once, but I opted to keep this similar to the aws_ami_launch_permission resource. I don't mind changing it if you prefer.

[catsby]

for my purposes, this needs to be its own resource, similar to aws_ami_launch_permission.

That's totally fair and reasonable. I can imagine my idea landing in ebs_snapshot eventually, but that does't invalidate the need for a separate resource here

[catsby]

I have an open question about support for multiple account ids instead of just a 1:1, and also we'll need documentation for this but I can work that out I suppose.

[jeremy-asher]

I'm happy to do docs on this as well. I mainly posted this in its current state to get feedback on the issues I called out in the description. As is, it's not really possible to use this resource in terraform without supplying a snapshot id as a variable directly. This is undesirable for most interesting use cases involving AMI sharing since the snapshot is not available directly.

[catsby]

@jeremy-asher if you're willing to contribute the docs, I'm happy to merge this as is. If we decide to support multiple account ids in the future we can change it by adding a new field.

Thanks!

[catsby]

@jeremy-asher thinking on it, I'm happy to just do the docs myself right now, you've already done plenty and I shouldn't have asked more of you. Hopefully you haven't started, but let me know, otherwise I'll get on that 😄

[jeremy-asher]

@catsby either way, though like I said, this currently doesn't fix the problem I opened it to solve. I don't think there's any modifications to the PR to be made to address that other than the tests. I could modify the tests to do something less practical, but they don't currently work.

[stack72]

Closed via #10624 :)

@catsby picked up the work in that PR

[ghost]

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.