[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 / behaviour change (for the release notes)

Which rules change. Rules already stored in the DB are re-encoded, with no migration, so some existing rules will start to be enforced as written:

• Narrower — protocol number 6 or 17 with a port range. Until now these matched all TCP or UDP, whatever ports they gave. "Protocol 6, port 22, allow" has been allowing every TCP port, and "protocol 17, port 53, deny" has been denying all UDP. After the change they match only the given ports.
• Narrower — protocol 1 (set by updateNetworkACLItem) with an ICMP type stored. This matched all ICMP; it now matches only the given type/code.
• Wider/different — protocol written as upper-case ICMP. These were applied as --icmp-type 0/0 (echo-reply only), whatever type was given. They now match the type/code given; type "any" now 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.

Which rules don't change. tcp/udp/icmp given by name (lower or upper case for TCP/UDP, lower case for icmp), protocol 6/17 without ports (still all of TCP/UDP), all, and every other protocol number.

When it takes effect. The encoding is done on the management server, so it applies from the first time the ACL is sent to the router after the management server is upgraded, whatever the VR template version. Until then the router keeps its old rules. The whole ACL list is re-sent when any rule in it is added, updated, deleted or moved, when the list is replaced on a tier or private gateway, and when the router is started, rebooted or redeployed (including a network/VPC restart, with or without cleanup, and the router recreation done for a system VM template upgrade).

What operators should do. The new behaviour is what those rules always said. But traffic that only got through (or was only blocked) because of the old over-broad match will be treated differently. Before upgrading, review VPC ACL rules (tier and private gateway lists) with protocol 6 or 17 and a port range, and rules with protocol 1 or ICMP. If the rule was relied on to cover the whole protocol, remove its ports (or widen the range to 1-65535). Rules of that shape can be found with:

SELECT id, acl_id, number, state, protocol, start_port, end_port, icmp_type, icmp_code, traffic_type, action
  FROM cloud.network_acl_item
 WHERE (TRIM(protocol) REGEXP '^0*(6|17)$' AND start_port IS NOT NULL)
    OR TRIM(protocol) REGEXP '^0*1$'
    OR (BINARY protocol <> 'icmp' AND LOWER(TRIM(protocol)) = 'icmp');  -- BINARY: the default collation ignores case

API change. createNetworkACL and updateNetworkACLItem now reject a port range given with a protocol number other than 6 or 17 (the router has always ignored those ports). Existing rules of that kind keep working as before.

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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 4.08%. Comparing base (8074c3e) to head (9fc97a4).
⚠️ Report is 14 commits behind head on 4.22.

❗ There is a different number of reports uploaded between BASE (8074c3e) and HEAD (9fc97a4). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (8074c3e)	HEAD (9fc97a4)
unittests	1	0

Additional details and impacted files

@@              Coverage Diff              @@
##               4.22   #14357       +/-   ##
=============================================
- Coverage     18.13%    4.08%   -14.05%     
=============================================
  Files          5939      449     -5490     
  Lines        537306    38283   -499023     
  Branches      65867     7084    -58783     
=============================================
- Hits          97439     1564    -95875     
+ Misses       428749    36504   -392245     
+ Partials      11118      215    -10903     

Flag	Coverage Δ
uitests	4.08% <ø> (+0.04%)	⬆️
unittests	?

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.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19606

[Damans227]

tested on kvm (ol8 host, advanced zone) with packages built from this PR. made a vpc with one tier, a custom acl list and one vm, then checked the router rules after each step. all cases pass.

• protocol 6, port 22: -p tcp -m tcp --dport 22 -j ACCEPT
• protocol 17, port 53: -p udp -m udp --dport 53 -j ACCEPT
• protocol 6, no ports: -p tcp -j ACCEPT
• protocol ICMP, type 8 code 0: -p icmp -m icmp --icmp-type 8/0 -j ACCEPT
• same rule updated to protocol 1, type 8: -p icmp -m icmp --icmp-type 8 -j ACCEPT
• egress protocol 6, port 443: -p tcp -m tcp --dport 443 -j ACCEPT
• protocol 132 with ports is refused: Start and end port can only be given for TCP or UDP (protocol number 6 or 17), not for protocol number [132]
• rebooted the router, all rules above came back the same

router rules after the reboot:

-A ACL_INBOUND_eth2 -p tcp -m tcp --dport 22 -j ACCEPT
-A ACL_INBOUND_eth2 -p udp -m udp --dport 53 -j ACCEPT
-A ACL_INBOUND_eth2 -p tcp -j ACCEPT
-A ACL_INBOUND_eth2 -p icmp -m icmp --icmp-type 8 -j ACCEPT
-A ACL_INBOUND_eth2 -j DROP
-A ACL_OUTBOUND_eth2 -p tcp -m tcp --dport 443 -j ACCEPT

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-17179)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 56775 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14357-t17179-kvm-ol8.zip
Smoke tests completed. 147 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestClusterDRS>:setup	Error	0.00	test_cluster_drs.py
test_1_userdata_on_ssvm	Error	6.41	test_systemvm_userdata.py
test_2_userdata_on_cpvm	Failure	129.45	test_systemvm_userdata.py

[bhouse-nexthop]

@weizhouapache @DaanHoogland I've pushed the changes you asked for (replies on each thread). The Trillian failures from 10-22 (test_cluster_drs setup and the test_systemvm_userdata tests) also show up on the 4.22 and main health-check PRs, so I don't think they're from this change. Could you take another look so it can make 4.22.2? I understand it's due out late this week or early next.

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19688

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[weizhouapache]

@bhouse-nexthop thanks for the update.

should we consider updateNetworkACLItemCmd.getSourcePortStart(), and then networkACLItemVo.getSourcePortStart() ?

[bhouse-nexthop]

@weizhouapache good question. It already works that way when the protocol is given. This check runs after transferDataToNetworkAclRulePojo(), so on a partial update networkACLItemVo.getSourcePortStart() already holds the command's port if one was passed, and the stored port if not. That is "the command's value, then the rule's value".

When the protocol is not given, I check only the ports in the command on purpose. Using the rule's stored ports there would re-check ports the caller didn't touch. A rule stored before this change with ports on, for example, protocol 47 would then be rejected for any edit at all, even changing only reason or number, until someone removes the ports. So in that case only ports passed in this call are checked against the stored protocol. A protocol change is always checked against the ports the rule ends up with (validatePortsAreUsableWithProtocolOnUpdateTestStoredPortsKeptWhenProtocolNotGiven / ...TestProtocolChangedOntoStoredPorts cover both).

If you would rather we reject any edit of such a rule until its ports are fixed, I can switch it to always use the rule's values. It's a one-line change.

[bhouse-nexthop]

@weizhouapache does this answer your question, or would you rather I switch to always using the rule's stored ports? Either way works for me. This is the only thing left open here: @Damans227 has approved, and the latest Trillian run (tid-17255) passed 147/149. The two errors, in test_primary_storage and test_primary_storage_scope, also showed up on #14360, which doesn't touch storage. I'd like to get it into 4.22.2 if possible.

[weizhouapache]

@bhouse-nexthop
I think it would be better to cleanup the existing start/end port and icmp type/code, and check with the values passed in API parameters when protocol is changed.

[weizhouapache]

@bhouse-nexthop I think it would be better to cleanup the existing start/end port and icmp type/code, and check with the values passed in API parameters when protocol is changed.

it will change the current behaviour btw

I am leaving to you to consider the following scenarios

• current rule has port range, but new protocol does not support it

• current rule has port range, new protocol without port range is passed

• current rule has port range, new protocol with new port range is passed

• current rule has icmp type/code, but new protocol does not support it

• current rule has icmp type/code, new protocol without icmp type/code is passed

• current rule has icmp type/code, new protocol with different icmp type/code is passed

[bhouse-nexthop]

@weizhouapache thanks for writing out the cases. Here's how each one behaves on this branch with the default partialupgrade=true. Take a stored rule with tcp and ports 80-80, or icmp with type 8 code 0:

Stored	Update	Result today
ports	protocol that can't carry ports (icmp, all, 47), no ports passed	rejected: the stored ports are checked against the new protocol
ports	new protocol that can carry ports (tcp to udp), no ports passed	ports kept
ports	new protocol with new ports	new ports applied, checked against the new protocol
icmp type/code	protocol other than icmp, no type/code passed	rejected ("Can specify icmpCode and icmpType for ICMP protocol only")
icmp type/code	protocol other than icmp, type/code passed	rejected, same check
icmp type/code	icmp again (or 1) with a different type/code	new type/code applied

With partialupgrade=false, every field is replaced from the call, so stored ports and ICMP type/code are dropped unless they're passed again. That already gives the "clean up, then check only what the API was given" behaviour when it's wanted.

I'd rather not change the partial-update semantics in this PR. Silently dropping stored ports on a protocol change would turn a call that's rejected today into one that succeeds with a wider rule than the caller may expect: tcp 22 changed to all would end up allowing everything. It would also change the default update path right before the 4.22.2 freeze. Callers who want a clean replace can use partialupgrade=false. If you think the partial path should clear them too, I can open a separate issue for it so it gets its own review.

[weizhouapache]

@bhouse-nexthop
thanks for the info, I understand now.

my original comment is still valid,

networkACLItemVo.getSourcePortStart() : updateNetworkACLItemCmd.getSourcePortStart()

should updateNetworkACLItemCmd.getSourcePortStart() be considered at first ? if it is null, then consider networkACLItemVo.getSourcePortStart()

[bhouse-nexthop]

@weizhouapache agreed, that reads better. Done in 9fc97a4: the ports passed in the command are checked first, and the rule's stored ports are used only when none was passed and the protocol is given.

It doesn't change behaviour. transferDataToNetworkAclRulePojo() runs before the check and already copies any port passed onto the rule, so both orders gave the same values. The new order doesn't rely on that call order, though. There's no ICMP type/code fallback of this kind in the PR, so only the ports changed. I also added a test for ports passed together with the protocol.

[Damans227]

thanks @bhouse-nexthop, the NetUtils move looks right. LGTM, tested on kvm earlier: protocol 6 and 17 with and without ports, and protocol 1 with an icmp type, all show up right on the router. the earlier commits are unchanged since then.

[blueorangutan]

[SF] Trillian test result (tid-17225)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 96001 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14357-t17225-kvm-ol8.zip
Smoke tests completed. 133 look OK, 16 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_enableHumanReadableLogs	Error	0.21	test_human_readable_logs.py
test_CRUD_operations_userdata	Error	1517.25	test_register_userdata.py
test_deploy_vm_with_registered_userdata	Error	3.54	test_register_userdata.py
test_deploy_vm_with_registered_userdata_with_override_policy_allow	Error	3.56	test_register_userdata.py
test_deploy_vm_with_registered_userdata_with_override_policy_append	Error	3.43	test_register_userdata.py
test_deploy_vm_with_registered_userdata_with_override_policy_deny	Error	3.42	test_register_userdata.py
test_deploy_vm_with_registered_userdata_with_params	Error	3.38	test_register_userdata.py
test_link_and_unlink_userdata_to_template	Error	3.53	test_register_userdata.py
test_user_userdata_crud	Error	3.74	test_register_userdata.py
ContextSuite context=TestResetVmOnReboot>:setup	Error	0.00	test_reset_vm_on_reboot.py
ContextSuite context=TestRAMCPUResourceAccounting>:setup	Error	0.00	test_resource_accounting.py
test_03_register_template	Error	1.12	test_resource_names.py
ContextSuite context=TestRestoreVM>:setup	Error	0.00	test_restore_vm.py
ContextSuite context=TestRouterDHCPHosts>:setup	Error	0.00	test_router_dhcphosts.py
ContextSuite context=TestRouterDHCPOpts>:setup	Error	0.00	test_router_dhcphosts.py
ContextSuite context=TestRouterDns>:setup	Error	0.00	test_router_dns.py
ContextSuite context=TestRouterDnsService>:setup	Error	0.00	test_router_dnsservice.py
ContextSuite context=TestRouterIpTablesPolicies>:setup	Error	0.00	test_routers_iptables_default_policy.py
ContextSuite context=TestVPCIpTablesPolicies>:setup	Error	0.00	test_routers_iptables_default_policy.py
ContextSuite context=TestIsolatedNetworks>:setup	Error	0.00	test_routers_network_ops.py
ContextSuite context=TestRedundantIsolateNetworks>:setup	Error	0.00	test_routers_network_ops.py
ContextSuite context=TestRouterServices>:setup	Error	0.00	test_routers.py
ContextSuite context=TestCpuCapServiceOfferings>:setup	Error	0.00	test_service_offerings.py
ContextSuite context=TestServiceOfferings>:setup	Error	0.39	test_service_offerings.py
ContextSuite context=TestSetSourceNatIp>:setup	Error	0.00	test_set_sourcenat.py
ContextSuite context=TestSnapshotRootDisk>:setup	Error	0.00	test_snapshots.py
ContextSuite context=TestSnapshotStandaloneBackup>:setup	Error	0.00	test_snapshots.py
ContextSuite context=TestSslOffloading>:setup	Error	0.00	test_ssl_offloading.py

[blueorangutan]

[SF] Trillian test result (tid-17255)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 54283 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14357-t17255-kvm-ol8.zip
Smoke tests completed. 147 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_primary_storage_nfs	Error	0.20	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	Error	0.36	test_primary_storage.py
test_01_primary_storage_scope_change	Error	0.13	test_primary_storage_scope.py

[bhouse-nexthop]

Test report: protocol-number and upper-case ICMP rules, and the six partial-update cases

This adds to @Damans227's test (protocol 6 and 17, ICMP 8/0, egress, a reboot). It covers the residual cases, plus @weizhouapache's six partial-update scenarios run against a live management server and router.

Environment

• A CloudStack 24.0 (main) build with this PR applied, alongside some of my other open PRs.
• Why the result carries over to 4.22:
  • The PR's changes are on the management server (NetworkACLServiceImpl, the create and update commands, SetNetworkACLCommand, NetUtils), and they apply cleanly to main.
  • The router side is system VM template 4.22.0, as on 4.22.
  • The VPC offering has no Firewall service, as on 4.22.
• Three KVM hosts on Ubuntu 24.04, nested virtualisation; Ceph RBD primary storage.
• System VM template 4.22.0 (Debian 12, nft 1.0.6).
• A dual-stack VPC: tier T1 holds the server V1, with the test list; tier T2 holds the client V2. There is also an external IPv6 host.

1. Rendering on the router

Rule (API)	Stored	Router
protocol=ICMP icmptype=3 icmpcode=1, 0.0.0.0/0	ICMP 3/1	-p icmp -m icmp --icmp-type 3/1 -j ACCEPT
protocol=ICMP icmptype=3 icmpcode=1 from the external /64, then update protocol=1 icmptype=128 icmpcode=0	icmp 128/0	ip6 saddr fd00:30::/64 icmpv6 type echo-request icmpv6 code no-route accept. ping6 from the external host to V1: 2/2
protocol=tcp (no ports), then update protocol=icmp with no type or code	icmp, type and code null	-p icmp -m icmp --icmp-type any -j ACCEPT. ping from V2: 2/2
protocol=006 startport=22 endport=22	006 22-22	-p tcp -m tcp --dport 22 -j ACCEPT. tcp 22 from V2 connects, tcp 8080 does not

No ACL apply failed: the agent log on the router's host has no ruleParts or Problem occurred line after these changes.

2. The six partial-update cases, default partialupgrade=true

Each case starts from a fresh rule from T2's CIDR with action allow: either tcp 80-80 or icmp 8/0.

#	Stored	Update	Result	Rule afterwards
1	tcp 80-80	protocol=icmp	rejected: "Can't specify start/end port when protocol is ICMP"	tcp 80-80 (unchanged)
2	tcp 80-80	protocol=udp	accepted	udp 80-80 (ports kept)
3	tcp 80-80	protocol=udp startport=53 endport=53	accepted	udp 53-53
4	icmp 8/0	protocol=tcp	rejected: "Can specify icmpCode and icmpType for ICMP protocol only"	icmp 8/0 (unchanged)
5	icmp 8/0	protocol=tcp icmptype=8	rejected, same message	icmp 8/0 (unchanged)
6	icmp 8/0	protocol=1 icmptype=0 icmpcode=0	accepted	icmp 0/0

This matches the table in the review thread: rejected / ports kept / new ports / rejected / rejected / new type.

3. The same cases with partialupgrade=false

When cidrlist, action and traffictype are passed as well, every field is replaced from the call:

#	Update	Rule afterwards
1	protocol=icmp	icmp, type/code -1 (all ICMP), no ports
2	protocol=udp	udp, no ports
3	protocol=udp startport=53 endport=53	udp 53-53
4	protocol=tcp	tcp, no ports, no ICMP type
5	protocol=tcp icmptype=8	accepted; tcp, the icmptype=8 silently dropped
6	protocol=1 icmptype=0 icmpcode=0	icmp 0/0

Not part of this PR, but worth knowing. A partialupgrade=false call that leaves out cidrlist, action or traffictype breaks the rule. The update fails with:

Cannot invoke "java.util.List.size()" because "cidrs" is null      (NetworkACLItemCidrsDaoImpl.updateCidrs)

• By then, NetworkACLItemDaoImpl.update() has already written the row with those fields set to null.
• listNetworkACLs for that list then fails with Cannot invoke "...TrafficType.toString()" because ... getTrafficType() is null, until the rule is updated again with every field.

That path (transferDataToNetworkAclRulePojo() and the DAO update) is the same on main without this PR. This PR only adds its port check ahead of it, so it is an existing bug, and it should get an issue of its own. It does mean that partialupgrade=false is a "clean replace" only when the caller passes every field. That corrects what I said in my reply in the review thread, where I described it as a clean replace without that condition.

Result: pass, against the residual cases and the six-case table.

Not covered

• The UI: the start and end port fields in the ACL rule form.
• A router with an older template.
• Egress ICMP rules.