Bug 1877838 - [Assisted-4.6][Advanced Networking] Adjust the UI to include recent Advanced Networking validations
Summary: [Assisted-4.6][Advanced Networking] Adjust the UI to include recent Advanced ...
Keywords:
Status: VERIFIED
Alias: None
Product: OpenShift Container Platform
Classification: Red Hat
Component: assisted-installer
Version: 4.5
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: ---
: ---
Assignee: Jonathan Kilzi
QA Contact: Udi Kalifon
URL:
Whiteboard:
: 1884989 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2020-09-10 14:32 UTC by Lital Alon
Modified: 2021-03-15 10:39 UTC (History)
5 users (show)

Fixed In Version: OCP-Metal-UI-v1.5.11
Doc Type: No Doc Update
Doc Text:
Clone Of:
Environment:
Last Closed:
Target Upstream Version:


Attachments (Terms of Use)
screen shot of false validation success on CID prefix (55.96 KB, image/png)
2021-03-03 09:57 UTC, nshidlin
no flags Details


Links
System ID Private Priority Status Summary Last Updated
Github mareklibra facet-lib pull 259 0 None closed OCPBUGSM-17318 Adjust advanced network validations to match backend requirements 2020-11-23 15:26:47 UTC
Github openshift-assisted assisted-ui-lib pull 451 0 None closed BZ-1877838 CIDR fields display wrong validation message 2021-03-03 10:57:14 UTC
Github openshift-assisted assisted-ui-lib pull 477 0 None closed MGMT-4272 Fixes CIDR validation for IPv6 2021-03-03 10:57:15 UTC
Github openshift-assisted assisted-ui-lib pull 495 0 None closed bz-1877838 Fixes CIDR netmask range validation 2021-03-04 11:49:36 UTC

Description Lital Alon 2020-09-10 14:32:48 UTC
Description of problem:
Due to defects around advanced networking validations, the BE validation had changed. There is a need to adjust the UI accordingly.
i.e prefix 32 is not allowed 

Additional info:
PR that resolved the advanced networking validation in the BE: https://github.com/openshift/assisted-service/pull/262

defects that were solved:
https://bugzilla.redhat.com/show_bug.cgi?id=1873105
https://bugzilla.redhat.com/show_bug.cgi?id=1873099
https://bugzilla.redhat.com/show_bug.cgi?id=1873089

Comment 1 Jiri Tomasek 2020-10-07 09:03:11 UTC
*** Bug 1884989 has been marked as a duplicate of this bug. ***

Comment 3 Lital Alon 2020-11-16 18:05:19 UTC
Can you please describe what was fixed?

Cause in the example from here: https://github.com/mareklibra/facet-lib/pull/259, the UI show an error message, but contain incorrect suggestion ("..must be between 1 to 25"). 
If user try to save with CIDR: 172.30.0.0/1, it correctly gets an error from BE:
172.30.0.0/1 is not a valid network CIDR
So it is better to not suggest, or suggest a correct suggestion

In addition, there are cases where im not sure what need to be enforced by UI and what by BE (I have a test for validating this Networking settings, with as much as possible end cases and there are cases where UI should enforce but, it doesn't: https://polarion.engineering.redhat.com/polarion/redirect/project/OSE/wiki/Install-validation%20_%20BM-Inventory/Assisted%20Install%20validation%20BM%20Inventory%20-%20E2E?selection=OCP-34222)

Comment 4 Jiri Tomasek 2021-02-11 14:15:35 UTC
(In reply to Lital Alon from comment #3)
> Can you please describe what was fixed?
> 
> Cause in the example from here:
> https://github.com/mareklibra/facet-lib/pull/259, the UI show an error
> message, but contain incorrect suggestion ("..must be between 1 to 25"). 
> If user try to save with CIDR: 172.30.0.0/1, it correctly gets an error from
> BE:
> 172.30.0.0/1 is not a valid network CIDR
> So it is better to not suggest, or suggest a correct suggestion

@lalon@redhat.com This bug has been filed because UI validations for advanced fields did not match the backend validations. That is what the linked PR fixes. If certain validation message does not match the expectation written in the test plan, please file a new bug specific to that. I don't think a validation message is valid to make this bug fail QA as the goal is to align the validations with backend.

> 
> In addition, there are cases where im not sure what need to be enforced by
> UI and what by BE (I have a test for validating this Networking settings,
> with as much as possible end cases and there are cases where UI should
> enforce but, it doesn't:
> https://polarion.engineering.redhat.com/polarion/redirect/project/OSE/wiki/
> Install-validation%20_%20BM-Inventory/
> Assisted%20Install%20validation%20BM%20Inventory%20-%20E2E?selection=OCP-
> 34222)

It would be really helpful if you could list these cases where UI does not enforce but doesn't.

Comment 5 Lital Alon 2021-02-11 20:46:13 UTC
Right so it wasn't clear to me what was fixed and what need to be tested as part of this bug, in order to be able to mark as verify.
Ill open a bug when ill find anything specific. As for this bug - the only thing left is the scenario where:
Service Network CIDR \ Cluster Network CIDR set to: 10.128.0.0/31, UI propmt a msg: "IPv4 netmask must be between 1-25 and include at least 128 addresses. IPv6 netmask must be between 8-128 and include at least 256 addresses"
The issue: 10.128.0.0/1 is an illegal suggestion, in fact 1, 2, 3, 4 and more are illegal, so the orig suggestion is wrong. What can we do about it?

Comment 8 Jonathan Kilzi 2021-02-14 20:22:46 UTC
The behavior described above actually seems to reflect a flaw in the design of the validation for the CIDR fields.

Right now the front-end validates the CIDR fields by testing, among other things, the prefix length is a number between 1 and 25.
However if the user enters a value like "10.128.0.0/1" and attempts to save and validate the form, the back-end responds with this error: "address must not be unspecified. Unspecified address is the address with all zeroes"

This message is problematic in many forms:
1. Is not clear how by specifying this value: "10.128.0.0/1" we ended up showing an error regarding the "Unspecified Address (0.0.0.0)".
2. Also, is not clear which CIDR field has an incorrect value.

The error message could be improved by mentioning that the "unspecified address" in the current message refers to the "resulting network address". The resulting network address is obtained by performing an AND operation between the IP part of the CIDR string and the netmask; which is obtained from the number after the slash '/' (see RFCs 4632, 4291).

Since this is a back-end validation issue, we'll track its progress here: https://bugzilla.redhat.com/show_bug.cgi?id=1928515

Comment 9 Jonathan Kilzi 2021-02-15 15:50:57 UTC
1. As agreed the error message needs will be changed to: "The specified CIDR is invalid because its resulting routing prefix matches the unspecified address." 
2. UI will add an additional validation that will check if the resulting routing prefix is "0.0.0.0" or "::"

Comment 10 Jonathan Kilzi 2021-02-15 20:37:16 UTC
I've added also an additional validation (stolen from the BE logic) which checks if the IP part of the CIDR value matches the first address (the one used to represent the network) in the range specified by the CIDR notation.

Comment 11 nshidlin 2021-03-03 09:57:08 UTC
Created attachment 1760369 [details]
screen shot of false validation success on CID prefix

Comment 12 nshidlin 2021-03-03 09:57:35 UTC
In the case where the resulting network address after applying the mask is the unspecified address (0.0.0.0) e.g 10.128.1.0/[1-4] the validation error displayed is "The specified CIDR is invalid because its resulting routing prefix matches the unspecified address." As expected.


However in the case of CIDR_prefix > 25 the validation on the Cluster Network CIDR passes and the user recives a validation error on the Cluster Network Host Prefix "The host prefix is a number between size of the cluster network CIDR range (26) and 25." e.g. "Cluster Network CIDR":10.128.1.0/26 "Cluster Network Host Prefix":25 (screen shot attached)

Therefore I am failing the validation on this bug

Comment 13 Jonathan Kilzi 2021-03-03 10:57:23 UTC
Fixed in [PR 495](https://github.com/openshift-assisted/assisted-ui-lib/pull/495)

Comment 14 nshidlin 2021-03-04 16:38:17 UTC
Verified on Staging 
Assisted-ui-lib version:  1.5.11


Note You need to log in before you can comment on or make changes to this bug.