Copilot commented on code in PR #14100:
URL: https://github.com/apache/cloudstack/pull/14100#discussion_r3970071549
##########
scripts/vm/network/security_group.py:
##########
@@ -1136,23 +1136,25 @@ def add_network_rules(vm_name, vm_id, vm_ip, vm_ip6,
signature, seqno, vmMac, ru
ip4s, ip6s = split_ips_by_family(vm_ip, vm_ip6, sec_ips,
str(ipv6_link_local_addr(vmMac)))
rules = parse_network_rules(rules)
- conntrack4_not_needed = False
- conntrack6_not_needed = False
+ ingress4_allow_all = False
+ egress4_allow_all = False
+ ingress6_allow_all = False
+ egress6_allow_all = False
Review Comment:
The previous block comment explaining why conntrack could be skipped (and
the example rule structure) was removed. Since this logic is subtle and
security/traffic-impacting, please add a short comment here explaining the
bidirectional effect of NOTRACK rules (source+destination match) and why both
ingress+egress allow-all are required.
##########
scripts/vm/network/security_group.py:
##########
@@ -1136,23 +1136,25 @@ def add_network_rules(vm_name, vm_id, vm_ip, vm_ip6,
signature, seqno, vmMac, ru
ip4s, ip6s = split_ips_by_family(vm_ip, vm_ip6, sec_ips,
str(ipv6_link_local_addr(vmMac)))
rules = parse_network_rules(rules)
- conntrack4_not_needed = False
- conntrack6_not_needed = False
+ ingress4_allow_all = False
+ egress4_allow_all = False
+ ingress6_allow_all = False
+ egress6_allow_all = False
for rule in rules:
- """
- If any of the rules has an explicit allow all protocols from
0.0.0.0/0 (ipv4)
- or ::/0 (ipv6), then that IP family doesn't need its connection
tracked
- Example contents of the rules list:
- [
- {'ipv4': ['1.0.0.0/24', '0.0.0.0/0'], 'ipv6': ['::/0'],
'ruletype': 'I', 'start': 0, 'end': 0, 'protocol': 'all'},
- {'ipv4': ['1.1.1.1/32'], 'ipv6': [], 'ruletype': 'I',
'start': 1, 'end': 65535, 'protocol': 'tcp'},
- {'ipv4': [], 'ipv6': ['2001:db8::/32'], 'ruletype': 'I',
'start': 2000, 'end': 3000, 'protocol': 'tcp'}
- ]
- """
- if '0.0.0.0/0' in rule['ipv4'] and rule['protocol'].lower() ==
'all':
- conntrack4_not_needed = True
- if '::/0' in rule['ipv6'] and rule['protocol'].lower() == 'all':
- conntrack6_not_needed = True
+ if rule['protocol'].lower() == 'all':
+ if '0.0.0.0/0' in rule['ipv4']:
+ if rule['ruletype'] == 'E':
+ egress4_allow_all = True
+ else:
+ ingress4_allow_all = True
+ if '::/0' in rule['ipv6']:
+ if rule['ruletype'] == 'E':
+ egress6_allow_all = True
+ else:
Review Comment:
The `else` branch treats any non-'E' `ruletype` as ingress. If `ruletype`
ever contains unexpected values (or differs in case, e.g. 'e'/'i'), this can
incorrectly mark ingress allow-all and potentially disable conntrack. Consider
normalizing `ruletype` (e.g., `upper()`) and handling only known values
explicitly (`'I'`/`'E'`), ignoring others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
