resource/aws_instance: Fix incorrect behavior of volume_tags (#12225) #12226
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
needs-triage
klebediev
force-pushed
the
bugfix/volume-tags
branch
2 times, most recently
from
a6d7d62 to
d1c237b
Compare
|
What could I do to speed up review of this PR? |
klebediev
force-pushed
the
bugfix/volume-tags
branch
from
d1c237b to
50679f9
Compare
ghost
added
the
tests
klebediev
force-pushed
the
bugfix/volume-tags
branch
2 times, most recently
from
0d52ee9 to
3291d66
Compare
klebediev
commented
Mar 10, 2020
aws/resource_aws_instance.go
Outdated
| @@ -818,7 +818,7 @@ func resourceAwsInstanceRead(d *schema.ResourceData, meta interface{}) error { | |||
| return fmt.Errorf("error setting tags: %s", err) | |||
| } | |||
|
|
|||
| volumeTags, err := readVolumeTags(conn, d.Id()) | |||
| volumeTags, err := readRootVolumeTags(d, instance, conn) | |||
There was a problem hiding this comment.
Main idea: work with root volume only, ignore tag changes for all other ebs_block_devices.
It's stated in the docs:
Currently, changes to the
ebs_block_deviceconfiguration of existing resources cannot be automatically detected by Terraform. To manage changes and attachments of an EBS block to an instance, use theaws_ebs_volumeandaws_volume_attachmentresources instead.
I believe, technically it's impossible to describe change for N volumes by single diff of volume_tags.
There was a problem hiding this comment.
Not a developer here, but maybe update
https://www.terraform.io/docs/providers/aws/r/instance.html#volume_tags
(Optional) A map of tags to assign to the devices created by the instance at launch time.
to
(Optional) A map of tags to assign to the root devices. For other ebs block devices, use the tag feature of the aws_ebs_volume resource
|
Added unit-test |
|
All |
|
@ewbankkit [sorry for mentioning], who might review this? |
|
@bflad (sorry for mentioning, I see you are the top contributor) might pls somebody take a look at this tiny pr fixing a bug affecting |
|
Shouldn't volume_tags be more useful within the root_block_device and/or ebs_block_device? |
bkmeneguello
approved these changes
Jun 16, 2020
|
I want this to be really merged. We are masking the issue by using |
|
After 6 months is there anybody to review this PR? 😯 |
|
Until the PR isn't merged I suggest you use workaround: |
It is not a nice workaround, since we need one resource per tag. A resource |
You can use foreach. Yes, it is still one resource per tag, but all of them are managed by terraform. |
|
@klebediev Thank you for your work on this! 👍 Are you willing to work on it with me over the next few days? First thing is that because of the time, it needs the conflicts to be resolved. After that, you and I can dig into it. |
YakDriver
added
partition/aws-us-gov
klebediev
force-pushed
the
bugfix/volume-tags
branch
from
3291d66 to
f1a0599
Compare
klebediev
force-pushed
the
bugfix/volume-tags
branch
from
f1a0599 to
799fe82
Compare
|
hi @YakDriver , rebased, conflicts resolved, acceptance tests are still fine. |
Yes, I'm ready |
YakDriver
added
the
breaking-change
|
@klebediev The biggest challenge is that this appears to be a breaking change. If it is, we'll have to wait until at least v4.0.0. Is this a breaking change?Although test What's the best way forward?Your PR is an awesome, narrowly-tailored fix to the problem. That is much appreciated and 99% of the time that's the perfect thing. However, stepping back, this might be the 1% when we need a broader fix. The issue really comes from the way the AWS SDK/API works. It would be fantastic to be able to put a clean and functional abstraction around it. Really |
|
Thanks for reviewing @YakDriver ! I do agree the source of this mess with Regarding this issue, It's not possible to describe tags attached to multiple volumes by a single parameter. Pretty much similar to the approach used for
Does this sound sane? |
|
@YakDriver I don't see why this being a breaking change should be a reason not to push it. The reason I say that is that I feel this is not a change, but a bug, because right now it's not working as documented, and it's not a problem in the documentation. I'm not saying the bug originates in Terraform per se, but I do feel that this is not a change but a fix, and that pushing this change doesn't break things, but that things are broken now and will remain broken until this change is pushed. For this reason I'd like to ask you not to hold off as this is in fact keeping us from tagging our root volumes properly. |
|
@klebediev @ToonSpinISAAC Thanks for your comments. To be sure, I agree that it is currently broken! However, it is broken for configurations using There is much you can do to help! Group 1: Using
|
Easy. Actually the simplest configuration without aws_volume_attachment is broken. For example: provider "aws" {
region = "us-east-1"
}
data "aws_ami" "ubuntu" {
most_recent = true
filter {
name = "name"
values = ["ubuntu/images/hvm-ssd/ubuntu-focal-20.04-amd64-server-*"]
}
filter {
name = "virtualization-type"
values = ["hvm"]
}
owners = ["099720109477"] # Canonical
}
resource "aws_instance" "web" {
ami = data.aws_ami.ubuntu.id
instance_type = "t3.micro"
tags = {
Name = "tfbug-tst"
}
volume_tags = {
"foo" = "bar"
"abc" = "xyz"
}
ebs_block_device {
device_name = "/dev/xvdf"
volume_size = 1
}
}terraform apply
# then go to AWS Console, modify tag `abc` -> `xyzzzzzz` for root EBS device
terraform apply
# Apply complete! Resources: 0 added, 0 changed, 0 destroyed.As single parameter is used to describe changes for multiple volumes we can "invent" more and more test cases. There will be issues with importing etc |
|
btw what's funny this PR will properly handle above particular situation 😄 |
|
@klebediev I will discuss this further internally. I see that the problem is worse than anticipated and your fix so far appears better than the current situation in all ways. My thinking is shifting to this being an incremental improvement rather than a breaking change. Really now the time-consuming thing will be to figure out the problems with the fix and if any of those are worse than as-is. What problems are there with your fix? How can it be tricked to still break? |
|
thanks @YakDriver |
|
We use
|
|
We have merged a fix to the Now that the fix is in place, if you find any problems with |
|
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
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Community Note
Closes #12225 #5878 #5609
Release note for CHANGELOG:
Output from acceptance testing: