diff --git a/CHANGELOG.md b/CHANGELOG.md index 507a1af12f..20667d4c6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ CHANGELOG **CHANGES** - Ubuntu 20.04 is no longer supported. +**BUG FIXES** +- Fix an issue where Security Group validation failed when a rule contained both IPv4 ranges (IpRanges) and security group references (UserIdGroupPairs). 3.13.0 ------ diff --git a/cli/src/pcluster/validators/cluster_validators.py b/cli/src/pcluster/validators/cluster_validators.py index e65921c9e5..82bbdb17a8 100644 --- a/cli/src/pcluster/validators/cluster_validators.py +++ b/cli/src/pcluster/validators/cluster_validators.py @@ -534,15 +534,18 @@ def _populate_allowed_src_or_dst(rule, ip_ranges, allowed_security_groups): """ if rule.get("PrefixListIds"): return True # Always assume prefix list is properly set for code simplicity - elif rule.get("IpRanges"): + if rule.get("IpRanges"): ip_ranges.extend(rule.get("IpRanges")) - return False # Ip Ranges have to be checked later. Return False because the rule allowance is not determined. - elif rule.get("UserIdGroupPairs"): + # Ip Ranges have to be checked later. Return False because the rule allowance is not determined. + if rule.get("Ipv4Ranges"): + # Currently the describe_security_groups API response syntax contains "IpRanges". + # This check is added for future compatibility if API changes to use "Ipv4Ranges" + ip_ranges.extend(rule.get("Ipv4Ranges")) + if rule.get("UserIdGroupPairs"): allowed_security_groups.update( {user_id_group_pair.get("GroupId") for user_id_group_pair in rule.get("UserIdGroupPairs")} ) # Security groups have to be checked later. Return False because the rule allowance is not determined. - return False return False diff --git a/cli/tests/pcluster/validators/test_cluster_validators.py b/cli/tests/pcluster/validators/test_cluster_validators.py index d43479a925..f19c78b078 100644 --- a/cli/tests/pcluster/validators/test_cluster_validators.py +++ b/cli/tests/pcluster/validators/test_cluster_validators.py @@ -1306,6 +1306,69 @@ def test_queue_name_validator(name, expected_message): r"allows inbound TCP traffic through ports \[111, 2049, 20001, 20002, 20003\]. Missing ports: " r"\[2049, 20001, 20002, 20003\]", ), + ( # working case: The rule has both IpRanges and UserIdGroupPairs. Wrong ip permissions. But SG matches. + "LUSTRE", + "vpc-06e4ab6c6cEXAMPLE", + [ + { + "IpProtocol": "-1", + "IpRanges": [{"CidrIp": "10.1.1.0/25"}], + "UserIdGroupPairs": [{"UserId": "123456789012", "GroupId": "sg-12345678"}], + } + ], + [ + { + "IpProtocol": "-1", + "IpRanges": [{"CidrIp": "10.1.1.0/25"}], + "UserIdGroupPairs": [{"UserId": "123456789012", "GroupId": "sg-12345678"}], + } + ], + {frozenset({"sg-12345678"}), frozenset({"sg-12345678", "sg-23456789"})}, + ["eni-09b9460295ddd4e5f"], + None, + ), + ( # not-working case:The rule has both IpRanges and UserIdGroupPairs. Both don't match. + "LUSTRE", + "vpc-06e4ab6c6cEXAMPLE", + [ + { + "IpProtocol": "-1", + "IpRanges": [{"CidrIp": "10.1.1.0/25"}], + "UserIdGroupPairs": [{"UserId": "123456789012", "GroupId": "sg-99999999"}], + } + ], + [ + { + "IpProtocol": "-1", + "IpRanges": [{"CidrIp": "10.1.1.0/25"}], + "UserIdGroupPairs": [{"UserId": "123456789012", "GroupId": "sg-99999999"}], + } + ], + {frozenset({"sg-12345678"}), frozenset({"sg-12345678", "sg-23456789"})}, + ["eni-09b9460295ddd4e5f"], + r"allows inbound and outbound TCP traffic through ports \[988\]", + ), + ( # working case:IpRanges covers the subnet, but UserIdGroupPairs do not match + "LUSTRE", + "vpc-06e4ab6c6cEXAMPLE", + [ + { + "IpProtocol": "-1", + "IpRanges": [{"CidrIp": "10.0.0.0/16"}], + "UserIdGroupPairs": [{"UserId": "123456789012", "GroupId": "sg-99999999"}], + } + ], + [ + { + "IpProtocol": "-1", + "IpRanges": [{"CidrIp": "10.0.0.0/16"}], + "UserIdGroupPairs": [{"UserId": "123456789012", "GroupId": "sg-99999999"}], + } + ], + {frozenset({"sg-12345678"})}, + ["eni-09b9460295ddd4e5f"], + None, + ), ], ) def test_fsx_network_validator(