[openstack-dev] [Horizon] Redundant checks in form values

[Suraj Deshmukh]

In file [1] in `class AddRule` in method `_clean_rule_icmp` why checkups
are performed on if `icmp_type` or `icmp_code` is `None` or in range of `-1
to 255`.

What I mean here is that in `class AddRule` while values are being accepted
from form data and being stored, validators do their job of checking
whether that field is valid, so why a redundant checkup in method
`_clean_rule_icmp`?

Please correct me if I am wrong in understanding anything. Currently I am
working on the Bug #1511748 [2]. Previously while checking the validity of
`icmp_type` and `icmp_code`, the functionality of tcp_ports was used. This
is wrong because, TCP ports have a range of 0 to 65535 while `icmp_type`
and `icmp_code` have a range of 0 to 255.

So now oslo_utils.netutils has dedicated functionality to check if
`icmp_type` and `icmp_code` are valid here is a recent code merger [3].

So I was trying to add this newly added functionality into Horizon but the
test cases run older code and hence needed help in getting my head around
with the source.


[1]
openstack_dashboard/dashboards/project/access_and_security/security_groups/forms.py
[2] https://bugs.launchpad.net/horizon/+bug/1511748
[3] https://review.openstack.org/#/c/240661/

Thanks and Regards
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Horizon] Redundant checks in form values

[Akihiro Motoki]

In the current AddRule form, validate_port_range is used as a validator,
so a value from 1 to 65535 is accepted.
I think this is the reason that _clean_rule_icmp check the exact value range.
You can improve it as you think.

bug 1511748 is a regression of validate_port_range change in
https://review.openstack.org/#/c/116508/.
Previously it accepts -1 and we can use it both for TCP/UDP and ICMP,
but after that change it no longer cannot be applied to ICMP.

IMO we need to use separate validators for TCP/UDP and ICMP.

Akihiro



2015-11-19 2:05 GMT+09:00 Suraj Deshmukh :
> In file [1] in `class AddRule` in method `_clean_rule_icmp` why checkups are
> performed on if `icmp_type` or `icmp_code` is `None` or in range of `-1 to
> 255`.
>
> What I mean here is that in `class AddRule` while values are being accepted
> from form data and being stored, validators do their job of checking whether
> that field is valid, so why a redundant checkup in method
> `_clean_rule_icmp`?
>
> Please correct me if I am wrong in understanding anything. Currently I am
> working on the Bug #1511748 [2]. Previously while checking the validity of
> `icmp_type` and `icmp_code`, the functionality of tcp_ports was used. This
> is wrong because, TCP ports have a range of 0 to 65535 while `icmp_type` and
> `icmp_code` have a range of 0 to 255.
>
> So now oslo_utils.netutils has dedicated functionality to check if
> `icmp_type` and `icmp_code` are valid here is a recent code merger [3].
>
> So I was trying to add this newly added functionality into Horizon but the
> test cases run older code and hence needed help in getting my head around
> with the source.
>
>
> [1]
> openstack_dashboard/dashboards/project/access_and_security/security_groups/forms.py
> [2] https://bugs.launchpad.net/horizon/+bug/1511748
> [3] https://review.openstack.org/#/c/240661/
>
> Thanks and Regards
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Horizon] Redundant checks in form values

[Suraj Deshmukh]

On Thu, Nov 19, 2015 at 3:10 AM, Akihiro Motoki  wrote:
> In the current AddRule form, validate_port_range is used as a validator,
> so a value from 1 to 65535 is accepted.
> I think this is the reason that _clean_rule_icmp check the exact value range.
> You can improve it as you think.
>
> bug 1511748 is a regression of validate_port_range change in
> https://review.openstack.org/#/c/116508/.
> Previously it accepts -1 and we can use it both for TCP/UDP and ICMP,
> but after that change it no longer cannot be applied to ICMP.
>
> IMO we need to use separate validators for TCP/UDP and ICMP.
>
> Akihiro
>

Yes we need seperate validators for ICMP's `icmp_type` and
`icmp_code`, I figured that out earlier and hence wrote validator code
in `oslo_utils.netutils` [1] and [2].

So `icmp_type` can be any integer between 0 to 255 and `icmp_code` can
be either None or integer between 0 to 255, so code at [2] does take
care of that. More information on ICMP can be found at [3].

So `_clean_rule_icmp` checks related to `icmp_type` and `icmp_code`
can go away. Since while validating itself Django sets the data to
None if `ValidationError` is raised by validator.


[1] https://review.openstack.org/#/c/240661/
[2] 
https://github.com/openstack/oslo.utils/blob/master/oslo_utils/netutils.py#L207
[3] http://www.nthelp.com/icmp.html

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev