From db093004228b74d6768dda59cafbeef31b49baad Mon Sep 17 00:00:00 2001 From: Thomas van Noort Date: Mon, 15 Apr 2024 19:15:56 +0200 Subject: [PATCH 1/9] [envoy][rbac] Add shadow_rule_prefix tag to shadow_allowed metric --- envoy/datadog_checks/envoy/metrics.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/envoy/datadog_checks/envoy/metrics.py b/envoy/datadog_checks/envoy/metrics.py index 1eee9e9fb68b8..30fa861abba1f 100644 --- a/envoy/datadog_checks/envoy/metrics.py +++ b/envoy/datadog_checks/envoy/metrics.py @@ -3889,7 +3889,7 @@ 'http.rbac.shadow_allowed': { 'tags': ( ('stat_prefix',), - (), + ('shadow_rule_prefix',), (), ), 'method': 'monotonic_count', From 752e1d4905e984ff47f4550d457a97ae0b4e001b Mon Sep 17 00:00:00 2001 From: Thomas van Noort Date: Mon, 15 Apr 2024 19:17:37 +0200 Subject: [PATCH 2/9] [envoy][rbac] Add rule_prefix tag to denied and allowed metric --- envoy/datadog_checks/envoy/metrics.py | 4 ++-- .../fixtures/legacy/rbac_enforce_metrics.txt | 4 ++++ envoy/tests/fixtures/legacy/rbac_metric.txt | 8 -------- .../fixtures/legacy/rbac_shadow_metrics.txt | 4 ++++ envoy/tests/legacy/common.py | 7 ++++++- envoy/tests/legacy/test_unit.py | 18 +++++++++++++----- 6 files changed, 29 insertions(+), 16 deletions(-) create mode 100644 envoy/tests/fixtures/legacy/rbac_enforce_metrics.txt delete mode 100644 envoy/tests/fixtures/legacy/rbac_metric.txt create mode 100644 envoy/tests/fixtures/legacy/rbac_shadow_metrics.txt diff --git a/envoy/datadog_checks/envoy/metrics.py b/envoy/datadog_checks/envoy/metrics.py index 30fa861abba1f..4038f562f8a55 100644 --- a/envoy/datadog_checks/envoy/metrics.py +++ b/envoy/datadog_checks/envoy/metrics.py @@ -3873,7 +3873,7 @@ 'http.rbac.allowed': { 'tags': ( ('stat_prefix',), - (), + ('rule_prefix',), (), ), 'method': 'monotonic_count', @@ -3881,7 +3881,7 @@ 'http.rbac.denied': { 'tags': ( ('stat_prefix',), - (), + ('rule_prefix',), (), ), 'method': 'monotonic_count', diff --git a/envoy/tests/fixtures/legacy/rbac_enforce_metrics.txt b/envoy/tests/fixtures/legacy/rbac_enforce_metrics.txt new file mode 100644 index 0000000000000..9d091b3891cd3 --- /dev/null +++ b/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 diff --git a/envoy/tests/fixtures/legacy/rbac_metric.txt b/envoy/tests/fixtures/legacy/rbac_metric.txt deleted file mode 100644 index 66373dfe47575..0000000000000 --- a/envoy/tests/fixtures/legacy/rbac_metric.txt +++ /dev/null @@ -1,8 +0,0 @@ -http.foo_buz_112.rbac.allowed: 0 -http.foo_buz_112.rbac.denied: 1 -http.foo_buz_112.rbac.shadow_allowed: 2 -http.foo_buz_112.rbac.shadow_denied: 3 -http.foo_buz_112.rbac.shadow_rule_prefix.allowed: 4 -http.foo_buz_112.rbac.shadow_rule_prefix.denied: 5 -http.foo_buz_112.rbac.shadow_rule_prefix.shadow_allowed: 6 -http.foo_buz_112.rbac.shadow_rule_prefix.shadow_denied: 7 diff --git a/envoy/tests/fixtures/legacy/rbac_shadow_metrics.txt b/envoy/tests/fixtures/legacy/rbac_shadow_metrics.txt new file mode 100644 index 0000000000000..5bfa3eaf31303 --- /dev/null +++ b/envoy/tests/fixtures/legacy/rbac_shadow_metrics.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 diff --git a/envoy/tests/legacy/common.py b/envoy/tests/legacy/common.py index 65502ddd7f837..76bd23793539e 100644 --- a/envoy/tests/legacy/common.py +++ b/envoy/tests/legacy/common.py @@ -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 diff --git a/envoy/tests/legacy/test_unit.py b/envoy/tests/legacy/test_unit.py index cf6d7a84ec5e3..e7c2db8aa48c7 100644 --- a/envoy/tests/legacy/test_unit.py +++ b/envoy/tests/legacy/test_unit.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' @@ -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( From e6f4e03c37c669063e458858354769681828894d Mon Sep 17 00:00:00 2001 From: Thomas van Noort Date: Tue, 16 Apr 2024 14:00:06 +0200 Subject: [PATCH 3/9] [envoy][rbac] Clarify how optional tags are tested --- envoy/tests/legacy/test_unit.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/envoy/tests/legacy/test_unit.py b/envoy/tests/legacy/test_unit.py index e7c2db8aa48c7..0f76041d1a618 100644 --- a/envoy/tests/legacy/test_unit.py +++ b/envoy/tests/legacy/test_unit.py @@ -257,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', @@ -293,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): From 720445ffa5e599b3d6a9af5fbec8622a7f665998 Mon Sep 17 00:00:00 2001 From: Thomas van Noort Date: Mon, 15 Apr 2024 19:37:30 +0200 Subject: [PATCH 4/9] [envoy][rbac] Changelog --- envoy/changelog.d/17392.added | 1 + 1 file changed, 1 insertion(+) create mode 100644 envoy/changelog.d/17392.added diff --git a/envoy/changelog.d/17392.added b/envoy/changelog.d/17392.added new file mode 100644 index 0000000000000..cc76707fe635a --- /dev/null +++ b/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 From 726dc0c0ff3d0e0b0922518a4bad804258024a74 Mon Sep 17 00:00:00 2001 From: steveny91 Date: Tue, 21 May 2024 14:36:04 -0400 Subject: [PATCH 5/9] test tag rename function --- envoy/datadog_checks/envoy/envoy.py | 1 - envoy/datadog_checks/envoy/metrics.py | 11 ++++++++--- envoy/datadog_checks/envoy/parser.py | 6 +++++- envoy/tests/fixtures/legacy/rbac_shadow_metrics.txt | 2 +- 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/envoy/datadog_checks/envoy/envoy.py b/envoy/datadog_checks/envoy/envoy.py index e79b4178359dc..747432d4a5950 100644 --- a/envoy/datadog_checks/envoy/envoy.py +++ b/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) diff --git a/envoy/datadog_checks/envoy/metrics.py b/envoy/datadog_checks/envoy/metrics.py index 4038f562f8a55..c37ae318111b0 100644 --- a/envoy/datadog_checks/envoy/metrics.py +++ b/envoy/datadog_checks/envoy/metrics.py @@ -3881,7 +3881,7 @@ 'http.rbac.denied': { 'tags': ( ('stat_prefix',), - ('rule_prefix',), + (), (), ), 'method': 'monotonic_count', @@ -3889,7 +3889,7 @@ 'http.rbac.shadow_allowed': { 'tags': ( ('stat_prefix',), - ('shadow_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,10 @@ } # fmt: on +LEGACY_TAG_OVERWRITE = { + '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) diff --git a/envoy/datadog_checks/envoy/parser.py b/envoy/datadog_checks/envoy/parser.py index 105858a7d16da..f4983e9053721 100644 --- a/envoy/datadog_checks/envoy/parser.py +++ b/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,10 @@ def parse_metric(metric, retry=False, metric_mapping=METRIC_TREE, disable_legacy except ValueError: pass + if parsed_metric in LEGACY_TAG_OVERWRITE and LEGACY_TAG_OVERWRITE[parsed_metric][0] in tag_names: + index = tag_names.index(LEGACY_TAG_OVERWRITE[parsed_metric][0]) + tag_names[index] = LEGACY_TAG_OVERWRITE[parsed_metric][1] + 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'] diff --git a/envoy/tests/fixtures/legacy/rbac_shadow_metrics.txt b/envoy/tests/fixtures/legacy/rbac_shadow_metrics.txt index 5bfa3eaf31303..ecdc4d98ea546 100644 --- a/envoy/tests/fixtures/legacy/rbac_shadow_metrics.txt +++ b/envoy/tests/fixtures/legacy/rbac_shadow_metrics.txt @@ -1,4 +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 +http.foo_buz_shadow.rbac.shadow_rule_prefix.shadow_denied: 3 \ No newline at end of file From b8beb8bfb2ac119520c4642b5cb67123a837aeda Mon Sep 17 00:00:00 2001 From: steveny91 Date: Tue, 21 May 2024 14:50:47 -0400 Subject: [PATCH 6/9] comments --- envoy/datadog_checks/envoy/metrics.py | 2 ++ envoy/datadog_checks/envoy/parser.py | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/envoy/datadog_checks/envoy/metrics.py b/envoy/datadog_checks/envoy/metrics.py index c37ae318111b0..0ccc2922df84f 100644 --- a/envoy/datadog_checks/envoy/metrics.py +++ b/envoy/datadog_checks/envoy/metrics.py @@ -3953,6 +3953,8 @@ # 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'], } diff --git a/envoy/datadog_checks/envoy/parser.py b/envoy/datadog_checks/envoy/parser.py index f4983e9053721..8cc7427a46896 100644 --- a/envoy/datadog_checks/envoy/parser.py +++ b/envoy/datadog_checks/envoy/parser.py @@ -135,7 +135,8 @@ def parse_metric(metric, retry=False, metric_mapping=METRIC_TREE, disable_legacy tag_values.append(tag_values[pos]) except ValueError: pass - + + # Check to see if this metric has a tag that needs to be overwritten if parsed_metric in LEGACY_TAG_OVERWRITE and LEGACY_TAG_OVERWRITE[parsed_metric][0] in tag_names: index = tag_names.index(LEGACY_TAG_OVERWRITE[parsed_metric][0]) tag_names[index] = LEGACY_TAG_OVERWRITE[parsed_metric][1] From 5bdd08ff0e17b5917f86fbe0a3f99f23378b1c99 Mon Sep 17 00:00:00 2001 From: steveny91 Date: Tue, 21 May 2024 15:17:57 -0400 Subject: [PATCH 7/9] lint --- envoy/datadog_checks/envoy/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/envoy/datadog_checks/envoy/parser.py b/envoy/datadog_checks/envoy/parser.py index 8cc7427a46896..a96dd1522b85d 100644 --- a/envoy/datadog_checks/envoy/parser.py +++ b/envoy/datadog_checks/envoy/parser.py @@ -135,7 +135,7 @@ def parse_metric(metric, retry=False, metric_mapping=METRIC_TREE, disable_legacy tag_values.append(tag_values[pos]) except ValueError: pass - + # Check to see if this metric has a tag that needs to be overwritten if parsed_metric in LEGACY_TAG_OVERWRITE and LEGACY_TAG_OVERWRITE[parsed_metric][0] in tag_names: index = tag_names.index(LEGACY_TAG_OVERWRITE[parsed_metric][0]) From 8205a70a495f766517a78f9e014280ecbd83acdc Mon Sep 17 00:00:00 2001 From: steveny91 Date: Wed, 22 May 2024 01:06:54 -0400 Subject: [PATCH 8/9] better implementation --- envoy/datadog_checks/envoy/metrics.py | 9 +++++++-- envoy/datadog_checks/envoy/parser.py | 12 ++++++++---- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/envoy/datadog_checks/envoy/metrics.py b/envoy/datadog_checks/envoy/metrics.py index 0ccc2922df84f..94b376cf6bdca 100644 --- a/envoy/datadog_checks/envoy/metrics.py +++ b/envoy/datadog_checks/envoy/metrics.py @@ -3952,11 +3952,16 @@ } # 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'], + 'http.rbac.shadow_denied': { + 'rule_prefix': 'shadow_rule_prefix', + }, + 'http.rbac.shadow_allowed': { + 'rule_prefix': 'shadow_rule_prefix', + }, } MOD_METRICS = modify_metrics_dict(METRICS) diff --git a/envoy/datadog_checks/envoy/parser.py b/envoy/datadog_checks/envoy/parser.py index a96dd1522b85d..bff30a05ffc75 100644 --- a/envoy/datadog_checks/envoy/parser.py +++ b/envoy/datadog_checks/envoy/parser.py @@ -136,10 +136,14 @@ def parse_metric(metric, retry=False, metric_mapping=METRIC_TREE, disable_legacy except ValueError: pass - # Check to see if this metric has a tag that needs to be overwritten - if parsed_metric in LEGACY_TAG_OVERWRITE and LEGACY_TAG_OVERWRITE[parsed_metric][0] in tag_names: - index = tag_names.index(LEGACY_TAG_OVERWRITE[parsed_metric][0]) - tag_names[index] = LEGACY_TAG_OVERWRITE[parsed_metric][1] + # Check to see if parsed_metric is in the LEGACY_TAG_OVERWRITE dict and if it has a tag that needs to be + # overwritten. Iterate over the key-value pairs in the corresponding dictionary and if the key exists in + # tag_names, replace it with the value: + 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)] From 5c1ab1e309fec93ef783d6ac9d229bbc375573e8 Mon Sep 17 00:00:00 2001 From: steveny91 Date: Wed, 22 May 2024 01:30:28 -0400 Subject: [PATCH 9/9] better comment --- envoy/datadog_checks/envoy/parser.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/envoy/datadog_checks/envoy/parser.py b/envoy/datadog_checks/envoy/parser.py index bff30a05ffc75..24e745e6f39ce 100644 --- a/envoy/datadog_checks/envoy/parser.py +++ b/envoy/datadog_checks/envoy/parser.py @@ -136,9 +136,7 @@ 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 if it has a tag that needs to be - # overwritten. Iterate over the key-value pairs in the corresponding dictionary and if the key exists in - # tag_names, replace it with the value: + # 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: