nautobot / nautobot-app-ssot

Single Source of Truth for Nautobot
https://docs.nautobot.com/projects/ssot/en/latest/
Other
35 stars 33 forks source link

Handling of protect_on_delete objects with contrib #372

Open gneville-ot opened 6 months ago

gneville-ot commented 6 months ago

Environment

Expected Behavior

SSoT Contrib handles the correct ordering of deleting objects without an error

Observed Behavior

SSoT Contrib fails to complete a sync and errors with a message:

celery_worker-1  |   File "/opt/nautobot/.local/lib/python3.11/site-packages/nautobot_firewall_models/signals.py", line 104, in on_delete_handler
celery_worker-1  |     raise ValidationError(f"{instance} is assigned to an {i} & `protect_on_delete` is enabled.")
celery_worker-1  | django.core.exceptions.ValidationError: ['st0.1700 is assigned to an zones & `protect_on_delete` is enabled.']

This means that in the source you have to make two changes for it to be able to handle the error. Once to delete the interface from the firewall zone then run the SSoT sync, then to remove the interface and run a SSoT sync again. This may not always be possible.

Either that or override the method that objects are deleted in, which negates reasons to use Contrib.

Steps to Reproduce

  1. Have a nautobot instance running with the Firewall Modelling and SSoT Apps installed
  2. Create a device
  3. Add new Interface to the device
  4. Create Firewall Zone
  5. Add the previously created interface for the device to a firewall zone
  6. Run SSoT (sing contrib) where a source deletes the interface from a device as well as the zone on the target
Kircheneer commented 6 months ago

I am not sure whether there is a generalizable easy solution for this problem, I think the closest we can get is using something like this https://github.com/networktocode/diffsync/issues/97#issuecomment-1059082882, i.e. a custom diffsync Diff class that the user can then customize order of operations in.

This would be my proposal because I think there is no one true order that would work over all possibly conceivable models, especially when taking into account apps.

For your problem specifically, a solution could be:

class CustomDiff(Diff):
    def get_children(self):
        deferred_children = []

        def _process_models(model_order_subset):
            for model in model_order_subset:
                for child in self.children[model].values():
                    if child.action == DiffSyncActions.DELETE:
                        # Insert the deferred deletion actions in reverse order from the general model order as to resolve
                        # dependencies correctly.
                        deferred_children.insert(0, child)
                    else:
                        yield child

        # Process the remainder of the models in an order as to properly resolve their dependencies.
        yield from _process_models(
            [
                "interface",
                "firewall_zone",
            ]
        )

        yield from deferred_children

If you use this in your sync_from or sync_to call then it should properly resolve the order of operations.

gneville-ot commented 6 months ago

Thanks @Kircheneer , I'll give this a go.

I did have a thought that could abstract this for the user by having some kind of weight that is applied to all models? Something like a "create", "update", "delete" weight that is a default value, say 1000, then if a user wants to override they can increase or decrease the weights for each model.

Kircheneer commented 6 months ago

That sounds like a useful idea I think!