UI: Removed redundant input fields from ACL List rules modal

[OlegChuev]

Description

This PR removes redundant input fields from the front end (FE) on the 'Add ACL Rules' modal window.

According to server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java, line 588:

ICMP code and ICMP type can't be passed in for any protocol other than ICMP.

In the current state, the input field exists but isn't functional.

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

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

Current state:

After fix:

How Has This Been Tested?

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

[OlegChuev]

Maybe it's also worth mentioning somewhere in the ACL Rules API documentation?

api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java

[DaanHoogland]

@blueorangutan ui

[blueorangutan]

@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

[DaanHoogland]

@OlegChuev , thanks for the fix. I am not sure if it covers all combinations this way, but I'll have a test/look.

[blueorangutan]

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/8253 (QA-JID-230)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (88ff77d) 12.93% compared to head (06f8c5a) 13.09%.
Report is 214 commits behind head on 4.18.

Additional details and impacted files

@@             Coverage Diff              @@
##               4.18    #8253      +/-   ##
============================================
+ Coverage     12.93%   13.09%   +0.15%     
- Complexity     8944     9120     +176     
============================================
  Files          2715     2720       +5     
  Lines        256107   257638    +1531     
  Branches      39938    40168     +230     
============================================
+ Hits          33139    33748     +609     
- Misses       218808   219625     +817     
- Partials       4160     4265     +105     

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

[DaanHoogland]

@OlegChuev , it works but...

shouldn't in the above case the icmp type and code be shown instead of the start and end port?
I think the showing of the icmp fields in case of Protocol number was on purpose.

[OlegChuev]

@DaanHoogland yep, I think it makes sense to make this field visible if the user enters Protocol Number equal to 1 (ICMP)

however, personally, I find it a bit weird to enter the protocol number directly instead of choosing the already predefined ICMP, don't you think so? :p

[DaanHoogland]

@DaanHoogland yep, I think it makes sense to make this field visible if the user enters Protocol Number equal to 1 (ICMP)

however, personally, I find it a bit weird to enter the protocol number directly instead of choosing the already predefined ICMP, don't you think so? :p

maybe, but there are 255 protocol numbers possible and I have no idea if all of those use port ranges or maybe some of them use service/qos or some other combination. I don't think we can cater for everything. Also I do not know if your change is affending someone's intended use. Given those remarks, it is fine by me!

[DaanHoogland]

@alexandremattioli you want to have a look?

[shwstppr]

UI code change looks okay but I'm not certain about the use-case.
@alexandremattioli @andrijapanicsb @NuxRo @rajujith can comment better

[andrijapanicsb]

Since we have "ICMP" as a dedicated menu item (next to Protocol Number) - I think this PR makes sense - hide other ICMP fields if "ICMP" is not specifically the chosen protocol.

[weizhouapache]

Since we have "ICMP" as a dedicated menu item (next to Protocol Number) - I think this PR makes sense - hide other ICMP fields if "ICMP" is not specifically the chosen protocol.

+1
ideally these 2 fields should be visible when protocol number is 1 (ICMP) or 58 (IPv6-ICMP), it requires some other changes.

[OlegChuev]

ideally these 2 fields should be visible when protocol number is 1 (ICMP) or 58 (IPv6-ICMP), it requires some other changes.

Just a quick question here: since we are making these two fields visible, does it make sense to change the backend API logic as well? Currently, even if we set the Protocol Number to 1 or 58, ICMP type and ICMP code will raise an error because of the following code:

boolean isIcmpProtocol = protocol.equalsIgnoreCase(NetUtils.ICMP_PROTO);
if (!isIcmpProtocol && (icmpCode != null || icmpType != null)) {
    throw new InvalidParameterValueException("Can specify icmpCode and icmpType for ICMP protocol only");
}

server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java

[andrijapanicsb]

ideally these 2 fields should be visible when protocol number is 1 (ICMP) or 58 (IPv6-ICMP), it requires some other changes.

Just a quick question here: since we are making these two fields visible, does it make sense to change the backend API logic as well? Currently, even if we set the Protocol Number to 1 or 58, ICMP type and ICMP code will raise an error because of the following code:

boolean isIcmpProtocol = protocol.equalsIgnoreCase(NetUtils.ICMP_PROTO);
if (!isIcmpProtocol && (icmpCode != null || icmpType != null)) {
    throw new InvalidParameterValueException("Can specify icmpCode and icmpType for ICMP protocol only");
}

server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java

If you can do that easily - sure, and with minimal effort/risk - otherwise the API error message is very descriptive and will help the operator learn his IP skills :)

[weizhouapache]

let's merge this. cc @DaanHoogland @OlegChuev @andrijapanicsb @shwstppr

I will take the rest (API, service, etc)

• list protocol numbers in the dropdown
• display icmp type/code if protocol number is 1 (ICMP) or protocol is ICMP, but start/end ports will be hidden then
• list icmp types/codes in the dropdown
• check icmp type/code if protocol number is 1 or protocol is ICMP