Set correct userIDGroupPairs defaults for SecurityGroups CRs. (aws-controllers-k8s#194)

Closes aws-controllers-k8s/community#2068,
aws-controllers-k8s/community#2061, and
aws-controllers-k8s/community#2058

The EC2 API for setting ingress/egress rules has many special restrictions,
making its behavior hard to predict. For example, `GroupName` should only be
used with default VPCs. When using non default VPCs users should use `GroupID`
instead

To address this problem, we are introducing a defaulting mechanism to help the
controller infer and use the correct `GroupID` when a user doesnt provide one.

You might wonder why all the trouble, and why not just use ACK resource references?
Well.. this is necessary because ACK resource references cannot do self
references, making fully declarative egress/ingress rule definition impossible in some
cases.

Changes:
- Mark `UserIDGroupPairs.GroupName` as non-required (at the CRD level)
- Default `UserIDGroupPairs.GroupID` to the parent security group ID
- Default `UserIDGroupPairs.VPCID` to the VPC of the parent security group
- Add more e2e tests for `UserIDGroupPairs`

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

Author: a-hilaly

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2024-05-31T06:34:31Z"
+  build_date: "2024-06-03T07:09:57Z"
   build_hash: 14cef51778d471698018b6c38b604181a6948248
   go_version: go1.22.3
   version: v0.34.0
 api_directory_checksum: 7fd395ceb7d5d8e35906991c7348d3498f384741
 api_version: v1alpha1
 aws_sdk_go_version: v1.44.93
 generator_config_info:
-  file_checksum: ae36cc7af80031c6de1461fa5fafad17631dbc99
+  file_checksum: b38071cec6cdb8156420ad489b68841fd9b30726
   original_file_name: generator.yaml
 last_modification:
   reason: API generation

apis/v1alpha1/generator.yaml

@@ -556,6 +556,7 @@ resources:
         references:
           resource: SecurityGroup
           path: Spec.Name
+        is_required: false
     renames:
       operations:
         CreateSecurityGroup:

generator.yaml

@@ -556,6 +556,7 @@ resources:
         references:
           resource: SecurityGroup
           path: Spec.Name
+        is_required: false
     renames:
       operations:
         CreateSecurityGroup:

pkg/resource/security_group/hooks.go

@@ -296,6 +296,18 @@ func (rm *resourceManager) createSecurityGroupRules(
 	// Authorize ingress rules
 	for _, i := range ingress {
 		ipInput := rm.newIPPermission(*i)
+		for _, userIDGroupPair := range ipInput.UserIdGroupPairs {
+			// If not provided, we need to default the VPC and SecurityGroup IDs.
+			//
+			// The newIPPermission function doesn't return nil UserIdGroupPair items. It is safe to
+			// access them here.
+			if userIDGroupPair.GroupId == nil && userIDGroupPair.GroupName == nil {
+				userIDGroupPair.GroupId = r.ko.Status.ID
+			}
+			if userIDGroupPair.VpcId == nil {
+				userIDGroupPair.VpcId = r.ko.Spec.VPCID
+			}
+		}
 		req := &svcsdk.AuthorizeSecurityGroupIngressInput{
 			GroupId:       r.ko.Status.ID,
 			IpPermissions: []*svcsdk.IpPermission{ipInput},
@@ -311,6 +323,18 @@ func (rm *resourceManager) createSecurityGroupRules(
 	// Authorize egress rules
 	for _, e := range egress {
 		ipInput := rm.newIPPermission(*e)
+		for _, userIDGroupPair := range ipInput.UserIdGroupPairs {
+			// If not provided, we need to default the security group ID and vpc ID.
+			//
+			// The newIPPermission function doesn't return nil UserIdGroupPair items. It is safe to
+			// access them here.
+			if userIDGroupPair.GroupId == nil && userIDGroupPair.GroupName == nil {
+				userIDGroupPair.GroupId = r.ko.Status.ID
+			}
+			if userIDGroupPair.VpcId == nil {
+				userIDGroupPair.VpcId = r.ko.Spec.VPCID
+			}
+		}
 		req := &svcsdk.AuthorizeSecurityGroupEgressInput{
 			GroupId:       r.ko.Status.ID,
 			IpPermissions: []*svcsdk.IpPermission{ipInput},

pkg/resource/security_group/references.go

@@ -14,18 +14,19 @@
 """Integration tests for the SecurityGroup API.
 """
 
+import logging
 import resource
-import pytest
 import time
-import logging
 
+import pytest
 from acktest import tags
-from acktest.resources import random_suffix_name
 from acktest.k8s import resource as k8s
-from e2e import service_marker, CRD_GROUP, CRD_VERSION, load_ec2_resource
-from e2e.replacement_values import REPLACEMENT_VALUES
+from acktest.resources import random_suffix_name
+from e2e import CRD_GROUP, CRD_VERSION, load_ec2_resource, service_marker
 from e2e.bootstrap_resources import get_bootstrap_resources
+from e2e.replacement_values import REPLACEMENT_VALUES
 from e2e.tests.helper import EC2Validator
+from acktest.aws.identity import get_account_id
 
 RESOURCE_PLURAL = "securitygroups"
 
@@ -241,17 +242,35 @@ def test_rules_create_update_delete(self, ec2_client, simple_security_group):
 
         # Add Egress rule via patch
         new_egress_rule = {
-                        "ipProtocol": "tcp",
-                        "fromPort": 25,
-                        "toPort": 25,
-                        "ipRanges": [
-                            {
-                                "cidrIP": "172.31.0.0/16",
-                                "description": "test egress"
-                            }
-                        ]
+            "ipProtocol": "tcp",
+            "fromPort": 25,
+            "toPort": 25,
+            "ipRanges": [
+                {
+                    "cidrIP": "172.31.0.0/16",
+                    "description": "test egress"
+                }
+            ]
         }
-        patch = {"spec": {"egressRules":[new_egress_rule]}}
+        # Add Egress rule via patch
+        new_egress_rule_with_sg_pair = {
+            "ipProtocol": "tcp",
+            "fromPort": 40,
+            "toPort": 40,
+            "ipRanges": [
+                {
+                    "cidrIP": "172.31.0.0/12",
+                    "description": "test egress"
+                }
+            ],
+            "userIDGroupPairs": [
+                {
+                    "description": "test userIDGroupPairs",
+                    "userID": str(get_account_id())
+                }
+            ]
+        }
+        patch = {"spec": {"egressRules":[new_egress_rule, new_egress_rule_with_sg_pair]}}
         _ = k8s.patch_custom_resource(ref, patch)
 
         time.sleep(CREATE_WAIT_AFTER_SECONDS)
@@ -262,28 +281,35 @@ def test_rules_create_update_delete(self, ec2_client, simple_security_group):
         # Check ingress and egress rules exist
         sg_group = ec2_validator.get_security_group(resource_id)
         assert len(sg_group["IpPermissions"]) == 1
-        assert len(sg_group["IpPermissionsEgress"]) == 1
+        assert len(sg_group["IpPermissionsEgress"]) == 2
 
         # Check egress rule data
         assert sg_group["IpPermissionsEgress"][0]["IpProtocol"] == "tcp"
         assert sg_group["IpPermissionsEgress"][0]["FromPort"] == 25
         assert sg_group["IpPermissionsEgress"][0]["ToPort"] == 25
         assert sg_group["IpPermissionsEgress"][0]["IpRanges"][0]["Description"] == "test egress"
 
+        assert sg_group["IpPermissionsEgress"][1]["IpProtocol"] == "tcp"
+        assert sg_group["IpPermissionsEgress"][1]["FromPort"] == 40
+        assert sg_group["IpPermissionsEgress"][1]["ToPort"] == 40
+        assert sg_group["IpPermissionsEgress"][1]["IpRanges"][0]["Description"] == "test egress"
+        assert len(sg_group["IpPermissionsEgress"][1]["UserIdGroupPairs"]) == 1
+        assert sg_group["IpPermissionsEgress"][1]["UserIdGroupPairs"][0]["Description"] == "test userIDGroupPairs"
+        assert sg_group["IpPermissionsEgress"][1]["UserIdGroupPairs"][0]["GroupId"] == resource_id
+
         # Remove Ingress rule
         patch = {"spec": {"ingressRules":[]}}
         _ = k8s.patch_custom_resource(ref, patch)
         time.sleep(CREATE_WAIT_AFTER_SECONDS)
 
         # assert patched state
         cr = k8s.get_resource(ref)
-        assert len(cr['status']['rules']) == 1
+        assert len(cr['status']['rules']) == 3
 
         # Check ingress rule removed; egress rule remains
-        assert len(cr["status"]["rules"]) == 1
         sg_group = ec2_validator.get_security_group(resource_id)
         assert len(sg_group["IpPermissions"]) == 0
-        assert len(sg_group["IpPermissionsEgress"]) == 1
+        assert len(sg_group["IpPermissionsEgress"]) == 2
 
         # Delete k8s resource
         _, deleted = k8s.delete_custom_resource(ref)