Add CEL validation and address comments

Signed-off-by: ocorriga <[email protected]>
strimzi · Feb 24, 2025 · 75a829f · 75a829f
1 parent 9b4868d
commit 75a829f
Show file tree

Hide file tree

Showing 20 changed files with 74 additions and 51 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,7 +6,7 @@
 * Support for MirrorMaker 1 has been removed
 * Support for storage class overrides has been removed
 * Added support to configure `dnsPolicy` and `dnsConfig` using the `template` sections.
-* Added support for Strimzi Metrics Reporter to the Kafka component.
+* Added support for Strimzi Metrics Reporter to the Kafka brokers/controller component.
 * Store Kafka node certificates in separate Secrets, one Secret per pod.
 * Allow configuring `ssl.principal.mapping.rules` and custom trusted CAs in Kafka brokers with `type: custom` authentication
 * Moved HTTP bridge configuration to the ConfigMap setup by the operator.

diff --git a/api/src/main/java/io/strimzi/api/kafka/model/common/metrics/MetricsConfig.java b/api/src/main/java/io/strimzi/api/kafka/model/common/metrics/MetricsConfig.java
@@ -8,6 +8,7 @@
 import com.fasterxml.jackson.annotation.JsonSubTypes;
 import com.fasterxml.jackson.annotation.JsonTypeInfo;
 import io.strimzi.api.kafka.model.common.UnknownPropertyPreserving;
+import io.strimzi.crdgenerator.annotations.CelValidation;
 import io.strimzi.crdgenerator.annotations.Description;
 import lombok.EqualsAndHashCode;
 import lombok.ToString;
@@ -28,6 +29,12 @@
 @JsonInclude(JsonInclude.Include.NON_NULL)
 @EqualsAndHashCode
 @ToString
+@CelValidation(rules = {
+    @CelValidation.CelValidationRule(
+            rule = "self.type != 'jmxPrometheusExporter' || has(self.valueFrom)",
+            message = "valueFrom property is required"
+        )
+})
 public abstract class MetricsConfig implements UnknownPropertyPreserving {
     private Map<String, Object> additionalProperties;
 

diff --git a/api/src/main/java/io/strimzi/api/kafka/model/kafka/cruisecontrol/CruiseControlSpec.java b/api/src/main/java/io/strimzi/api/kafka/model/kafka/cruisecontrol/CruiseControlSpec.java
@@ -19,6 +19,7 @@
 import io.strimzi.api.kafka.model.common.UnknownPropertyPreserving;
 import io.strimzi.api.kafka.model.common.metrics.MetricsConfig;
 import io.strimzi.api.kafka.model.kafka.entityoperator.TlsSidecar;
+import io.strimzi.crdgenerator.annotations.CelValidation;
 import io.strimzi.crdgenerator.annotations.Description;
 import io.strimzi.crdgenerator.annotations.DescriptionFile;
 import io.strimzi.crdgenerator.annotations.KubeLink;
@@ -127,6 +128,16 @@ public void setApiUsers(CruiseControlApiUsers apiUsers) {
 
     @Description("Metrics configuration.")
     @JsonInclude(JsonInclude.Include.NON_EMPTY)
+    @CelValidation(rules = {
+        @CelValidation.CelValidationRule(
+                rule = "self.type != 'jmxPrometheusExporter' || has(self.valueFrom)",
+                message = "valueFrom property is required"
+            ),
+        @CelValidation.CelValidationRule(
+                rule = "self.type != 'strimziMetricsReporter'",
+                message = "value type not supported"
+            )
+        })
     @Override
     public MetricsConfig getMetricsConfig() {
         return metricsConfig;

diff --git a/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/CruiseControl.java b/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/CruiseControl.java
@@ -22,7 +22,6 @@
 import io.fabric8.kubernetes.api.model.networking.v1.NetworkPolicyPeer;
 import io.strimzi.api.kafka.model.common.JvmOptions;
 import io.strimzi.api.kafka.model.common.metrics.JmxPrometheusExporterMetrics;
-import io.strimzi.api.kafka.model.common.metrics.StrimziMetricsReporter;
 import io.strimzi.api.kafka.model.common.template.DeploymentTemplate;
 import io.strimzi.api.kafka.model.common.template.InternalServiceTemplate;
 import io.strimzi.api.kafka.model.common.template.PodTemplate;
@@ -48,7 +47,6 @@
 import io.strimzi.operator.common.Annotations;
 import io.strimzi.operator.common.Reconciliation;
 import io.strimzi.operator.common.Util;
-import io.strimzi.operator.common.model.InvalidResourceException;
 import io.strimzi.operator.common.model.Labels;
 import io.strimzi.operator.common.model.cruisecontrol.CruiseControlApiProperties;
 import io.strimzi.operator.common.model.cruisecontrol.CruiseControlConfigurationParameters;
@@ -224,10 +222,6 @@ public static CruiseControl fromCrd(
 
             if (ccSpec.getMetricsConfig() instanceof JmxPrometheusExporterMetrics) {
                 result.metrics = new JmxPrometheusExporterModel(ccSpec);
-            } else if (ccSpec.getMetricsConfig() instanceof StrimziMetricsReporter) {
-                // Cruise Control own metrics are only exported through JMX
-                LOGGER.errorCr(reconciliation, "The Strimzi Metrics Reporter is not supported with Cruise Control");
-                throw new InvalidResourceException("The Strimzi Metrics Reporter is not supported with Cruise Control");
             }
 
             result.logging = new LoggingModel(ccSpec, result.getClass().getSimpleName(), true, false);
@@ -250,7 +244,7 @@ public static CruiseControl fromCrd(
     }
 
     private boolean hasMetricsConfig() {
-        return metrics != null && metrics.isEnabled();
+        return metrics != null;
     }
 
     private void updateConfigurationWithDefaults(CruiseControlSpec ccSpec, KafkaConfiguration kafkaConfiguration) {

diff --git a/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java b/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java
@@ -327,6 +327,8 @@ public static KafkaCluster fromCrd(Reconciliation reconciliation,
             result.metrics = new JmxPrometheusExporterModel(kafkaClusterSpec);
         } else if (kafkaClusterSpec.getMetricsConfig() instanceof StrimziMetricsReporter) {
             result.metrics = new StrimziMetricsReporterModel(kafkaClusterSpec);
+        } else if (kafkaClusterSpec.getMetricsConfig() != null) {
+            throw new InvalidResourceException("Unsupported metrics type");
         }
 
         result.logging = new LoggingModel(kafkaClusterSpec, result.getClass().getSimpleName(), false, true);

diff --git a/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaConnectCluster.java b/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaConnectCluster.java
@@ -630,7 +630,7 @@ protected String getCommand() {
 
     protected List<EnvVar> getEnvVars() {
         List<EnvVar> varList = new ArrayList<>();
-        varList.add(ContainerUtils.createEnvVar(ENV_VAR_KAFKA_CONNECT_METRICS_ENABLED, String.valueOf(metrics.isEnabled())));
+        varList.add(ContainerUtils.createEnvVar(ENV_VAR_KAFKA_CONNECT_METRICS_ENABLED, String.valueOf(metrics != null && metrics.isEnabled())));
         varList.add(ContainerUtils.createEnvVar(ENV_VAR_STRIMZI_KAFKA_GC_LOG_ENABLED, String.valueOf(gcLoggingEnabled)));
 
         JvmOptionUtils.heapOptions(varList, 75, 0L, jvmOptions, resources);

diff --git a/...r/src/main/java/io/strimzi/operator/cluster/model/metrics/JmxPrometheusExporterModel.java b/...r/src/main/java/io/strimzi/operator/cluster/model/metrics/JmxPrometheusExporterModel.java
@@ -40,14 +40,11 @@ public class JmxPrometheusExporterModel implements MetricsModel {
      */
     public JmxPrometheusExporterModel(HasConfigurableMetrics spec) {
         if (spec.getMetricsConfig() != null) {
-            if (spec.getMetricsConfig() instanceof JmxPrometheusExporterMetrics config) {
-                validateJmxExporterMetricsConfig(config);
-                this.isEnabled = true;
-                this.configMapName = config.getValueFrom().getConfigMapKeyRef().getName();
-                this.configMapKey = config.getValueFrom().getConfigMapKeyRef().getKey();
-            } else {
-                throw new InvalidResourceException("Unsupported metrics type " + spec.getMetricsConfig().getType());
-            }
+            JmxPrometheusExporterMetrics config = (JmxPrometheusExporterMetrics) spec.getMetricsConfig();
+            validateJmxExporterMetricsConfig(config);
+            this.isEnabled = true;
+            this.configMapName = config.getValueFrom().getConfigMapKeyRef().getName();
+            this.configMapKey = config.getValueFrom().getConfigMapKeyRef().getKey();
         } else {
             this.isEnabled = false;
             this.configMapName = null;
@@ -123,8 +120,7 @@ public String metricsJson(Reconciliation reconciliation, ConfigMap configMap) {
     /* test */ static void validateJmxExporterMetricsConfig(JmxPrometheusExporterMetrics config) {
         List<String> errors = new ArrayList<>();
 
-        if (config.getValueFrom() != null
-                && config.getValueFrom().getConfigMapKeyRef() != null) {
+        if (config.getValueFrom().getConfigMapKeyRef() != null) {
             // The Config Map reference exists
             if (config.getValueFrom().getConfigMapKeyRef().getName() == null
                     || config.getValueFrom().getConfigMapKeyRef().getName().isEmpty()) {

diff --git a/.../src/main/java/io/strimzi/operator/cluster/model/metrics/StrimziMetricsReporterModel.java b/.../src/main/java/io/strimzi/operator/cluster/model/metrics/StrimziMetricsReporterModel.java
@@ -33,14 +33,11 @@ public class StrimziMetricsReporterModel implements MetricsModel {
      */
     public StrimziMetricsReporterModel(HasConfigurableMetrics spec) {
         if (spec.getMetricsConfig() != null) {
-            if (spec.getMetricsConfig() instanceof StrimziMetricsReporter config) {
-                validate(config);
-                this.isEnabled = true;
-                this.allowList = config.getValues() != null && config.getValues().getAllowList() != null
-                        ? config.getValues().getAllowList() : null;
-            } else {
-                throw new InvalidResourceException("Unsupported metrics type " + spec.getMetricsConfig().getType());
-            }
+            StrimziMetricsReporter config = (StrimziMetricsReporter) spec.getMetricsConfig();
+            validate(config);
+            this.isEnabled = true;
+            this.allowList = config.getValues() != null && config.getValues().getAllowList() != null
+                    ? config.getValues().getAllowList() : null;
         } else {
             this.isEnabled = false;
             this.allowList = null;

diff --git a/cluster-operator/src/test/java/io/strimzi/operator/cluster/model/CruiseControlTest.java b/cluster-operator/src/test/java/io/strimzi/operator/cluster/model/CruiseControlTest.java
@@ -61,7 +61,6 @@
 import io.strimzi.operator.cluster.model.cruisecontrol.CpuCapacity;
 import io.strimzi.operator.common.Annotations;
 import io.strimzi.operator.common.Reconciliation;
-import io.strimzi.operator.common.model.InvalidResourceException;
 import io.strimzi.operator.common.model.Labels;
 import io.strimzi.operator.common.model.cruisecontrol.CruiseControlApiProperties;
 import io.strimzi.operator.common.model.cruisecontrol.CruiseControlConfigurationParameters;
@@ -102,7 +101,6 @@
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.hasItems;
 import static org.hamcrest.Matchers.hasProperty;
-import static org.junit.jupiter.api.Assertions.assertThrows;
 
 @SuppressWarnings({
     "checkstyle:ClassDataAbstractionCoupling",
@@ -1098,26 +1096,6 @@ public void testMetricsParsingNoMetrics() {
         assertThat(cc.metrics(), is(nullValue()));
     }
 
-    @ParallelTest
-    public void testStrimziReporterMetricsConfig() {
-        Kafka kafka = new KafkaBuilder(KAFKA)
-            .editSpec()
-                .withNewCruiseControl()
-                    .withNewStrimziMetricsReporterConfig()
-                        .withNewValues()
-                            .withAllowList(List.of("kafka_log.*", "kafka_network.*"))
-                        .endValues()
-                    .endStrimziMetricsReporterConfig()
-                .endCruiseControl()
-            .endSpec()
-            .build();
-
-        InvalidResourceException ex = assertThrows(InvalidResourceException.class, 
-            () -> createCruiseControl(kafka, NODES, STORAGE, Map.of()));
-
-        assertThat(ex.getMessage(), is("The Strimzi Metrics Reporter is not supported with Cruise Control"));
-    }
-
     @ParallelTest
     public void testDefaultTopicNames() {
         CruiseControl cc = createCruiseControl(KAFKA, NODES, STORAGE, Map.of());

diff --git a/.../src/test/java/io/strimzi/operator/cluster/model/metrics/JmxExporterMetricsModelTest.java b/.../src/test/java/io/strimzi/operator/cluster/model/metrics/JmxExporterMetricsModelTest.java
@@ -82,8 +82,8 @@ public void testPrometheusJmxMetricsValidation() {
 
         ex = assertThrows(InvalidResourceException.class, () -> JmxPrometheusExporterModel.validateJmxExporterMetricsConfig(new JmxPrometheusExporterMetricsBuilder().withNewValueFrom().endValueFrom().build()));
         assertThat(ex.getMessage(), is("Metrics configuration is invalid: [Config Map reference is missing]"));
-
-        ex = assertThrows(InvalidResourceException.class, () -> JmxPrometheusExporterModel.validateJmxExporterMetricsConfig(new JmxPrometheusExporterMetricsBuilder().build()));
+        ex = assertThrows(InvalidResourceException.class, () -> JmxPrometheusExporterModel.validateJmxExporterMetricsConfig(new JmxPrometheusExporterMetricsBuilder().withNewValueFrom().endValueFrom().build()));
         assertThat(ex.getMessage(), is("Metrics configuration is invalid: [Config Map reference is missing]"));
+
     }
 }
diff --git a/packaging/examples/metrics/strimzi-metrics-reporter/README.md b/packaging/examples/metrics/strimzi-metrics-reporter/README.md
@@ -0,0 +1,4 @@
+# Metrics configuration examples
+
+The examples in this directory demonstrate how you can use configure metrics with Strimzi Metrics Reporter plugin.
+There are also some Grafana dashboards that you can use with them.
diff --git a/...ter/grafana-dashboards/strimzi-kafka.json → ...ter/grafana-dashboards/strimzi-kafka.json b/...ter/grafana-dashboards/strimzi-kafka.json → ...ter/grafana-dashboards/strimzi-kafka.json
diff --git a/...ter/grafana-dashboards/strimzi-kraft.json → ...ter/grafana-dashboards/strimzi-kraft.json b/...ter/grafana-dashboards/strimzi-kraft.json → ...ter/grafana-dashboards/strimzi-kraft.json
diff --git a/...mples/metrics/reporter/kafka-metrics.yaml → ...rimzi-metrics-reporter/kafka-metrics.yaml b/...mples/metrics/reporter/kafka-metrics.yaml → ...rimzi-metrics-reporter/kafka-metrics.yaml
diff --git a/packaging/helm-charts/helm3/strimzi-kafka-operator/crds/040-Crd-kafka.yaml b/packaging/helm-charts/helm3/strimzi-kafka-operator/crds/040-Crd-kafka.yaml
@@ -943,6 +943,9 @@ spec:
                       required:
                         - type
                       description: Metrics configuration.
+                      x-kubernetes-validations:
+                        - rule: self.type != 'jmxPrometheusExporter' || has(self.valueFrom)
+                          message: valueFrom property is required
                     logging:
                       type: object
                       properties:
@@ -2523,6 +2526,9 @@ spec:
                       required:
                         - type
                       description: Metrics configuration.
+                      x-kubernetes-validations:
+                        - rule: self.type != 'jmxPrometheusExporter' || has(self.valueFrom)
+                          message: valueFrom property is required
                     logging:
                       type: object
                       properties:
@@ -6547,6 +6553,11 @@ spec:
                       required:
                         - type
                       description: Metrics configuration.
+                      x-kubernetes-validations:
+                        - rule: self.type != 'jmxPrometheusExporter' || has(self.valueFrom)
+                          message: valueFrom property is required
+                        - rule: self.type != 'strimziMetricsReporter'
+                          message: value type not supported
                     apiUsers:
                       type: object
                       properties:

diff --git a/packaging/helm-charts/helm3/strimzi-kafka-operator/crds/041-Crd-kafkaconnect.yaml b/packaging/helm-charts/helm3/strimzi-kafka-operator/crds/041-Crd-kafkaconnect.yaml
@@ -482,6 +482,9 @@ spec:
                   required:
                     - type
                   description: Metrics configuration.
+                  x-kubernetes-validations:
+                    - rule: self.type != 'jmxPrometheusExporter' || has(self.valueFrom)
+                      message: valueFrom property is required
                 tracing:
                   type: object
                   properties:

diff --git a/packaging/helm-charts/helm3/strimzi-kafka-operator/crds/048-Crd-kafkamirrormaker2.yaml b/packaging/helm-charts/helm3/strimzi-kafka-operator/crds/048-Crd-kafkamirrormaker2.yaml
@@ -699,6 +699,9 @@ spec:
                   required:
                     - type
                   description: Metrics configuration.
+                  x-kubernetes-validations:
+                    - rule: self.type != 'jmxPrometheusExporter' || has(self.valueFrom)
+                      message: valueFrom property is required
                 tracing:
                   type: object
                   properties:

diff --git a/packaging/install/cluster-operator/040-Crd-kafka.yaml b/packaging/install/cluster-operator/040-Crd-kafka.yaml
@@ -942,6 +942,9 @@ spec:
                     required:
                     - type
                     description: Metrics configuration.
+                    x-kubernetes-validations:
+                    - rule: self.type != 'jmxPrometheusExporter' || has(self.valueFrom)
+                      message: valueFrom property is required
                   logging:
                     type: object
                     properties:
@@ -2522,6 +2525,9 @@ spec:
                     required:
                     - type
                     description: Metrics configuration.
+                    x-kubernetes-validations:
+                    - rule: self.type != 'jmxPrometheusExporter' || has(self.valueFrom)
+                      message: valueFrom property is required
                   logging:
                     type: object
                     properties:
@@ -6546,6 +6552,11 @@ spec:
                     required:
                     - type
                     description: Metrics configuration.
+                    x-kubernetes-validations:
+                    - rule: self.type != 'jmxPrometheusExporter' || has(self.valueFrom)
+                      message: valueFrom property is required
+                    - rule: self.type != 'strimziMetricsReporter'
+                      message: value type not supported
                   apiUsers:
                     type: object
                     properties:

diff --git a/packaging/install/cluster-operator/041-Crd-kafkaconnect.yaml b/packaging/install/cluster-operator/041-Crd-kafkaconnect.yaml
@@ -481,6 +481,9 @@ spec:
                 required:
                 - type
                 description: Metrics configuration.
+                x-kubernetes-validations:
+                - rule: self.type != 'jmxPrometheusExporter' || has(self.valueFrom)
+                  message: valueFrom property is required
               tracing:
                 type: object
                 properties:

diff --git a/packaging/install/cluster-operator/048-Crd-kafkamirrormaker2.yaml b/packaging/install/cluster-operator/048-Crd-kafkamirrormaker2.yaml
@@ -698,6 +698,9 @@ spec:
                 required:
                 - type
                 description: Metrics configuration.
+                x-kubernetes-validations:
+                - rule: self.type != 'jmxPrometheusExporter' || has(self.valueFrom)
+                  message: valueFrom property is required
               tracing:
                 type: object
                 properties: