Support security groups with cyclic references (#213)

TiberiuGC · web-flow · commit 6d61fbc669b5 · 2024-08-29T20:42:16.000Z
Issue #, if available: aws-controllers-k8s/community#2119 Description of changes: Cyclic references support is done via. the following workflow: 1. skip runtime reference state validations by setting `SecurityGroup.Rules.UserIDGroupPairs.GroupID.skip_resource_state_validations: true` (see aws-controllers-k8s/code-generator#544). This allows runtime to proceed with the `sdkCreate` call. 2. inside `sdkCreate` and `sdkUpdate` add custom logic that checks whether referenced security groups are being created on AWS end (i.e. `groupID != nil`). If the checks succeed, move forward with syncing SG rules. Otherwise, requeue and wait for all referenced SGs to be created. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml
@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2024-08-29T17:15:01Z"
+  build_date: "2024-08-29T20:21:49Z"
   build_hash: f8f98563404066ac3340db0a049d2e530e5c51cc
   go_version: go1.22.5
   version: v0.38.1
 api_directory_checksum: 1b53401670898ce50e6d6cc8bfba6b63ea7d5683
 api_version: v1alpha1
 aws_sdk_go_version: v1.44.93
 generator_config_info:
-  file_checksum: ff3f54d44dba872977fef4f23c5f766a1bebbbc2
+  file_checksum: b6cf44fddbe38dd354160538b750818e10bda45c
   original_file_name: generator.yaml
 last_modification:
   reason: API generation
diff --git a/apis/v1alpha1/generator.yaml b/apis/v1alpha1/generator.yaml
@@ -528,6 +528,7 @@ resources:
         references:
           resource: SecurityGroup
           path: Status.ID
+          skip_resource_state_validations: true
         is_required: false
     renames:
       operations:
@@ -553,6 +554,8 @@ resources:
         template_path: hooks/security_group/sdk_create_post_set_output.go.tpl
       sdk_read_many_post_set_output:
         template_path: hooks/security_group/sdk_read_many_post_set_output.go.tpl
+      sdk_delete_pre_build_request:
+        template_path: hooks/security_group/sdk_delete_pre_build_request.go.tpl
     update_operation:
       custom_method_name: customUpdateSecurityGroup
   NetworkAcl:
diff --git a/generator.yaml b/generator.yaml
@@ -528,6 +528,7 @@ resources:
         references:
           resource: SecurityGroup
           path: Status.ID
+          skip_resource_state_validations: true
         is_required: false
     renames:
       operations:
@@ -553,6 +554,8 @@ resources:
         template_path: hooks/security_group/sdk_create_post_set_output.go.tpl
       sdk_read_many_post_set_output:
         template_path: hooks/security_group/sdk_read_many_post_set_output.go.tpl
+      sdk_delete_pre_build_request:
+        template_path: hooks/security_group/sdk_delete_pre_build_request.go.tpl
     update_operation:
       custom_method_name: customUpdateSecurityGroup
   NetworkAcl:
diff --git a/go.mod b/go.mod
@@ -2,7 +2,7 @@ module github.com/aws-controllers-k8s/ec2-controller
 
 go 1.22.0
 
-toolchain go1.22.5
+toolchain go1.22.6
 
 require (
 	github.com/aws-controllers-k8s/runtime v0.38.0
diff --git a/pkg/resource/security_group/hooks.go b/pkg/resource/security_group/hooks.go
@@ -17,9 +17,11 @@ import (
 	"context"
 
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
+	ackcondition "github.com/aws-controllers-k8s/runtime/pkg/condition"
 	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"
+	corev1 "k8s.io/api/core/v1"
 
 	svcapitypes "github.com/aws-controllers-k8s/ec2-controller/apis/v1alpha1"
 	"github.com/aws-controllers-k8s/ec2-controller/pkg/tags"
@@ -102,6 +104,29 @@ func (rm *resourceManager) requiredFieldsMissingForSGRule(
 	return r.ko.Status.ID == nil
 }
 
+// referencesResolved checks that any referenced security group actually exists in AWS, before proceeding with syncSGRules.
+// This is required because Rules.UserIDGroupPairs.GroupID.skip_resource_state_validations is set to true,
+// meaning that any state validations performed at runtime, during ResolveReferences step, are being skipped.
+func (rm *resourceManager) referencesResolved(
+	r *resource,
+) bool {
+	for _, rule := range r.ko.Spec.IngressRules {
+		for _, groupPair := range rule.UserIDGroupPairs {
+			if groupPair.GroupRef != nil && groupPair.GroupID == nil {
+				return false
+			}
+		}
+	}
+	for _, rule := range r.ko.Spec.EgressRules {
+		for _, groupPair := range rule.UserIDGroupPairs {
+			if groupPair.GroupRef != nil && groupPair.GroupID == nil {
+				return false
+			}
+		}
+	}
+	return true
+}
+
 // syncSGRules analyzes desired and latest (if any)
 // resources and executes API calls to Create/Delete
 // rules in order to achieve desired state.
@@ -357,6 +382,11 @@ func (rm *resourceManager) customUpdateSecurityGroup(
 	updated = rm.concreteResource(desired.DeepCopy())
 
 	if delta.DifferentAt("Spec.IngressRules") || delta.DifferentAt("Spec.EgressRules") {
+		if !rm.referencesResolved(desired) {
+			ackcondition.SetSynced(latest, corev1.ConditionFalse, nil, nil)
+			return latest, nil
+		}
+
 		if err := rm.syncSGRules(ctx, desired, latest); err != nil {
 			return nil, err
 		}
diff --git a/pkg/resource/security_group/references.go b/pkg/resource/security_group/references.go
diff --git a/pkg/resource/security_group/sdk.go b/pkg/resource/security_group/sdk.go
diff --git a/templates/hooks/security_group/sdk_create_post_set_output.go.tpl b/templates/hooks/security_group/sdk_create_post_set_output.go.tpl
@@ -8,6 +8,11 @@
 		return &resource{ko}, err
 	}
 
+	if !rm.referencesResolved(&resource{ko}) {
+		ackcondition.SetSynced(&resource{ko}, corev1.ConditionFalse, nil, nil)
+        return &resource{ko}, nil
+	}
+
 	if err = rm.syncSGRules(ctx, &resource{ko}, nil); err != nil {
 		return &resource{ko}, err
 	}
diff --git a/templates/hooks/security_group/sdk_delete_pre_build_request.go.tpl b/templates/hooks/security_group/sdk_delete_pre_build_request.go.tpl
@@ -0,0 +1,6 @@
+	sgCpy := r.ko.DeepCopy()
+	sgCpy.Spec.IngressRules = nil
+    sgCpy.Spec.EgressRules = nil
+	if err := rm.syncSGRules(ctx, &resource{ko: sgCpy}, r); err != nil {
+		return nil, err
+	}
diff --git a/test/e2e/resources/security_group_with_sg_ref.yaml b/test/e2e/resources/security_group_with_sg_ref.yaml
@@ -0,0 +1,17 @@
+apiVersion: ec2.services.k8s.aws/v1alpha1
+kind: SecurityGroup
+metadata:
+  name: $SECURITY_GROUP_NAME
+spec:
+  name: $SECURITY_GROUP_NAME
+  description: test sg
+  vpcID: $VPC_ID
+  ingressRules:
+    - fromPort: 443
+      toPort: 443
+      ipProtocol: tcp
+      userIDGroupPairs:
+        - description: test UID group pair
+          groupRef:
+            from:
+              name: $SECURITY_GROUP_REF_NAME
diff --git a/test/e2e/tests/test_security_group.py b/test/e2e/tests/test_security_group.py
@@ -34,6 +34,9 @@
 DELETE_WAIT_AFTER_SECONDS = 10
 MODIFY_WAIT_AFTER_SECONDS = 5
 
+CREATE_CYCLIC_REF_AFTER_SECONDS = 60
+DELETE_CYCLIC_REF_AFTER_SECONDS = 30
+
 @pytest.fixture
 def simple_security_group(request):
     resource_name = random_suffix_name("security-group-test", 24)
@@ -140,6 +143,62 @@ def security_group_with_vpc(request, simple_vpc):
     except:
         pass
 
+def create_security_group_with_sg_ref(resource_name, reference_name):
+    replacements = REPLACEMENT_VALUES.copy()
+    replacements["VPC_ID"] = get_bootstrap_resources().SharedTestVPC.vpc_id
+    replacements["SECURITY_GROUP_NAME"] = resource_name
+    replacements["SECURITY_GROUP_REF_NAME"] = reference_name
+
+    # Load Security Group CR
+    resource_data = load_ec2_resource(
+        "security_group_with_sg_ref",
+        additional_replacements=replacements,
+    )
+    logging.debug(resource_data)
+
+    # Create k8s resource
+    ref = k8s.CustomResourceReference(
+        CRD_GROUP, CRD_VERSION, RESOURCE_PLURAL,
+        resource_name, namespace="default",
+    )
+    k8s.create_custom_resource(ref, resource_data)
+    
+    return ref
+    
+@pytest.fixture
+def security_groups_cyclic_ref():
+    resource_name_1 = random_suffix_name("security-group-test", 24)
+    resource_name_2 = random_suffix_name("security-group-test", 24)
+    resource_name_3 = random_suffix_name("security-group-test", 24)
+    
+    ref_1 = create_security_group_with_sg_ref(resource_name_1, resource_name_2)
+    ref_2 = create_security_group_with_sg_ref(resource_name_2, resource_name_3)
+    ref_3 = create_security_group_with_sg_ref(resource_name_3, resource_name_1)
+    
+    time.sleep(CREATE_CYCLIC_REF_AFTER_SECONDS)
+    
+    cr_1 = k8s.wait_resource_consumed_by_controller(ref_1)
+    cr_2 = k8s.wait_resource_consumed_by_controller(ref_2)
+    cr_3 = k8s.wait_resource_consumed_by_controller(ref_3)
+    assert cr_1 is not None
+    assert cr_2 is not None
+    assert cr_3 is not None
+    
+    yield [(ref_1, cr_1), (ref_2, cr_2), (ref_3, cr_3)]
+    
+    try:
+        k8s.delete_custom_resource(ref, 3, 10)
+        k8s.delete_custom_resource(ref, 3, 10)
+        k8s.delete_custom_resource(ref, 3, 10)
+        
+        time.sleep(DELETE_CYCLIC_REF_AFTER_SECONDS)
+        
+        assert not k8s.get_resource_exists(ref_1)
+        assert not k8s.get_resource_exists(ref_2)
+        assert not k8s.get_resource_exists(ref_3)
+    except:
+        pass
+    
 @service_marker
 @pytest.mark.canary
 class TestSecurityGroup:
@@ -424,4 +483,56 @@ def test_crud_tags(self, ec2_client, simple_security_group):
         time.sleep(DELETE_WAIT_AFTER_SECONDS)
 
         # Check SecurityGroup no longer exists in AWS
-        ec2_validator.assert_security_group(resource_id, exists=False)
+        ec2_validator.assert_security_group(resource_id, exists=False)
+
+    def test_cyclic_ref(self, ec2_client, security_groups_cyclic_ref):
+        sgs = security_groups_cyclic_ref
+        (ref_1, cr_1) = sgs[0]
+        (ref_2, cr_2) = sgs[1]
+        (ref_3, cr_3) = sgs[2]
+        
+        
+        
+        # Check Security Groups exists in AWS
+        resource_id_1 = cr_1["status"]["id"]
+        resource_id_2 = cr_2["status"]["id"]
+        resource_id_3 = cr_3["status"]["id"]
+        
+        ec2_validator = EC2Validator(ec2_client)
+        ec2_validator.assert_security_group(resource_id_1)
+        ec2_validator.assert_security_group(resource_id_2)
+        ec2_validator.assert_security_group(resource_id_3)
+        
+        # Check resources are synced successfully
+        assert k8s.wait_on_condition(ref_1, "ACK.ResourceSynced", "True", wait_periods=5)
+        assert k8s.wait_on_condition(ref_2, "ACK.ResourceSynced", "True", wait_periods=5)
+        assert k8s.wait_on_condition(ref_3, "ACK.ResourceSynced", "True", wait_periods=5)
+        
+        # Check ingress rules exist
+        sg_group_1 = ec2_validator.get_security_group(resource_id_1)
+        sg_group_2 = ec2_validator.get_security_group(resource_id_2)
+        sg_group_3 = ec2_validator.get_security_group(resource_id_3)
+        assert len(sg_group_1["IpPermissions"]) == 1
+        assert len(sg_group_2["IpPermissions"]) == 1
+        assert len(sg_group_3["IpPermissions"]) == 1
+        
+        # Check ingress rules cyclic data
+        assert sg_group_1["IpPermissions"][0]["UserIdGroupPairs"][0]["GroupId"] == resource_id_2
+        assert sg_group_2["IpPermissions"][0]["UserIdGroupPairs"][0]["GroupId"] == resource_id_3
+        assert sg_group_3["IpPermissions"][0]["UserIdGroupPairs"][0]["GroupId"] == resource_id_1
+        
+        # Delete k8s resources
+        k8s.delete_custom_resource(ref_1)
+        k8s.delete_custom_resource(ref_2)
+        k8s.delete_custom_resource(ref_3)
+        
+        time.sleep(DELETE_CYCLIC_REF_AFTER_SECONDS)
+        
+        assert not k8s.get_resource_exists(ref_1)
+        assert not k8s.get_resource_exists(ref_2)
+        assert not k8s.get_resource_exists(ref_3)
+        
+        # Check Security Group no longer exists in AWS
+        ec2_validator.assert_security_group(resource_id_1, exists=False)
+        ec2_validator.assert_security_group(resource_id_2, exists=False)
+        ec2_validator.assert_security_group(resource_id_3, exists=False)

Original file line number	Diff line number	Diff line change
`@@ -8,6 +8,11 @@`
`8`	`8`	`return &resource{ko}, err`
`9`	`9`	`}`
`10`	`10`
	`11`	`+ if !rm.referencesResolved(&resource{ko}) {`
	`12`	`+ ackcondition.SetSynced(&resource{ko}, corev1.ConditionFalse, nil, nil)`
	`13`	`+ return &resource{ko}, nil`
	`14`	`+ }`
	`15`	`+`
`11`	`16`	`if err = rm.syncSGRules(ctx, &resource{ko}, nil); err != nil {`
`12`	`17`	`return &resource{ko}, err`
`13`	`18`	`}`