[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 18.32%. Comparing base (d02adcd) to head (59c4500).

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.22   #14643   +/-   ##
=========================================
  Coverage     18.32%   18.32%           
- Complexity    16644    16649    +5     
=========================================
  Files          5939     5939           
  Lines        537768   537775    +7     
  Branches      65929    65931    +2     
=========================================
+ Hits          98551    98574   +23     
+ Misses       428008   427994   -14     
+ Partials      11209    11207    -2     

Flag	Coverage Δ
uitests	4.08% <ø> (ø)
unittests	19.42% <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.