Simplify Ca and CaReconciler logic

* Remove code in CaReconciler that is never being executed. * Move Secret existence checking from CaReconciler to Ca. Signed-off-by: Katherine Stanley <[email protected]>
strimzi · Nov 6, 2024 · b94cc00 · b94cc00
1 parent 2af12e0
commit b94cc00
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 69 deletions.
diff --git a/...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/CaReconciler.java b/...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/CaReconciler.java
@@ -45,7 +45,6 @@
 import io.strimzi.operator.common.auth.TlsPemIdentity;
 import io.strimzi.operator.common.model.Ca;
 import io.strimzi.operator.common.model.ClientsCa;
-import io.strimzi.operator.common.model.InvalidResourceException;
 import io.strimzi.operator.common.model.Labels;
 import io.strimzi.operator.common.model.PasswordGenerator;
 import io.strimzi.operator.common.operator.resource.ReconcileResult;
@@ -258,9 +257,6 @@ Future<Void> reconcileCas(Clock clock) {
                         }
                     }
 
-                    // When we are not supposed to generate the CA, but it does not exist, we should just throw an error
-                    checkCustomCaSecret(clusterCaConfig, clusterCaCertSecret, clusterCaKeySecret, "Cluster CA");
-
                     clusterCa = new ClusterCa(reconciliation, certManager, passwordGenerator, reconciliation.name(), clusterCaCertSecret,
                             clusterCaKeySecret,
                             ModelUtils.getCertificateValidity(clusterCaConfig),
@@ -273,9 +269,6 @@ Future<Void> reconcileCas(Clock clock) {
                             clusterCaSecrets,
                             Util.isMaintenanceTimeWindowsSatisfied(reconciliation, maintenanceWindows, clock.instant()));
 
-                    // When we are not supposed to generate the CA, but it does not exist, we should just throw an error
-                    checkCustomCaSecret(clientsCaConfig, clientsCaCertSecret, clientsCaKeySecret, "Clients CA");
-
                     clientsCa = new ClientsCa(reconciliation, certManager,
                             passwordGenerator, clientsCaCertName,
                             clientsCaCertSecret, clientsCaKeyName,
@@ -321,22 +314,6 @@ Future<Void> reconcileCas(Clock clock) {
                 });
     }
 
-    /**
-     * Utility method for checking the Secret existence when custom CA is used. The custom CA is configured but the
-     * secrets do not exist, it will throw InvalidConfigurationException.
-     *
-     * @param ca            The CA Configuration from the Custom Resource
-     * @param certSecret    Secret with the certificate public key
-     * @param keySecret     Secret with the certificate private key
-     * @param caDescription The name of the CA for which this check is executed ("Cluster CA" or "Clients CA" - used
-     *                      in the exception message)
-     */
-    private void checkCustomCaSecret(CertificateAuthority ca, Secret certSecret, Secret keySecret, String caDescription)   {
-        if (ca != null && !ca.isGenerateCertificateAuthority() && (certSecret == null || keySecret == null))   {
-            throw new InvalidResourceException(caDescription + " should not be generated, but the secrets were not found.");
-        }
-    }
-
     /**
      * Asynchronously reconciles the cluster operator Secret used to connect to Kafka and ZooKeeper.
      * This only updates the Secret if the latest Cluster CA is fully trusted across the cluster, otherwise if

diff --git a/...perator/src/test/java/io/strimzi/operator/cluster/operator/assembly/CaReconcilerTest.java b/...perator/src/test/java/io/strimzi/operator/cluster/operator/assembly/CaReconcilerTest.java
@@ -327,7 +327,7 @@ public void testReconcileCasWhenCustomCertsAreMissingThrows(Vertx vertx, VertxTe
         reconcileCa(vertx, certificateAuthority, certificateAuthority)
             .onComplete(context.failing(e -> context.verify(() -> {
                 assertThat(e, instanceOf(InvalidResourceException.class));
-                assertThat(e.getMessage(), is("Cluster CA should not be generated, but the secrets were not found."));
+                assertThat(e.getMessage(), is("cluster-ca should not be generated, but the secrets were not found."));
                 async.flag();
             })));
     }

diff --git a/operator-common/src/main/java/io/strimzi/operator/common/model/Ca.java b/operator-common/src/main/java/io/strimzi/operator/common/model/Ca.java
@@ -54,7 +54,6 @@
 import static java.time.temporal.ChronoField.SECOND_OF_MINUTE;
 import static java.time.temporal.ChronoField.YEAR;
 import static java.util.Collections.emptyList;
-import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonList;
 import static java.util.Collections.singletonMap;
 
@@ -525,12 +524,15 @@ public void createRenewOrReplace(
         int caKeyGeneration = keyGeneration();
 
         if (!generateCa) {
-            certData = caCertSecret != null ? caCertSecret.getData() : emptyMap();
-            keyData = caKeySecret != null ? singletonMap(CA_KEY, caKeySecret.getData().get(CA_KEY)) : emptyMap();
+            if (caCertSecret == null || caKeySecret == null)   {
+                throw new InvalidResourceException(this + " should not be generated, but the secrets were not found.");
+            }
+            certData = caCertSecret.getData();
+            keyData = singletonMap(CA_KEY, caKeySecret.getData().get(CA_KEY));
             renewalType = hasCaCertGenerationChanged(existingServerSecrets) ? RenewalType.REPLACE_KEY : RenewalType.NOOP;
             caCertsRemoved = false;
         } else {
-            this.renewalType = shouldCreateOrRenew(currentCert, namespace, clusterName, maintenanceWindowSatisfied);
+            this.renewalType = shouldCreateOrRenew(currentCert, maintenanceWindowSatisfied);
             LOGGER.debugCr(reconciliation, "{} renewalType {}", this, renewalType);
 
             switch (renewalType) {
@@ -636,7 +638,7 @@ private Subject nextCaSubject(int version) {
             .withOrganizationName(IO_STRIMZI).build();
     }
 
-    private RenewalType shouldCreateOrRenew(X509Certificate currentCert, String namespace, String clusterName, boolean maintenanceWindowSatisfied) {
+    private RenewalType shouldCreateOrRenew(X509Certificate currentCert, boolean maintenanceWindowSatisfied) {
         String reason = null;
         RenewalType renewalType = RenewalType.NOOP;
         if (caKeySecret == null
@@ -685,50 +687,15 @@ && certNeedsRenewal(currentCert)) {
             }
         }
 
-        logRenewalState(currentCert, namespace, clusterName, renewalType, reason);
-        return renewalType;
-    }
-
-    private void logRenewalState(X509Certificate currentCert, String namespace, String clusterName, RenewalType renewalType, String reason) {
         switch (renewalType) {
-            case REPLACE_KEY:
-            case RENEW_CERT:
-            case CREATE:
-                if (generateCa) {
+            case REPLACE_KEY, RENEW_CERT, CREATE ->
                     LOGGER.debugCr(reconciliation, "{}: {}: {}", this, renewalType.preDescription(caKeySecretName, caCertSecretName), reason);
-                } else {
+            case POSTPONED ->
                     LOGGER.warnCr(reconciliation, "{}: {}: {}", this, renewalType.preDescription(caKeySecretName, caCertSecretName), reason);
-                }
-                break;
-            case POSTPONED:
-                LOGGER.warnCr(reconciliation, "{}: {}: {}", this, renewalType.preDescription(caKeySecretName, caCertSecretName), reason);
-                break;
-            case NOOP:
-                LOGGER.debugCr(reconciliation, "{}: The CA certificate in secret {} already exists and does not need renewing", this, caCertSecretName);
-                break;
-        }
-        if (!generateCa) {
-            if (renewalType.equals(RenewalType.RENEW_CERT)) {
-                LOGGER.warnCr(reconciliation, "The certificate (data.{}) in Secret {} in namespace {} needs to be renewed " +
-                                "and it is not configured to automatically renew. This needs to be manually updated before that date. " +
-                                "Alternatively, configure Kafka.spec.tlsCertificates.generateCertificateAuthority=true in the Kafka resource with name {} in namespace {}.",
-                        CA_CRT.replace(".", "\\."), this.caCertSecretName, namespace,
-                        currentCert.getNotAfter());
-            } else if (renewalType.equals(RenewalType.REPLACE_KEY)) {
-                LOGGER.warnCr(reconciliation, "The private key (data.{}) in Secret {} in namespace {} needs to be renewed " +
-                                "and it is not configured to automatically renew. This needs to be manually updated before that date. " +
-                                "Alternatively, configure Kafka.spec.tlsCertificates.generateCertificateAuthority=true in the Kafka resource with name {} in namespace {}.",
-                        CA_KEY.replace(".", "\\."), this.caKeySecretName, namespace,
-                        currentCert.getNotAfter());
-            } else if (caCertSecret == null) {
-                LOGGER.warnCr(reconciliation, "The certificate (data.{}) in Secret {} and the private key (data.{}) in Secret {} in namespace {} " +
-                                "needs to be configured with a Base64 encoded PEM-format certificate. " +
-                                "Alternatively, configure Kafka.spec.tlsCertificates.generateCertificateAuthority=true in the Kafka resource with name {} in namespace {}.",
-                        CA_CRT.replace(".", "\\."), this.caCertSecretName,
-                        CA_KEY.replace(".", "\\."), this.caKeySecretName, namespace,
-                        clusterName, namespace);
-            }
+            case NOOP ->
+                    LOGGER.debugCr(reconciliation, "{}: The CA certificate in secret {} already exists and does not need renewing", this, caCertSecretName);
         }
+        return renewalType;
     }
 
     /**