Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r/aws_instance: volume_tags works incorrectly with aws_volume_attachment resources #12225

Closed
klebediev opened this issue Mar 2, 2020 · 20 comments · Fixed by #15474
Closed

r/aws_instance: volume_tags works incorrectly with aws_volume_attachment resources #12225

klebediev opened this issue Mar 2, 2020 · 20 comments · Fixed by #15474
Assignees
Labels
partition/aws-us-gov Pertains to the aws-us-gov partition. service/ec2 Issues and PRs that pertain to the ec2 service.
Milestone

Comments

@klebediev
Copy link
Contributor

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform Version

$ terraform -v
Terraform v0.12.20
+ provider.aws v2.51.0

Affected Resource(s)

  • aws_instance

Terraform Configuration Files

variable az {
  default = "us-east-1a"
}

variable region {
  default = "us-east-1"
}

variable subnet {
  default = "subnet-XXXXXXXX"
}

variable ami {
  default = "ami-046842448f9e74e7d"
}

provider "aws" {
  region = var.region
}

resource "aws_instance" "tst" {
  ami                         = var.ami
  instance_type               = "t2.micro"
  associate_public_ip_address = false
  subnet_id                   = var.subnet

  tags = {
    "Name"        = "tfbug-instance"
    "Environment" = "dev"
    "instance"    = "yes"
  }

  volume_tags = {
    "Name"        = "tfbug-root"
    "Environment" = "dev"
    "root"        = "yes"
  }
}

resource "aws_volume_attachment" "tst" {
  device_name = "/dev/sdf"
  volume_id   = aws_ebs_volume.tst.id
  instance_id = aws_instance.tst.id
}

resource "aws_ebs_volume" "tst" {
  availability_zone = var.az
  size              = 1
  type              = "gp2"

  tags = {
    "Name"        = "tfbug-data"
    "Environment" = "dev"
    "data"        = "yes"
  }
}

Expected Behavior

After first terraform apply:
root EBS device should have tags:

  volume_tags = {
    "Name"        = "tfbug-root"
    "Environment" = "dev"
    "root"        = "yes"
  }

attached EBS device tags:

  tags = {
    "Name"        = "tfbug-data"
    "Environment" = "dev"
    "data"        = "yes"
  }

Subsequent terraform plan shouldn't show any changes.

Actual Behavior

After first apply volume tags are correct, but subsequent terraform apply/plan shows changes:

# aws_instance.tst will be updated in-place
~ resource "aws_instance" "tst" {
      ami                          = "ami-046842448f9e74e7d"
....
      tags                         = {
          "Environment" = "dev"
          "Name"        = "tfbug-instance"
          "instance"    = "yes"
      }
      tenancy                      = "default"
    ~ volume_tags                  = {
          "Environment" = "dev"
          "Name"        = "tfbug-root"
        - "data"        = "yes" -> null
          "root"        = "yes"
      }

....

      ebs_block_device {
          delete_on_termination = false
          device_name           = "/dev/sdf"
          encrypted             = false
          iops                  = 100
          volume_id             = "vol-XXXXXXXXXXXXX"
          volume_size           = 1
          volume_type           = "gp2"
      }

      root_block_device {
          delete_on_termination = true
          encrypted             = false
          iops                  = 100
          volume_id             = "vol-YYYYYYYYYYYYYY"
          volume_size           = 8
          volume_type           = "gp2"
      }
  }

Plan: 0 to add, 1 to change, 0 to destroy.

Note that attached EBS Volume comes up in aws_instance resource state.

Steps to Reproduce

  1. terraform apply
  2. terraform plan
@ghost ghost added the service/ec2 Issues and PRs that pertain to the ec2 service. label Mar 2, 2020
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Mar 2, 2020
@terachan-in-git
Copy link

Sorry, when will this requirement be released?

klebediev added a commit to klebediev/terraform-provider-aws that referenced this issue Sep 11, 2020
@YakDriver YakDriver removed the needs-triage Waiting for first response or review from a maintainer. label Sep 22, 2020
@YakDriver YakDriver self-assigned this Sep 22, 2020
@YakDriver
Copy link
Member

@klebediev Thank you for your continued help on this issue! I've been discussing design with the PR when really that conversation is better with the issue. Also, I apologize for being unclear in my understanding of your fix. I understand it better now. As always, correct me if I'm wrong.

The Tag Problem Currently

When running a new instance, the AWS API allows a blanket tagging of any volumes created with the launch. For Terraform, this translates to any root_block_device or ebs_block_device declared in the aws_instance. If this were the only way to create and tag volumes, we would not have a problem.

However, there is configuration overlap between aws_instance and aws_ebs_volume plus aws_volume_attachment. Volumes and volume tags can be created in both ways. When aws_instance reads and updates, it does not distinguish between volumes and volume tags created in the aws_instance config or an aws_ebs_volume config. As a result, the aws_instance will try to update tags that originated in an aws_ebs_volume.

A Fix: aws_instance Only Tags The Root Volume

One possible fix, which is currently implemented in #12226, is to limit the aws_instance to only blanket tagging at creation. All subsequent reads and updates will only handle the root volume.

However, this fix has two problems:

  1. Tags will not be updated for volumes created as part of the aws_instance creation using the ebs_block_device attribute.
  2. Configurations that knowingly use volume_tags to add and update tags for volumes created with the aws_ebs_volume resource will no longer be set or updated.

This fix is a breaking change. Here is an example of why this is a breaking change.

Breaking Change Example

  1. Starting config:
resource "aws_instance" "test" {
  volume_tags = {
    Name     = "test-tag-1"
  }

  root_block_device {
    volume_type = "standard"
    volume_size = "100"
  }

  ebs_block_device {
    device_name = "/dev/sdb"
    volume_size = "9"
  }
}
  1. terraform apply
  2. Update the config with volume_tags.Name = 'test-tag-2'
  3. terraform apply
  4. The ebs_block_device (size 9) still has tag Name = 'test-tag-1'

Another Fix Idea

What about adding an attribute called root_volume_tags and leaving volume_tags as they are? That way we could avoid breaking the config above. At the same time, people who use the aws_ebs_volume resource can avoid tagging problems by using root_volume_tags in the aws_instance declaration. root_volume_tags would basically behave the way volume_tags do in #12226. It's not a complete solution but perhaps an intermediate step.

@YakDriver YakDriver added partition/aws-us-gov Pertains to the aws-us-gov partition. waiting-response Maintainers are waiting on response from community or contributor. labels Sep 22, 2020
@klebediev
Copy link
Contributor Author

Thank you for detailed analysis @YakDriver !
I love the idea of introducing root_volume_tags. It works well in case of aws_instance + aws_ebs_volume + aws_volume_attachment and at the same time the nameroot_volume_tags should be more user-friendly than volume_tags with "specific" behavior (like Create => tag all volumes, but Update => manage only root volume tags).

Concerns/sidenotes/questions:

  1. volume_tags and root_volume_tags should be be mutually ConflictsWith, right?
  2. In case root_volume_tags is specified, one more CreateTags API call for root volume is required (don't believe this introduces any issues, putting it here just in case)
  3. root_volume_tags itself won't fix the rest of known issues with volume_tags similar to described here.

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 23, 2020
@YakDriver
Copy link
Member

YakDriver commented Sep 23, 2020

@klebediev Thank you for helping to think through any potential issues with root_volume_tags!

I agree that root_volume_tags would only address a subset of the overall volume_tags problems and that volume_tags and root_volume_tags would mutually conflict. For people who rely on volume_tags currently and are not using aws_ebs_volume, they would be stuck with the existing problems until we come up with something better for that.

I don't like the way that AWS has implemented this feature. They want it to be convenient at launch time to tag a bunch of resources but that convenience creates problems down the road.

Since we are opening design, feel free to throw out any ideas you might have (even half-baked ones) for how we can move ahead.

Ideas

  1. Limit aws_instance to only tag root volume on update to avoid conflicts with aws_ebs_volume. Problems: breaks current configs relying on volume_tags to tag more than just the root volume.
  2. Add a new attribute called root_volume_tags and leave volume_tags as is. Those who rely on volume_tags still have that option. Those who want to avoid conflicts with aws_ebs_volume can use root_volume_tags. Problems: doesn't fix the volume_tags issues
  3. Document that users can tag volumes with a special tag if they want them to be managed by aws_instance with volume_tags. Then when updating tags, check for that tag before modifying. This might fix all known issues. Problems: Adds complexity (magic) to aws_instance.
  4. Re-eval config on update to see which volume and tags should be updated. Problems: violates a design principle to avoid d.Get() outside of create, creates problems with import.

@klebediev
Copy link
Contributor Author

All approaches seem to have inevitable drawbacks (because of AWS API).
IMHO option #2 is the closest to being ideal among all 4.
If we follow #2 we decompose the big issue with volume tags into 2 smaller and non-intersecting:

  1. Configurations with attached volumes i.e.aws_instance + aws_volume_attachment + aws_ebs_volume => perfectly solved by implementing root_volume_tags (+ adding note into docs like "don't use volume_tags in configurations with aws_volume_attachment, manage tags in aws_ebs_volume, manage tags of root volume by setting root_volume_tags")

  2. Configurations with inline volumes (created by ebs_block_device). Problem: One attribute describes differences for multiple volumes. In particular, Read works not good enough. How this might be solved (raw idea): get tags for all volumes and for each key concatenate all unique values.
    For example, if we have

resource "aws_instance" "test" {
  volume_tags = {
    Name     = "test-tag-1"
  }

  root_block_device {
    volume_type = "standard"
    volume_size = "100"
  }

  ebs_block_device {
    device_name = "/dev/sdb"
    volume_size = "9"
  }
}

If state is actual for both volumes then on Read we get {Name = "test-tag-1"}
If Name = "changedOutsideOfTF" for root volume then we get {Name = "test-tag-1;changedOutsideOfTF"}

@YakDriver WDYT?

@bflad
Copy link
Contributor

bflad commented Sep 23, 2020

Just to provide some alternative implementation considerations, it may be desirable to have tags arguments associated within root_block_device and each ebs_block_device (conflicting with the root volume_tags attribute), e.g.

resource "aws_instance" "test" {
  # ... other configuration ...

  root_block_device {
    volume_type = "standard"
    volume_size = "100"
    tags = {
      Purpose = "OS"
    }
  }

  ebs_block_device {
    device_name = "/dev/sdb"
    volume_size = "9"
    tags = {
      Purpose = "Widgets Data"
    }
  }

  ebs_block_device {
    device_name = "/dev/sdc"
    volume_size = "9"
    tags = {
      Purpose = "Cogs Data"
    }
  }
}

While being the most verbose, it provides the greatest configuration flexibility for practitioners while removing any ambiguity about the desired tagging state of all the various block devices. It should also represent a hopefully easier implementation, as the associated volume ID will be with each configuration block. The verbosity issue can be solved via variables, locals, merge() function usage, etc. in configurations.

Another consideration that achieves the same end goal and exists today would be to recommend the newer aws_ec2_tag resource to handle the tagging of root_block_device and/or ebs_block_devices. Although this may be a potentially less desirable option since it is "outside" the resource managing the block devices.

Aside from these though is one caveat to any implementation outside the usage of the TagSpecifications parameter of the RunInstances API call is that it will prevent the usage of IAM tag-on-create policies. This may just require documentation to denote the limitation.

@klebediev
Copy link
Contributor Author

@bflad tags inside *_block_device blocks would be the most user-friendly, I guess, but it conflicts with (see docs):

Currently, changes to the ebs_block_device configuration of existing resources cannot be automatically detected by Terraform. To manage changes and attachments of an EBS block to an instance, use the aws_ebs_volume and aws_volume_attachment resources instead.

Not sure what was the reason why Update wasn't implemented for ebs_block_devices

@YakDriver
Copy link
Member

Although less than ideal, the shortest path to getting people up and running feels like root_volume_tags. It's basically #12226 with some minor changes (make root_volume_tags perform like the PR has volume_tags behaving, and revert volume_tags to previous behavior). It's a punt, I know.

Then we could set up a design issue to get a better plan in place.

@YakDriver
Copy link
Member

@klebediev We have discussed the design internally and have come up with this:

  1. Keep volume_tags as they are today
  2. Add tags inside the root_block_device and ebs_block_device blocks.

By keeping volume_tags, we can avoid the breaking change. There are valid uses for it as long as you aren't using tags in aws_ebs_volume.

By adding tags to root_block_device and ebs_block_device blocks, you have an alternate way of tagging on creation that doesn't mess up aws_ebs_volume tags.

Are you interested in modifying your PR along these lines? We understand people have other priorities but also want to give you the first opportunity. Let me know!

@YakDriver YakDriver added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 2, 2020
@klebediev
Copy link
Contributor Author

@YakDriver working on this in #15474

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 4, 2020
@klebediev
Copy link
Contributor Author

klebediev commented Oct 5, 2020

#15474 is ready for review

@terachan-in-git
Copy link

In conclusion, will this bug be fixed?
I'm having trouble with my work.

@klebediev
Copy link
Contributor Author

@terachan-in-git work is in progress.
It's decided to add tag map parameter to *_block_device blocks instead of changing volume_tags behavior.

@bevanbennett
Copy link

FYI: As of (recent) updates aws_instance resources that have never specified volume_tags are now constantly fighting with additional aws_ebs_volumes over whether those volumes should or should not have tags. The old behaviour of not specifying volume_tags in the instance resource and having them separately in aws_ebs_volume tags blocks needs to be restored.

@mdelagrange
Copy link

FYI: As of (recent) updates aws_instance resources that have never specified volume_tags are now constantly fighting with additional aws_ebs_volumes over whether those volumes should or should not have tags. The old behaviour of not specifying volume_tags in the instance resource and having them separately in aws_ebs_volume tags blocks needs to be restored.

I'm seeing this also. I opened an issue today with an example: #17074. It was marked as a duplicate to #5609, but I believe this is a separate issue. Until recently, we never saw this issue on instances that didn't define `volume_tags. It's much more pervasive now.

@ewbankkit
Copy link
Contributor

ewbankkit commented Jan 12, 2021

@mdelagrange There is a whole slew of overlapping issues: #5878, #5609, #5080, #770, #729, #884, #12225. If your case is not covered by one of these then we can reopen the issue.

@mdelagrange
Copy link

@mdelagrange There is a whole slew of overlapping issues: #5878, #5609, #5080, #770, #729, #884, #12225. If your case is not covered by one of these then we can reopen the issue.

I think it may be separate, since all the others involve explicit use of volume_tags. It's possible that the underlying cause is similar though.

YakDriver added a commit that referenced this issue Jan 13, 2021
resource/aws_instance: Add `tags` parameter to `root_block_device`, `ebs_block_device` blocks (#12225)
@github-actions github-actions bot added this to the v3.24.0 milestone Jan 13, 2021
@ghost
Copy link

ghost commented Jan 15, 2021

This has been released in version 3.24.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Feb 13, 2021

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Feb 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
partition/aws-us-gov Pertains to the aws-us-gov partition. service/ec2 Issues and PRs that pertain to the ec2 service.
Projects
None yet
7 participants