VPC ACL: keep the ports and ICMP type of rules given by protocol number or as upper-case ICMP

[bhouse-nexthop]

Description

A VPC network ACL rule given by protocol number 1, 6 or 17, or with the protocol written as upper-case ICMP, can lose its port range or ICMP type on the virtual router. It is then applied to the whole protocol, and nothing is reported. A rule with ports on any other protocol number is accepted, and its ports are ignored.

Fixes #14356

Why it breaks

The VR applies ports only to a rule it receives as tcp or udp, and an ICMP type/code only to one it receives as icmp. SetNetworkACLCommand.generateFwRules() sent the protocol exactly as stored. SetNetworkAclConfigItem turns a protocol number into a ProtocolAclRule: a bare number with no port or ICMP fields. So:

1. Protocol 6 or 17 with ports is applied to the whole protocol. The UI offers ports for "Protocol number", and the API accepts them. "Protocol 6, port 22, allow" becomes -p tcp -j ACCEPT, i.e. all TCP.
2. Protocol 1 set by updateNetworkACLItem. createNetworkACL maps protocol 1 to icmp, but updateNetworkACLItem did not. The ICMP type is then cleared, so the VR allows all ICMP.
3. An upper-case ICMP (stored as given) fails generateFwRules()'s case-sensitive "icmp" check. The rule is sent with the empty port range, 0:0, where the ICMP type and code belong, so the VR applies --icmp-type 0/0 (echo-reply only), whatever type was asked for. Upper-case TCP/UDP are not affected, because the VR side lower-cases those.
4. Ports with any other protocol number (e.g. 132) are accepted by the UI and API, and silently ignored by the VR.

How it is fixed

• SetNetworkACLCommand.generateFwRules():
  • Protocol 1, in any form, is sent to the VR as icmp.
  • Protocols 6 and 17 are sent as tcp and udp only when the rule has ports. Without ports they stay bare numbers, which match the whole of TCP or UDP on every VR path. A tcp/udp rule with no ports is sent the range 0:0, which the VR's iptables path has applied as --dport 0 (VPC ACL tcp/udp rule without ports matches only port 0 (--dport 0) on the VR #14363, fixed separately in VPC ACL: match all ports for a tcp/udp rule given without ports #14364).
  • The protocol is read as a number (006 is 6, the same as the API's own validation reads it), and the case of a name doesn't matter.
  • A missing ICMP type or code is sent as -1 (any) instead of null. The VR side parseInts these fields, so a null would fail the whole ACL apply. That can happen today, judging from the code: a rule switched to icmp by a partial update with no ICMP type stores none.
  • Normalising where the rule is encoded also fixes rules already stored this way, with no DB migration.
• UpdateNetworkACLItemCmd.getProtocol() maps protocol 1 to icmp, the same as CreateNetworkACLCmd.
• NetworkACLServiceImpl rejects ports given with a protocol number other than 6 or 17, since they can't be applied. Ports are checked on create, and on update whenever the request gives ports or a protocol. A protocol change therefore can't leave stored ports on a rule. An existing rule that already carries such ports can still have its other fields edited.
• UI (AclRulesTab.vue) only shows and sends Start/End port for TCP, UDP, and protocol number 6 or 17.

Upgrade note

Existing rules affected by 1–3 will be applied as configured from the next ACL apply:

• Narrower: "protocol 6, port 22" stops allowing every other TCP port.
• Wider: an ICMP rule with type "any" stops being echo-reply only and matches all ICMP.
• IPv6: a rule stored as protocol 1 (only possible through update) was rendered for IPv6 as protocol number 1, which never matches. It now renders as an ICMPv6 rule, the same as any icmp rule.

That is what those rules always asked for, but traffic that only got through (or was only blocked) because of the bug will be treated differently. Rules for protocol 6 or 17 without ports are unchanged.

How to reproduce the old behaviour

1. In a VPC ACL list, add an ingress rule: Protocol number 6, start/end port 22, CIDR 0.0.0.0/0, Allow. Attach the list to a tier.
2. On the VR, run iptables -S ACL_INBOUND_ethX:
   • Before: -A ACL_INBOUND_ethX -p tcp -j ACCEPT (every TCP port open)
   • After: -A ACL_INBOUND_ethX -p tcp -m tcp --dport 22 -j ACCEPT
3. createNetworkACL protocol=ICMP icmptype=8 icmpcode=0 ...:
   • Before: -p icmp -m icmp --icmp-type 0/0
   • After: --icmp-type 8/0

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

How Has This Been Tested?

• SetNetworkACLCommandTest checks the encoded rules:
  • 6/17 with ports keep them, and 6/17 without ports stay bare numbers;
  • 006, 01 and 047 are read as numbers;
  • 1 keeps its ICMP type and code, or gets -1;-1 when it has none;
  • upper-case ICMP keeps its type;
  • other numbers and all pass through unchanged.
• UpdateNetworkACLItemCmdTest: protocol 1 becomes icmp, and anything else is left as is.
• NetworkACLServiceImplTest:
  • ports are accepted with tcp/udp/6/17, and rejected with another protocol number (start port alone included);
  • on update, stored ports are left alone when no protocol is given, and rejected when the protocol is changed onto them.
• The api, core and server modules build with checkstyle, and the three test classes pass (2 + 7 + 110 tests).
• ESLint (the UI's own config) is clean for AclRulesTab.vue.
• I checked what the 4.22 VR scripts render for a tcp rule with ports 0:0 versus a bare protocol 6 by running the rules through configure.py: --dport 0 versus all TCP. I have not run this on a VR.

How did you try to break this feature and the system with this change?

• Revoked rules keep their existing encoding.
• Other protocols: all, other protocol numbers and lower-case names are encoded as before. A protocol number written with leading zeros is now sent without them.
• Protocol 6/17 with no ports keeps its old meaning, all of TCP/UDP.
• Existing rules with ports on another protocol number can still be updated (e.g. renumbered) without touching the protocol. Editing one in the UI, which sends a full update without ports, clears those ignored ports.

[bhouse-nexthop]

@vladimirpetrov @sureshanaparti could you take a look at this one? We'd like this fix to make it into the upcoming 4.22.2 release.

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 18.04%. Comparing base (2974af8) to head (631f707).
⚠️ Report is 1 commits behind head on 4.22.

Files with missing lines	Patch %	Lines
.../cloud/agent/api/routing/SetNetworkACLCommand.java	81.48%	3 Missing and 2 partials ⚠️
...a/com/cloud/network/vpc/NetworkACLServiceImpl.java	93.75%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.22   #14357      +/-   ##
============================================
+ Coverage     18.02%   18.04%   +0.01%     
- Complexity    16250    16275      +25     
============================================
  Files          5936     5936              
  Lines        535823   535867      +44     
  Branches      65612    65622      +10     
============================================
+ Hits          96582    96673      +91     
+ Misses       428242   428189      -53     
- Partials      10999    11005       +6     

Flag	Coverage Δ
uitests	4.04% <ø> (-0.01%)	⬇️
unittests	19.11% <87.50%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.