[envoy][rbac] Rule prefix tags (#17392)

* [envoy][rbac] Add shadow_rule_prefix tag to shadow_allowed metric

* [envoy][rbac] Add rule_prefix tag to denied and allowed metric

* [envoy][rbac] Clarify how optional tags are tested

* [envoy][rbac] Changelog

* test tag rename function

* comments

* lint

* better implementation

* better comment

---------

Co-authored-by: steveny91 <steven.yuen@datadoghq.com>

Author: thomasvnoort

envoy/changelog.d/17392.added

@@ -0,0 +1 @@
+Add shadow_rule_prefix tag for shadow_allowed RBAC metric and rule_prefix tag for allowed and denied RBAC metric

envoy/datadog_checks/envoy/envoy.py

@@ -149,7 +149,6 @@ def check(self, _):
                 continue
 
             tags.extend(self.custom_tags)
-
             try:
                 value = int(value)
                 get_method(self, method)(metric, value, tags=tags)

envoy/datadog_checks/envoy/metrics.py

@@ -3873,7 +3873,7 @@
     'http.rbac.allowed': {
         'tags': (
             ('stat_prefix',),
-            (),
+            ('rule_prefix',),
             (),
         ),
         'method': 'monotonic_count',
@@ -3897,7 +3897,7 @@
     'http.rbac.shadow_denied': {
         'tags': (
             ('stat_prefix',),
-            ('shadow_rule_prefix',),
+            (),
             (),
         ),
         'method': 'monotonic_count',
@@ -3952,5 +3952,17 @@
 }
 # fmt: on
 
+
+LEGACY_TAG_OVERWRITE = {
+    # The legacy approach gave very little ability for modifications to be done to tags.
+    # This dict allows us to fine tune and replace tags as needed.
+    'http.rbac.shadow_denied': {
+        'rule_prefix': 'shadow_rule_prefix',
+    },
+    'http.rbac.shadow_allowed': {
+        'rule_prefix': 'shadow_rule_prefix',
+    },
+}
+
 MOD_METRICS = modify_metrics_dict(METRICS)
 METRIC_TREE = make_metric_tree(METRICS)

envoy/datadog_checks/envoy/parser.py

@@ -9,7 +9,7 @@
 from six.moves import range, zip
 
 from .errors import UnknownMetric, UnknownTags
-from .metrics import METRIC_PREFIX, METRIC_TREE, MOD_METRICS
+from .metrics import LEGACY_TAG_OVERWRITE, METRIC_PREFIX, METRIC_TREE, MOD_METRICS
 
 HISTOGRAM = re.compile(r'([P0-9.]+)\(([^,]+)')
 PERCENTILE_SUFFIX = {
@@ -136,6 +136,13 @@ def parse_metric(metric, retry=False, metric_mapping=METRIC_TREE, disable_legacy
             except ValueError:
                 pass
 
+    # Check to see if parsed_metric is in the LEGACY_TAG_OVERWRITE dict and replace the tag name if it is
+    if parsed_metric in LEGACY_TAG_OVERWRITE:
+        for k, v in LEGACY_TAG_OVERWRITE[parsed_metric].items():
+            if k in tag_names:
+                idx = tag_names.index(k)
+                tag_names[idx] = v
+
     tags = ['{}:{}'.format(tag_name, tag_value) for tag_name, tag_value in zip(tag_names, tag_values)]
 
     return METRIC_PREFIX + parsed_metric, tags, MOD_METRICS[parsed_metric]['method']

envoy/tests/fixtures/legacy/rbac_enforce_metrics.txt

@@ -0,0 +1,4 @@
+http.foo_buz_enforce.rbac.allowed: 0
+http.foo_buz_enforce.rbac.denied: 1
+http.foo_buz_enforce.rbac.rule_prefix.allowed: 2
+http.foo_buz_enforce.rbac.rule_prefix.denied: 3

envoy/tests/fixtures/legacy/rbac_metric.txt

@@ -0,0 +1,4 @@
+http.foo_buz_shadow.rbac.shadow_allowed: 0
+http.foo_buz_shadow.rbac.shadow_denied: 1
+http.foo_buz_shadow.rbac.shadow_rule_prefix.shadow_allowed: 2
+http.foo_buz_shadow.rbac.shadow_rule_prefix.shadow_denied: 3

envoy/tests/fixtures/legacy/rbac_shadow_metrics.txt

@@ -61,9 +61,14 @@
 
 CONNECTION_LIMIT_STAT_PREFIX_TAG = ['stat_prefix:ingress_http']
 
-RBAC_METRICS = [
+RBAC_ENFORCE_METRICS = [
     "envoy.http.rbac.allowed",
     "envoy.http.rbac.denied",
+]
+
+RBAC_SHADOW_METRICS = [
     "envoy.http.rbac.shadow_allowed",
     "envoy.http.rbac.shadow_denied",
 ]
+
+RBAC_METRICS = RBAC_ENFORCE_METRICS + RBAC_SHADOW_METRICS

envoy/tests/legacy/common.py

@@ -21,7 +21,8 @@
     INSTANCES,
     LOCAL_RATE_LIMIT_METRICS,
     RATE_LIMIT_STAT_PREFIX_TAG,
-    RBAC_METRICS,
+    RBAC_ENFORCE_METRICS,
+    RBAC_SHADOW_METRICS,
 )
 
 CHECK_NAME = 'envoy'
@@ -256,7 +257,7 @@ def test_metadata_not_collected(datadog_agent, check):
 
 
 @pytest.mark.parametrize(
-    ('fixture_file', 'metrics', 'standard_tags', 'additional_tags'),
+    ('fixture_file', 'metrics', 'standard_tags', 'optional_tags'),
     [
         (
             './legacy/stat_prefix',
@@ -265,15 +266,22 @@ def test_metadata_not_collected(datadog_agent, check):
             ['stat_prefix:bar'],
         ),
         (
-            './legacy/rbac_metric.txt',
-            RBAC_METRICS,
-            ['stat_prefix:foo_buz_112'],
+            './legacy/rbac_enforce_metrics.txt',
+            RBAC_ENFORCE_METRICS,
+            ['stat_prefix:foo_buz_enforce'],
+            ['rule_prefix:rule_prefix'],
+        ),
+        (
+            './legacy/rbac_shadow_metrics.txt',
+            RBAC_SHADOW_METRICS,
+            ['stat_prefix:foo_buz_shadow'],
             ['shadow_rule_prefix:shadow_rule_prefix'],
         ),
     ],
     ids=[
         "stats_prefix_ext_auth",
-        "rbac_prefix_shadow",
+        "rbac_enforce_metrics",
+        "rbac_shadow_metrics",
     ],
 )
 def test_stats_prefix_optional_tags(
@@ -285,25 +293,26 @@ def test_stats_prefix_optional_tags(
     fixture_file,
     metrics,
     standard_tags,
-    additional_tags,
+    optional_tags,
 ):
     instance = INSTANCES['main']
     standard_tags.append('endpoint:{}'.format(instance["stats_url"]))
-    tags_prefix = standard_tags + additional_tags
     c = check(instance)
     mock_http_response(file_path=fixture_path(fixture_file))
     dd_run_check(c)
 
-    # To ensure that this change didn't break the old behavior, both the value and the tags are asserted.
-    # The fixture is created with a specific value and the EXT_METRICS list is done in alphabetical order
-    # allowing for value to also be asserted
+    # To test the absence and presence of the optional tags, both the value and the tags are asserted.
+    # The fixtures must list all metrics, first without and then with the optional tags.
     for index, metric in enumerate(metrics):
+        # Without optional tags.
+        aggregator.assert_metric(metric, value=index, tags=standard_tags)
+
+        # With optional tags.
         aggregator.assert_metric(
             metric,
             value=index + len(metrics),
-            tags=tags_prefix,
+            tags=standard_tags + optional_tags,
         )
-        aggregator.assert_metric(metric, value=index, tags=standard_tags)
 
 
 def test_local_rate_limit_metrics(aggregator, fixture_path, mock_http_response, check, dd_run_check):