CASMTRIAGE-6650 fix and test for 'IndexError: list index out of range…

…' with 'canu (#472)

validate shcd-cabling'

When running 'canu validate shcd-cabling' with an SHCD that does not
follow currently-supported naming conventions can cause this stacktrace:

```
  File "cli.py", line 314, in <module>
  File "click/core.py", line 1130, in __call__
  File "click/core.py", line 1055, in main
  File "click/core.py", line 1657, in invoke
  File "click/core.py", line 1657, in invoke
  File "click/core.py", line 1404, in invoke
  File "click/core.py", line 760, in invoke
  File "click/decorators.py", line 26, in new_func
  File "validate/shcd_cabling/shcd_cabling.py", line 286, in shcd_cabling
  File "validate/network/cabling/cabling.py", line 412, in node_model_from_canu
IndexError: list index out of range
[48583] Failed to execute script 'cli' due to unhandled exception!
```

canu is currently not setup to accurately test this situation since it
requires an SHCD and a functioning system with switches that can be
logged into.

To that end, I broke the problematic code into its own function so it
could be unit-tested.  I then created some unit tests with various
inputs to test against.  Both the line in question, and the one adjacent
suffer from a similiar problem: a lack of checking if an index exists
before trying to access it.  Thus, I changed the code to check these two
lines:

```
                    dst_middle = re.findall(r"(?:sw-)([a-z-]+)", dst_name)[0]
                    dst_digits = re.findall(r"(\d+)", dst_name)[0]
```

This is quite a small bug, but we often have issues with inconsistencies
in the content of cells in the SHCD.  Really, we should define the input
we expect for every cell, attempt to sanitize any input, and fail if it
does not meet specifications.

With all of the monolithic functions and complexity of this app, this is
just one step in that directions.  Many of our functions are cognitively
complex and do not unit test easily. We should also start adding
testdata fixtures to test the same data against the entire app.

even this fix, which breaks some functionality into a simple function
contains tech debt and while far from perfect, it is a step in the right
direction to begin decomposing these functions into more unit-testable
pieces.  once we can do complete unit tests and proper integration tests
with shellspec, we can easily reproduce customer issues and fix them
permanently.

unfortunately, csm has entered EOL (while still years away) so it is not
worth it at this point to do a complete refactor and enhance things, so
take this fix with that in mind

Signed-off-by: Jacob Salmela <[email protected]>

.github/CHANGELOG.md

@@ -2,6 +2,12 @@
 
 ## [UNRELEASED]
 
+
+## [1.9.2]
+
+- fixed a bug, which fails to parse an SHCD because of a poorly-named cell
+- this release halts the changelog file in favor of the github release notes associated with each tag.  manually keeping this log up-to-date is a chore and with csm entering EOL, we should focus on the fixes, not painful manual release notes
+
 ## [1.9.1]
 
 - remove canu user going forward. permissions are now default (root) 

canu/validate/network/cabling/cabling.py

@@ -298,6 +298,60 @@ def validate_cabling_port_data(lldp_info, warnings):
     return None
 
 
+class FormatSwitchDestinationNameError(Exception):
+    """Exception raised when the switch destination name cannot be formatted."""
+
+    def __init__(self, message="Unable to parse and format switch destination name. Should follow the format: 'sw-NAME-NNN'"):
+        # noqa: DAR101
+        """Initialize the exception."""
+        self.message = message
+        super().__init__(self.message)
+
+
+def format_switch_dst_name(dst_name):
+    # noqa: DAR101, DAR201, DAR401
+    """
+    Format the destination name.
+
+    Parameters:
+    - dst_name (str): The destination name to format.
+
+    Raises:
+    - FormatSwitchDestinationNameError (Exception): If the destination name cannot be formatted according to the expected pattern.
+
+      Returns:
+    - formatted_dst_name (str): The formatted destination name.
+    """
+    # TODO: the naming requirement is fairly arbitrary and should be documented or improved
+    # regex match for the following patterns:
+    #   1. a prefix ending with a letter (excluding g/G) followed by digits
+    #   2. a prefix that may include 'g' or 'G' as part of the middle section followed by digits
+    pattern = re.compile(r'(sw-)((?:.*[^gG\d]|.*[gG])?)(\d+)')
+    match = pattern.match(dst_name)
+    # if the name is close to what we need, try to format it
+    if match:
+        prefix = match.group(1).rstrip('-')
+        middle = match.group(2).rstrip('-').replace('-', '')
+        # some other commands like 'report switch firmware|cabling' and even
+        # certain variants of 'validate network cabling', where this code gets
+        # called there is some weird interdependencies here that are in the
+        # existing integration tests.  since I broke this out into its own
+        # function in an effort to start a better testing pattern, it has some
+        # unintended consequences.  that said, this could potentially go away
+        # once the other monolithinc functions are decomposed and the tests in
+        # general are more robust
+        if middle == "leafbmc":
+            middle = "leaf-bmc"
+        number = match.group(3)
+        # format the number to have three digits, padding with zeros if necessary
+        formatted_number = f"{int(number):03d}"
+        # combine the parts into the desired format
+        return f"{prefix}-{middle}-{formatted_number}"
+    # otherwise, return an error
+    else:
+        raise FormatSwitchDestinationNameError
+
+
 def node_model_from_canu(factory, canu_cache, ips):
     """Create a list of nodes from CANU cache.
 
@@ -407,12 +461,7 @@ def node_model_from_canu(factory, canu_cache, ips):
 
                 # If starts with 'sw-' then add an extra '-' before the number, and convert to 3 digit
                 if dst_name.startswith("sw-"):
-                    dst_start = "sw-"
-                    dst_middle = re.findall(r"(?:sw-)([a-z-]+)", dst_name)[0]
-                    dst_digits = re.findall(r"(\d+)", dst_name)[0]
-                    dst_name = (
-                        f"{dst_start}{dst_middle.rstrip('-')}-{int(dst_digits) :03d}"
-                    )
+                    dst_name = format_switch_dst_name(dst_name)
 
                 log.debug(f"Destination Data: {dst_name} {dst_slot} {dst_port}")
                 dst_node_name = dst_name

tests/test_validate_shcd_cabling_format_dst_name.py

@@ -0,0 +1,91 @@
+# MIT License
+#
+# (C) Copyright 2024 Hewlett Packard Enterprise Development LP
+#
+# Permission is hereby granted, free of charge, to any person obtaining a
+# copy of this software and associated documentation files (the "Software"),
+# to deal in the Software without restriction, including without limitation
+# the rights to use, copy, modify, merge, publish, distribute, sublicense,
+# and/or sell copies of the Software, and to permit persons to whom the
+# Software is furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included
+# in all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+# OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+# ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+# OTHER DEALINGS IN THE SOFTWARE.
+"""Test CANU validate shcd-cabling commands."""
+import unittest
+
+import canu.validate.network.cabling.cabling as cabling
+
+# Click commands do not expose their functions as attributes, so we need to
+# import directly and bypass the click command structure.  This is akin to a
+# regular unit test one might like to write.
+
+
+class TestValidateShcdCablingFormatDstName(unittest.TestCase):
+    """Unit tests for CANU validate shcd-cabling format_dst_name command."""
+
+    def test_format_switch_dst_name(self):
+        """Test the format_switch_dst_name function."""
+        # Test case 1: Name starts with "sw-" and has a string with multiple digits
+        dst_name = "sw-spine01"
+        result = cabling.format_switch_dst_name(dst_name)
+        expected_result = "sw-spine-001"
+        self.assertEqual(result, expected_result)
+
+        # Test case 2: Name starts with "sw-" has a string in the middle, followed by a dash and multiple digits
+        dst_name = "sw-spine-002"
+        result = cabling.format_switch_dst_name(dst_name)
+        expected_result = "sw-spine-002"
+        self.assertEqual(result, expected_result)
+
+        # Test case 3: Name starts with "sw-" has a string in the middle, followed by a two dashes and multiple digits
+        dst_name = "sw-leaf--bmc99"
+        result = cabling.format_switch_dst_name(dst_name)
+        expected_result = "sw-leaf-bmc-099"
+        self.assertEqual(result, expected_result)
+
+        # Test case 4: Name starts with "sw-" and has a string with multiple digits
+        dst_name = "sw-smn04"
+        result = cabling.format_switch_dst_name(dst_name)
+        expected_result = "sw-smn-004"
+        self.assertEqual(result, expected_result)
+
+        # Test case 5: Name starts with "sw-" and has a string with multiple digits but has a trailing dash
+        dst_name = "sw-25g06-"
+        result = cabling.format_switch_dst_name(dst_name)
+        expected_result = "sw-25g-006"
+        self.assertEqual(result, expected_result)
+
+        # Test case 6: Name starts with "sw-" and has a string that starts with digits but ends with all digits
+        dst_name = "sw-100g01"
+        result = cabling.format_switch_dst_name(dst_name)
+        expected_result = "sw-100g-001"
+        self.assertEqual(result, expected_result)
+
+        # Test case 7: Name does not start with "sw-" and has only non-digit characters
+        dst_name = "sw---=%$)"
+        with self.assertRaises(cabling.FormatSwitchDestinationNameError):
+            cabling.format_switch_dst_name(dst_name)
+
+        # Test case 11: Name starts with "sw-" but has no digits
+        dst_name = "sw-leaf"
+        with self.assertRaises(cabling.FormatSwitchDestinationNameError):
+            cabling.format_switch_dst_name(dst_name)
+
+        # Test case 12: Name starts with "sw-" and has leaf-bmc and multiple digits
+        dst_name = "sw-leaf-bmc99"
+        result = cabling.format_switch_dst_name(dst_name)
+        expected_result = "sw-leaf-bmc-099"
+        self.assertEqual(result, expected_result)
+
+
+if __name__ == "__main__":
+    unittest.main()