neutron doesn't prevent the network update from external to internal when floatingIPs present

[lina He]

The floatingIP can be created in an external network, but after updating the external network to internal, the floatingIP is still in that internal network.
The updating from internal network to external should be prevented if there are FloatingIPs existing.

Reproduction:
~$ neutron net-create net1 --router:external true
~$ neutron floatingip-create net1
~$ neutron net-update net1 --router:external fasle

This is based on master and stable/queens.

[Bence Romsics (bence-romsics)]

Thank you for your bug report.

I was able to reproduce the problem, here it is with openstack client:

openstack network create --external net1
openstack subnet create subnet1 --network net1 --subnet-range 10.0.4.0/24
openstack floating ip create net1
# succeeds as expected

openstack network create --internal net2
openstack subnet create subnet2 --network net2 --subnet-range 10.0.5.0/24
openstack floating ip create net2
# fails as expected

openstack network set net1 --internal
# succeeds, but it should not

If you had made this comment 5 years ago on the original patch introducing external nets, I'd suggest disabling the updatability of the router:external attribute. However today, after 5 years of having this attribute updatable we clearly cannot make it non-updatable.

While it is technically possible to check the floating ips of a network when the network is being updated, that introduces a dependency (and implicit knowledge) in the wrong direction between networks and floating ips.

Because the first option is impossible and the 2nd is quite undesirable, I'm inclined to set this bug's status to Incomplete and ask about your use case. Could you please write about the reasons you're updating a network's external flag? Can you do that in an alternative way?

[lina He (linahe)]

In neutron, it is allowed to update internal network to external. So we want to explore some related scenarios that supported by neutron. Then we find the unexpected support of updating an external network with FIPs to internal.
We don't have any usecases about this update. But it is a bug for this update feature obviously.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Change abandoned by Bence Romsics (<email address hidden>) on branch: master
Review: https://review.openstack.org/622170

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/631515
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=461be560769f4c591cd6a6513a873fcef92c0789
Submitter: Zuul
Branch: master

commit 461be560769f4c591cd6a6513a873fcef92c0789
Author: Bence Romsics <email address hidden>
Date: Thu Jan 17 15:30:02 2019 +0100

    Delete floating IPs on network turned internal

    The neutron-lib part of this change: https://review.openstack.org/624751

    This change introduces a new shim extension
    (floatingip-autodelete-internal) to signal a change in API behavior
    introduced by the fix of a bug referred below. That is now we autodelete
    unused floating IPs when changing a network to router:external=False.

    It also documents the API change.

    Change-Id: I247106e95abb5d6822aa35ae793531a1d79edbd4
    Needed-By: https://review.openstack.org/624751
    Partial-Bug: #1806032

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/637935
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=cb0d01db91a1f82b17117a7bb64e5dea3e6b3b28
Submitter: Zuul
Branch: master

commit cb0d01db91a1f82b17117a7bb64e5dea3e6b3b28
Author: Bence Romsics <email address hidden>
Date: Mon Feb 18 14:32:23 2019 +0100

    Remove ml2's accidental dependency on l3

    The accidental dependency was never in effect since the neutron side of
    the relevant changes was not merged yet.

    I think I made a mistake in https://review.openstack.org/631515.
    We added the 'router' extension as a dependency of the
    'floatingip-autodelete-internal' extension. Which looks a perfectly
    reasonable thing to do at first sight. However since the 'external-net'
    extension was de-extensionalized and made part of the ml2 plugin, the
    'floatingip-autodelete-internal' extension also had to be implemented by
    the ml2 plugin. This complicated setup practically made the l3 plugin
    a dependency of the ml2 plugin. (That's why unit tests started failing
    in patch set #3 of the neutron change.) Which of course is non-sense.

    So this change removes the dependency. The neutron side of this
    change still degrades gracefully even without the explicit dependency
    between the extensions, so I don't think we're losing anything by not
    having that dependency.

    Change-Id: I8825eaf4f46ea2639131e34f9b833af1de6ab1b4
    Needed-By: https://review.openstack.org/624751
    Partial-Bug: #1806032
    Related-Change: https://review.openstack.org/631515

[Slawek Kaplonski (slaweq)]

This bug has had a related patch abandoned and has been automatically un-assigned due to inactivity. Please re-assign yourself if you are continuing work or adjust the state as appropriate if it is no longer valid.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Slawek Kaplonski (<email address hidden>) on branch: master
Review: https://review.openstack.org/624751
Reason: This review is > 4 weeks without comment, and failed Jenkins the last time it was checked. We are abandoning this for now. Feel free to reactivate the review by pressing the restore button and leaving a 'recheck' comment to get fresh test results.

[songminglong (songminglong)]

Directly deleting floatingip is a dangerous and unhumanized behavior when updating router:external to False. It should be a more reasonable behavior to prompt the user to delete these fips first before updating router:external to False.

[Bence Romsics (bence-romsics)]

FYI: In the abandoned patch I went for autodeleting for two reasons:

* IIRC the presence of in-use floating IPs already prompt the user before the router can be updated to internal.
* There's another operation which already autodeletes not-in-use FIPs: deleting an external net.

I'm not saying we need to stick with this, just wanted to recall the reasons discovered earlier.

[Brian Haley (brian-haley)]

So I'm a little confused. It seems we merged the extension in neutron-lib but nothing in neutron to consume it? Is there more work that needs to be done? Thanks.