Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[envoy][rbac] Rule prefix tags #17392

Conversation

thomasvnoort
Copy link
Contributor

@thomasvnoort thomasvnoort commented Apr 15, 2024

What does this PR do?

This PR adds support for the rule_stat_prefix option of Envoy's RBAC metrics with the rule_prefix tag, as implemented in envoyproxy/envoy#31835. This is not released yet, but will be included in an upcoming 1.30 Envoy release. Since this prefix is optional, we can add support for this here before the release is out.

This PR also fixes a missing tag shadow_rule_prefix on the shadow_allowed metric which was omitted in #16453. TIL the parser "inherits" tags across metrics of the same prefix, so it wasn't missing I was just misunderstanding the implementation of the legacy parser. Because of this, we also had to add a workaround since it couldn't parse different tags for the rule_prefix and shadow_rule_prefix; you had to pick one for all http.rbac metrics. This means we either had to use the same existing shadow_rule_prefix tag for normal metrics or make a breaking change to use rule_prefix for shadow metrics. Luckily, @steveny91 found a way to rewrite the tags after parsing to allow different prefix tags for normal and shadow metrics.

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Changelog entries must be created for modifications to shipped code
  • Add the qa/skip-qa label if the PR doesn't need to be tested during QA.
  • If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

Copy link

github-actions bot commented Apr 15, 2024

Test Results

94 tests   79 ✅  1m 7s ⏱️
 1 suites  13 💤
 1 files     2 ❌

For more details on these failures, see this check.

Results for commit 1031241.

♻️ This comment has been updated with latest results.

@thomasvnoort thomasvnoort force-pushed the thomas.vannoort/envoy-rbac-rule-prefix-tags branch from 493bca8 to 217dac7 Compare April 15, 2024 17:37
@thomasvnoort thomasvnoort marked this pull request as ready for review April 15, 2024 17:39
@thomasvnoort thomasvnoort requested a review from a team as a code owner April 15, 2024 17:39
@thomasvnoort thomasvnoort marked this pull request as draft April 15, 2024 17:45
@thomasvnoort thomasvnoort force-pushed the thomas.vannoort/envoy-rbac-rule-prefix-tags branch 6 times, most recently from 2ed09ec to f918d56 Compare April 16, 2024 12:00
@thomasvnoort thomasvnoort marked this pull request as ready for review April 16, 2024 12:06
@thomasvnoort thomasvnoort force-pushed the thomas.vannoort/envoy-rbac-rule-prefix-tags branch from f918d56 to 1031241 Compare April 16, 2024 12:15
Copy link
Contributor

@iliakur iliakur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to fix the CI failures, otherwise lgtm. Really appreciate you adding/updating the tests for this too!

envoy/tests/legacy/test_unit.py Show resolved Hide resolved
@thomasvnoort thomasvnoort requested a review from iliakur May 21, 2024 19:00
Copy link

codecov bot commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.18%. Comparing base (2a6ec26) to head (5c1ab1e).
Report is 99 commits behind head on master.

Additional details and impacted files
Flag Coverage Δ
active_directory ?
activemq_xml ?
aerospike ?
airflow ?
amazon_msk ?
ambari ?
arangodb ?
avi_vantage ?
azure_iot_edge ?
ceph ?
cisco_aci ?
citrix_hypervisor ?
clickhouse ?
cloudera ?
cockroachdb ?
couch ?
couchbase ?
crio ?
datadog_checks_base ?
datadog_checks_downloader ?
datadog_cluster_agent ?
dcgm ?
ddev ?
dns_check ?
dotnetclr ?
druid ?
ecs_fargate ?
elastic ?
envoy 95.18% <100.00%> (+0.12%) ⬆️
exchange_server ?
external_dns ?
gearmand ?
gitlab_runner ?
glusterfs ?
gunicorn ?
haproxy ?
harbor ?
http_check ?
ibm_db2 ?
ibm_was ?
iis ?
karpenter ?
kong ?
kube_apiserver_metrics ?
kube_controller_manager ?
kube_dns ?
kube_metrics_server ?
kube_proxy ?
kube_scheduler ?
kubernetes_state ?
kyototycoon ?
linux_proc_extras ?
mapr ?
marathon ?
marklogic ?
mcache ?
mesos_slave ?
nagios ?
network ?
nfsstat ?
nginx ?
nginx_ingress_controller ?
nvidia_triton ?
openmetrics ?
oracle ?
pdh_check ?
pgbouncer ?
php_fpm ?
postfix ?
postgres ?
process ?
prometheus ?
rabbitmq ?
ray ?
redisdb ?
riakcs ?
sap_hana ?
scylla ?
silk ?
snmp ?
snowflake ?
sonarqube ?
spark ?
sqlserver ?
ssh_check ?
statsd ?
strimzi ?
system_core ?
system_swap ?
tcp_check ?
teamcity ?
tekton ?
temporal ?
twemproxy ?
twistlock ?
varnish ?
vault ?
vertica ?
voltdb ?
vsphere ?
weaviate ?
win32_event_log ?
windows_performance_counters ?
zk ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@thomasvnoort thomasvnoort force-pushed the thomas.vannoort/envoy-rbac-rule-prefix-tags branch from 32b951e to b8beb8b Compare May 21, 2024 19:09
Copy link
Contributor

@iliakur iliakur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for the contribution!

@steveny91 steveny91 merged commit d5d280c into DataDog:master May 23, 2024
40 checks passed
@thomasvnoort thomasvnoort deleted the thomas.vannoort/envoy-rbac-rule-prefix-tags branch May 23, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants