Skip to content

Commit 663731f

Browse files
tinaselengekatheris
tinaselenge
and
katheris
committed
Address review comments:
- Add CEL validation rules for CertificateAuthority - Remove redundant checks that are covered by the CEL validation rules - Add a new system test for migration between CA types Co-authored-by: Kate Stanley <11195226+katheris@users.noreply.github.com> Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
1 parent 6d564d3 commit 663731f

19 files changed

Lines changed: 522 additions & 124 deletions

File tree

‎api/src/main/java/io/strimzi/api/kafka/model/common/CertificateAuthority.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import com.fasterxml.jackson.annotation.JsonInclude;
88
import com.fasterxml.jackson.annotation.JsonPropertyOrder;
99
import io.strimzi.api.kafka.model.kafka.certmanager.CertManager;
10+
import io.strimzi.crdgenerator.annotations.CelValidation;
1011
import io.strimzi.crdgenerator.annotations.Description;
1112
import io.strimzi.crdgenerator.annotations.Minimum;
1213
import io.sundr.builder.annotations.Buildable;
@@ -23,6 +24,20 @@
2324
editableEnabled = false,
2425
builderPackage = Constants.FABRIC8_KUBERNETES_API
2526
)
27+
@CelValidation(rules = {
28+
@CelValidation.CelValidationRule(
29+
rule = "(has(self.type) && self.type != 'strimzi') || !has(self.certManager)",
30+
message = "'certManager' cannot be configured with 'type: strimzi'"
31+
),
32+
@CelValidation.CelValidationRule(
33+
rule = "!has(self.type) || self.type != 'cert-manager' || has(self.certManager)",
34+
message = "'certManager' must be set for 'type: cert-manager'"
35+
),
36+
@CelValidation.CelValidationRule(
37+
rule = "!has(self.type) || self.type != 'cert-manager' || (has(self.generateCertificateAuthority) && !self.generateCertificateAuthority)",
38+
message = "'generateCertificateAuthority' must be set to false for 'type: cert-manager'"
39+
)
40+
})
2641
@JsonInclude(JsonInclude.Include.NON_DEFAULT)
2742
@JsonPropertyOrder({ "generateCertificateAuthority", "type", "generateSecretOwnerReference", "validityDays",
2843
"renewalDays", "certificateExpirationPolicy", "certManager" })

‎api/src/test/java/io/strimzi/api/kafka/model/CelValidationIT.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,12 +66,21 @@ static Stream<Arguments> celRules() {
6666
final String renewalMustBeLessThanValidity = "'renewalDays' must be less than 'validityDays'.";
6767
final String validityRenewalMustBeSetUnderTls = "'validityDays' and 'renewalDays' can be configured only with 'type: tls'";
6868

69+
final String certManagerGenerateCaMustBeFalse = "'generateCertificateAuthority' must be set to false for 'type: cert-manager'";
70+
final String certManagerMissingRequiredField = "'certManager' must be set for 'type: cert-manager'";
71+
final String strimziCaInvalidField = "'certManager' cannot be configured with 'type: strimzi'";
72+
6973
return Stream.of(
7074
Arguments.of(Kafka.class, "Kafka-cel-rack-topology-label-missing-key.yaml", topologyKeyRequired),
7175
Arguments.of(Kafka.class, "Kafka-cel-rack-environment-variable-missing-name.yaml", envVarNameRequired),
7276
Arguments.of(Kafka.class, "Kafka-cel-metrics-jmx-missing-valueFrom.yaml", valueFromRequired),
7377
Arguments.of(Kafka.class, "Kafka-cel-cc-metrics-jmx-missing-valueFrom.yaml", valueFromRequired),
7478
Arguments.of(Kafka.class, "Kafka-cel-cc-metrics-strimzi-rejected.yaml", ccStrimziTypeNotSupported),
79+
Arguments.of(Kafka.class, "Kafka-cel-cert-manager-missing-generate-ca.yaml", certManagerGenerateCaMustBeFalse),
80+
Arguments.of(Kafka.class, "Kafka-cel-cert-manager-invalid-generate-ca.yaml", certManagerGenerateCaMustBeFalse),
81+
Arguments.of(Kafka.class, "Kafka-cel-cert-manager-missing-required-field.yaml", certManagerMissingRequiredField),
82+
Arguments.of(Kafka.class, "Kafka-cel-strimzi-ca-rejected-invalid-field.yaml", strimziCaInvalidField),
83+
Arguments.of(Kafka.class, "Kafka-cel-default-ca-rejected-invalid-field.yaml", strimziCaInvalidField),
7584

7685
Arguments.of(KafkaConnect.class, "KafkaConnect-cel-rack-topology-label-missing-key.yaml", topologyKeyRequired),
7786
Arguments.of(KafkaConnect.class, "KafkaConnect-cel-rack-environment-variable-missing-name.yaml", envVarNameRequired),

‎api/src/test/resources/io/strimzi/api/kafka/model/kafka/Kafka-cel-cert-manager-invalid-generate-ca.yaml‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
apiVersion: kafka.strimzi.io/v1
2+
kind: Kafka
3+
metadata:
4+
name: my-cluster
5+
spec:
6+
clusterCa:
7+
generateCertificateAuthority: true
8+
type: cert-manager
9+
certManager:
10+
issuerRef:
11+
name: my-issuer
12+
kind: Issuer
13+
caCertRef:
14+
secretName: my-ca-secret
15+
certificate: ca.crt
16+
kafka:
17+
listeners:
18+
- name: plain
19+
type: internal
20+
tls: false
21+
port: 9092

‎api/src/test/resources/io/strimzi/api/kafka/model/kafka/Kafka-cel-cert-manager-missing-generate-ca.yaml‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
apiVersion: kafka.strimzi.io/v1
2+
kind: Kafka
3+
metadata:
4+
name: my-cluster
5+
spec:
6+
clusterCa:
7+
type: cert-manager
8+
certManager:
9+
issuerRef:
10+
name: my-issuer
11+
kind: Issuer
12+
caCertRef:
13+
secretName: my-ca-secret
14+
certificate: ca.crt
15+
kafka:
16+
listeners:
17+
- name: plain
18+
type: internal
19+
tls: false
20+
port: 9092

‎api/src/test/resources/io/strimzi/api/kafka/model/kafka/Kafka-cel-cert-manager-missing-required-field.yaml‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
apiVersion: kafka.strimzi.io/v1
2+
kind: Kafka
3+
metadata:
4+
name: my-cluster
5+
spec:
6+
clusterCa:
7+
generateCertificateAuthority: false
8+
type: cert-manager
9+
kafka:
10+
listeners:
11+
- name: plain
12+
type: internal
13+
tls: false
14+
port: 9092

‎api/src/test/resources/io/strimzi/api/kafka/model/kafka/Kafka-cel-default-ca-rejected-invalid-field.yaml‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
apiVersion: kafka.strimzi.io/v1
2+
kind: Kafka
3+
metadata:
4+
name: my-cluster
5+
spec:
6+
clusterCa:
7+
certManager:
8+
issuerRef:
9+
name: my-issuer
10+
kind: Issuer
11+
caCertRef:
12+
secretName: my-ca-secret
13+
certificate: ca.crt
14+
kafka:
15+
listeners:
16+
- name: plain
17+
type: internal
18+
tls: false
19+
port: 9092

‎api/src/test/resources/io/strimzi/api/kafka/model/kafka/Kafka-cel-strimzi-ca-rejected-invalid-field.yaml‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
apiVersion: kafka.strimzi.io/v1
2+
kind: Kafka
3+
metadata:
4+
name: my-cluster
5+
spec:
6+
clusterCa:
7+
type: strimzi
8+
certManager:
9+
issuerRef:
10+
name: my-issuer
11+
kind: Issuer
12+
caCertRef:
13+
secretName: my-ca-secret
14+
certificate: ca.crt
15+
kafka:
16+
listeners:
17+
- name: plain
18+
type: internal
19+
tls: false
20+
port: 9092

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

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -173,10 +173,11 @@ public static EntityUserOperator fromCrd(Reconciliation reconciliation,
173173
if (kafkaAssembly.getSpec().getClientsCa().getRenewalDays() > 0) {
174174
result.clientsCaRenewalDays = kafkaAssembly.getSpec().getClientsCa().getRenewalDays();
175175
}
176+
176177
result.certificateManagerType = kafkaAssembly.getSpec().getClientsCa().getType();
178+
177179
if (CertificateManagerType.CERT_MANAGER.equals(result.certificateManagerType)
178-
&& kafkaAssembly.getSpec().getClientsCa().getCertManager() != null
179-
&& kafkaAssembly.getSpec().getClientsCa().getCertManager().getIssuerRef() != null) {
180+
&& kafkaAssembly.getSpec().getClientsCa().getCertManager() != null) {
180181
result.certManagerIssuerName = kafkaAssembly.getSpec().getClientsCa().getCertManager().getIssuerRef().getName();
181182
result.certManagerIssuerKind = kafkaAssembly.getSpec().getClientsCa().getCertManager().getIssuerRef().getKind();
182183
result.certManagerIssuerGroup = kafkaAssembly.getSpec().getClientsCa().getCertManager().getIssuerRef().getGroup();
@@ -262,15 +263,9 @@ protected List<EnvVar> getEnvVars() {
262263

263264
// if CA type is cert-manager, set cert-manager env vars
264265
if (CertificateManagerType.CERT_MANAGER.equals(certificateManagerType)) {
265-
if (certManagerIssuerName != null && !certManagerIssuerName.isEmpty()) {
266-
varList.add(ContainerUtils.createEnvVar(ENV_VAR_CERT_MANAGER_ISSUER_NAME, certManagerIssuerName));
267-
}
268-
if (certManagerIssuerKind != null) {
269-
varList.add(ContainerUtils.createEnvVar(ENV_VAR_CERT_MANAGER_ISSUER_KIND, certManagerIssuerKind.toValue()));
270-
}
271-
if (certManagerIssuerGroup != null && !certManagerIssuerGroup.isEmpty()) {
272-
varList.add(ContainerUtils.createEnvVar(ENV_VAR_CERT_MANAGER_ISSUER_GROUP, certManagerIssuerGroup));
273-
}
266+
varList.add(ContainerUtils.createEnvVar(ENV_VAR_CERT_MANAGER_ISSUER_NAME, certManagerIssuerName));
267+
varList.add(ContainerUtils.createEnvVar(ENV_VAR_CERT_MANAGER_ISSUER_KIND, certManagerIssuerKind.toValue()));
268+
varList.add(ContainerUtils.createEnvVar(ENV_VAR_CERT_MANAGER_ISSUER_GROUP, certManagerIssuerGroup));
274269
}
275270

276271
return varList;

‎cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/CaProvider.java‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
import io.fabric8.kubernetes.api.model.OwnerReferenceBuilder;
99
import io.fabric8.kubernetes.api.model.Secret;
1010
import io.fabric8.kubernetes.api.model.SecretBuilder;
11-
import io.strimzi.api.kafka.model.common.CertificateManagerType;
1211
import io.strimzi.api.kafka.model.kafka.Kafka;
1312
import io.strimzi.certs.CertIssuer;
1413
import io.strimzi.operator.cluster.model.AbstractModel;
@@ -88,12 +87,8 @@ yield new CustomCaProvider(reconciliation, caRole, caConfig, kafkaCr, certIssuer
8887
);
8988
}
9089
}
91-
case CERT_MANAGER -> {
92-
if (caConfig.isGenerateCa()) {
93-
throw new IllegalArgumentException("Certificate Manager type is set to " + CertificateManagerType.CERT_MANAGER.toValue() + ", but generateCertificateAuthority is set to true. Set generateCertificateAuthority to false when using cert-manager as the certificate manager type.");
94-
}
95-
yield new CertManagerCaProvider(reconciliation, caRole, caConfig, kafkaCr, existingCaCertSecret, clusterOperatorCertSecret, certManagerCertificateOperator, secretOperator);
96-
}
90+
case CERT_MANAGER -> new CertManagerCaProvider(reconciliation, caRole, caConfig, kafkaCr, existingCaCertSecret,
91+
clusterOperatorCertSecret, certManagerCertificateOperator, secretOperator);
9792
};
9893
}
9994

‎cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/CertManagerCaProvider.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
import java.security.cert.CertificateException;
2525
import java.util.HashMap;
2626
import java.util.Map;
27-
import java.util.concurrent.CompletableFuture;
2827
import java.util.concurrent.CompletionStage;
2928

3029
import static io.strimzi.operator.common.ca.Ca.ANNO_STRIMZI_IO_CA_KEY_GENERATION;
@@ -72,9 +71,6 @@ public CertManagerCaProvider(Reconciliation reconciliation,
7271

7372
@Override
7473
public CompletionStage<CaProviderResult> createAndReconcileCa() {
75-
if (certificateAuthority.getCertManager() == null) {
76-
return CompletableFuture.failedFuture(new InvalidResourceException("When CA type is set to cert-manager, certManager property is required (e.g. clusterCa.certManager)."));
77-
}
7874
return getCaCertForCertManager()
7975
.thenCompose(newCaCertAsBase64 -> {
8076
CertManagerCa certManagerCa = new CertManagerCa(reconciliation, caRole,

0 commit comments

Comments
 (0)