Copilot commented on code in PR #14100:
URL: https://github.com/apache/cloudstack/pull/14100#discussion_r4284271875
##########
scripts/vm/network/security_group.py:
##########
@@ -1136,23 +1136,33 @@ 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
+ egress4_has_rules = False
+ egress6_has_rules = 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['ruletype'] == 'E':
+ egress4_has_rules = egress4_has_rules or bool(rule['ipv4'])
+ egress6_has_rules = egress6_has_rules or bool(rule['ipv6'])
+ 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:
+ ingress6_allow_all = True
+
+ # The NOTRACK rules apply to the IP in both directions, so conntrack
can only be skipped when
+ # ingress allows all and egress is open as well: either an explicit
allow-all egress rule or no
+ # egress rules at all (the egress chain then accepts everything).
+ conntrack4_not_needed = ingress4_allow_all and (egress4_allow_all or
not egress4_has_rules)
+ conntrack6_not_needed = ingress6_allow_all and (egress6_allow_all or
not egress6_has_rules)
Review Comment:
The PR description says conntrack is only skipped when *both* an ingress
allow-all and an egress allow-all rule exist for that family, but the
implementation also skips conntrack when there are *no egress rules at all*
(`not egress*_has_rules`). Please align the PR description with this behavior,
or (if the description is the intended behavior) adjust the condition to
require an explicit egress allow-all rule.
##########
scripts/vm/network/security_group.py:
##########
@@ -1136,23 +1136,33 @@ 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
+ egress4_has_rules = False
+ egress6_has_rules = 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['ruletype'] == 'E':
+ egress4_has_rules = egress4_has_rules or bool(rule['ipv4'])
+ egress6_has_rules = egress6_has_rules or bool(rule['ipv6'])
Review Comment:
This boolean accumulation pattern is harder to scan than necessary. Consider
using idiomatic boolean in-place updates (e.g., `egress4_has_rules |=
bool(rule['ipv4'])`) to make the intent clearer and reduce repetition.
--
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]
