Merge ~alexsander-souza/maas:lp1966474_ignore_unused_creds into maas:master

[Alexsander de Souza]

Ignore unused creds when deploying a KVM host

fixes LP#1966474

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1966474_ignore_unused_creds lp:~alexsander-souza/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 72818047ae4f821cadd7588af6c07b02e74b7290

review: Approve

[Alberto Donato (ack)]

The change itself looks good as a fallback, but _create_vmhost_for_deployment() (from metadataserver.api_twisted) is supposed to delete credentials when registering the VM hosts.

It'd be good to understand why this is not happening, which is the real cause of the bug.

review: Needs Information

[Alberto Donato (ack)]

Actually, I guess the reason is that if the deployment doesn't complete, the secrets are leftover.

I think the right fix would be to
 - delete all secrets when a machine is released
 - delete all secrets (before creating new ones) when the machine is deployed

7b95791... by Alexsander de Souza on 2022-04-03

delete credentials on node release

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1966474_ignore_unused_creds lp:~alexsander-souza/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 65d4d4cb9799d200e12ddc0aa74169582242d781

review: Approve

467fb58... by Alexsander de Souza on 2022-04-03

remove leftover credentials

[Alexsander de Souza (alexsander-souza)]

Updated with the suggested fixes

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1966474_ignore_unused_creds lp:~alexsander-souza/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/12257/console
COMMIT: c9654cc12c9ea8dda8a0f6dd2da372ee3b443f85

review: Needs Fixing

3ece4a7... by Alexsander de Souza on 2022-04-03

lint

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1966474_ignore_unused_creds lp:~alexsander-souza/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: fead4ad066dddfe92944c1a3c5e8364b47d67fe4

review: Approve

[Alberto Donato (ack)]

A few additional comments

54b55de... by Alexsander de Souza on 2022-04-11

fixes

93037f9... by Alexsander de Souza on 2022-04-11

revert unrequired change

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1966474_ignore_unused_creds lp:~alexsander-souza/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/12322/console
COMMIT: 04f963359a11170841d3d97f05e625ec936ab3c8

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1966474_ignore_unused_creds lp:~alexsander-souza/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/12323/console
COMMIT: 93037f922ef22e2745a372ba48a611db3a4defad

review: Needs Fixing

e804ab3... by Alexsander de Souza on 2022-04-11

fix test

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1966474_ignore_unused_creds lp:~alexsander-souza/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: e804ab39588d1c1a76477ab2c6747cb7ade62e0f

review: Approve

[Alberto Donato (ack)]

Thanks for the changes, looks good! +1

review: Approve

diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 5740b70..cdcedfa 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -3718,6 +3718,11 @@ class Node(CleanSave, TimestampedModel):
         # If this node has non-installable children, remove them.
         self.children.all().delete()
 
+        # Release volatile metadata
+        from maasserver.models import NodeMetadata
+
+        NodeMetadata.objects.release_volatile(self)
+
         # Power was off or cannot be powered off so release to ready now.
         if finalize_release:
             self._finalize_release()
diff --git a/src/maasserver/models/nodemetadata.py b/src/maasserver/models/nodemetadata.py
index 0daa3d0..8ad361b 100644
--- a/src/maasserver/models/nodemetadata.py
+++ b/src/maasserver/models/nodemetadata.py
@@ -22,6 +22,24 @@ class NodeMetadataManager(Manager):
         except NodeMetadata.DoesNotExist:
             return default
 
+    def release_volatile(cls, node):
+        """Remove volatile information.
+
+        Should be called when releasing the node to remove all data that
+        is related to this deployment.
+        """
+        from metadataserver import vendor_data
+
+        volatile_meta = (
+            vendor_data.LXD_CERTIFICATE_METADATA_KEY,
+            vendor_data.VIRSH_PASSWORD_METADATA_KEY,
+        )
+
+        NodeMetadata.objects.filter(
+            node=node,
+            key__in=volatile_meta,
+        ).delete()
+
 
 class NodeMetadata(CleanSave, TimestampedModel):
     """A `NodeMetadata` represents a key/value storage for Node metadata.
diff --git a/src/maasserver/models/tests/test_nodemetadata.py b/src/maasserver/models/tests/test_nodemetadata.py
index 9a19956..6e979d9 100644
--- a/src/maasserver/models/tests/test_nodemetadata.py
+++ b/src/maasserver/models/tests/test_nodemetadata.py
@@ -9,6 +9,7 @@ from django.core.exceptions import ValidationError
 from maasserver.models import NodeMetadata
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.utils.orm import reload_object
 from maastesting.crochet import wait_for
 
 wait_for_reactor = wait_for()
@@ -50,3 +51,23 @@ class TestNodeMetadata(MAASServerTestCase):
             default,
             NodeMetadata.objects.get(node=node, key=key, default=default),
         )
+
+    def test_release_volatile(self):
+        from metadataserver.vendor_data import (
+            LXD_CERTIFICATE_METADATA_KEY,
+            VIRSH_PASSWORD_METADATA_KEY,
+        )
+
+        node = factory.make_Node()
+        dummy = factory.make_NodeMetadata(node=node)
+        cred_virsh = factory.make_NodeMetadata(
+            key=VIRSH_PASSWORD_METADATA_KEY, node=node
+        )
+        cred_lxd = factory.make_NodeMetadata(
+            key=LXD_CERTIFICATE_METADATA_KEY, node=node
+        )
+
+        NodeMetadata.objects.release_volatile(node)
+        self.assertIsNotNone(reload_object(dummy))
+        self.assertIsNone(reload_object(cred_virsh))
+        self.assertIsNone(reload_object(cred_lxd))
diff --git a/src/metadataserver/api_twisted.py b/src/metadataserver/api_twisted.py
index 6460184..77cac8c 100644
--- a/src/metadataserver/api_twisted.py
+++ b/src/metadataserver/api_twisted.py
@@ -184,13 +184,16 @@ POD_CREATION_ERROR = (
 
 
 def _create_vmhost_for_deployment(node):
+    cred_types = set()
+    if node.register_vmhost:
+        cred_types.add(LXD_CERTIFICATE_METADATA_KEY)
+    if node.install_kvm:
+        cred_types.add(VIRSH_PASSWORD_METADATA_KEY)
+
     creds_meta = list(
         NodeMetadata.objects.filter(
             node=node,
-            key__in=(
-                LXD_CERTIFICATE_METADATA_KEY,
-                VIRSH_PASSWORD_METADATA_KEY,
-            ),
+            key__in=cred_types,
         )
     )
     if not creds_meta:
diff --git a/src/metadataserver/tests/test_api_twisted.py b/src/metadataserver/tests/test_api_twisted.py
index a58e200..20332fd 100644
--- a/src/metadataserver/tests/test_api_twisted.py
+++ b/src/metadataserver/tests/test_api_twisted.py
@@ -1263,3 +1263,69 @@ class TestCreateVMHostForDeployment(MAASServerTestCase):
             node=node, key="virsh_password", value="xyz123"
         )
         self.assertRaises(DatabaseError, _create_vmhost_for_deployment, node)
+
+    def test_ignore_unused_lxd_creds(self):
+        user = factory.make_User()
+        node = factory.make_Node_with_Interface_on_Subnet(
+            owner=user,
+            status=NODE_STATUS.DEPLOYING,
+            agent_name="maas-kvm-pod",
+            install_kvm=True,
+        )
+        ip = factory.make_StaticIPAddress(interface=node.boot_interface)
+        virsh_password_meta = NodeMetadata.objects.create(
+            node=node, key="virsh_password", value="xyz123"
+        )
+        cert = generate_certificate("maas")
+        lxd_cert_meta = NodeMetadata.objects.create(
+            node=node,
+            key="lxd_certificate",
+            value=cert.certificate_pem() + cert.private_key_pem(),
+        )
+        addr = ip.ip
+        if IPAddress(addr).version == 6:
+            addr = f"[{addr}]"
+        _create_vmhost_for_deployment(node)
+        virsh_vmhost = Pod.objects.get(power_type="virsh")
+        self.assertEqual(node.status, NODE_STATUS.DEPLOYED)
+        self.mock_discover_and_sync.assert_has_calls(
+            [
+                call(virsh_vmhost, user),
+            ],
+            any_order=True,
+        )
+        self.assertIsNone(reload_object(virsh_password_meta))
+        self.assertIsNotNone(reload_object(lxd_cert_meta))
+
+    def test_ignore_unused_virsh_creds(self):
+        user = factory.make_User()
+        node = factory.make_Node_with_Interface_on_Subnet(
+            owner=user,
+            status=NODE_STATUS.DEPLOYING,
+            agent_name="maas-kvm-pod",
+            register_vmhost=True,
+        )
+        ip = factory.make_StaticIPAddress(interface=node.boot_interface)
+        virsh_password_meta = NodeMetadata.objects.create(
+            node=node, key="virsh_password", value="xyz123"
+        )
+        cert = generate_certificate("maas")
+        lxd_cert_meta = NodeMetadata.objects.create(
+            node=node,
+            key="lxd_certificate",
+            value=cert.certificate_pem() + cert.private_key_pem(),
+        )
+        addr = ip.ip
+        if IPAddress(addr).version == 6:
+            addr = f"[{addr}]"
+        _create_vmhost_for_deployment(node)
+        lxd_vmhost = Pod.objects.get(power_type="lxd")
+        self.assertEqual(node.status, NODE_STATUS.DEPLOYED)
+        self.mock_discover_and_sync.assert_has_calls(
+            [
+                call(lxd_vmhost, user),
+            ],
+            any_order=True,
+        )
+        self.assertIsNotNone(reload_object(virsh_password_meta))
+        self.assertIsNone(reload_object(lxd_cert_meta))
diff --git a/src/metadataserver/tests/test_vendor_data.py b/src/metadataserver/tests/test_vendor_data.py
index 6f625a2..a2044a7 100644
--- a/src/metadataserver/tests/test_vendor_data.py
+++ b/src/metadataserver/tests/test_vendor_data.py
@@ -26,6 +26,7 @@ from maasserver.testing.factory import factory
 from maasserver.testing.fixtures import RBACEnabled
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils.converters import systemd_interval_to_calendar
+from maasserver.utils.orm import reload_object
 from maastesting.matchers import MockNotCalled
 from metadataserver import vendor_data
 from metadataserver.vendor_data import (
@@ -45,6 +46,8 @@ from metadataserver.vendor_data import (
     HARDWARE_SYNC_MACHINE_TOKEN_PATH,
     HARDWARE_SYNC_SERVICE_TEMPLATE,
     HARDWARE_SYNC_TIMER_TEMPLATE,
+    LXD_CERTIFICATE_METADATA_KEY,
+    VIRSH_PASSWORD_METADATA_KEY,
 )
 from provisioningserver.drivers.pod.lxd import LXD_MAAS_PROJECT_CONFIG
 
@@ -335,6 +338,17 @@ class TestGenerateKVMPodConfiguration(MAASServerTestCase):
             netboot=False,
             install_kvm=True,
         )
+        factory.make_NodeMetadata(
+            key=VIRSH_PASSWORD_METADATA_KEY,
+            node=node,
+            value="old value",
+        )
+        cred_lxd = factory.make_NodeMetadata(
+            key=LXD_CERTIFICATE_METADATA_KEY,
+            node=node,
+            value="old value",
+        )
+
         config = list(generate_kvm_pod_configuration(node))
         self.assertEqual(
             config,
@@ -389,6 +403,7 @@ class TestGenerateKVMPodConfiguration(MAASServerTestCase):
         password_meta = NodeMetadata.objects.first()
         self.assertEqual(password_meta.key, "virsh_password")
         self.assertEqual(password_meta.value, password)
+        self.assertIsNone(reload_object(cred_lxd))
 
     def test_yields_configuration_when_machine_register_vmhost_true(self):
         cert = vendor_data.generate_certificate("maas")
@@ -399,6 +414,16 @@ class TestGenerateKVMPodConfiguration(MAASServerTestCase):
             netboot=False,
             register_vmhost=True,
         )
+        cred_virsh = factory.make_NodeMetadata(
+            key=VIRSH_PASSWORD_METADATA_KEY,
+            node=node,
+            value="old value",
+        )
+        factory.make_NodeMetadata(
+            key=LXD_CERTIFICATE_METADATA_KEY,
+            node=node,
+            value="old value",
+        )
         config = list(generate_kvm_pod_configuration(node))
         self.assertEqual(
             config,
@@ -436,6 +461,7 @@ class TestGenerateKVMPodConfiguration(MAASServerTestCase):
         self.assertEqual(
             creds_meta.value, cert.certificate_pem() + cert.private_key_pem()
         )
+        self.assertIsNone(reload_object(cred_virsh))
 
     def test_includes_smt_off_for_install_kvm_on_ppc64(self):
         password = "123secure"
diff --git a/src/metadataserver/vendor_data.py b/src/metadataserver/vendor_data.py
index b82fd80..7ccfcb5 100644
--- a/src/metadataserver/vendor_data.py
+++ b/src/metadataserver/vendor_data.py
@@ -189,15 +189,17 @@ def generate_kvm_pod_configuration(node):
     if node.netboot or not (node.install_kvm or node.register_vmhost):
         return
 
+    NodeMetadata.objects.release_volatile(node)
+
     arch, _ = node.split_arch()
 
     if node.register_vmhost:
         cert = generate_certificate(Config.objects.get_config("maas_name"))
         cert_pem = cert.certificate_pem() + cert.private_key_pem()
-        NodeMetadata.objects.update_or_create(
+        NodeMetadata.objects.create(
             node=node,
             key=LXD_CERTIFICATE_METADATA_KEY,
-            defaults={"value": cert_pem},
+            value=cert_pem,
         )
         # write out the LXD cert on node to add it to the trust after setup
         maas_project = "maas"
@@ -230,10 +232,10 @@ def generate_kvm_pod_configuration(node):
 
     if node.install_kvm:
         password = _generate_password()
-        NodeMetadata.objects.update_or_create(
+        NodeMetadata.objects.create(
             node=node,
             key=VIRSH_PASSWORD_METADATA_KEY,
-            defaults={"value": password},
+            value=password,
         )
         # Make sure SSH password authentication is enabled.
         yield "ssh_pwauth", True