aws_security_group does not destroy rules when all nested blocks are removed

[mattvanstone]

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 "me too" comments, 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 v0.12.16
+ provider.aws v2.40.0

Affected Resource(s)

• aws_security_group

Terraform Configuration Files

provider "aws" {
  version = "~> 2.40"
  region  = "us-east-1"
}

resource "aws_security_group" "test" {
  name   = "test"
  vpc_id = "vpc-################"

#   egress {
#     from_port   = 443
#     to_port     = 443
#     protocol    = "tcp"
#     cidr_blocks = ["0.0.0.0/0"]
#     description = "HTTPS"
#   }

  ingress {
    from_port   = 443
    to_port     = 443
    protocol    = "tcp"
    cidr_blocks = ["0.0.0.0/0"]
    description = "HTTPS"
  }
}

Debug Output

https://gist.github.com/mattvanstone/3cbe440a17ca30f2b4e836b5feceec3b

Expected Behavior

When you deploy an aws_security_group with nested ingress or egress rules, then remove all the rules from the code and reapply the the infrastructure should be updated by removing the rules.

Actual Behavior

If you remove all of the egress or all of the ingress rules the provider ignores all existing rules in the state, leaving them in place in the infrastructure. If you have at least one rule then the provider will remove all of the other rules as expected.

1. Apply the terraform with both egress and ingress rules.

C:\Users\x\Desktop\bug>terraform apply

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # aws_security_group.test will be created
  + resource "aws_security_group" "test" {
      + arn                    = (known after apply)
      + description            = "Managed by Terraform"
      + egress                 = [
          + {
              + cidr_blocks      = [
                  + "0.0.0.0/0",
                ]
              + description      = "HTTPS"
              + from_port        = 443
              + ipv6_cidr_blocks = []
              + prefix_list_ids  = []
              + protocol         = "tcp"
              + security_groups  = []
              + self             = false
              + to_port          = 443
            },
        ]
      + id                     = (known after apply)
      + ingress                = [
          + {
              + cidr_blocks      = [
                  + "0.0.0.0/0",
                ]
              + description      = "HTTPS"
              + from_port        = 443
              + ipv6_cidr_blocks = []
              + prefix_list_ids  = []
              + protocol         = "tcp"
              + security_groups  = []
              + self             = false
              + to_port          = 443
            },
        ]
      + name                   = "test"
      + owner_id               = (known after apply)
      + revoke_rules_on_delete = false
      + vpc_id                 = "vpc-################"
    }

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

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

aws_security_group.test: Creating...
aws_security_group.test: Creation complete after 7s [id=sg-03015bd0a76015cd3]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

2. Remove or comment out the egress rule and then run plan. You would expect the rules to be removed from the infrastructure, but instead:

C:\Users\x\Desktop\bug>terraform plan
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.

aws_security_group.test: Refreshing state... [id=sg-03015bd0a76015cd3]

------------------------------------------------------------------------

No changes. Infrastructure is up-to-date.

This means that Terraform did not detect any differences between your
configuration and real physical resources that exist. As a result, no
actions need to be performed.

Steps to Reproduce

1. Create a terraform file with a security group that has ingress or egress rule blocks.
2. terraform apply
3. Remove all of the ingress or all of the egress blocks
4. terraform plan or terraform apply
   This will result in No changes. Infrastructure is up-to-date. when it should remove the rules.

Important Factoids

• none

References

• none

[obourdon]

Just added a new test case for your issue: see this repository

[obourdon]

Putting tests in DEBUG mode show some strange things (from my perspective) when calling HasChange in resourceAwsSecurityGroupUpdateRules

Old => (*schema.Set)(0xc000a520c0)({
 F: (schema.SchemaSetFunc) 0x4ec7530,
 m: (map[string]interface {}) <nil>,
 once: (sync.Once) {
  done: (uint32) 0,
  m: (sync.Mutex) {
   state: (int32) 0,
   sema: (uint32) 0
  }
 }
})

New => (*schema.Set)(0xc000a52120)({
 F: (schema.SchemaSetFunc) 0x4ec7530,
 m: (map[string]interface {}) <nil>,
 once: (sync.Once) {
  done: (uint32) 0,
  m: (sync.Mutex) {
   state: (int32) 0,
   sema: (uint32) 0
  }
 }
})

=> HasChange false which seems OK to me but this is using the Equal function defined in
vendor/github.com/hashicorp/terraform-plugin-sdk/helper/schema/set.go
and if one use DeepEqual (from reflect or awsutils) it gives true as some pointer addresses change

However when re-reading existing security group (plan/update)

Old => (*schema.Set)(0xc000bb8f20)({                                                                                                                                                                                                        
 F: (schema.SchemaSetFunc) 0x4ec7530,                                                                                                                                                                                                       
 m: (map[string]interface {}) (len=1) {                                                                                                                                                                                                     
  (string) (len=10) "2216620219": (map[string]interface {}) (len=9) {                                                                                                                                                                       
   (string) (len=11) "cidr_blocks": ([]interface {}) (len=1 cap=1) {                                                                                                                                                                        
    (string) (len=10) "10.0.0.0/8"                                                                                                                                                                                                          
   },                                                                                                                                                                                                                                       
   (string) (len=15) "prefix_list_ids": ([]interface {}) {                                                                                                                                                                                  
   },                                                                                                                                                                                                                                       
   (string) (len=15) "security_groups": (*schema.Set)(0xc000bb8fe0)({                                                                                                                                                                       
    F: (schema.SchemaSetFunc) 0x1b922d0,                                                                                                                                                                                                    
    m: (map[string]interface {}) <nil>,                                                                                                                                                                                                     
    once: (sync.Once) {                                                                                                                                                                                                                     
     done: (uint32) 0,                                                                                                                                                                                                                      
     m: (sync.Mutex) {                                                                                                                                                                                                                      
      state: (int32) 0,                                                                                                                                                                                                                     
      sema: (uint32) 0                                                                                                                                                                                                                      
     }                                                                                                                                                                                                                                      
    }                                                                                                                                                                                                                                       
   }),                                                                                                                                                                                                                                      
   (string) (len=11) "description": (string) "",                                                                                                                                                                                            
   (string) (len=7) "to_port": (int) 443,                                                                                                                                                                                                   
   (string) (len=16) "ipv6_cidr_blocks": ([]interface {}) {                                                                                                                                                                                 
   },                                                                                                                                                                                                                                       
   (string) (len=8) "protocol": (string) (len=3) "tcp",                                                                                                                                                                                     
   (string) (len=4) "self": (bool) false,                                                                                                                                                                                                   
   (string) (len=9) "from_port": (int) 443                                                                                                                                                                                                  
  }                                                                                                                                                                                                                                         
 },                                                                                                                                                                                                                                         
 once: (sync.Once) {                                                                                                                                                                                                                        
  done: (uint32) 1,                                                                                                                                                                                                                         
  m: (sync.Mutex) {                                                                                                                                                                                                                         
   state: (int32) 0,                                                                                                                                                                                                                        
   sema: (uint32) 0                                                                                                                                                                                                                         
  }                                                                                                                                                                                                                                         
 }                                                                                                                                                                                                                                          
})

and

New => (*schema.Set)(0xc000bb9720)({                                                                                                                                                                                                        
 F: (schema.SchemaSetFunc) 0x4ec7530,                                                                                                                                                                                                       
 m: (map[string]interface {}) (len=1) {                                                                                                                                                                                                     
  (string) (len=10) "2216620219": (map[string]interface {}) (len=9) {                                                                                                                                                                       
   (string) (len=11) "cidr_blocks": ([]interface {}) (len=1 cap=1) {                                                                                                                                                                        
    (string) (len=10) "10.0.0.0/8"                                                                                                                                                                                                          
   },                                                                                                                                                                                                                                       
   (string) (len=15) "prefix_list_ids": ([]interface {}) {                                                                                                                                                                                  
   },                                                                                                                                                                                                                                       
   (string) (len=15) "security_groups": (*schema.Set)(0xc000bb9860)({                                                                                                                                                                       
    F: (schema.SchemaSetFunc) 0x1b922d0,                                                                                                                                                                                                    
    m: (map[string]interface {}) <nil>,                                                                                                                                                                                                     
    once: (sync.Once) {                                                                                                                                                                                                                     
     done: (uint32) 0,                                                                                                                                                                                                                      
     m: (sync.Mutex) {                                                                                                                                                                                                                      
      state: (int32) 0,                                                                                                                                                                                                                     
      sema: (uint32) 0                                                                                                                                                                                                                      
     }                                                                                                                                                                                                                                      
    }                                                                                                                                                                                                                                       
   }),                                                                                                                                                                                                                                      
   (string) (len=11) "description": (string) "",                                                                                                                                                                                            
   (string) (len=7) "to_port": (int) 443,                                                                                                                                                                                                   
   (string) (len=16) "ipv6_cidr_blocks": ([]interface {}) {                                                                                                                                                                                 
   },                                                                                                                                                                                                                                       
   (string) (len=8) "protocol": (string) (len=3) "tcp",                                                                                                                                                                                     
   (string) (len=4) "self": (bool) false,                                                                                                                                                                                                   
   (string) (len=9) "from_port": (int) 443                                                                                                                                                                                                  
  }                                                                                                                                                                                                                                         
 },                                                                                                                                                                                                                                         
 once: (sync.Once) {                                                                                                                                                                                                                        
  done: (uint32) 1,                                                                                                                                                                                                                         
  m: (sync.Mutex) {                                                                                                                                                                                                                         
   state: (int32) 0,                                                                                                                                                                                                                        
   sema: (uint32) 0                                                                                                                                                                                                                         
  }                                                                                                                                                                                                                                         
 }                                                                                                                                                                                                                                          
})

gives HasChange = true which is definitely wrong !!! and this because here, due to the more nested aspect of the object, DeepEqual is used and therefore returns a "wrong" value due to
change in addresses but not in data contents

This is using reflect.DeepEqual so this is even weirder to me but may be someone very knowledgeable with Terraform code could explain it.

[obourdon]

Further deep checking after deriving reflect library code and adding debug statements to it, indeed showed that reflect.DeepEqual returns false at the very first pointer difference and therefore because of the security_groups address difference above, security rules are updated even though they are perfectly equivalent which, I think, is a shame

[obourdon]

Note that, also as stated above, the 2 completely empty objects are also DeepEqual false because of the very first addresses which differ:
Old => (*schema.Set)(0xc000a520c0)({
vs
New => (*schema.Set)(0xc000a52120)({

therefore, I understand why Equal function in set.go is used because it 'skips' this 1st level: see this line of code as reference

[obourdon]

In fact, I also found out that because of this line in reflect.DeepEqual the test fails where it should not.

Ih have therefore logged an issue for a potential trivial fix against go

[obourdon]

In fact, experiencing more on this issue, the cases where there are nested schema.Set structures completely annihilates the purpose of set.Equal function which only works at level 1

[samsullivan]

Similarly, if an ingress/egress rule is added outside of TF directly in AWS, refreshing the state will not notice this addition and no change will be necessary in your plan.

[justinretzolk]

Hey @mattvanstone 👋 Thank you for taking the time to file this issue. This may require a newer version of the AWS provider, but these blocks are now processed as attributes as blocks. With that in mind, the attributes as blocks documentation talks about explicitly calling zero objects by passing something like egress = []. Can you test this to see if that resolves the issue?

[skeggse]

I think this is just #1555, but slightly more generic to also include ingress rules.

[github-actions]

This functionality has been released in v5.8.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. Thank you!

[github-actions]

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.