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

Changing AWS Security Group description forces new resource #14330

Closed
spanktar opened this issue May 9, 2017 · 6 comments
Closed

Changing AWS Security Group description forces new resource #14330

spanktar opened this issue May 9, 2017 · 6 comments

Comments

@spanktar
Copy link

spanktar commented May 9, 2017

Terraform Version

Terraform v0.8.8

Affected Resource(s)

  • aws_security_group

Terraform Configuration Files

resource "aws_security_group" "elb-public-sg" {
  description       = "HTTP traffic to ELB from CloudFront and backends"
  name              = "elb-public-sg"
  vpc_id            = "${var.vpc-id}"
  lifecycle {
    ignore_changes  = ["description"]
  }
}

Expected Behavior

Changes to description should either not be allowed (Throw error. This is not allowed via AWS Console), or throw with a warning (didn't change description). Ignore_changes lifecycle block should not be required.

Actual Behavior

Changes to description causes a new resource to be created.

Steps to Reproduce

Please list the steps required to reproduce the issue, for example:

  1. terraform apply
  2. change description of a security group
  3. terraform apply
@apparentlymart
Copy link
Member

Hi @spanktar. Sorry things aren't working as you expected here.

Replacing a resource when the config requests an impossible edit is the standard, expected behavior for Terraform resources. If Terraform were to produce an error in this case then you would still need to use ignore_changes to allow you to proceed with a different description.

Something I am missing from your message here is the subtext for why you consider the current behavior sub-optimal. This is the standard behavior for attributes that can't be updated, but if there's a special reason why you think this resource in particular should deviate from the norm then I'm happy to consider it!

@apparentlymart apparentlymart added the waiting-response An issue/pull request is waiting for a response from the community label May 9, 2017
@spanktar
Copy link
Author

It's just unexpected behavior (IMHO) that changing a description would rebuild the resource is all. Normally you would expect a simple description change to just apply as an update, just as it would for say, an ELB or something. Also, creating a new SG resource can cause issues, for example, it may be impossible to destroy the SG because it has dependent resources.

My suggestion for throwing an error would prompt the end user to understand this and then make the conscious change to ignore_changes rather than be surprised by a recreation of the SG and possibly ending up in a bind with a dependency issue.

I guess what I'm saying is: it would be nice.

@apparentlymart
Copy link
Member

Hi @spanktar,

I can understand that this could be unexpected. Terraform's general mechanism for dealing with this is to include an explicit note in the plan output to flag that an update requires a resource to be replaced rather than just updated:

-/+ aws_security_group.elb-public-sg
        description: "old-description" -> "new_description" (forces new resource)

This first-class modeling of replacement is a core part of Terraform, and it's the foundation also of the create_before_destroy lifecycle model which allows certain resources (those where old and new instances can safely coexist) to be replaced without downtime.

I can see a couple different ways to address your root concern here while remaining consistent with Terraform's model:

  • Make the indication of this more visible in the plan output. I think we would stop short of rendering it visually as a warning, since replacing resources is a normal part of using Terraform and we would not want to give the impression that it is something that should be avoided. But if you think the current indication is not prominent enough we could consider ways to make it more prominent.
  • Allow a new option to plan like -allow-replace=false that allows opting in to replacement of resources being an error, for situations where users wish to be more conservative.

Generally-speaking though, we will not change the existing replacement behavior in the default case because it is undesirable for users to have to "work around" Terraform to get things done... we want Terraform to be able to make any change a user might want to make, while being explicit about how it will get done so that there are no surprises.

@spanktar
Copy link
Author

All sounds good, and while I agree with all of your points, I still feel like this behavior (and mind you, this is NOT a Terraform problem) differs from the usual expected behavior. Nobody I've worked with expects a description change to force a new resource, and we have had to add ignore_changes for all SG's with a comment explaining why so nobody gets confused. We don't have to do this for ANY other type of resource. This is why I flagged it.

Frankly, this really sounds to me like an AWS problem that then Terraform has to deal with. For example, there is no way in the AWS web console to actually even edit the description of a security group, and I haven't checked the API.

I do think it should be more prominent somehow. Allow-replace seems like overkill though.

My solution would be to simply error with something like: "Security Group descriptions cannot be changed. To change a description, taint the resource", but that's just me.

I'm happy to close this and leave this conversation in the record in case someone else runs across it and wants to make any other suggestions.

Thoughts?

@apparentlymart
Copy link
Member

This behavior is true of anything that the underlying API doesn't support updates for. That includes, for example, the names of ELBs and Autoscaling Groups, the engine of an RDS cluster, and many more. The intent is that the plan is being explicit about this and letting the user decide, rather than simply drawing a hard line that the change isn't possible.

In most cases it seems that users do not use ignore_changes in situations like these, but rather just accept that descriptions can't be changed and not try to change them. If a particular resource is critical and you want to reduce the chance of mistakes, the prevent_destroy lifecycle flag is actually closer to the behavior you asked for: it'll fail plan if the only way to apply a given change is to replace the resource, effectively acting like -allow-replace=false but for a single resource only.

It seems like this is one of those "agree to disagree" situations. I can certainly see your point, but yet Terraform's design is intentionally the opposite of what you suggest: it figures out how to make what you asked for happen, even if that requires destructive actions, but is always explicit about what is going to happen so that the operator can decide.

With all of that said, I am thankful of you raising this and having this discussion with me. It was nice to think through the motivating use-cases here again, since it's easy for us to start taking them for granted after a while, and indeed this thread will likely serve to be a useful reference in future.

@paddycarver paddycarver removed the waiting-response An issue/pull request is waiting for a response from the community label May 10, 2017
@ghost
Copy link

ghost commented Apr 12, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants