Config spec field from the CRD need to have validation

[alinalex1392]

We use the CRD from Strimzi for creating Kafka topics, we had a case where the Config object was misconfigured with the wrong parameters type for retention.ms and segment.bytes, but the CR was applied.

Because we want to be in line with the upstream solution, we would benefit from the introduction of validation for this Config object. The fast feedback for creating wrong topics configuration will be improved.

[scholzj]

@tombentley @sknot-rh Do we validate .spec.kafka.config in Kafka CRs now? Was there some reason why it was not done also for KafkaTopic CRs?

[sknot-rh]

I think we are doing some basic validation for Kafka resource (https://github.com/strimzi/strimzi-kafka-operator/blob/main/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java#L384). IIRC there is no validation in the KafkaTopic spec.config.

[tombentley]

@sknot-rh we also do validation in io.strimzi.operator.common.AbstractOperator#validate. Why are we doing validation in two places? Maybe there's a good reason, but in that case a comment in the code explaining why wouldn't go amiss.

@scholzj I don't recall a good reason, except that the TO doesn't extend AbstractOperator. I guess we could try to reuse the AbstractOperator#validate mechanism.

[sknot-rh]

I think io.strimzi.operator.common.AbstractOperator#validate does validate the entire spec but not the config (since it is Map<String, Object>). However the spec.config validation is done in io.strimzi.operator.cluster.model.KafkaConfiguration#validate. It reads config model and for each entry it checks the type. Currently we cannot do this for topics as the config model is generated only for brokers.

[tombentley]

@sknot-rh true, but then why aren't we calling validateIntConfigProperty in validateConfiguration?

We could generate a config model for topics (any maybe one for Kafka Connect workers?)

[sknot-rh]

Good question. I think it could be refactored. I can take a look at topic config model generation.

[scholzj]

Does #5960 PR resolve this? There does not seem to be any link.

[alinalex1392]

5960 seem to resolve this improvement.

[scholzj]

Note: The PR #5960 was reverted in #6195 because it didn't support different Kafka versions.

Triaged on 28.4.2022: We need to re-consider if there is some easy way to get the configuration model for given Kafka version. @tombentley will think about it. To be revisited.

[tombentley]

I think this would work to support validation of topic config types, for arbitrary broker versions:

1. During TO start up use the Admin client to do describeConfigs for the default topic configs (basically use an empty topic name). Use DescribeConfigOptions to include synonyms (IDK off hand whether there are any for topic configs, but in principal if there are any we should allow the user to use them).
2. Use the ConfigEntrys for the DescribeConfigResult to discover the ConfigEntry.ConfigType
3. Keep a Map<String, ConfigEntry.ConfigType> for use during the lifetime of the TO.
4. Use that map whenever we need to validate a KafkaTopic.spec.configs.

It's not quite perfect, since it could go wrong if topic configs were added/removed in broker version X and the TO is running against a cluster in the middle of upgrade. It would then use an incorrect model. But TBH this not solvable, because topic configs get sent to the least loaded broker (i.e. you don't know which), so you could have this problem even manually.

[scholzj]

Triaged on 10.5.2022: Should be worth trying to improve the validation.