Fix default egress rule deletion logic in case of an egress rule defi…

…nition (#123)

**Issue:** aws-controllers-k8s/community#1604

**Description of changes:**
If the user defines an egress rule which is the same as the “default“ egress rule (defined implicitly by AWS), then the ec2-controller raises the below error. Although the SG is created in AWS, the controller is unable to sync the resource.
```txt
2023-01-26T16:19:39.988Z    DEBUG    ackrt    <<<<< rm.syncSGRules    {"account": "647927084307", "role": "", "region": "eu-west-1", "kind": "SecurityGroup", "namespace": "ack-system", "name": "ack-sg-test", "is_adopted": false, "generation": 1}
2023-01-26T16:19:39.988Z    DEBUG    ackrt    <<<< rm.sdkCreate    {"account": "647927084307", "role": "", "region": "eu-west-1", "kind": "SecurityGroup", "namespace": "ack-system", "name": "ack-sg-test", "is_adopted": false, "generation": 1, "error": "InvalidPermission.Duplicate: the specified rule \"peer: 0.0.0.0/0, ALL, ALLOW\" already exists\n\tstatus code: 400, request id: 160a5101-48d5-4de3-afc9-7ff6d7b99f16"}
2023-01-26T16:19:39.988Z    DEBUG    ackrt    <<< rm.Create    {"account": "647927084307", "role": "", "region": "eu-west-1", "kind": "SecurityGroup", "namespace": "ack-system", "name": "ack-sg-test", "is_adopted": false, "generation": 1, "error": "InvalidPermission.Duplicate: the specified rule \"peer: 0.0.0.0/0, ALL, ALLOW\" already exists\n\tstatus code: 400, request id: 160a5101-48d5-4de3-afc9-7ff6d7b99f16"}
2023-01-26T16:19:39.988Z    DEBUG    ackrt    << r.createResource    {"account": "647927084307", "role": "", "region": "eu-west-1", "kind": "SecurityGroup", "namespace": "ack-system", "name": "ack-sg-test", "is_adopted": false, "generation": 1, "error": "InvalidPermission.Duplicate: the specified rule \"peer: 0.0.0.0/0, ALL, ALLOW\" already exists\n\tstatus code: 400, request id: 160a5101-48d5-4de3-afc9-7ff6d7b99f16"}
```
The change in this PR just changes the order of the flow executing the deletion of the “default“ egress rule in case an egress rule is defined by the user. Because the `syncSGRules` during creation does not really sync the SG rules as it does  not have a `latest` state during performing the sync, the “default“ egress rule hangs there and basically causes the issue.

**Note:** I am not sure whether this case requires a specific e2e test case; the current `test_rules_create_update_delete` case covers the situation where only an ingress rule is defined and then it only patches the egress rule definition which does not 100% cover this situation, because this particular case only pops up during the creation step where the default egress rule check occurs. In order to cover it, the egress rule should be defined from the beginning on I suppose. Just wanted to share my limited perspective 😄 

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2022-10-13T20:54:09Z"
+  build_date: "2023-02-01T15:41:09Z"
   build_hash: 5ee0ac052c54f008dff50f6f5ebb73f2cf3a0bd7
   go_version: go1.18.1
   version: v0.20.1-4-g5ee0ac0
 api_directory_checksum: b3a2878ca8a156389214b900257c4d572ad4e3a5
 api_version: v1alpha1
 aws_sdk_go_version: v1.44.93
 generator_config_info:
-  file_checksum: cc2c6590c6e77a6125d5eec82ff5f693109d4f99
+  file_checksum: d9d0156fc1156be66ef8542caa31686764629ad7
   original_file_name: generator.yaml
 last_modification:
   reason: API generation

apis/v1alpha1/generator.yaml

@@ -419,6 +419,7 @@ resources:
         custom_field:
           list_of: IpPermission
       EgressRules:
+        late_initialize: {}
         custom_field:
           list_of: IpPermission
       Rules:

config/crd/common/bases/services.k8s.aws_adoptedresources.yaml

@@ -1,10 +1,9 @@
-
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.7.0
+    controller-gen.kubebuilder.io/version: v0.9.2
   creationTimestamp: null
   name: adoptedresources.services.k8s.aws
 spec:
@@ -170,6 +169,7 @@ spec:
                           - name
                           - uid
                           type: object
+                          x-kubernetes-map-type: atomic
                         type: array
                     type: object
                 required:
@@ -224,9 +224,3 @@ spec:
     storage: true
     subresources:
       status: {}
-status:
-  acceptedNames:
-    kind: ""
-    plural: ""
-  conditions: []
-  storedVersions: []

config/crd/common/bases/services.k8s.aws_fieldexports.yaml

@@ -1,10 +1,9 @@
-
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.7.0
+    controller-gen.kubebuilder.io/version: v0.9.2
   creationTimestamp: null
   name: fieldexports.services.k8s.aws
 spec:
@@ -133,9 +132,3 @@ spec:
     storage: true
     subresources:
       status: {}
-status:
-  acceptedNames:
-    kind: ""
-    plural: ""
-  conditions: []
-  storedVersions: []

generator.yaml

@@ -419,6 +419,7 @@ resources:
         custom_field:
           list_of: IpPermission
       EgressRules:
+        late_initialize: {}
         custom_field:
           list_of: IpPermission
       Rules:

pkg/resource/security_group/hooks.go

@@ -20,6 +20,7 @@ import (
 
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
 	ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
+	awserr "github.com/aws/aws-sdk-go/aws/awserr"
 	svcsdk "github.com/aws/aws-sdk-go/service/ec2"
 )
 
@@ -353,6 +354,12 @@ func (rm *resourceManager) deleteDefaultSecurityGroupRule(
 	_, err = rm.sdkapi.RevokeSecurityGroupEgressWithContext(ctx, req)
 	rm.metrics.RecordAPICall("DELETE", "RevokeSecurityGroupEgress", err)
 	if err != nil {
+		if aerr, ok := err.(awserr.Error); ok {
+			switch aerr.Code() {
+			case "InvalidPermission.NotFound":
+				return
+			}
+		}
 		return err
 	}
 
@@ -463,18 +470,6 @@ func compareTags(
 	}
 }
 
-// defaultEgressRule returns the egress rule that
-// is created and associated with a security group by default
-func (rm *resourceManager) defaultEgressRule() *svcapitypes.IPPermission {
-	defaultRule := &svcapitypes.IPPermission{
-		IPRanges:   []*svcapitypes.IPRange{{CIDRIP: toStrPtr("0.0.0.0/0")}},
-		FromPort:   toInt64Ptr(-1),
-		IPProtocol: toStrPtr("-1"),
-		ToPort:     toInt64Ptr(-1),
-	}
-	return defaultRule
-}
-
 // containsRule returns true if security group rule
 // is found in the rule collection (all fields must match);
 // otherwise, return false.

templates/hooks/security_group/sdk_create_post_set_output.go.tpl

@@ -2,6 +2,14 @@
 	if rm.requiredFieldsMissingForSGRule(&resource{ko}) {
 		return nil, ackerr.NotFound
 	}
+
+	// if user defines any egress rule, then remove the default egress rule
+	if len(desired.ko.Spec.EgressRules) > 0 {
+		if err = rm.deleteDefaultSecurityGroupRule(ctx, &resource{ko}); err != nil {
+			return nil, err
+		}
+	}
+
 	if err = rm.syncSGRules(ctx, &resource{ko}, nil); err != nil {
 		return nil, err
 	}
@@ -14,14 +22,3 @@
 	} else {
 		ko.Status.Rules = rules
 	}
-
-	// if user defines any egress rule, then remove the default
-	// egress rule; otherwise, add default rule Spec to align with
-	// resource's server-side state (i.e. Status.Rules)
-	if len(desired.ko.Spec.EgressRules) > 0 {
-		if err = rm.deleteDefaultSecurityGroupRule(ctx, &resource{ko}); err != nil {
-			return nil, err
-		}
-	} else {
-		ko.Spec.EgressRules = append(ko.Spec.EgressRules, rm.defaultEgressRule())
-	}

test/e2e/tests/test_security_group.py

@@ -35,7 +35,7 @@
 
 @pytest.fixture
 def simple_security_group(request):
-    resource_name = random_suffix_name("security-group-tes", 24)
+    resource_name = random_suffix_name("security-group-test", 24)
     resource_file = "security_group"
     test_vpc = get_bootstrap_resources().SharedTestVPC
 
@@ -159,7 +159,6 @@ def test_create_delete(self, ec2_client, simple_security_group):
         # Check Security Group no longer exists in AWS
         ec2_validator.assert_security_group(resource_id, exists=False)
 
-    @pytest.mark.xfail
     def test_create_with_vpc_egress_dups_default_delete(self, ec2_client, security_group_with_vpc):
         (ref, cr) = security_group_with_vpc
         resource_id = cr["status"]["id"]