Skip to content

Fixes: 17292 - Detect infinite loop condition while iterating terminations in CablePath.from_origin #17293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions netbox/dcim/models/cables.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import itertools
import logging
from collections import defaultdict

from django.conf import settings
from django.contrib.contenttypes.fields import GenericForeignKey
from django.core.exceptions import ValidationError
from django.db import models
Expand All @@ -27,6 +29,8 @@
'CableTermination',
)

logger = logging.getLogger('netbox.dcim.cable')


trace_paths = Signal()

Expand Down Expand Up @@ -521,7 +525,7 @@ def segment_count(self):
return int(len(self.path) / 3)

@classmethod
def from_origin(cls, terminations):
def from_origin(cls, terminations, max_length=settings.CABLE_TRACE_MAX_LENGTH):
"""
Create a new CablePath instance as traced from the given termination objects. These can be any object to which a
Cable or WirelessLink connects (interfaces, console ports, circuit termination, etc.). All terminations must be
Expand Down Expand Up @@ -585,9 +589,13 @@ def from_origin(cls, terminations):
# Step 4: Record the links, keeping cables in order to allow for SVG rendering
cables = []
for link in links:
if object_to_path_node(link) not in cables:
cables.append(object_to_path_node(link))
cable = object_to_path_node(link)
if cable not in cables:
cables.append(cable)
path.append(cables)
if len(path) >= max_length:
logger.warning('Infinite loop detected while updating cable path trace')
break

# Step 5: Update the path status if a link is not connected
links_status = [link.status for link in links if link.status != LinkStatusChoices.STATUS_CONNECTED]
Expand Down
53 changes: 53 additions & 0 deletions netbox/dcim/tests/test_cablepaths.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
from django.test import TestCase
from django.contrib.contenttypes.models import ContentType

from circuits.models import *
from dcim.choices import LinkStatusChoices
from dcim.models import *
from dcim.svg import CableTraceSVG
from dcim.utils import object_to_path_node
from dcim.choices import CableEndChoices


class CablePathTestCase(TestCase):
Expand Down Expand Up @@ -2343,3 +2345,54 @@ def test_401_exclude_midspan_devices(self):
is_active=True
)
self.assertEqual(CablePath.objects.count(), 0)

def test_detect_infinite_loop(self):
Copy link
Member

@DanSheps DanSheps Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test suppose to fail or succeed? As it stands I don't see a loop in the topology unless I am missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's an infinite loop then the test will just keep executing. I'm not sure of an elegant way to test the infinite looping condition, but as long as it executes without the whole test process hanging then the test passes.

I've tried a few different approaches for making it assert if the execution takes too long or loops too many times, mostly involving timeouts and/or threading, but I don't know if it's worth going to that extreme. What would you suggest?

In any case if you comment out lines 600-602 in cables.py, the test will hang indefinitely.

Copy link
Member

@DanSheps DanSheps Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, for testing purposes we may want to add a parameter that sets a maximum path length (might not be bad anyways to set it to an unreasonable number) and still create a loop.

IDK if it is worth it though TBH. It is just this test doesn't really do anything as far as testing for a "loop" though., which was more my point. Might not be worth including a test even for this.

"""
Tests the ability to detect a non-resolving path and break out with a logged warning.
Assertion will fail if max_length is exceeded (meaning infinite loop detection failed), or not reached
(meaning this test is no longer valid as existing synthetic path no longer creates an infinite loop).
[IF1] --C1-- [FP1][Test Device][Rear Splice]
[FP2] --C2-- [ Rear Splice
"""
manufacturer = Manufacturer.objects.create(name='Infinite Loop Co', slug='infinite-loop-co')
role = DeviceRole.objects.create(name='Infinite Loop Device Role', slug='infinite-loop-device-role')
device_type = DeviceType.objects.create(manufacturer=manufacturer, model='Infinite Loop Device Type', slug='infinite-loop-device-type')
device = Device.objects.create(site=self.site, device_type=device_type, role=role, name='Infinite Loop Device')
interface = Interface.objects.create(device=device, name='Interface 1')

patch_panel_device_type = DeviceType.objects.create(
manufacturer=manufacturer,
front_port_template_count=48,
rear_port_template_count=48,
)
patch_panel = Device.objects.create(site=self.site, device_type=patch_panel_device_type, role=role)
rear_splice = RearPort.objects.create(device=patch_panel, positions=48, name='Rear Splice')
front_port_1 = FrontPort.objects.create(device=patch_panel, rear_port=rear_splice, rear_port_position=1, name='Front Port 1')
front_port_2 = FrontPort.objects.create(device=patch_panel, rear_port=rear_splice, rear_port_position=2, name='Front Port 2')

ct_interface = ContentType.objects.get(app_label='dcim', model='interface')
ct_frontport = ContentType.objects.get(app_label='dcim', model='frontport')
ct_rearport = ContentType.objects.get(app_label='dcim', model='rearport')

cable_1 = Cable.objects.create()
CableTermination.objects.create(cable=cable_1, cable_end='A', termination_type=ct_interface, termination_id=interface.id)
CableTermination.objects.create(cable=cable_1, cable_end='B', termination_type=ct_frontport, termination_id=front_port_1.id)

cable_2 = Cable.objects.create()
CableTermination.objects.create(cable=cable_2, cable_end='A', termination_type=ct_frontport, termination_id=front_port_2.id)
CableTermination.objects.create(cable=cable_2, cable_end='B', termination_type=ct_rearport, termination_id=rear_splice.id)

cable_1.save()

max_length = 50
a_terminations = []
b_terminations = []
for t in cable_1.terminations.all():
if t.cable_end == CableEndChoices.SIDE_A:
a_terminations.append(t.termination)
else:
b_terminations.append(t.termination)
cp = CablePath.from_origin(a_terminations, max_length=max_length)
self.assertEqual(len(cp.path), max_length)
cp = CablePath.from_origin(b_terminations, max_length=max_length)
self.assertLess(len(cp.path), max_length)
1 change: 1 addition & 0 deletions netbox/netbox/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
},
])
BASE_PATH = trailing_slash(getattr(configuration, 'BASE_PATH', ''))
CABLE_TRACE_MAX_LENGTH = getattr(configuration, 'CABLE_TRACE_MAX_LENGTH', 99999)
CHANGELOG_SKIP_EMPTY_CHANGES = getattr(configuration, 'CHANGELOG_SKIP_EMPTY_CHANGES', True)
CENSUS_REPORTING_ENABLED = getattr(configuration, 'CENSUS_REPORTING_ENABLED', True)
CORS_ORIGIN_ALLOW_ALL = getattr(configuration, 'CORS_ORIGIN_ALLOW_ALL', False)
Expand Down
Loading