Open Stack

On 04/03/2017 09:23 PM, Ghanshyam Mann wrote:
> On Sun, Mar 26, 2017 at 9:31 PM, Sridhar Gaddam <sgaddam at redhat.com
> <mailto:sgaddam at redhat.com>> wrote:
>
>     On Fri, Mar 24, 2017 at 7:27 PM, Brian Haley <haleyb.dev at gmail.com
>     <mailto:haleyb.dev at gmail.com>> wrote:
>
>         On 03/24/2017 06:41 AM, Ghanshyam Mann wrote:
>
>             Hi All,
>
>             Tempest is testing SG rule creation and pinging scenario
>             tests with
>             ethertype='IPv6' and protocol='icmp' [0].
>             In case of ethertype='IPv6', currently neutron accept
>             protocol type
>             as 'icmp', 'icmpv6' and 'ipv6-icmp' which again seems like
>             duplication
>             of SG rules bug on neutron side but not sure [1]
>
>             But it seems like some driver does not work with 'icmp' on
>             IPv6, at
>             least ODL as mentioned in bug [2]. Where few others like ML2/OVS
>             iptables driver convert 'icmp' to 'icmpv6' when
>             ethertype='IPv6' and had
>             no issue with 'icmp'.
>
>             IMO neutron should keep accepting 'icmp' for IPv6 for backward
>             compatibility and legacy usage and tempest should test
>             'icmp' also along
>             with other protocol type.
>             But we need more feedback on that what is right way (as per
>             backward
>             compatibility pov) and recommended way for having best
>             behaviour for SG
>             rules on IPv6. What best can work for all plugins also?
>
>
>         Thanks for raising this issue.  Let me just restate it a little
>         so it's clear.
>
>         1. One can create an IPv6 rule using protocol value "icmp"
>         today, and the base security group code does the right thing
>         changing the rule to be correct for the underlying
>         implementation, for example, "ipv6-icmp" for iptables.  It
>         doesn't look like all other drivers handle this properly.
>
>     ​Well, let the Neutron API accept multiple values like
>     "icmp/icmpv6/ipv6-icmp", but IMO it should store a single Security
>     Group rule in the DB and raise "Duplicate error when similar rule is
>     configured once again".
>     Currently, Neutron treats each of them as a different Security Group
>     rule and stores them as separate entries in the DB.
>     However, IPtables Firewall driver in Neutron is converting[1] the
>     "ethertype=IPv6 and protocol=icmp" as a request to ICMPv6 and
>     applying the necessary ip-table rule.
>     https://github.com/openstack/neutron/blob/stable/newton/neutron/agent/linux/iptables_firewall.py#L373
>     <https://github.com/openstack/neutron/blob/stable/newton/neutron/agent/linux/iptables_firewall.py#L373>
>
>     Since this is not a documented behavior, other firewall drivers
>     (which I guess could be an issue even with OVS firewall driver) may
>     be missing this info.​
>
> ​++ for this, documentation could have helped this better way. ​
>
>         2. The neutron API will accept multiple values - "icmp",
>         "ipv6-icmp" and "icmpv6" for an IPv6 rule, but it will create
>         unique database entries for each (I just verified that).  While
>         that shouldn't create multiple entries in the base iptables
>         code, it will probably generate a warning in the logs about a
>         duplicate being suppressed.
>
>
>         So there are a few things that could be done:
>
>         1. Drivers need to accept "icmp" in order to be
>         backwards-compatible with the current code.
>
>         2. Duplicates should be detected and generate 409 (?) errors.
>
>         3. We should add a migration (IMO) where any duplicates are
>         squashed.
>
>     ​Agree with your points.

I just created https://review.openstack.org/453346 as a start at #2, 
will try and add code to squash existing duplicates too (#3).

-Brian


> ​Yes, 409 on same type again make sense. And it can be documented
> properly that 'icmp'​ will be treated same as other protocol type for
> ​
> Ethertype=IPv6 and user will get 409 if they try to create and expect 2
> different SG rules with those different protocol_type.
> This is something change in current behavior where both requests('icmp'
> and 'icmpv6') are treated as 2 separate rules. And so this new Tempest
> tests will fail [1] which can be modified later.
>
> With those points and current situation, I feel Tempest should tests the
> current behavior proposed in [1] which will discover the drivers for
> point 1. Later based on bug [2] consensus we can change the tests
> accordingly.
> I want to merge Tempest changes so that while changing anything on
> neutron side, we know exactly what behavior we are going to change and
> its impact etc.
> Please leave comment on patch if any more feedback.
>
>
>
>     ​
>
>         The open question is, do we want to change the DB to have a
>         different value here, like "icmpv6" ?  We could obviously add a
>         migration where we update the value.  The problem is that flag
>         day could pose a problem if out-of-tree drivers don't support
>         the new value.  I think we should leave it "icmp" for that
>         reason, thoughts from others?
>
>     IMO, the out-of-tree drivers should be supporting the icmpv6
>     protocol​ rule properly. There is a possibility that they may hit
>     this issue[https://bugs.launchpad.net/tempest/+bug/1671366
>     <https://bugs.launchpad.net/tempest/+bug/1671366>] for not being
>     aware that a request for "
>     ​​
>     Ethertype=IPv6 and protocol=icmp" should be treated as ICMPv6 SG rule.
>
>     Please see the DB entries for the two SG rules
>     - http://paste.openstack.org/show/604190/
>     <http://paste.openstack.org/show/604190/>
>
>
>
> ​..1
> https://review.openstack.org/#/c/431276/16/tempest/api/network/test_security_groups.py
>
> ..2 https://bugs.launchpad.net/neutron/+bug/1582500
>
> -gmann​