nice! just a few nits inline, and and an issue with the migration
Diff comments: > diff --git > a/src/maasserver/migrations/maasserver/0290_migrate_node_power_parameters.py > b/src/maasserver/migrations/maasserver/0290_migrate_node_power_parameters.py > new file mode 100644 > index 0000000..aec854e > --- /dev/null > +++ > b/src/maasserver/migrations/maasserver/0290_migrate_node_power_parameters.py > @@ -0,0 +1,63 @@ > +# Generated by Django 3.2.12 on 2022-11-15 15:12 > + > +from datetime import datetime > + > +from django.db import migrations > + > +from provisioningserver.drivers.power.registry import PowerDriverRegistry we can't depend on generic MAAS code inside migrations, as the migration would break if the related code changes. I think for this we could collect a dict of secrets by power_type, like: power_type_secrets = { "amt": [...], "ipmi": [...], ... } and update the loop in a similar way as suggested for sanitise_power_parameters() below to split params and secrets > + > + > +def move_secrets(apps, schema_editor): > + BMC = apps.get_model("maasserver", "BMC") > + Secret = apps.get_model("maasserver", "Secret") > + > + now = datetime.utcnow() > + > + bmc_secrets = {} > + > + for bmc_id, power_type, power_parameters in BMC.objects.values_list( > + "id", "power_type", "power_parameters" > + ): > + > + power_driver = PowerDriverRegistry.get_item(power_type) > + if not power_driver: > + continue > + > + parameters = power_parameters.copy() > + > + for power_parameter in power_parameters.keys(): > + setting = power_driver.get_setting(power_parameter) > + > + if not setting.get("secret"): > + continue > + > + field_name = setting.get("name") > + field_value = power_parameters.get(field_name) > + > + bmc_secrets[bmc_id] = { > + **bmc_secrets.get(bmc_id, {}), > + **{field_name: field_value}, > + } > + > + power_parameters.pop(field_name, None) > + > + BMC.objects.filter(id=bmc_id).update(power_parameters=parameters) > + > + Secret.objects.bulk_create( > + Secret( > + path=f"bmc/{bmc_id}/power-parameters", > + value=secret, > + created=now, > + updated=now, > + ) > + for bmc_id, secret in bmc_secrets.items() > + ) > + > + > +class Migration(migrations.Migration): > + > + dependencies = [ > + ("maasserver", "0289_vault_secret"), > + ] > + > + operations = [migrations.RunPython(move_secrets)] > diff --git a/src/maasserver/models/bmc.py b/src/maasserver/models/bmc.py > index aab469c..b902288 100644 > --- a/src/maasserver/models/bmc.py > +++ b/src/maasserver/models/bmc.py > @@ -333,6 +377,9 @@ class BMC(CleanSave, TimestampedModel): > def delete(self): > """Delete this BMC.""" > maaslog.info("%s: Deleting BMC", self) > + from maasserver.secrets import SecretManager > + > + SecretManager().delete_all_object_secrets(self) this also needs to be self.as_bmc() (in case it's called on a subclass) > super().delete() > > def save(self, *args, **kwargs): > @@ -356,10 +403,35 @@ class BMC(CleanSave, TimestampedModel): > else: > break > > + def get_power_parameters(self): > + from maasserver.secrets import SecretManager > + > + power_parameters = self.power_parameters or {} > + return { > + **power_parameters, > + **SecretManager().get_composite_secret( > + "power-parameters", obj=self.as_bmc(), default={} > + ), > + } > + > + def set_power_parameters(self, power_parameters): > + power_parameters, secrets = sanitise_power_parameters( > + self.power_type, power_parameters > + ) > + from maasserver.secrets import SecretManager the import can be moved inside the "if" > + > + if secrets: > + SecretManager().set_composite_secret( > + "power-parameters", secrets, obj=self.as_bmc() > + ) > + self.power_parameters = power_parameters > + > def clean(self): > """Update our ip_address if the address extracted from our power > parameters has changed.""" > - new_ip = BMC.extract_ip_address(self.power_type, > self.power_parameters) > + new_ip = BMC.extract_ip_address( > + self.power_type, self.get_power_parameters() > + ) > current_ip = None if self.ip_address is None else self.ip_address.ip > if new_ip != current_ip: > if not new_ip: > diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py > index ba2f710..b6a0590 100644 > --- a/src/maasserver/models/node.py > +++ b/src/maasserver/models/node.py > @@ -1436,23 +1439,47 @@ class Node(CleanSave, TimestampedModel): > def power_type(self): > return "" if self.bmc is None else self.bmc.power_type > > - @property > - def power_parameters(self): > + def get_instance_power_parameters(self): > + from maasserver.secrets import SecretManager > + > + power_parameters = self.instance_power_parameters or {} > + return { > + **power_parameters, > + **SecretManager().get_composite_secret( > + "power-parameters", obj=self.as_node(), default={} > + ), > + } > + > + def set_instance_power_parameters(self, power_parameters): > + power_parameters, secrets = sanitise_power_parameters( > + self.power_type, power_parameters > + ) > + from maasserver.secrets import SecretManager same here > + > + if secrets: > + SecretManager().set_composite_secret( > + "power-parameters", secrets, obj=self.as_node() > + ) > + self.instance_power_parameters = power_parameters > + > + def get_power_parameters(self): > # Overlay instance power parameters over bmc power parameters. > - instance_parameters = self.instance_power_parameters > + instance_parameters = self.get_instance_power_parameters() > if not instance_parameters: > instance_parameters = {} > bmc_parameters = {} > - if self.bmc and self.bmc.power_parameters: > - bmc_parameters = self.bmc.power_parameters > + if self.bmc and ( > + bmc_power_parameters := self.bmc.get_power_parameters() > + ): > + bmc_parameters = bmc_power_parameters > return {**bmc_parameters, **instance_parameters} > > @property > def instance_name(self): > """Return the name of the VM instance for this machine, or None.""" > - return self.instance_power_parameters.get( > + return self.get_instance_power_parameters().get( > "instance_name" > - ) or self.instance_power_parameters.get( # for LXD > + ) or self.get_instance_power_parameters().get( # for LXD > "power_id" > ) # for virsh unrelated to the change, but the comments placement is a bit weird, mind putting a single comment before the return with something like: # LXD uses "instance_name", virsh uses "power_id" > > diff --git a/src/maasserver/models/tests/test_bmc.py > b/src/maasserver/models/tests/test_bmc.py > index 183da8a..eb4b0b6 100644 > --- a/src/maasserver/models/tests/test_bmc.py > +++ b/src/maasserver/models/tests/test_bmc.py > @@ -374,6 +381,20 @@ class TestBMC(MAASServerTestCase): > bmc.delete() > self.assertEqual(0, StaticIPAddress.objects.filter(id=ip.id).count()) > > + def test_delete_deletes_bmc_secrets(self): > + bmc = factory.make_BMC() > + secret_manager = SecretManager() > + secret_manager.set_composite_secret( > + "power-parameters", {"foo": "bar"}, obj=bmc > + ) > + > + bmc.delete() > + self.assertIsNone( > + secret_manager.get_simple_secret( > + "power-parameters", obj=bmc, default=None > + ) > + ) > + can you please add a similar test for a Pod object? > def test_delete_bmc_spares_non_orphaned_ip_address(self): > machine, bmc, machine_ip = self.make_machine_and_bmc_with_shared_ip() > bmc.delete() > diff --git a/src/provisioningserver/drivers/power/registry.py > b/src/provisioningserver/drivers/power/registry.py > index 2cedfaf..4ffdae3 100644 > --- a/src/provisioningserver/drivers/power/registry.py > +++ b/src/provisioningserver/drivers/power/registry.py > @@ -82,3 +82,37 @@ for driver in power_drivers: > # Pod drivers are also power drivers. > for driver_name, driver in PodDriverRegistry: > PowerDriverRegistry.register_item(driver_name, driver) > + > + > +def sanitise_power_parameters(power_type, power_parameters): > + """Performs extraction of sensitive parameters and returns them > separately. > + Extraction relies on a `secret` flag of the power parameters property. > + > + :param power_type: BMC power driver type > + :param power_parameters: BMC power parameters > + """ > + power_driver = PowerDriverRegistry.get_item(power_type) > + > + if not power_driver: > + return power_parameters, {} > + > + parameters = power_parameters.copy() > + secrets = {} > + > + for power_parameter in power_parameters.keys(): > + setting = power_driver.get_setting(power_parameter) > + if not setting: > + continue > + if not setting.get("secret"): > + continue > + > + field_name = setting.get("name") > + field_value = power_parameters.get(field_name) > + > + secrets = { > + **secrets, > + **{field_name: field_value}, > + } > + parameters.pop(field_name, None) > + > + return parameters, secrets We should be able to get all secrets for the power driver and fill out the two dicts with something like this (untested): parameters = {} secrets = {} secret_params = set( setting["name"] for setting in power_driver.settings if setting.get("secret") ) for name, value in power_parameters.items(): if name in secret_params: secrets[name] = value else: parameters[name] = value return parameters, secrets -- https://code.launchpad.net/~troyanov/maas/+git/maas/+merge/431804 Your team MAAS Maintainers is requested to review the proposed merge of ~troyanov/maas:migrate-node-power-credentials into maas:master. -- Mailing list: https://launchpad.net/~sts-sponsors Post to : sts-sponsors@lists.launchpad.net Unsubscribe : https://launchpad.net/~sts-sponsors More help : https://help.launchpad.net/ListHelp