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

provider/aws: More noisy about aws_security_group consistency issues #9719

Closed
wants to merge 1 commit into from
Closed

Conversation

apparentlymart
Copy link
Contributor

It appears, based on the report in #6991, that the EC2 API is being inconsistent in reporting that a security group exists shortly after it has been created; we've seen Terraform get past the "Waiting for
Security Group to exist" step but then apparently detect that it's gone again once we get into the Update function.

This is not a fix for the issue, but it does make Terraform complain more loudly should the situation arise:

  • If it happens in the Update function, as I suspect is the case for bug Resource does not have attribute 'id' for variable #6991, we will treat this as a fatal error and bail out with partial state, whereas before Terraform would lose track of the security group altogether. This seems more correct in any case, since it's not the Update function's job to refresh the state.
  • If it happens in the Read function, we will still drop it from the state but we'll first generate a debug log acknowledging that this is happening, to give us more to work with when debugging should this issue
    continue to arise for those who have seen it so far.

It appears, based on the report in #6991, that the EC2 API is being
inconsistent in reporting that a security group exists shortly after it
has been created; we've seen Terraform get past the "Waiting for
Security Group to exist" step but then apparently detect that it's gone
again once we get into the Update function.

This is not a fix for the issue, but it does make Terraform complain more
loudly should the situation arise:

- If it happens in the Update function, as I suspect is the case for bug
#6991, we will treat this as a fatal error and bail out with partial
state, whereas before Terraform would lose track of the security group
altogether. This seems more correct in any case, since it's not the
Update function's job to refresh the state.

- If it happens in the Read function, we will still drop it from the
state but we'll first generate a debug log acknowledging that this is
happening, to give us more to work with when debugging should this issue
continue to arise for those who have seen it so far.
@mitchellh
Copy link
Contributor

Even better: we should probably pull the update logic out into its own function that can take a *ec2.SecurityGroup* as an arg and have Create/Update call that after querying it themselves.

This will cause the Create path to just never have this failure scenario chance, and Update should probably continue to drop the security group (after a few, short retries).

@apparentlymart
Copy link
Contributor Author

I'm down with all of that but it still seems weird to me for Update to drop the security group... to my mind, in an ideal world it should be a fatal error if Update finds anything that is inconsistent with what the plan was expecting, since otherwise Terraform isn't being true to what the plan said.

We can't always guarantee that, but I'd consider it better to catch it if we have an opportunity to do so.

@mitchellh
Copy link
Contributor

I agree with that!

@carlossg
Copy link
Contributor

I have created #11361 based on Mitchell feedback, wdyt @apparentlymart ?

@apparentlymart
Copy link
Contributor Author

Closing this out in favor of #11361.

@ghost
Copy link

ghost commented Apr 14, 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 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants