Invalid Handling of controller_managed_device_groups and Empty controller_group Causes Sync Errors in Nautobot SSOT

[TheHack42]

Environment

• Python version: 3.11.9
• Nautobot version: 2.3.2
• nautobot-ssot version: develop branch

Expected Behavior

The devices should be properly synchronized into Nautobot without exceptions. The controller_group should be set correctly, and the related ControllerManagedDeviceGroup should be retrieved based on the provided value or gracefully handled if empty.

Observed Behavior

There are two issues observed:

1. In the following code: https://github.com/nautobot/nautobot-app-ssot/blob/develop/nautobot_ssot/integrations/aci/diffsync/models/nautobot.py#L182

   controller_group=(
       self.job.apic.controller_managed_device_groups.name
       if self.job.apic.controller_managed_device_groups
       else ""
   )

   The variable self.job.apic.controller_managed_device_groups is a ManyRelation, not an instance of ControllerManagedDeviceGroup. As a result:

   • Accessing the name attribute on a ManyRelation return a None value.
   • The condition if self.job.apic.controller_managed_device_groups will always evaluate to true, which makes the current logic invalid.
   • Additionally, None is not allowed, and since the Pydantic validation requires a valid string, it throws an error: "Input should be a valid string".

2. In the following code: https://github.com/nautobot/nautobot-app-ssot/blob/develop/nautobot_ssot/integrations/aci/diffsync/adapters/aci.py#L438

   def create(cls, adapter, ids, attrs):
       """Create Device object in Nautobot."""
       _device = OrmDevice(
           name=ids["name"],
           role=Role.objects.get(name=attrs["device_role"]),
           device_type=OrmDeviceType.objects.get(model=attrs["device_type"]),
           serial=attrs["serial"],
           comments=attrs["comments"],
           controller_managed_device_group=ControllerManagedDeviceGroup.objects.get(name=attrs["controller_group"]),
           location=Location.objects.get(name=ids["site"], location_type=LocationType.objects.get(name="Site")),
           status=Status.objects.get(name="Active"),
       )

   The value of attrs["controller_group"] can be an empty string, and if so, the query for ControllerManagedDeviceGroup.objects.get(name=attrs["controller_group"]) will fail and raise a DoesNotExist exception.

Steps to Reproduce

1. Run the Nautobot SSOT synchronization job.
2. Ensure the synchronization involves a device with an empty or missing controller_group field.
3. The Pydantic validation error or DoesNotExist exception will occur.

Potential Fix

For the first issue:

• The controller_managed_device_groups being a ManyRelation should be handled accordingly, either by iterating through the related objects or adjusting the logic to handle the collection properly.

For the second issue:

• A check should be added to ensure attrs["controller_group"] is not empty or None before attempting to retrieve the ControllerManagedDeviceGroup. If it’s empty, either set a default value or skip the retrieval.

Thanks

[jdrew82]

@TheHack42 Thank you for opening this Issue. I've reviewed your comment and I think you might have some things flipped around. Specifically, it appears that the code examples you provide are flipped for the lines you cite. With that addressed, I wanted to respond to a few things:

• Prior to either adapters or CRUD operations happening there is a helper function that is called in the Job to ensure that there is a ControllerManagedDeviceGroup (CMDG) that exists and is attached to the specified APIC here. Due to this occurring there is an assumption that the specified APIC should have a defined CMDG attached to it.

• For point 1 you mention that controller_managed_device_groups is a ManyRelation. I wanted to correct you on that as you can see it's an FK from the ControllerManagedDeviceGroup class here. Because of the above point about expecting a CMDG exist on the APIC we should always get a resolved name for that object when loading the Device object. Also, it's explicitly written to return an empty string if for some reason there isn't a CMDG on the APIC.

• For point 2, again the expectation is that there should be a CMDG name defined when the adapter loads the Device. If for some reason there isn't one then it sounds like the helper function isn't working as expected?

[jdrew82]

@TheHack42 I think I see the ManyRelation now. That's a bit odd as the code does show it should be an FK. I'll work on getting a fix in for that.

[TheHack42]

Hello,

First of all, thank you @jdrew82 for your interest in my issue. Indeed, I mistakenly inverted the link citing the line of code in the examples I provided. My apologies for that.

• The helper function is doing its job correctly and properly verifies that a CMDG is attached to the apic (Controller model). There's no issue with that.
• controller_managed_device_groups is indeed a ManyRelation present in the Controller model. The apic is represented by an instance of Controller. The ControllerManagedDeviceGroup model indeed has a foreign key to Controller with the related_name set to "controller_managed_device_groups". Naturally, the Controller model has an attribute controller_managed_device_groups, which is a ManyRelation to retrieve the associated CMDGs. The line self.job.apic.controller_managed_device_groups returns a ManyRelation that cannot have a name attribute. Additionally, it is illogical to have a condition that checks if self.job.apic.controller_managed_device_groups returns "True", knowing that its type is a ManyRelation. https://github.com/nautobot/nautobot-app-ssot/blob/develop/nautobot_ssot/integrations/aci/diffsync/adapters/aci.py#L438
• If we place an empty string in controller_group, the line ControllerManagedDeviceGroup.objects.get(name=attrs["controller_group"]) will inevitably raise a DoesNotExist exception because no CMDG has an empty name. https://github.com/nautobot/nautobot-app-ssot/blob/develop/nautobot_ssot/integrations/aci/diffsync/models/nautobot.py#L182

I hope my explanations are clear.

Thanks in advance.

[jdrew82]

@TheHack42 Thanks for the response. I've actually made the correction for the ManyField in #574 so the fix should be in the next release we cut. As for your second point, we should never actually have an empty string in that field though from the way the integration is designed.

[jdrew82]

@TheHack42 This should be resolved now with 3.2.0. Please let me know if you're seeing instances where the CMDG get happens with a blank string. That shouldn't be possible with how things are structured but maybe there's something that's been missed.