Skip to content

Commit 68c5e39

Browse files
a-hilalya-hilaly
authored
Set correct userIDGroupPairs defaults for SecurityGroups CRs. (#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.
1 parent de92e14 commit 68c5e39

File tree

6 files changed

+73
-24
lines changed

6 files changed

+73
-24
lines changed

‎apis/v1alpha1/ack-generate-metadata.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
ack_generate_info:
2-
build_date: "2024-05-31T06:34:31Z"
2+
build_date: "2024-06-03T07:09:57Z"
33
build_hash: 14cef51778d471698018b6c38b604181a6948248
44
go_version: go1.22.3
55
version: v0.34.0
66
api_directory_checksum: 7fd395ceb7d5d8e35906991c7348d3498f384741
77
api_version: v1alpha1
88
aws_sdk_go_version: v1.44.93
99
generator_config_info:
10-
file_checksum: ae36cc7af80031c6de1461fa5fafad17631dbc99
10+
file_checksum: b38071cec6cdb8156420ad489b68841fd9b30726
1111
original_file_name: generator.yaml
1212
last_modification:
1313
reason: API generation

‎apis/v1alpha1/generator.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,7 @@ resources:
556556
references:
557557
resource: SecurityGroup
558558
path: Spec.Name
559+
is_required: false
559560
renames:
560561
operations:
561562
CreateSecurityGroup:

‎generator.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,7 @@ resources:
556556
references:
557557
resource: SecurityGroup
558558
path: Spec.Name
559+
is_required: false
559560
renames:
560561
operations:
561562
CreateSecurityGroup:

‎pkg/resource/security_group/hooks.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,18 @@ func (rm *resourceManager) createSecurityGroupRules(
296296
// Authorize ingress rules
297297
for _, i := range ingress {
298298
ipInput := rm.newIPPermission(*i)
299+
for _, userIDGroupPair := range ipInput.UserIdGroupPairs {
300+
// If not provided, we need to default the VPC and SecurityGroup IDs.
301+
//
302+
// The newIPPermission function doesn't return nil UserIdGroupPair items. It is safe to
303+
// access them here.
304+
if userIDGroupPair.GroupId == nil && userIDGroupPair.GroupName == nil {
305+
userIDGroupPair.GroupId = r.ko.Status.ID
306+
}
307+
if userIDGroupPair.VpcId == nil {
308+
userIDGroupPair.VpcId = r.ko.Spec.VPCID
309+
}
310+
}
299311
req := &svcsdk.AuthorizeSecurityGroupIngressInput{
300312
GroupId: r.ko.Status.ID,
301313
IpPermissions: []*svcsdk.IpPermission{ipInput},
@@ -311,6 +323,18 @@ func (rm *resourceManager) createSecurityGroupRules(
311323
// Authorize egress rules
312324
for _, e := range egress {
313325
ipInput := rm.newIPPermission(*e)
326+
for _, userIDGroupPair := range ipInput.UserIdGroupPairs {
327+
// If not provided, we need to default the security group ID and vpc ID.
328+
//
329+
// The newIPPermission function doesn't return nil UserIdGroupPair items. It is safe to
330+
// access them here.
331+
if userIDGroupPair.GroupId == nil && userIDGroupPair.GroupName == nil {
332+
userIDGroupPair.GroupId = r.ko.Status.ID
333+
}
334+
if userIDGroupPair.VpcId == nil {
335+
userIDGroupPair.VpcId = r.ko.Spec.VPCID
336+
}
337+
}
314338
req := &svcsdk.AuthorizeSecurityGroupEgressInput{
315339
GroupId: r.ko.Status.ID,
316340
IpPermissions: []*svcsdk.IpPermission{ipInput},

‎pkg/resource/security_group/references.go‎

Lines changed: 0 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/e2e/tests/test_security_group.py‎

Lines changed: 45 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -14,18 +14,19 @@
1414
"""Integration tests for the SecurityGroup API.
1515
"""
1616

17+
import logging
1718
import resource
18-
import pytest
1919
import time
20-
import logging
2120

21+
import pytest
2222
from acktest import tags
23-
from acktest.resources import random_suffix_name
2423
from acktest.k8s import resource as k8s
25-
from e2e import service_marker, CRD_GROUP, CRD_VERSION, load_ec2_resource
26-
from e2e.replacement_values import REPLACEMENT_VALUES
24+
from acktest.resources import random_suffix_name
25+
from e2e import CRD_GROUP, CRD_VERSION, load_ec2_resource, service_marker
2726
from e2e.bootstrap_resources import get_bootstrap_resources
27+
from e2e.replacement_values import REPLACEMENT_VALUES
2828
from e2e.tests.helper import EC2Validator
29+
from acktest.aws.identity import get_account_id
2930

3031
RESOURCE_PLURAL = "securitygroups"
3132

@@ -241,17 +242,35 @@ def test_rules_create_update_delete(self, ec2_client, simple_security_group):
241242

242243
# Add Egress rule via patch
243244
new_egress_rule = {
244-
"ipProtocol": "tcp",
245-
"fromPort": 25,
246-
"toPort": 25,
247-
"ipRanges": [
248-
{
249-
"cidrIP": "172.31.0.0/16",
250-
"description": "test egress"
251-
}
252-
]
245+
"ipProtocol": "tcp",
246+
"fromPort": 25,
247+
"toPort": 25,
248+
"ipRanges": [
249+
{
250+
"cidrIP": "172.31.0.0/16",
251+
"description": "test egress"
252+
}
253+
]
253254
}
254-
patch = {"spec": {"egressRules":[new_egress_rule]}}
255+
# Add Egress rule via patch
256+
new_egress_rule_with_sg_pair = {
257+
"ipProtocol": "tcp",
258+
"fromPort": 40,
259+
"toPort": 40,
260+
"ipRanges": [
261+
{
262+
"cidrIP": "172.31.0.0/12",
263+
"description": "test egress"
264+
}
265+
],
266+
"userIDGroupPairs": [
267+
{
268+
"description": "test userIDGroupPairs",
269+
"userID": str(get_account_id())
270+
}
271+
]
272+
}
273+
patch = {"spec": {"egressRules":[new_egress_rule, new_egress_rule_with_sg_pair]}}
255274
_ = k8s.patch_custom_resource(ref, patch)
256275

257276
time.sleep(CREATE_WAIT_AFTER_SECONDS)
@@ -262,28 +281,35 @@ def test_rules_create_update_delete(self, ec2_client, simple_security_group):
262281
# Check ingress and egress rules exist
263282
sg_group = ec2_validator.get_security_group(resource_id)
264283
assert len(sg_group["IpPermissions"]) == 1
265-
assert len(sg_group["IpPermissionsEgress"]) == 1
284+
assert len(sg_group["IpPermissionsEgress"]) == 2
266285

267286
# Check egress rule data
268287
assert sg_group["IpPermissionsEgress"][0]["IpProtocol"] == "tcp"
269288
assert sg_group["IpPermissionsEgress"][0]["FromPort"] == 25
270289
assert sg_group["IpPermissionsEgress"][0]["ToPort"] == 25
271290
assert sg_group["IpPermissionsEgress"][0]["IpRanges"][0]["Description"] == "test egress"
272291

292+
assert sg_group["IpPermissionsEgress"][1]["IpProtocol"] == "tcp"
293+
assert sg_group["IpPermissionsEgress"][1]["FromPort"] == 40
294+
assert sg_group["IpPermissionsEgress"][1]["ToPort"] == 40
295+
assert sg_group["IpPermissionsEgress"][1]["IpRanges"][0]["Description"] == "test egress"
296+
assert len(sg_group["IpPermissionsEgress"][1]["UserIdGroupPairs"]) == 1
297+
assert sg_group["IpPermissionsEgress"][1]["UserIdGroupPairs"][0]["Description"] == "test userIDGroupPairs"
298+
assert sg_group["IpPermissionsEgress"][1]["UserIdGroupPairs"][0]["GroupId"] == resource_id
299+
273300
# Remove Ingress rule
274301
patch = {"spec": {"ingressRules":[]}}
275302
_ = k8s.patch_custom_resource(ref, patch)
276303
time.sleep(CREATE_WAIT_AFTER_SECONDS)
277304

278305
# assert patched state
279306
cr = k8s.get_resource(ref)
280-
assert len(cr['status']['rules']) == 1
307+
assert len(cr['status']['rules']) == 3
281308

282309
# Check ingress rule removed; egress rule remains
283-
assert len(cr["status"]["rules"]) == 1
284310
sg_group = ec2_validator.get_security_group(resource_id)
285311
assert len(sg_group["IpPermissions"]) == 0
286-
assert len(sg_group["IpPermissionsEgress"]) == 1
312+
assert len(sg_group["IpPermissionsEgress"]) == 2
287313

288314
# Delete k8s resource
289315
_, deleted = k8s.delete_custom_resource(ref)

0 commit comments

Comments
 (0)