feat(operator): KafkaSQL with TLS support #5444
Conversation
| public static final String TRUSTSTORE_SECRET_VOLUME_NAME = "registry-kafkasql-tls-truststore"; | ||
|
|
||
| /** | ||
| * Plain KafkaSQL must be already configured. |
There was a problem hiding this comment.
This comment shows that we are not choosing the right level of encapsulation here.
Please, remove the new KafkaSQLTLS class and squash the logic in KafkaSQL.
| public static boolean configureKafkaSQLTLS(ApicurioRegistry3 primary, Deployment deployment, | ||
| String containerName, Map<String, EnvVar> env) { | ||
|
|
||
| if (primary.getSpec().getApp().getKafkasql() == null |
There was a problem hiding this comment.
after squashing the logic this check goes away
| throw new OperatorException("Plain KafkaSQL must be already configured."); | ||
| } | ||
|
|
||
| if (primary.getSpec().getApp().getKafkasql().getSecurity() == null) { |
There was a problem hiding this comment.
the following 3 if statements can be collapsed into one, something similar to:
var kafkasql = primary.getSpec().getApp().getKafkasql();
if (kafkasql.getSecurity() != null && kafkasql.getSecurity().getTls() != null &&
!isBlank(kafkasql.getSecurity().getTls().getKeystoreSecretName()) &&
!isBlank(kafkasql.getSecurity().getTls().getTruststoreSecretName())) {assigning empty placeholder for null values to go to the next check is confusing.
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| import io.apicurio.registry.operator.OperatorException; | |||
| import io.apicurio.registry.operator.api.v1.ApicurioRegistry3; | |||
| import io.apicurio.registry.operator.feat.KafkaSql; | |||
| import io.apicurio.registry.operator.feat.KafkaSQL; | |||
There was a problem hiding this comment.
what's the reason for this rename?
if we rename KafkaSql to KafkaSQL, it would be best to rename also PostgresSql for consistency.
Not mandatory, but I encourage you to keep this change in separate PR.
| .create(); | ||
| final var clusterNAme = "my-cluster"; | ||
| client.load(getClass().getResourceAsStream("/k8s/examples/kafkasql/plain/example-cluster.kafka.yaml")) | ||
| .createOrReplace(); |
There was a problem hiding this comment.
the test should start on an empty namespace, this change will mask issues.
If you happen to be in the situation where this is needed, we should probably first merge #5364 and debug it.
| } | ||
|
|
||
| @Test | ||
| void testKafkaSQLTLS() { |
There was a problem hiding this comment.
I would keep this test in the KafkaSQLITTest, no need to have separate classes with a single test inside.
| env: | ||
| - name: QUARKUS_LOG_LEVEL | ||
| value: debug | ||
| - name: QUARKUS_LOG_CATEGORY_IO_APICURIO_LEVEL |
There was a problem hiding this comment.
curious about this, I always used only the former env var to debug...
| @Getter | ||
| @Setter | ||
| @ToString | ||
| public class ApicurioRegistry3SpecKafkaSqlSecurity implements KubernetesResource { |
There was a problem hiding this comment.
Instead of keep adding suffixes I think we should consolidate on using package/namespaces like we did for Sql:
https://github.com/Apicurio/apicurio-registry/tree/main/operator/model/src/main/java/io/apicurio/registry/operator/api/v1/spec
ApicurioRegistry3SpecKafkaSqlSecurity is ways too long of a name ...
| @Getter | ||
| @Setter | ||
| @ToString | ||
| public class ApicurioRegistry3SpecKafkaSqlTLS implements KubernetesResource { |
No description provided.