sts-sponsors team mailing list archive

[Alberto Donato <mp+431804@xxxxxxxxxxxxxxxxxx>]

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.