Collect Kafka ACL inventory in kafka_consumer - #24241
Conversation
Add ACL collection to the kafka_consumer cluster monitoring path. When enable_cluster_monitoring is on and the new collect_acls option is not disabled, the check enumerates all ACL bindings via AdminClient.describe_acls and emits: - kafka.acl.count: a low-cardinality gauge tagged by resource_type, pattern_type, acl_operation, and permission_type (no principal/host). - One data-streams-message config event per binding (config_type: acl) carrying the full binding detail, change-tracked through the existing event cache. Clusters without an authorizer (for example AWS MSK Serverless) raise a KafkaException from describe_acls; this is logged at debug and skipped so the rest of metadata collection is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
piochelepiotr
added
the
qa/required
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 5f47f5a | Docs | Datadog PR Page | Give us feedback! |
The local model generator strips the Datadog license header that CI's canonical generator preserves, causing "Run Validations / Validate" to report config_models out of sync. Restore the headers to match master. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Use collision-proof JSON serialization for the ACL change-tracking key so free-form resource_name/principal containing '|' cannot collide and silently drop a config event. - Broaden _collect_acls inner except to also debug-skip concurrent.futures.TimeoutError, avoiding error-level noise from a slow or unavailable authorizer. - Move ACL_CACHE_MAX_SIZE into the grouped cache-size constant block. - Tally ACL counts with collections.Counter. - Add a second-run change-tracking test asserting unchanged ACLs do not re-emit config events. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the kafka.acl.count gauge and its per-mechanism Counter tally; ACLs are now collected exclusively as data-streams config events. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ngle-pass emit, timeout test - Reword collect_acls spec.yaml description to events-only (kafka.acl.count metric was removed) and regenerate conf.yaml.example. - Stop shadowing the builtin TimeoutError in cluster_metadata.py; use concurrent.futures.TimeoutError. - Collapse _emit_acl_events double pass into a single pass computing each ACL key once. - Add test_collect_acls_skipped_when_describe_times_out covering the describe_acls future timeout skip path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-3 review suggestion: the except clause also catches a timed-out describe_acls future, so document that in the docstring alongside the authorizer-unavailable case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the collect_acls config option entirely. ACL collection now runs whenever enable_cluster_monitoring is enabled, alongside the rest of cluster metadata, instead of being gated behind a separate flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Validation ReportAll 21 validations passed. Show details
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f47f5a56b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| self.log.error("Error collecting schema registry info: %s", e) | ||
|
|
||
| try: | ||
| self._collect_acls(shared_metadata) |
There was a problem hiding this comment.
When cluster monitoring is enabled, this call runs on every check regardless of the collect_acls: false setting described for the feature; KafkaConfig never reads that field and the spec/example were not updated, so setting it is silently ignored. On clusters where ACL enumeration is sensitive/expensive or the check user should not make DescribeAcls requests, users have no way to disable the new AdminClient.describe_acls call without disabling all cluster monitoring.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This seems legitimate. Did you mean to implement this check?
A new boolean opt-out option collect_acls (default true) lets users disable ACL collection while keeping cluster monitoring enabled.
There was a problem hiding this comment.
I updated the PR description. I dropped the collect_acls flag completely. I want all the data to be collected via the enable_cluster_monitoring flag. The config should be as easy as possible
2 participants
What does this PR do?
Adds Kafka ACL inventory collection to the
kafka_consumerintegration under the existingenable_cluster_monitoringflag that already gates broker, topic, consumer group, and schema metadata. ACLs are collected on every check run whenever cluster monitoring is enabled — there is no separate opt-out, consistent with everything else collected under that flag.The check enumerates all ACL bindings through
AdminClient.describe_acls(an ANY/ANY match-all filter) and emits onedata-streams-messageconfiguration event per binding (config_type: acl) carrying the full detail: principal, host, resource type, resource name, pattern type, operation, and permission type. Events are change-tracked through the shared configuration-event cache — re-emitted when a binding changes and otherwise refreshed on the standard 1-hour TTL used for broker/topic/schema config events.Clusters without an authorizer (for example AWS MSK Serverless, or a security protocol with no ACL support) raise a
KafkaExceptionfromdescribe_acls(or time out); this is caught, logged at debug, and ACL collection is skipped gracefully so the rest of metadata collection is never affected.Motivation
ACL inventory gives Data Streams Monitoring users visibility into the authorization posture of their Kafka clusters alongside the broker, topic, consumer group, and schema metadata already collected under cluster monitoring.
Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged