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

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 is what matches the whole of TCP or UDP. A tcp/udp rule with no ports would be sent the range 0:0, and the VR's iptables path applies that as --dport 0.
  • 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 VR renders 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.