ML2 security groups only work with agent drivers

[Kyle Mestery]

The current ML2 integration with security groups makes a bunch of assumptions which don't work for controller based architectures like OpenDaylight and OVN. This bug will track the fixing of these issues.

The main issues include the fact it assumes an agent-based approach and will send SG updates via RPC calls to the agents. This isn't true for ODL or OVN.

[Armando Migliaccio (armando-migliaccio)]

Based on the current initialization path:

https://github.com/openstack/neutron/blob/master/neutron/plugins/ml2/plugin.py#L143
https://github.com/openstack/neutron/blob/master/neutron/plugins/ml2/rpc.py#L203

And calls being made unconditionally like here:

https://github.com/openstack/neutron/blob/master/neutron/plugins/ml2/plugin.py#L784

I confirm this bug. I am honored to be assigned this bug, my sire. I'll get this sorted with the best of my abilities and the soonest I can.

[Armando Migliaccio (armando-migliaccio)]

I am digging a bit deeper to figure out the best course of action...bear with me.

[Kyle Mestery (mestery)]

Awesome, thanks Armando!

[Salvatore Orlando (salvatore-orlando)]

possibly this bug report should be renamed to "ML2" is not a control plane and should not pretend to be one.

[Armando Migliaccio (armando-migliaccio)]

Ok, even though it's true that ML2 ties tightly into the RPC fabric, I am starting to realize that decoupling the two is way more that we can chew in the span of a single bug fix. One could argue that ML2 is indeed not a control plane, as Salvatore pointed out, but this goes way beyond the scope of a bug report.

Having said that, if drivers are interested in learning about security groups events I can see two possible approaches:

1) Modify the ML2 Driver API contract: we could add more methods to [1] to expose the securitygroups artifacts. I believe this is problematic and it feels like feature creep. IMO, ML2 should only be about L2, and it's already bad, IMO, that we expose L3 concepts like subnets. The other drawback is that the change would be pretty invasive for the already crowded ML2 plugin. Furthermore, the change is very heavyweight and smells of spec.

2) Leverage the Registry: we could add notification calls to trigger securitygroups events. This approach has the advantage to be minimally invasive and changes would be limited to adding the proper triggers in the security groups layer so that 'anyone' interested in learning about secgroup events can consume them, regardless of them being mech_drivers, service plugins, or core plugins. No API is to be modified, and we'd only need to extend the registry to be able to handle these new resources. The code has already plenty of examples where the registry is leveraged to dispatch resource events and a number of patches [2,3,4,5] have been proving its effectiveness.

[1] https://github.com/openstack/neutron/blob/master/neutron/plugins/ml2/driver_api.py#L480
[2] https://review.openstack.org/#/c/174268/
[3] https://review.openstack.org/#/c/169876/
[4] https://review.openstack.org/#/c/164466/
[5] https://review.openstack.org/#/c/167275/

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/174588

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/174666

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/175179

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/174666
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=05a9f16257c2953bf40d11ca2a2f9651ba4e86b2
Submitter: Jenkins
Branch: master

commit 05a9f16257c2953bf40d11ca2a2f9651ba4e86b2
Author: armando-migliaccio <email address hidden>
Date: Thu Apr 16 17:37:51 2015 -0700

    Avoid double-hopping deletes for security group rules

    There is no need to get and delete; we can delete with one bullet.
    This will most likely have quite a decent performance benefit overall.

    The patch preserves the existing logic of raising and error on the missing
    element; a test was added to spur up the coverage.

    Related-bug: #1444112

    Change-Id: Iaef77bd3f7775ed91d374838fb5488d925b4062c

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/174588
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=868e67b480b08cc815d802cf950547c6b5ac0153
Submitter: Jenkins
Branch: master

commit 868e67b480b08cc815d802cf950547c6b5ac0153
Author: armando-migliaccio <email address hidden>
Date: Thu Apr 16 12:45:32 2015 -0700

    Add security groups events

    ML2 mech drivers have no direct exposure to security groups,
    and they can only infer them from the associated network/ports.
    This is problematic as agentless ML2 mech drivers have no way of
    intercepting securitygroups events and propagate the information
    to their backend, or more generally, react to them.

    This patch leverages the callback registry to dispatch such events
    so that interested ML2 mech drivers (or any interested party like
    service plugins) can be notified and react accordingly.

    This patch addresses create/update/delete of security groups and
    create/delete of security groups rules. Other events may be added
    over time, if need be.

    This patch is only about emitting the events. The actual subscription
    and implementation of the event handlers will have to take place where
    deemed appropriate.

    Closes-bug: #1444112

    Change-Id: Ifa1d7ee9c967576f824f1129dd68e6e3abd48f5c

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/175179
Committed: https://git.openstack.org/cgit/stackforge/networking-odl/commit/?id=60cf8d88b6dcfb869230738c5dc241afae57f635
Submitter: Jenkins
Branch: master

commit 60cf8d88b6dcfb869230738c5dc241afae57f635
Author: Kyle Mestery <email address hidden>
Date: Sat Apr 18 20:03:46 2015 +0000

    Add logic to pass sg and sg-rules to ODL

    This commit adds a callback hook into the ODL MechanismDriver to handle
    security-group and security-group-rules createn, update, and delete.

    Closes-Bug: #1444112

    Change-Id: I07ef8e0277a66294513693d011d4ef998ae134ec
    Depends-On: Ifa1d7ee9c967576f824f1129dd68e6e3abd48f5c

[Kyle Mestery (mestery)]

Re-opening this one. The original commit had problems, so it was reverted. Working on a better solution now.

[Kyle Mestery (mestery)]

The OVN-side fix merged here: https://review.openstack.org/#/c/177560/

[Nell Jerram (neil-jerram)]

In the Calico mechanism driver, (I believe) I've handled this problem by hooking self.db.notifier. The mechanism driver does:

            if self.db.notifier.__class__ != CalicoNotifierProxy:
                self.db.notifier = CalicoNotifierProxy(self.db.notifier, self)

and then:

class CalicoNotifierProxy(object):
    """Proxy pattern class used to intercept security-related notifications
    from the ML2 plugin.
    """

    def __init__(self, ml2_notifier, calico_driver):
        self.ml2_notifier = ml2_notifier
        self.calico_driver = calico_driver

    def __getattr__(self, name):
        return getattr(self.ml2_notifier, name)

    def security_groups_rule_updated(self, context, sgids):
        LOG.info("security_groups_rule_updated: %s %s" % (context, sgids))
        self.calico_driver.send_sg_updates(sgids, context)
        self.ml2_notifier.security_groups_rule_updated(context, sgids)

    def security_groups_member_updated(self, context, sgids):
        LOG.info("security_groups_member_updated: %s %s" % (context, sgids))
        self.calico_driver.send_sg_updates(sgids, context)
        self.ml2_notifier.security_groups_member_updated(context, sgids)

I guess it's clear that this is a fairly hacky solution, and hence just confirms the problem for which this bug exists - but does it help at all as to how the problem might be solved more properly?

(https://github.com/Metaswitch/calico/blob/master/calico/openstack/mech_calico.py)

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: neutron-pecan
Review: https://review.openstack.org/185072

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/kilo
Review: https://review.openstack.org/196763

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/kilo
Review: https://review.openstack.org/196794

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/196763
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=86584f5da2bcec0fe5c1e9790dac6a4f3f35955d
Submitter: Jenkins
Branch: stable/kilo

commit 86584f5da2bcec0fe5c1e9790dac6a4f3f35955d
Author: armando-migliaccio <email address hidden>
Date: Thu Apr 16 17:37:51 2015 -0700

    Avoid double-hopping deletes for security group rules

    There is no need to get and delete; we can delete with one bullet.
    This will most likely have quite a decent performance benefit overall.

    The patch preserves the existing logic of raising and error on the missing
    element; a test was added to spur up the coverage.

    Related-bug: #1444112

    Change-Id: Iaef77bd3f7775ed91d374838fb5488d925b4062c
    (cherry picked from commit 05a9f16257c2953bf40d11ca2a2f9651ba4e86b2)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/196794
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=86f80d4c9cd37f19ae8dfd1488c4af73805e6b1e
Submitter: Jenkins
Branch: stable/kilo

commit 86f80d4c9cd37f19ae8dfd1488c4af73805e6b1e
Author: armando-migliaccio <email address hidden>
Date: Thu Apr 16 12:45:32 2015 -0700

    Add security groups events

    ML2 mech drivers have no direct exposure to security groups,
    and they can only infer them from the associated network/ports.
    This is problematic as agentless ML2 mech drivers have no way of
    intercepting securitygroups events and propagate the information
    to their backend, or more generally, react to them.

    This patch leverages the callback registry to dispatch such events
    so that interested ML2 mech drivers (or any interested party like
    service plugins) can be notified and react accordingly.

    This patch addresses create/update/delete of security groups and
    create/delete of security groups rules. Other events may be added
    over time, if need be.

    This patch is only about emitting the events. The actual subscription
    and implementation of the event handlers will have to take place where
    deemed appropriate.

    Closes-bug: #1444112

    Change-Id: Ifa1d7ee9c967576f824f1129dd68e6e3abd48f5c
    (cherry picked from commit 868e67b480b08cc815d802cf950547c6b5ac0153)

[Kyle Mestery (mestery)]

Marking this as "Fix Committed" for ODL, as it was fixed with this patch [1] which neglected to include the bug ID.

[1] https://review.openstack.org/#/c/178325/

[Armando Migliaccio (armando-migliaccio)]

This has been fully backported to Kilo.

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/363198

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/363198
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=97338258967d3b95f382f188ab2ab573a3432c17
Submitter: Jenkins
Branch: master

commit 97338258967d3b95f382f188ab2ab573a3432c17
Author: Dariusz Smigiel <email address hidden>
Date: Tue Aug 30 20:43:42 2016 +0000

    Cleanup of SecurityGroup classes

    Commit Ifb70a118cef48c3c4cd313e22e907aa47bc51ad0 intended to remove some
    classes, but without DeprecationWarnings. To prevent from possible
    problems, if it would be now removed, added Warnings and included info,
    about future removal.

    Change-Id: Iacb93abc363bf638efc3acb5b29c02a7508bc43a
    Related-Bug: #1444112