[bhouse-nexthop]

Description

This PR fixes two problems with updateNetworkACLItem when partialupgrade=false, which replaces every field of the rule.

1. Missing fields broke the rule. Without cidrlist or traffictype, the rule was written with those fields null, and then the call failed with an NPE on the null CIDR list. The row had already been committed, so listNetworkACLs and the next ACL apply failed on the null traffic type until the rule was fully updated again. Without protocol, the call failed with an NPE.
   • A full update without protocol, cidrlist or traffictype is now refused before anything is changed: "The protocol, cidrlist and traffictype must be given when the ACL rule is updated with partialupgrade=false".
   • I chose refusing over defaulting them as createNetworkACL does (0.0.0.0/0,::/0 and Ingress). A default would silently open a rule to every address or turn an egress rule into an ingress rule.
   • NetworkACLItemDaoImpl.update() now writes the rule and its CIDRs in one transaction, as persist() already does. A failure on the CIDRs now rolls back the rule as well.
   • action is unchanged: when it is left out it is still Allow, as before and as on create.
2. An ICMP type or code given with another protocol was dropped. partialupgrade=false protocol=tcp icmptype=8 was accepted and stored as tcp. The values are now kept, so the existing check refuses them with "Can specify icmpCode and icmpType for ICMP protocol only", as createNetworkACL and a partial update already do. When none is given, changing an ICMP rule to another protocol still clears the ICMP type and code.

Behaviour changes to note:

• An invalid traffictype (for example egres) is now refused with "Invalid traffic type egres", as on create. Before, a partial update silently ignored it.
• The traffictype and partialupgrade parameter descriptions are updated to match.

The UI's edit dialog always sends protocol, cidrlist and traffictype with partialupgrade=false, so it is not affected.

Relation to other open PRs:

• server: avoid NPE updating a network ACL rule with no protocol #14046 removes the NPE for a missing protocol. With this PR, a full update without a protocol doesn't reach that code any more. The two PRs don't touch the same lines.
• VPC ACL: keep the ports and ICMP type of rules given by protocol number or as upper-case ICMP #14357 maps protocol number 1 to icmp in the same command. It is independent of this PR.

Fixes: #14642

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?

• Found while testing VPC ACL changes on a 3-host KVM lab running 24.0 (main). The steps in updateNetworkACLItem with partialupgrade=false leaves a rule broken when cidrlist or traffictype is left out, and drops an ICMP type given with tcp #14642 reproduce both problems there.
• Unit tests in NetworkACLServiceImplTest:
  • a full update without protocol, without cidrlist, or without traffictype is refused, and nothing is set on the rule;
  • a full update with only the required values sets them and clears the rest;
  • tcp with icmptype=8 is refused by validateNetworkACLItem;
  • changing an ICMP rule to tcp clears the ICMP fields;
  • traffictype parsing: blank, valid in either case, and invalid.
• mvn -pl server -am test -Dtest=NetworkACLServiceImplTest,NetworkACLManagerTest: 109 and 8 tests, all pass.

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

• Checked the UI's edit call (AclRulesTab.vue). It sends every field, and an empty cidrlist is still accepted as an empty list, as before.
• Checked the other callers of NetworkACLItemDaoImpl.update() (rule state changes and revoke). They pass rules loaded with their CIDRs, and the nested transaction commits with the outer one.
• Partial updates (the default) behave as before, apart from refusing an invalid traffictype.

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.47%. Comparing base (e6f193d) to head (a536057).

Files with missing lines	Patch %	Lines
...m/cloud/network/vpc/dao/NetworkACLItemDaoImpl.java	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #14643      +/-   ##
============================================
+ Coverage     16.45%   16.47%   +0.01%     
- Complexity    13710    13718       +8     
============================================
  Files          5669     5669              
  Lines        501734   501741       +7     
  Branches      60961    60963       +2     
============================================
+ Hits          82584    82638      +54     
+ Misses       409895   409846      -49     
- Partials       9255     9257       +2     

Flag	Coverage Δ
uitests	4.16% <ø> (ø)
unittests	17.33% <80.00%> (+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.

[bhouse-nexthop]

Test report: full updates of a network ACL rule

Summary: an incomplete updateNetworkACLItem partialupgrade=false is refused before anything is written, and the rule and listNetworkACLs stay intact. protocol=tcp icmptype=8 is refused. Complete full updates still work, including changing an ICMP rule to tcp, which clears the ICMP fields.

Environment

• A CloudStack 24.0 (main) build with this PR, alongside some of my other open PRs. One management server.
• The PR targets 4.22; it wasn't run on a 4.22 build. NetworkACLServiceImpl and NetworkACLItemDaoImpl have the same update path on both branches.
• A new ACL list in a VPC, with two rules:
  • rule 10: tcp 22, 10.0.0.0/8, Ingress, Allow;
  • rule 20: icmp type 8 code 0, 10.0.0.0/8, Ingress, Allow.

Results

#	Call (partialupgrade=false unless noted)	Result
1	rule 10: protocol=tcp startport=80 endport=80 (no cidrlist, no traffictype)	refused, 431
2	rule 10: cidrlist=... traffictype=Ingress startport=80 endport=80 (no protocol)	refused, 431
3	rule 10: protocol=tcp cidrlist=... startport=80 endport=80 (no traffictype)	refused, 431
4	rule 10: complete, with protocol=tcp icmptype=8	refused, 431
5	rule 10, partial update: traffictype=egres	refused, 431
6	rule 10: protocol=udp 53-53 cidrlist=10.1.0.0/16,192.168.0.0/24 traffictype=Egress action=Deny	OK
7	rule 20 (icmp): protocol=tcp 443-443 cidrlist=10.0.0.0/8 traffictype=Ingress, no ICMP fields	OK

Refusal messages

1-3: The protocol, cidrlist and traffictype must be given when the ACL rule is updated with partialupgrade=false
4:   Can specify icmpCode and icmpType for ICMP protocol only
5:   Invalid traffic type egres

Nothing is written by a refused call. The rule's row and CIDRs (network_acl_item, network_acl_item_cidrs), before call 1 and after calls 1-4:

number protocol start end icmp_type icmp_code traffic_type action cidrs
10     tcp      22    22  NULL      NULL      Ingress      Allow  10.0.0.0/8

listNetworkACLs aclid=<list> returned both rules, and listNetworkACLs listall=true succeeded (6 rules), after the refusals. Before this PR, call 1 wrote the rule with a null traffic type and CIDR list, failed with an NPE, and left listNetworkACLs failing.

Complete full updates

• After call 6: 10 udp 53 53 NULL NULL Egress Deny 10.1.0.0/16,192.168.0.0/24. Every field was replaced, and both CIDRs were stored.
• After call 7: 20 tcp 443 443 NULL NULL Ingress Allow 10.0.0.0/8. The ICMP type and code were cleared, since none was given.
• The management server log for the whole run has no NullPointerException.

Result: pass.

Not covered

• The ACL list wasn't attached to a tier, so no rule was pushed to a router. The refusals happen before anything is applied, and a complete full update goes through the same apply path as before.
• A failure while writing the CIDRs, which the PR now rolls back in one transaction: I had no way to make that happen live. It is covered by the PR's reasoning.
• The UI's edit dialog.

[DaanHoogland]

same here @bhouse-nexthop , is it worth your trouble to rebase on 4.20?

[bhouse-nexthop]

@DaanHoogland no trouble. It's rebased onto 4.20 and the PR now targets 4.20 (a536057). The commit applied cleanly, and NetworkACLServiceImplTest passes on 4.20 (109 tests). It will reach 4.22 and main through the forward merge.

[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 20017

[DaanHoogland]

@blueorangutan test

[blueorangutan]

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