fix(internal): support URLs in DD_TAGS variable (#4183) (#4228)

mergify[bot] · P403n1x87 · web-flow · commit 6ef35fb3ade8 · 2022-09-22T12:17:15.000-04:00
This change allows for URLs to be passed as values to tag labels via the DD_TAGS environment variable. NOTE This might constitute a slight breaking change, as configuration that was valid before might no longer work with the new logic. (cherry picked from commit e315198) Co-authored-by: Gabriele N. Tornetta <P403n1x87@users.noreply.github.com>
diff --git a/ddtrace/internal/utils/formats.py b/ddtrace/internal/utils/formats.py
@@ -1,10 +1,10 @@
 import logging
-import re
 from typing import Any
 from typing import Dict
 from typing import List
 from typing import Optional
 from typing import Text
+from typing import Tuple
 from typing import TypeVar
 from typing import Union
 
@@ -22,9 +22,6 @@
 
 T = TypeVar("T")
 
-# Tags `key:value` must be separated by either comma or space
-_TAGS_NOT_SEPARATED = re.compile(r":[^,\s]+:")
-
 log = logging.getLogger(__name__)
 
 
@@ -79,21 +76,43 @@ def parse_tags_str(tags_str):
     :param tags_str: A string of the above form to parse tags from.
     :return: A dict containing the tags that were parsed.
     """
-    parsed_tags = {}  # type: Dict[str, str]
     if not tags_str:
-        return parsed_tags
+        return {}
 
-    if _TAGS_NOT_SEPARATED.search(tags_str):
-        log.error("Malformed tag string with tags not separated by comma or space '%s'.", tags_str)
-        return parsed_tags
+    TAGSEP = ", "
 
-    # Identify separator based on which successfully identifies the correct
-    # number of valid tags
-    numtagseps = tags_str.count(":")
-    for sep in [",", " "]:
-        if sum(":" in _ for _ in tags_str.split(sep)) == numtagseps:
-            break
-    else:
+    def parse_tags(tags):
+        # type: (List[str]) -> Tuple[List[Tuple[str, str]], List[str]]
+        parsed_tags = []
+        invalids = []
+
+        for tag in tags:
+            key, sep, value = tag.partition(":")
+            if not sep or not key or "," in key:
+                invalids.append(tag)
+            else:
+                parsed_tags.append((key, value))
+
+        return parsed_tags, invalids
+
+    tags_str = tags_str.strip(TAGSEP)
+
+    # Take the maximal set of tags that can be parsed correctly for a given separator
+    tag_list = []  # type: List[Tuple[str, str]]
+    invalids = []
+    for sep in TAGSEP:
+        ts = tags_str.split(sep)
+        tags, invs = parse_tags(ts)
+        if len(tags) > len(tag_list):
+            tag_list = tags
+            invalids = invs
+        elif len(tags) == len(tag_list) > 1:
+            # Both separators produce the same number of tags.
+            # DEV: This only works when checking between two separators.
+            tag_list[:] = []
+            invalids[:] = []
+
+    if not tag_list:
         log.error(
             (
                 "Failed to find separator for tag string: '%s'.\n"
@@ -103,25 +122,11 @@ def parse_tags_str(tags_str):
             ),
             tags_str,
         )
-        return parsed_tags
 
-    for tag in tags_str.split(sep):
-        try:
-            key, value = tag.split(":", 1)
-
-            # Validate the tag
-            if key == "" or value == "" or value.endswith(":"):
-                raise ValueError
-        except ValueError:
-            log.error(
-                "Malformed tag in tag pair '%s' from tag string '%s'.",
-                tag,
-                tags_str,
-            )
-        else:
-            parsed_tags[key] = value
-
-    return parsed_tags
+    for tag in invalids:
+        log.error("Malformed tag in tag pair '%s' from tag string '%s'.", tag, tags_str)
+
+    return dict(tag_list)
 
 
 def stringify_cache_args(args):
diff --git a/releasenotes/notes/fix-dd-tags-0763e940d889b8da.yaml b/releasenotes/notes/fix-dd-tags-0763e940d889b8da.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fix parsing of the ``DD_TAGS`` environment variable value to include
+    support for values with colons (e.g. URLs). Also fixed the parsing of
+    invalid tags that begin with a space (e.g. ``DD_TAGS=" key:val"`` will now
+    produce a tag with label ``key``, instead of `` key``, and value ``val``).
diff --git a/tests/tracer/test_utils.py b/tests/tracer/test_utils.py
@@ -52,89 +52,44 @@ def test_asbool(self):
         ("key: val", dict(key=" val"), None),
         ("key key: val", {"key key": " val"}, None),
         ("key: val,key2:val2", dict(key=" val", key2="val2"), None),
-        (" key: val,key2:val2", {" key": " val", "key2": "val2"}, None),
+        (" key: val,key2:val2", {"key": " val", "key2": "val2"}, None),
         ("key key2:val1", {"key key2": "val1"}, None),
-        (
-            "key:val key2:val:2",
-            dict(),
-            [mock.call(_LOG_ERROR_MALFORMED_TAG_STRING, "key:val key2:val:2")],
-        ),
+        ("key:val key2:val:2", {"key": "val", "key2": "val:2"}, None),
         (
             "key:val,key2:val2 key3:1234.23",
             dict(),
             [mock.call(_LOG_ERROR_FAIL_SEPARATOR, "key:val,key2:val2 key3:1234.23")],
         ),
-        (
-            "key:val key2:val2 key3: ",
-            dict(key="val", key2="val2"),
-            [
-                mock.call(_LOG_ERROR_MALFORMED_TAG, "key3:", "key:val key2:val2 key3: "),
-                mock.call(_LOG_ERROR_MALFORMED_TAG, "", "key:val key2:val2 key3: "),
-            ],
-        ),
+        ("key:val key2:val2 key3: ", dict(key="val", key2="val2", key3=""), None),
         (
             "key:val key2:val 2",
             dict(key="val", key2="val"),
             [mock.call(_LOG_ERROR_MALFORMED_TAG, "2", "key:val key2:val 2")],
         ),
         (
             "key: val key2:val2 key3:val3",
-            {"key2": "val2", "key3": "val3"},
-            [
-                mock.call(_LOG_ERROR_MALFORMED_TAG, "key:", "key: val key2:val2 key3:val3"),
-                mock.call(_LOG_ERROR_MALFORMED_TAG, "val", "key: val key2:val2 key3:val3"),
-            ],
-        ),
-        (
-            "key:,key3:val1,",
-            dict(key3="val1"),
-            [
-                mock.call(_LOG_ERROR_MALFORMED_TAG, "key:", "key:,key3:val1,"),
-                mock.call(_LOG_ERROR_MALFORMED_TAG, "", "key:,key3:val1,"),
-            ],
-        ),
-        (
-            ",",
-            dict(),
-            [
-                mock.call(_LOG_ERROR_MALFORMED_TAG, "", ","),
-                mock.call(_LOG_ERROR_MALFORMED_TAG, "", ","),
-            ],
-        ),
-        (
-            ":,:",
-            dict(),
-            [
-                mock.call(_LOG_ERROR_MALFORMED_TAG, ":", ":,:"),
-                mock.call(_LOG_ERROR_MALFORMED_TAG, ":", ":,:"),
-            ],
-        ),
-        (
-            "key,key2:val1",
-            dict(key2="val1"),
-            [mock.call(_LOG_ERROR_MALFORMED_TAG, "key", "key,key2:val1")],
-        ),
-        ("key2:val1:", dict(), [mock.call(_LOG_ERROR_MALFORMED_TAG_STRING, "key2:val1:")]),
-        (
-            "key,key2,key3",
-            dict(),
-            [
-                mock.call(_LOG_ERROR_MALFORMED_TAG, "key", "key,key2,key3"),
-                mock.call(_LOG_ERROR_MALFORMED_TAG, "key2", "key,key2,key3"),
-                mock.call(_LOG_ERROR_MALFORMED_TAG, "key3", "key,key2,key3"),
-            ],
+            {"key": "", "key2": "val2", "key3": "val3"},
+            [mock.call(_LOG_ERROR_MALFORMED_TAG, "val", "key: val key2:val2 key3:val3")],
         ),
+        ("key:,key3:val1,", dict(key3="val1", key=""), None),
+        (",", dict(), [mock.call(_LOG_ERROR_FAIL_SEPARATOR, "")]),
+        (":,:", dict(), [mock.call(_LOG_ERROR_FAIL_SEPARATOR, ":,:")]),
+        ("key,key2:val1", {"key2": "val1"}, [mock.call(_LOG_ERROR_MALFORMED_TAG, "key", "key,key2:val1")]),
+        ("key2:val1:", {"key2": "val1:"}, None),
+        ("key,key2,key3", dict(), [mock.call(_LOG_ERROR_FAIL_SEPARATOR, "key,key2,key3")]),
+        ("foo:bar,foo:baz", dict(foo="baz"), None),
+        ("hash:asd url:https://github.com/foo/bar", dict(hash="asd", url="https://github.com/foo/bar"), None),
     ],
 )
 def test_parse_env_tags(tag_str, expected_tags, error_calls):
     with mock.patch("ddtrace.internal.utils.formats.log") as log:
         tags = parse_tags_str(tag_str)
         assert tags == expected_tags
         if error_calls:
-            assert log.error.call_count == len(error_calls)
+            assert log.error.call_count == len(error_calls), log.error.call_args_list
             log.error.assert_has_calls(error_calls)
         else:
-            assert log.error.call_count == 0
+            assert log.error.call_count == 0, log.error.call_args_list
 
 
 def test_no_states():
