Merge pull request #50 from phenixblue/issue-49-fix

Add logic to handle `:<port>` in key or value for swap map entries

CHANGELOG.md

@@ -104,10 +104,25 @@ This release fixes a bug in the image swap logic related to a scenario where a l
 
 More info can be found in this issue: #46
 
-### Enahncements
+### Enhancements
 
 - Add fix for dotted tag on library image (#47)
 
 ### Acknowledgements
 
 - Thanks to @adavenpo for bringing this to our attention
+
+## 1.4.3
+
+This release moves to using a new syntax (`::`) to separate the key and value portions of a map definition in the maps file. Backwards compatibility is maintained for the existing `:` syntax, but this has been deprecated and should not be used. Please update any existing map configurations to use the new syntax.
+
+This release also adds additional validation to catch errors associated with specifying a registry in a map definition key that includes the `:<port_number>` syntax. Previously this would result in an error and a stack trace. This is now handled gracefully and the new map separator syntax should allow for registries to include ports going forward.
+
+### Enhancements
+
+- Add support for new map definition deparator syntax (#50)
+
+### Acknowledgements
+
+- Thanks to @sblair-metrostar for bringing this to our attention
+

README.md

@@ -125,27 +125,31 @@ MAPS Mode enables a high degree of flexibility for the ImageSwap Webhook.
 
 In MAPS mode, the webhook reads from a `map file` that defines one or more mappings (key/value pairs) for imageswap logic. With the `map file` configuration, swap logic for multiple registries and patterns can be configured. In contrast, the LEGACY mode only allowed for a single `IMAGE_PREFIX` to be defined for image swaps.
 
-A `map file` is composed of key/value pairs separated by a `:` and looks like this:
+A `map file` is composed of key/value pairs separated by a `::` and looks like this:
 
 ```
-default:default.example.com
-docker.io:my.example.com/mirror-
-quay.io:quay.example3.com
-gitlab.com:registry.example.com/gitlab
-#gcr.io: # This is a comment 
-cool.io:
-registry.internal.twr.io:registry.example.com
-harbor.geo.pks.twr.io:harbor2.com ###### This is a comment with many symbols
-noswap_wildcards:twr.io, walrus.io
+default::default.example.com
+docker.io::my.example.com/mirror-
+quay.io::quay.example3.com
+gitlab.com::registry.example.com/gitlab
+#gcr.io:: # This is a comment 
+cool.io::
+registry.internal.twr.io::registry.example.com
+harbor.geo.pks.twr.io::harbor2.com ###### This is a comment with many symbols
+noswap_wildcards::twr.io, walrus.io
 ```
 
 NOTE: Lines in the `map file` that are commented out with a leading `#` are ignored. Trailing comments following a map definition are also ignored.
 
+NOTE: Previous versions of ImageSwap used a single `:` syntax to separate the key and value portions of a map definition. This syntax is deprecated as of v1.4.3 and will be removed in future versions. Please be sure to update any existing map file configurations to use the new syntax (ie. `::`).
+
+NOTE: Prior to v1.4.3 any use of a registry that includes a port for the key of a map definition will result in errors.
+
 The only mapping that is required in the `map_file` is the `default` map. The `default` map alone provides similar functionality to the `LEGACY` mode.
 
 A map definition that includes a `key` only can be used to disable image swapping for that particular registry.
 
-A map file can also include a special `noswap_wildcards` mapping that disables swapping based on greedy pattern matching. Don't actually include an `*` in this section. A value of `example` is essentialy equivalent to `*example*`. [See examples below for more detail](#example-maps-configs)
+A map file can also include a special `noswap_wildcards` mapping that disables swapping based on greedy pattern matching. Don't actually include an `*` in this section. A value of `example` is essentially equivalent to `*example*`. [See examples below for more detail](#example-maps-configs)
 
 By adding additional mappings to the `map file`, you can have much finer granularity to control swapping logic per registry.
 
@@ -156,30 +160,30 @@ By adding additional mappings to the `map file`, you can have much finer granula
 
   ```
   default:
-  gcr.io:harbor.internal.example.com
+  gcr.io::harbor.internal.example.com
   ```
 
 - Enable image swapping for all registries except `gcr.io`
 
   ```
-  default: harbor.internal.example.com
-  gcr.io:
+  default::harbor.internal.example.com
+  gcr.io::
   ```
 
 - Imitate LEGACY functionality as close as possible
 
   ```
-  default:harbor.internal.example.com
-  noswap_wildcards:harbor.internal.example.com
+  default::harbor.internal.example.com
+  noswap_wildcards::harbor.internal.example.com
   ```
 
   With this, all images will be swapped except those that already match the `harbor.internal.example.com` pattern
 
 - Enable swapping for all registries except those that match the `example.com` pattern
 
   ```
-  default:harbor.internal.example.com
-  noswap_wildcards:example.com
+  default::harbor.internal.example.com
+  noswap_wildcards::example.com
   ```
 
   With this, images that have any part of the registry that matches `example.com` will skip the swap logic
@@ -208,9 +212,9 @@ By adding additional mappings to the `map file`, you can have much finer granula
   [Official Docker documentation on image naming](https://docs.docker.com/registry/introduction/#understanding-image-naming)
 
   ```
-  default:
-  docker.io:
-  docker.io/library:harbor.example.com/library
+  default::
+  docker.io::
+  docker.io/library::harbor.example.com/library
   ```
 
   This map uses a special syntax of adding `/library` to a registry for the key in map file.

app/imageswap/imageswap.py

@@ -209,13 +209,32 @@ def build_swap_map(map_file):
             # Skip commented lines
             if line[0] == "#":
                 continue
-            (key, val) = line.split(":")
             # Trim trailing comments
-            if "#" in val:
-                val_trimmed = re.sub(r"(^.*[^#])(#.*$)", r"\1", val)
-                maps[key] = re.sub(r" ", "", val_trimmed.rstrip())
+            if "#" in line:
+                line = re.sub(r"(^.*[^#])(#.*$)", r"\1", line)
+            # Trim whitespace
+            line = re.sub(r" ", "", line.rstrip())
+            # Check for new style separator ("::") and verify the map splits correctly
+            if "::" in line and len(line.split("::")) == 2:
+                (key, val) = line.split("::")
+            # Check for old style separator (":") and verify the map splits correctly
+            elif ":" in line and len(line.split(":")) == 2:
+                app.logger.warning(
+                    f'Map defined with ":" as separator. This syntax is now deprecated. Please use "::" to separate the key and value in the map file: {line}'
+                )
+                (key, val) = line.split(":")
             else:
-                maps[key] = re.sub(r" ", "", val.rstrip())
+                # Check if map key contains a ":port" and that the new style separator ("::") is not used
+                if line.count(":") > 1 and "::" not in line:
+                    app.logger.warning(
+                        f'Invalid map is specified. A port in the map key or value requires using "::" as the separator. Skipping map for line: {line}'
+                    )
+                # Warn for any other invalid map syntax
+                else:
+                    app.logger.warning(f"Invalid map is specified. Incorrect syntax for map definition. Skipping map for line: {line}")
+                continue
+            # Store processed line key/value pair in map
+            maps[key] = val
 
     f.close()
 
@@ -246,11 +265,9 @@ def swap_image(container_spec):
         # Set docker.io if no registry is detected
         image_registry = "docker.io"
         no_registry = True
-    # Check if Registry portion includes a ":<port_number>"
-    if ":" in image_registry:
-        image_registry_noport = image_registry.partition(":")[0]
-    else:
-        image_registry_noport = image_registry
+
+    # Set the image registry key to work with
+    image_registry_key = image_registry
 
     # Check the imageswap mode
     if imageswap_mode.lower() == "maps":
@@ -259,6 +276,17 @@ def swap_image(container_spec):
 
         swap_maps = build_swap_map(imageswap_maps_file)
 
+        app.logger.debug(f"Swap Maps:\n{swap_maps}")
+
+        # Check if Registry portion includes a ":<port_number>"
+        if ":" in image_registry:
+            image_registry_noport = image_registry.partition(":")[0]
+        else:
+            image_registry_noport = image_registry
+
+        if image_registry not in swap_maps and image_registry_noport in swap_maps:
+            image_registry_key = image_registry_noport
+
         # Verify the default map exists or skip swap
         if imageswap_maps_default_key not in swap_maps:
             app.logger.warning(f'You don\'t have a "{imageswap_maps_default_key}" entry in your ImageSwap Map config, skipping swap')
@@ -268,8 +296,8 @@ def swap_image(container_spec):
         if imageswap_maps_wildcard_key in swap_maps and swap_maps[imageswap_maps_wildcard_key] != "":
             wildcard_maps = str(swap_maps[imageswap_maps_wildcard_key]).split(",")
 
-        # Check if bare registry/registry+library has a map specified
-        if image_registry_noport in swap_maps or image_registry_noport + "/library" in swap_maps:
+        # Check if registry or registry+library has a map specified
+        if image_registry_key in swap_maps or image_registry_key + "/library" in swap_maps:
 
             # Check for Library image (ie. empty strings for index 1 an 2 in image_split)
             if image_split[1] == "" and image_split[2] == "":
@@ -278,30 +306,30 @@ def swap_image(container_spec):
             else:
                 app.logger.debug("Image is not a Library image")
 
-            if library_image and image_registry_noport + "/library" in swap_maps:
+            if library_image and image_registry_key + "/library" in swap_maps:
 
-                image_registry_noport = image_registry_noport + "/library"
-                app.logger.info(f"Library Image detected and matching Map found: {image_registry_noport}")
+                image_registry_key = image_registry_key + "/library"
+                app.logger.info(f"Library Image detected and matching Map found: {image_registry_key}")
                 app.logger.debug("More info on Library Image: https://docs.docker.com/registry/introduction/#understanding-image-naming")
 
             # If the swap map has no value, swapping should be skipped
-            if swap_maps[image_registry_noport] == "":
-                app.logger.debug(f'Swap map for "{image_registry_noport}" has no value assigned, skipping swap')
+            if swap_maps[image_registry_key] == "":
+                app.logger.debug(f'Swap map for "{image_registry_key}" has no value assigned, skipping swap')
                 return False
             # If the image prefix ends with "-" just append existing image (minus any ":<port_number>")
-            elif swap_maps[image_registry_noport][-1] == "-":
+            elif swap_maps[image_registry_key][-1] == "-":
                 if no_registry:
-                    new_image = swap_maps[image_registry_noport] + image_registry_noport + "/" + re.sub(r":.*/", "/", image)
+                    new_image = swap_maps[image_registry_key] + image_registry_noport + "/" + re.sub(r":.*/", "/", image)
                 else:
-                    new_image = swap_maps[image_registry_noport] + re.sub(r":.*/", "/", image)
-            # If the image registry without a port pattern is found in the original image
-            elif image_registry_noport in image:
-                new_image = re.sub(image_registry_noport, swap_maps[image_registry_noport], image)
+                    new_image = swap_maps[image_registry_key] + re.sub(r":.*/", "/", image)
+            # If the image registry pattern is found in the original image
+            elif image_registry_key in image:
+                new_image = re.sub(image_registry_key, swap_maps[image_registry_key], image)
             # For everything else
             else:
-                new_image = swap_maps[image_registry_noport] + "/" + image
+                new_image = swap_maps[image_registry_key] + "/" + image
 
-            app.logger.debug(f'Swap Map = "{image_registry_noport}" : "{swap_maps[image_registry_noport]}"')
+            app.logger.debug(f'Swap Map = "{image_registry_key}" : "{swap_maps[image_registry_key]}"')
 
         # Check if any of the noswap wildcard patterns from the swap map exist within the original image
         elif len(wildcard_maps) > 0 and any(noswap in image for noswap in wildcard_maps):
@@ -311,15 +339,15 @@ def swap_image(container_spec):
         # Using Default image swap map
         else:
 
-            app.logger.debug(f'No Swap map for "{image_registry_noport}" detected, using default map')
+            app.logger.debug(f'No Swap map for "{image_registry_key}" detected, using default map')
             app.logger.debug(f'Swap Map = "default" : "{swap_maps[imageswap_maps_default_key]}"')
 
             if swap_maps[imageswap_maps_default_key] == "":
                 app.logger.debug(f"Default map has no value assigned, skipping swap")
                 return False
             elif swap_maps[imageswap_maps_default_key][-1] == "-":
                 new_image = swap_maps[imageswap_maps_default_key] + image_registry_noport + "/" + image
-            elif image_registry_noport in image:
+            elif image_registry_key in image:
                 new_image = re.sub(image_registry, swap_maps[imageswap_maps_default_key], image)
             else:
                 new_image = swap_maps[imageswap_maps_default_key] + "/" + image

app/imageswap/test/test_image_map_configs.py

@@ -54,6 +54,38 @@ def test_map_config_no_default(self):
         self.assertFalse(result)
         self.assertEqual(container_spec["image"], expected_image)
 
+    def test_map_config_with_port_in_key_and_old_separator(self):
+
+        """Method to test Map File config (map with port in key and old separator ":")"""
+
+        imageswap.imageswap_maps_file = "./testing/map_files/map_file.conf"
+
+        container_spec = {}
+        container_spec["name"] = "test-container"
+        container_spec["image"] = "registry2.bar.com/jmsearcy/twrtools:latest"
+
+        expected_image = "default.example.com/jmsearcy/twrtools:latest"
+        result = imageswap.swap_image(container_spec)
+
+        self.assertTrue(result)
+        self.assertEqual(container_spec["image"], expected_image)
+
+    def test_map_config_with_port_in_value_and_old_separator(self):
+
+        """Method to test Map File config (map with port in value and old separator ":")"""
+
+        imageswap.imageswap_maps_file = "./testing/map_files/map_file.conf"
+
+        container_spec = {}
+        container_spec["name"] = "test-container"
+        container_spec["image"] = "registry3.bar.com:8443/jmsearcy/twrtools:latest"
+
+        expected_image = "default.example.com/jmsearcy/twrtools:latest"
+        result = imageswap.swap_image(container_spec)
+
+        self.assertTrue(result)
+        self.assertEqual(container_spec["image"], expected_image)
+
 
 @patch("imageswap.imageswap_mode", "MAPS")
 @patch("imageswap.imageswap_maps_file", "./testing/map_files/map_file.conf")
@@ -214,6 +246,48 @@ def test_map_config_with_library_image_with_dotted_tag(self):
         self.assertTrue(result)
         self.assertEqual(container_spec["image"], expected_image)
 
+    def test_map_config_with_port_in_key(self):
+
+        """Method to test Map File config (map with port in key)"""
+
+        container_spec = {}
+        container_spec["name"] = "test-container"
+        container_spec["image"] = "registry.waldo.com:8443/jmsearcy/twrtools:latest"
+
+        expected_image = "registry.garply.com/jmsearcy/twrtools:latest"
+        result = imageswap.swap_image(container_spec)
+
+        self.assertTrue(result)
+        self.assertEqual(container_spec["image"], expected_image)
+
+    def test_map_config_with_port_in_value(self):
+
+        """Method to test Map File config (map with port in value)"""
+
+        container_spec = {}
+        container_spec["name"] = "test-container"
+        container_spec["image"] = "registry.foo.com/jmsearcy/twrtools:latest"
+
+        expected_image = "localhost:30003/foo/jmsearcy/twrtools:latest"
+        result = imageswap.swap_image(container_spec)
+
+        self.assertTrue(result)
+        self.assertEqual(container_spec["image"], expected_image)
+
+    def test_map_config_with_port_in_key_and_value(self):
+
+        """Method to test Map File config (map with port in key & value)"""
+
+        container_spec = {}
+        container_spec["name"] = "test-container"
+        container_spec["image"] = "registry.bar.com:8443/jmsearcy/twrtools:latest"
+
+        expected_image = "registry.baz.com:30003/bar/jmsearcy/twrtools:latest"
+        result = imageswap.swap_image(container_spec)
+
+        self.assertTrue(result)
+        self.assertEqual(container_spec["image"], expected_image)
+
 
 if __name__ == "__main__":
     unittest.main()

testing/map_files/map_file.conf

@@ -8,4 +8,10 @@ gitlab.com:registry.example.com/gitlab
 cool.io:
 registry.internal.twr.io:registry.example.com
 harbor.geo.pks.twr.io:harbor2.com ###### This is a comment with many symbols
+registry.waldo.com:8443::registry.garply.com # Swap map key that includes a port
+registry.foo.com::localhost:30003/foo # Swap map value that includes a port
+registry.bar.com:8443::registry.baz.com:30003/bar # Swap map key & value that include a port
+registry2.bar.com:registry2.baz.com:30003/bar # Bad config (port in value) without "::" separator
+registry3.bar.com:8443:registry.internal.baz.com:30003/bar # Bad config (port in key and value) without "::" separator
+registry4.bar.com;registry.internal.baz.com/bar # Bad config with invalid separator
 noswap_wildcards:twr.io, walrus.io