Use role-based default allowlists for Strimzi Metrics Reporter by saksham869 · Pull Request #12966 · strimzi/strimzi-kafka-operator

saksham869 · 2026-07-17T21:02:06Z

Description

Currently, the Strimzi Metrics Reporter uses a single default allowlist for all Kafka nodes regardless of their role. This means broker-only metrics are sent to controller nodes and controller-only metrics are sent to broker nodes.

Changes

Split DEFAULT_METRICS_ALLOW_LIST into three role-specific lists:
- BROKER_DEFAULT_METRICS_ALLOW_LIST — broker-only metrics
- CONTROLLER_DEFAULT_METRICS_ALLOW_LIST — controller-only metrics
- MIXED_DEFAULT_METRICS_ALLOW_LIST — union of both (for mixed nodes)
Added metricsForPool(KafkaPool) helper in KafkaCluster to select the appropriate model per pool at config generation time
Added isCustomAllowList() to StrimziMetricsReporterModel to distinguish user-configured vs default allowlists
Added StrimziMetricsReporterModel(List<String>) constructor for direct default injection
Added test testStrimziMetricsReporterDefaultAllowListIsRoleBased verifying role-based defaults per pool

Type of change

New feature

Checklist

Write tests
Make sure all tests pass

snyk-io · 2026-07-17T21:02:14Z

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

scholzj

Thanks for the PR. I had a look and left some comments. However, I wonder if in general the correct way forward would be to:

In the StrimziMetricsReporterModel constructor, pass only the configuration and if the allow list is set, validate and store it. Otherwise, just store null
Change getAllowList to something like getAllowListOrDefault where you pass the default as the parameter. It would check if the allow list is not null and return it. If it is null, it would return the default.

That way, you do not to have multiple instances of the object with different defaults etc. And the impact on the other classes should be minimal as they anyway have the defaults in the constsnt.

PS: Ideally you should use the PR template we have for the PR description.

scholzj · 2026-07-19T19:37:37Z

            validate(config);
-            this.allowList = config.getValues() != null && config.getValues().getAllowList() != null
-                    ? config.getValues().getAllowList() : defaultAllowList;
+            boolean hasCustomList = config.getValues() != null && config.getValues().getAllowList() != null;


Can this be directly this.customAllowList?

scholzj · 2026-07-19T19:42:36Z

+    private static final List<String> MIXED_DEFAULT_METRICS_ALLOW_LIST = Stream.concat(
+            BROKER_DEFAULT_METRICS_ALLOW_LIST.stream(),
+            CONTROLLER_DEFAULT_METRICS_ALLOW_LIST.stream()
+    ).distinct().toList();


Why can't you just create a new list and add both lists to it? Or maybe even better, join them on demand when needed instead of keeping it as a constant?

scholzj · 2026-07-30T14:29:47Z

@saksham869 Any chance/plans to get back to this? Thanks.

saksham869 · 2026-07-30T21:12:34Z

@saksham869 Any chance/plans to get back to this? Thanks.

Hi @scholzj , yes — I'm actively working on the refactor based on your feedback. Implementing the getAllowListOrDefault(List<String>) approach and removing the secondary constructor. Will push the updated commit shortly.

scholzj · 2026-07-30T21:13:44Z

Great, thanks.

Split the single DEFAULT_METRICS_ALLOW_LIST into three role-specific lists: BROKER_DEFAULT_METRICS_ALLOW_LIST, CONTROLLER_DEFAULT_METRICS_ALLOW_LIST, and MIXED_DEFAULT_METRICS_ALLOW_LIST (union of both). When no custom allowlist is configured, each Kafka node pool now receives only the metrics relevant to its role. Broker-only nodes get broker metrics, controller-only nodes get controller metrics, and mixed nodes get the union of both. Adds isCustomAllowList() to StrimziMetricsReporterModel and a new StrimziMetricsReporterModel(List<String>) constructor for direct default injection. Adds metricsForPool(KafkaPool) helper in KafkaCluster to select the appropriate model per pool. Closes strimzi#12181 Signed-off-by: saksham869 <[email protected]>

scholzj

Thanks for the changes. I think they look good.

One thing I noticed through:

The default metrics are now always defined in a different class from where they are used. So, I think it would make sense to move them to the configuration builder classes and use them locally?
- It would save us the effort to pass them through the builder methods
- The KafkaBrokerConfigurationBuilder already has the node field that can be used to decide which metric to use.
It would also simplify the tesst as the broker/controler stuff will be now testable from the KafkaBrokerConfigrationBuilder tests directly without the need to test it in KafkaClusterTest.

What do you think?

scholzj · 2026-07-31T09:17:10Z

+        for (String s : CONTROLLER_DEFAULT_METRICS_ALLOW_LIST) {
+            if (!mixed.contains(s)) {
+                mixed.add(s);
+            }
+        }


I assume you do this to avoid duplicates? I wonder if we should use Sets instead of Lists for the defaults 🤔. But maybe that is something to fix separately.

saksham869 force-pushed the fix/metrics-reporter-role-based-allowlist branch from 04f0516 to c698ea9 Compare July 18, 2026 19:24

scholzj reviewed Jul 19, 2026

View reviewed changes

scholzj mentioned this pull request Jul 20, 2026

Split Strimzi Metrics Reporter defaults by Kafka node role #12970

Closed

saksham869 force-pushed the fix/metrics-reporter-role-based-allowlist branch from c698ea9 to 8a174ca Compare July 30, 2026 21:29

saksham869 force-pushed the fix/metrics-reporter-role-based-allowlist branch from 8a174ca to 8d089ae Compare July 30, 2026 22:15

scholzj reviewed Jul 31, 2026

View reviewed changes

Navigation Menu

Uh oh!

Use role-based default allowlists for Strimzi Metrics Reporter - #12966

Use role-based default allowlists for Strimzi Metrics Reporter#12966
saksham869 wants to merge 1 commit into
strimzi:mainfrom
saksham869:fix/metrics-reporter-role-based-allowlist

saksham869 commented Jul 17, 2026

Uh oh!

snyk-io Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

scholzj left a comment

Uh oh!

scholzj Jul 19, 2026

Uh oh!

scholzj Jul 19, 2026

Uh oh!

scholzj commented Jul 30, 2026

Uh oh!

saksham869 commented Jul 30, 2026

Uh oh!

scholzj commented Jul 30, 2026

Uh oh!

scholzj left a comment

Uh oh!

scholzj Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

saksham869 commented Jul 17, 2026

Description

Changes

Type of change

Checklist

Uh oh!

snyk-io Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Snyk checks have passed. No issues have been found so far.

Uh oh!

scholzj left a comment

Choose a reason for hiding this comment

Uh oh!

scholzj Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

scholzj Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

scholzj commented Jul 30, 2026

Uh oh!

saksham869 commented Jul 30, 2026

Uh oh!

scholzj commented Jul 30, 2026

Uh oh!

scholzj left a comment

Choose a reason for hiding this comment

Uh oh!

scholzj Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

snyk-io Bot commented Jul 17, 2026 •

edited

Loading