-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/aws: read instance source_dest_check and save to state #3152
provider/aws: read instance source_dest_check and save to state #3152
Conversation
👍 |
@@ -475,6 +475,9 @@ func resourceAwsInstanceRead(d *schema.ResourceData, meta interface{}) error { | |||
d.Set("subnet_id", instance.SubnetId) | |||
} | |||
d.Set("ebs_optimized", instance.EbsOptimized) | |||
if instance.SubnetId != nil && *instance.SubnetId != "" { | |||
d.Set("source_dest_check", instance.SourceDestCheck) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we forcefully set it to true
if there isn't a subnet, rather than just letting it stick on whatever it's already set to? Doesn't really matter I guess, since the Update
path won't do anything with it, but it could avoid leaving a misleading value in the state.
Alternatively the path in Update
could return an error if source_dest_check
is set to false
when subnet isn't set, to make it more obvious that this combination is invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulling this in as is, but is there a reason not to just read whatever it is without the guard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phinze Since the attribute only applies in VPCs, I figured it was better to only set it for those instances and keep it out of the state file for non-VPC instances.
Completely non-important feedback inline, but otherwise LGTM! 👍 |
Subnet should always be defined for VPC instances, which is the only place On Wednesday, September 2, 2015, Martin Atkins notifications@github.com
|
@radeksimko anything holding this up? |
Ouch, sorry that this caused an outage for you @dwradcliffe. This looks good to me - pulling it in! |
provider/aws: read instance source_dest_check and save to state
Thanks @phinze! |
According to hashicorp#3116, it seems like this parameter isn't used. I couldn't trigger any differences by playing around with transit signing function, and could not find anything in the source code that actually parses this param. Presumably, it is unused?
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. |
For some reason the
source_dest_check
attribute is never being read from AWS and saved to the state. This means the state and plan are often wrong.Recently this caused an outage for us. The real value on the instance was
false
. The config was set totrue
(via the default value) and the state was (incorrectly) set totrue
. A plan revealed no change to the attribute, but when the plan was applied, terraform changed the value totrue
.