Skip to content

Commit c698ea9

Browse files
saksham869saksham869
committed
Use role-based default allowlists for Strimzi Metrics Reporter
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 #12181 Signed-off-by: saksham869 <mishrasatyam3456@gmail.com>
1 parent ffa8bb6 commit c698ea9

3 files changed

Lines changed: 162 additions & 16 deletions

File tree

‎cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java‎

Lines changed: 71 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@
107107
import java.util.Set;
108108
import java.util.function.Function;
109109
import java.util.stream.Collectors;
110+
import java.util.stream.Stream;
110111

111112
import static java.util.Collections.emptyMap;
112113
import static java.util.Collections.singletonMap;
@@ -117,13 +118,11 @@
117118
@SuppressWarnings({"checkstyle:ClassDataAbstractionCoupling", "checkstyle:ClassFanOutComplexity"})
118119
public class KafkaCluster extends AbstractModel implements SupportsMetrics, SupportsLogging, SupportsJmx {
119120
/**
120-
* Default Strimzi Metrics Reporter allow list.
121+
* Default Strimzi Metrics Reporter allow list for broker nodes.
121122
* If modifying this list, make sure example dashboards are compatible with the regexes.
122123
*/
123-
private static final List<String> DEFAULT_METRICS_ALLOW_LIST = List.of(
124+
private static final List<String> BROKER_DEFAULT_METRICS_ALLOW_LIST = List.of(
124125
"kafka_cluster_partition.*",
125-
"kafka_controller_kafkacontroller.*",
126-
"kafka_controller_controllerstats_uncleanleaderelectionspersec",
127126
"kafka_log_log_size",
128127
"kafka_network_requestmetrics.*",
129128
"kafka_network_socketserver_networkprocessoravgidlepercent",
@@ -133,12 +132,39 @@ public class KafkaCluster extends AbstractModel implements SupportsMetrics, Supp
133132
"kafka_server_kafkaserver_brokerstate",
134133
"kafka_server_kafkaserver_clusterid",
135134
"kafka_server_kafkaserver_linux.*",
136-
"kafka_server_raft.*",
137135
"kafka_server_replicamanager.*",
138136
"kafka_server_request_queue_size",
139137
"kafka_server_socket_server.*"
140138
);
141139

140+
/**
141+
* Default Strimzi Metrics Reporter allow list for controller nodes.
142+
* If modifying this list, make sure example dashboards are compatible with the regexes.
143+
*/
144+
private static final List<String> CONTROLLER_DEFAULT_METRICS_ALLOW_LIST = List.of(
145+
"kafka_controller_kafkacontroller.*",
146+
"kafka_controller_controllerstats_uncleanleaderelectionspersec",
147+
"kafka_network_requestmetrics.*",
148+
"kafka_network_socketserver_networkprocessoravgidlepercent",
149+
"kafka_server_app_info.*",
150+
"kafka_server_kafkarequesthandlerpool_requesthandleravgidlepercent",
151+
"kafka_server_kafkaserver_clusterid",
152+
"kafka_server_kafkaserver_linux.*",
153+
"kafka_server_raft.*",
154+
"kafka_server_request_queue_size",
155+
"kafka_server_socket_server.*"
156+
);
157+
158+
/**
159+
* Default Strimzi Metrics Reporter allow list for mixed (broker + controller) nodes.
160+
* This is the union of broker and controller allow lists.
161+
* If modifying this list, make sure example dashboards are compatible with the regexes.
162+
*/
163+
private static final List<String> MIXED_DEFAULT_METRICS_ALLOW_LIST = Stream.concat(
164+
BROKER_DEFAULT_METRICS_ALLOW_LIST.stream(),
165+
CONTROLLER_DEFAULT_METRICS_ALLOW_LIST.stream()
166+
).distinct().toList();
167+
142168
/**
143169
* Component type used by Kubernetes labels
144170
*/
@@ -335,7 +361,7 @@ public static KafkaCluster fromCrd(Reconciliation reconciliation,
335361
if (kafkaClusterSpec.getMetricsConfig() instanceof JmxPrometheusExporterMetrics) {
336362
result.metrics = new JmxPrometheusExporterModel(kafkaClusterSpec);
337363
} else if (kafkaClusterSpec.getMetricsConfig() instanceof StrimziMetricsReporter) {
338-
result.metrics = new StrimziMetricsReporterModel(kafkaClusterSpec, DEFAULT_METRICS_ALLOW_LIST);
364+
result.metrics = new StrimziMetricsReporterModel(kafkaClusterSpec, MIXED_DEFAULT_METRICS_ALLOW_LIST);
339365
}
340366

341367
result.logging = new LoggingModel(kafkaClusterSpec, result.getClass().getSimpleName());
@@ -1799,6 +1825,44 @@ public String generatePerBrokerConfiguration(int nodeId, Map<Integer, Map<String
17991825
);
18001826
}
18011827

1828+
/**
1829+
* Returns the metrics model for the given pool. If the cluster uses StrimziMetricsReporter with a
1830+
* user-defined allowlist, that is returned as-is. If the default allowlist is in use, a role-specific
1831+
* default is returned instead.
1832+
*
1833+
* @param pool The Kafka node pool
1834+
* @return MetricsModel appropriate for the pool role
1835+
*/
1836+
private MetricsModel metricsForPool(KafkaPool pool) {
1837+
if (metrics instanceof StrimziMetricsReporterModel reporterModel) {
1838+
// If user specified a custom allowlist, use it as-is for all pools
1839+
if (reporterModel.isCustomAllowList()) {
1840+
return metrics;
1841+
}
1842+
// Otherwise return a new model with the role-specific default
1843+
return new StrimziMetricsReporterModel(defaultMetricsAllowListForPool(pool));
1844+
}
1845+
return metrics;
1846+
}
1847+
1848+
/**
1849+
* Returns the default metrics allow list for the given pool based on its role.
1850+
* Broker-only pools get the broker list, controller-only pools get the controller list,
1851+
* and mixed pools get the union of both.
1852+
*
1853+
* @param pool The Kafka node pool
1854+
* @return The appropriate default metrics allow list
1855+
*/
1856+
private static List<String> defaultMetricsAllowListForPool(KafkaPool pool) {
1857+
if (pool.isBroker() && pool.isController()) {
1858+
return MIXED_DEFAULT_METRICS_ALLOW_LIST;
1859+
} else if (pool.isBroker()) {
1860+
return BROKER_DEFAULT_METRICS_ALLOW_LIST;
1861+
} else {
1862+
return CONTROLLER_DEFAULT_METRICS_ALLOW_LIST;
1863+
}
1864+
}
1865+
18021866
/**
18031867
* Internal method used to generate a Kafka configuration for given broker node.
18041868
*
@@ -1825,7 +1889,7 @@ private String generatePerBrokerConfiguration(NodeRef node, KafkaPool pool, Map<
18251889
.withCruiseControl(cluster, ccMetricsReporter, node.broker())
18261890
.withTieredStorage(cluster, tieredStorage)
18271891
.withQuotas(cluster, quotas)
1828-
.withStrimziMetricsReporter(metrics)
1892+
.withStrimziMetricsReporter(metricsForPool(pool))
18291893
.withUserConfiguration(
18301894
configuration,
18311895
node.broker() && ccMetricsReporter != null,

‎cluster-operator/src/main/java/io/strimzi/operator/cluster/model/metrics/StrimziMetricsReporterModel.java‎

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,27 +19,53 @@
1919
*/
2020
public class StrimziMetricsReporterModel implements MetricsModel {
2121
/**
22-
* Fully qualified class name of the Strimzi Metrics Reporter.
22+
* The allow list of regex patterns for metrics collection.
2323
*/
2424
private final List<String> allowList;
2525

26-
/**
27-
* Constructs the Metrics Model for managing configurable metrics to Strimzi.
28-
*
29-
* @param spec Custom resource section configuring metrics.
30-
* @param defaultAllowList Default allow list to be used when no value is provided.
31-
*/
26+
/**
27+
* Whether the allow list was explicitly set by the user (true) or is a role-based default (false).
28+
*/
29+
private final boolean customAllowList;
30+
31+
/**
32+
* Constructs the Metrics Model from a custom resource spec.
33+
* If the user provided an explicit allowlist, it is used; otherwise the provided default is used.
34+
*
35+
* @param spec Custom resource section configuring metrics.
36+
* @param defaultAllowList Default allow list to be used when no value is provided.
37+
*/
3238
public StrimziMetricsReporterModel(HasConfigurableMetrics spec, List<String> defaultAllowList) {
3339
if (spec.getMetricsConfig() != null) {
3440
StrimziMetricsReporter config = (StrimziMetricsReporter) spec.getMetricsConfig();
3541
validate(config);
36-
this.allowList = config.getValues() != null && config.getValues().getAllowList() != null
37-
? config.getValues().getAllowList() : defaultAllowList;
42+
boolean hasCustomList = config.getValues() != null && config.getValues().getAllowList() != null;
43+
this.allowList = hasCustomList ? config.getValues().getAllowList() : defaultAllowList;
44+
this.customAllowList = hasCustomList;
3845
} else {
3946
throw new InvalidConfigurationException("Unexpected empty metrics config");
4047
}
4148
}
4249

50+
/**
51+
* Constructs the Metrics Model directly from a default allow list (used for role-based defaults).
52+
*
53+
* @param defaultAllowList The role-specific default allow list.
54+
*/
55+
public StrimziMetricsReporterModel(List<String> defaultAllowList) {
56+
this.allowList = defaultAllowList;
57+
this.customAllowList = false;
58+
}
59+
60+
/**
61+
* Returns whether the allow list was explicitly configured by the user.
62+
*
63+
* @return true if the user provided a custom allowlist, false if using a role-based default.
64+
*/
65+
public boolean isCustomAllowList() {
66+
return customAllowList;
67+
}
68+
4369
/**
4470
* Gets the comma-separated list of allow regex expressions.
4571
*

‎cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaClusterTest.java‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,62 @@ public void testStrimziMetricsReporterConfig() {
337337
assertThat(rules.size(), is(1));
338338
}
339339

340+
@Test
341+
public void testStrimziMetricsReporterDefaultAllowListIsRoleBased() {
342+
// When no custom allowlist is set, each node pool should receive a role-specific default allowlist
343+
Kafka kafkaAssembly = new KafkaBuilder(KAFKA)
344+
.editSpec()
345+
.editKafka()
346+
.withNewStrimziMetricsReporterConfig()
347+
.endStrimziMetricsReporterConfig()
348+
.endKafka()
349+
.endSpec()
350+
.build();
351+
352+
List<KafkaPool> pools = NodePoolUtils.createKafkaPools(Reconciliation.DUMMY_RECONCILIATION, kafkaAssembly, List.of(POOL_CONTROLLERS, POOL_MIXED, POOL_BROKERS), Map.of(), KafkaVersionTestUtils.DEFAULT_KRAFT_VERSION_CHANGE, SHARED_ENV_PROVIDER);
353+
KafkaCluster kc = KafkaCluster.fromCrd(Reconciliation.DUMMY_RECONCILIATION, kafkaAssembly, pools, VERSIONS, KafkaVersionTestUtils.DEFAULT_KRAFT_VERSION_CHANGE, null, SHARED_ENV_PROVIDER);
354+
355+
List<ConfigMap> cms = kc.generatePerBrokerConfigurationConfigMaps(new MetricsAndLogging(null, null), ADVERTISED_HOSTNAMES, ADVERTISED_PORTS);
356+
assertThat(cms.size(), is(8));
357+
358+
for (ConfigMap cm : cms) {
359+
String podName = cm.getMetadata().getName();
360+
String data = cm.getData().toString();
361+
362+
if (podName.startsWith("controllers-")) {
363+
// Controller-only nodes: must have controller metrics, must NOT have broker-only metrics
364+
assertThat(podName + " should have controller metrics",
365+
data, containsString("kafka_controller_kafkacontroller"));
366+
assertThat(podName + " should have raft metrics",
367+
data, containsString("kafka_server_raft"));
368+
assertThat(podName + " should NOT have broker-only partition metrics",
369+
data, not(containsString("kafka_cluster_partition")));
370+
assertThat(podName + " should NOT have broker topic metrics",
371+
data, not(containsString("kafka_server_brokertopicmetrics")));
372+
} else if (podName.startsWith("mixed-")) {
373+
// Mixed nodes: must have both broker and controller metrics
374+
assertThat(podName + " should have controller metrics",
375+
data, containsString("kafka_controller_kafkacontroller"));
376+
assertThat(podName + " should have raft metrics",
377+
data, containsString("kafka_server_raft"));
378+
assertThat(podName + " should have broker partition metrics",
379+
data, containsString("kafka_cluster_partition"));
380+
assertThat(podName + " should have broker topic metrics",
381+
data, containsString("kafka_server_brokertopicmetrics"));
382+
} else if (podName.startsWith("brokers-")) {
383+
// Broker-only nodes: must have broker metrics, must NOT have controller-only metrics
384+
assertThat(podName + " should have broker partition metrics",
385+
data, containsString("kafka_cluster_partition"));
386+
assertThat(podName + " should have broker topic metrics",
387+
data, containsString("kafka_server_brokertopicmetrics"));
388+
assertThat(podName + " should NOT have controller metrics",
389+
data, not(containsString("kafka_controller_kafkacontroller")));
390+
assertThat(podName + " should NOT have raft metrics",
391+
data, not(containsString("kafka_server_raft")));
392+
}
393+
}
394+
}
395+
340396
@Test
341397
public void testJavaSystemProperties() {
342398
Kafka kafka = new KafkaBuilder(KAFKA)

0 commit comments

Comments
 (0)