Merge ~rodrigo-barbieri2010/maas:bug/1898122 into maas:master

[Rodrigo Barbieri]

LP: #1898122 Fix release API for DISCOVERED IPs

When using the release API, the case of DISCOVERED IPs is not handled. According to the code, they ought to be released the same way as DHCP IPs.

[Rodrigo Barbieri (rodrigo-barbieri2010)]

More context about the fix in the LP bug comments

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bug/1898122 lp:~rodrigo-barbieri2010/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8484/console
COMMIT: 792ba559773a7422013b2c9944f274b9ae107d24

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bug/1898122 lp:~rodrigo-barbieri2010/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8485/console
COMMIT: 389de80693f45141ed7417f1824f64299aa284b5

review: Needs Fixing

[Rodrigo Barbieri (rodrigo-barbieri2010)]

Looks like the CI didn't get re-triggered on commit 383bbb1

[Adam Collard (adam-collard)]

CI was stuck, it'll catch up and retrigger - thanks for the ping

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bug/1898122 lp:~rodrigo-barbieri2010/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8486/console
COMMIT: 383bbb11a8967bd72d1e5d3e12e4a5ade1104371

review: Needs Fixing

[Adam Collard (adam-collard)]

jenkins: !test

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bug/1898122 lp:~rodrigo-barbieri2010/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 383bbb11a8967bd72d1e5d3e12e4a5ade1104371

review: Approve

[Lee Trager (ltrager)]

A discovered IP address can be a static or come from DHCP. I'm not sure what side effects assuming its static will have. To delete a discovered IP we may want to see if we can avoid this code path.

[Björn Tillenius (bjornt)]

I'm not sure a DISCOVERED IP can actually be a static address, since should have a STICKY IP address in that case.

So I do think that treating DISCOVERED as DHCP is probably more correct.

It would also be good if you could add a test to ensure that the API works if you specify discovered=true force=true. This has obviously worked at some point of time, and it would be good to have a test that prevents it from regressing in the future.

review: Needs Fixing

[Rodrigo Barbieri (rodrigo-barbieri2010)]

Hi Bjorn and Lee. Thanks for your review.

The reason I chose to return INTERFACE_LINK_TYPE.STATIC was to achieve a behavior closer as the code path when the DHCP lease expires, which is [1] and [2]. In other words, it is handled as a static IP, has its IP set to None and then save(). You can find more info about this in comment #2 from https://bugs.launchpad.net/maas/+bug/1896292

However, it ends here [3] which just deletes the entry.

I just tested modifying the code to return INTERFACE_LINK_TYPE.DHCP instead, and the end result is exactly the same, as it ends in [4], which deletes the entry.

I haven't inspected the MaaS database itself to see if there is any difference there between when the leases naturally expires vs when forcefully release by the API (in other words, sip.ip = None and sip.save() vs sip.delete(). Curiously it does delete() in [5] for old dynamic IPs as well, so I don't understand why [2] is different for dynamic IPs.

Also, I highly suspect that the code being changed is never invoked anywhere else in case of discovered IPs, as the exception "ValueError("Unknown alloc_type.")" doesn't seem to be captured in any of the code that invokes get_interface_link_type(), however I am not 100% sure. So it doesn't seem to matter whether we return a STATIC or DHCP INTERFACE_LINK.

I will work on implementing the functional test next.

[1] https://github.com/maas/maas/blob/04a456af79ebdfd4858bd317f48e42959cc97670/src/maasserver/rpc/leases.py#L152

[2] https://github.com/maas/maas/blob/04a456af79ebdfd4858bd317f48e42959cc97670/src/maasserver/rpc/leases.py#L212

[3] https://github.com/maas/maas/blob/8eaf8a312c4cd809ae95bba5ca18b2b9584666ad/src/maasserver/models/interface.py#L1177

[4] https://github.com/maas/maas/blob/8eaf8a312c4cd809ae95bba5ca18b2b9584666ad/src/maasserver/models/interface.py#L1191

[5] https://github.com/maas/maas/blob/04a456af79ebdfd4858bd317f48e42959cc97670/src/maasserver/rpc/leases.py#L149

[Rodrigo Barbieri (rodrigo-barbieri2010)]

Pushed new patch:

- Changed return value to DHCP instead of STATIC_IP, as suggested by reviewers (please refer to my previous comment for re-analysis of this)

- Added new unit test for the Release API. Found 1 that almost captured the bug, so cloned it with a slight modification to make it go through the failed code

- Tested unit test, it fails without the fix

@Bjorn: I had previously assumed you meant a functional test, but I didn't find existing Release API functional tests to add a new test to (not sure if I didn't look in the right places), so I added a unit test, which captured the bug.

Waiting for reviews on new patchset.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bug/1898122 lp:~rodrigo-barbieri2010/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 63697d15685fea1bb9e4825b5fa95726ec652be4

review: Approve

[Björn Tillenius (bjornt)]

Looks good to me. Sorry if I was unclear, I meant an API unit test, like the one you added.

review: Approve

[MAAS Lander (maas-lander)]

UNABLE TO START LANDING

STATUS: MISSING COMMIT MESSAGE

[Rodrigo Barbieri (rodrigo-barbieri2010)]

Copy-pasted commit message from patch to LP Merge Request

seems like MAAS LANDER isn't smart enough to pick up the commit message from the commit itself

diff --git a/src/maasserver/api/tests/test_ipaddresses.py b/src/maasserver/api/tests/test_ipaddresses.py
index 10719db..684559d 100644
--- a/src/maasserver/api/tests/test_ipaddresses.py
+++ b/src/maasserver/api/tests/test_ipaddresses.py
@@ -354,6 +354,36 @@ class TestIPAddressesReleaseAPI(APITransactionTestCase.ForUserAndAdmin):
             self.assertThat(response.content.decode("utf-8"), Equals(""))
 
     @transactional
+    def test_POST_release_allows_admin_to_release_discovered_ip_with_interface(
+        self,
+    ):
+        # Make an "orphaned" IP address, like the one in bug #1898122.
+        static_ip = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.DISCOVERED
+        )
+        interface = factory.make_Interface()
+        interface.ip_addresses.add(static_ip)
+
+        response = self.post_release_request(
+            str(static_ip.ip), discovered=True
+        )
+        if self.expect_forbidden:
+            self.assertEqual(http.client.FORBIDDEN, response.status_code)
+            self.assertEqual(
+                "Force-releasing an IP address requires admin privileges.",
+                response.content.decode("utf-8"),
+            )
+        elif not self.force:
+            self.assertEqual(http.client.BAD_REQUEST, response.status_code)
+            self.assertThat(
+                response.content.decode("utf-8"),
+                DocTestMatches("...does not belong to the requesting user..."),
+            )
+        else:
+            self.assertEqual(http.client.NO_CONTENT, response.status_code)
+            self.assertThat(response.content.decode("utf-8"), Equals(""))
+
+    @transactional
     def test_POST_release_deallocates_address(self):
         ipaddress = factory.make_StaticIPAddress(
             alloc_type=IPADDRESS_TYPE.USER_RESERVED, user=self.user
diff --git a/src/maasserver/models/staticipaddress.py b/src/maasserver/models/staticipaddress.py
index 382df5b..c9ec441 100644
--- a/src/maasserver/models/staticipaddress.py
+++ b/src/maasserver/models/staticipaddress.py
@@ -871,7 +871,10 @@ class StaticIPAddress(CleanSave, TimestampedModel):
         """Return the `INTERFACE_LINK_TYPE`."""
         if self.alloc_type == IPADDRESS_TYPE.AUTO:
             return INTERFACE_LINK_TYPE.AUTO
-        elif self.alloc_type == IPADDRESS_TYPE.DHCP:
+        elif self.alloc_type in (
+            IPADDRESS_TYPE.DHCP,
+            IPADDRESS_TYPE.DISCOVERED,
+        ):
             return INTERFACE_LINK_TYPE.DHCP
         elif self.alloc_type == IPADDRESS_TYPE.USER_RESERVED:
             return INTERFACE_LINK_TYPE.STATIC
diff --git a/src/maasserver/models/tests/test_staticipaddress.py b/src/maasserver/models/tests/test_staticipaddress.py
index 2f187c1..4e5defd 100644
--- a/src/maasserver/models/tests/test_staticipaddress.py
+++ b/src/maasserver/models/tests/test_staticipaddress.py
@@ -1394,6 +1394,12 @@ class TestStaticIPAddress(MAASServerTestCase):
             INTERFACE_LINK_TYPE.STATIC, ip.get_interface_link_type()
         )
 
+    def test_get_interface_link_type_returns_STATIC_for_DISCOVERED(self):
+        ip = StaticIPAddress(alloc_type=IPADDRESS_TYPE.DISCOVERED)
+        self.assertEqual(
+            INTERFACE_LINK_TYPE.DHCP, ip.get_interface_link_type()
+        )
+
     def test_get_interface_link_type_returns_STATIC_for_STICKY_with_ip(self):
         ip = StaticIPAddress(
             alloc_type=IPADDRESS_TYPE.STICKY, ip=factory.make_ipv4_address()