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
