From 1bbf58c794a9bcb4ccdeea7bde2a390427a34abf Mon Sep 17 00:00:00 2001 From: Stanislav Knot Date: Wed, 1 Apr 2020 22:10:57 +0200 Subject: [PATCH] Accidentally removed CA should trigger crt recreation (and rolling update) (#2756) * Accidentally removed CA should trigger crt recreation (and rolling update) Signed-off-by: Stanislav Knot * comments Signed-off-by: Stanislav Knot --- .../operator/cluster/model/ModelUtils.java | 4 +- .../io/strimzi/operator/cluster/model/Ca.java | 22 +++++++---- .../strimzi/systemtest/RollingUpdateST.java | 37 +++++++++++++++++++ 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ModelUtils.java b/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ModelUtils.java index c7d3a5011f..f6cb32714a 100644 --- a/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ModelUtils.java +++ b/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ModelUtils.java @@ -234,14 +234,14 @@ public static Secret buildSecret(ClusterCa clusterCa, Secret secret, String name reasons.add("certificate doesn't exist yet"); shouldBeRegenerated = true; } else { - if (clusterCa.certRenewed() || (clusterCa.isExpiring(secret, keyCertName + ".crt") && isMaintenanceTimeWindowsSatisfied)) { + if (clusterCa.keyCreated() || clusterCa.certRenewed() || (isMaintenanceTimeWindowsSatisfied && clusterCa.isExpiring(secret, keyCertName + ".crt"))) { reasons.add("certificate needs to be renewed"); shouldBeRegenerated = true; } } if (shouldBeRegenerated) { - log.debug("Certificate for pod {} need to be regenerated because:", keyCertName, String.join(", ", reasons)); + log.debug("Certificate for pod {} need to be regenerated because: {}", keyCertName, String.join(", ", reasons)); try { certAndKey = clusterCa.generateSignedCert(commonName, Ca.IO_STRIMZI); diff --git a/operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java b/operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java index b63edb7ebd..2a32df5ffe 100644 --- a/operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java +++ b/operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java @@ -387,8 +387,12 @@ protected Map maybeCopyOrGenerateCerts( reasons.add("certificate is expiring"); } + if (renewalType.equals(RenewalType.CREATE)) { + reasons.add("certificate added"); + } + if (!reasons.isEmpty()) { - log.debug("Certificate for pod {} need to be regenerated because:", podName, String.join(", ", reasons)); + log.debug("Certificate for pod {} need to be regenerated because: {}", podName, String.join(", ", reasons)); CertAndKey newCertAndKey = generateSignedCert(subject, brokerCsrFile, brokerKeyFile, brokerCertFile, brokerKeyStoreFile); certs.put(podName, newCertAndKey); @@ -552,7 +556,7 @@ public void createRenewOrReplace(String namespace, String clusterName, Map certAnnotations = new HashMap<>(2); certAnnotations.put(ANNO_STRIMZI_IO_CA_CERT_GENERATION, String.valueOf(caCertGeneration)); - if (renewalType == RenewalType.POSTPONED + if (renewalType.equals(RenewalType.POSTPONED) && this.caCertSecret.getMetadata() != null && Annotations.hasAnnotation(caCertSecret, ANNO_STRIMZI_IO_FORCE_RENEW)) { certAnnotations.put(ANNO_STRIMZI_IO_FORCE_RENEW, Annotations.stringAnnotation(caCertSecret, ANNO_STRIMZI_IO_FORCE_RENEW, "false")); @@ -561,7 +565,7 @@ public void createRenewOrReplace(String namespace, String clusterName, Map keyAnnotations = new HashMap<>(2); keyAnnotations.put(ANNO_STRIMZI_IO_CA_KEY_GENERATION, String.valueOf(caKeyGeneration)); - if (renewalType == RenewalType.POSTPONED + if (renewalType.equals(RenewalType.POSTPONED) && this.caKeySecret.getMetadata() != null && Annotations.hasAnnotation(caKeySecret, ANNO_STRIMZI_IO_FORCE_REPLACE)) { keyAnnotations.put(ANNO_STRIMZI_IO_FORCE_REPLACE, Annotations.stringAnnotation(caKeySecret, ANNO_STRIMZI_IO_FORCE_REPLACE, "false")); @@ -650,13 +654,13 @@ private void logRenewalState(X509Certificate currentCert, String namespace, Stri break; } if (!generateCa) { - if (renewalType == RenewalType.RENEW_CERT) { + if (renewalType.equals(RenewalType.RENEW_CERT)) { log.warn("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 == RenewalType.REPLACE_KEY) { + } else if (renewalType.equals(RenewalType.REPLACE_KEY)) { log.warn("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 {}.", @@ -725,7 +729,7 @@ public boolean certsRemoved() { * @return Whether the certificate was renewed. */ public boolean certRenewed() { - return renewalType == RenewalType.RENEW_CERT || renewalType == RenewalType.REPLACE_KEY; + return renewalType.equals(RenewalType.RENEW_CERT) || renewalType.equals(RenewalType.REPLACE_KEY); } /** @@ -734,7 +738,11 @@ public boolean certRenewed() { * @return Whether the key was replaced. */ public boolean keyReplaced() { - return renewalType == RenewalType.REPLACE_KEY; + return renewalType.equals(RenewalType.REPLACE_KEY); + } + + public boolean keyCreated() { + return renewalType.equals(RenewalType.CREATE); } private int removeExpiredCerts(Map newData) { diff --git a/systemtest/src/test/java/io/strimzi/systemtest/RollingUpdateST.java b/systemtest/src/test/java/io/strimzi/systemtest/RollingUpdateST.java index 52db91f378..e5f58aca3a 100644 --- a/systemtest/src/test/java/io/strimzi/systemtest/RollingUpdateST.java +++ b/systemtest/src/test/java/io/strimzi/systemtest/RollingUpdateST.java @@ -64,6 +64,7 @@ import static java.util.Collections.singletonMap; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.collection.IsMapContaining.hasEntry; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -777,6 +778,42 @@ void testTriggerRollingUpdateAfterOverrideBootstrap() throws CertificateExceptio // TODO: send and recv messages via this new bootstrap (after client builder) https://github.com/strimzi/strimzi-kafka-operator/pull/2520 } + @Test + void testClusterCaRemovedTriggersRollingUpdate() { + KafkaResource.kafkaPersistent(CLUSTER_NAME, 3, 3).done(); + String topicName = "test-topic-" + new Random().nextInt(Integer.MAX_VALUE); + KafkaTopicResource.topic(CLUSTER_NAME, topicName, 2, 2).done(); + + KafkaUser user = KafkaUserResource.tlsUser(CLUSTER_NAME, USER_NAME).done(); + + KafkaClientsResource.deployKafkaClients(true, CLUSTER_NAME + "-" + Constants.KAFKA_CLIENTS, user).done(); + final String defaultKafkaClientsPodName = + ResourceManager.kubeClient().listPodsByPrefixInName(CLUSTER_NAME + "-" + Constants.KAFKA_CLIENTS).get(0).getMetadata().getName(); + + internalKafkaClient.setPodName(defaultKafkaClientsPodName); + + Map kafkaPods = StatefulSetUtils.ssSnapshot(KafkaResources.kafkaStatefulSetName(CLUSTER_NAME)); + Map zkPods = StatefulSetUtils.ssSnapshot(KafkaResources.zookeeperStatefulSetName(CLUSTER_NAME)); + + int sent = internalKafkaClient.sendMessagesTls(topicName, NAMESPACE, CLUSTER_NAME, USER_NAME, MESSAGE_COUNT, "TLS"); + assertThat(sent, is(MESSAGE_COUNT)); + + String zkCrtBeforeAccident = kubeClient(NAMESPACE).getSecret(CLUSTER_NAME + "-zookeeper-nodes").getData().get(CLUSTER_NAME + "-zookeeper-0.crt"); + String kCrtBeforeAccident = kubeClient(NAMESPACE).getSecret(CLUSTER_NAME + "-kafka-brokers").getData().get(CLUSTER_NAME + "-kafka-0.crt"); + + // some kind of accident happened and the CA secret has been deleted + kubeClient().deleteSecret(KafkaResources.clusterCaKeySecretName(CLUSTER_NAME)); + StatefulSetUtils.waitTillSsHasRolled(KafkaResources.kafkaStatefulSetName(CLUSTER_NAME), 3, kafkaPods); + + assertThat(kubeClient(NAMESPACE).getSecret(CLUSTER_NAME + "-zookeeper-nodes").getData().get(CLUSTER_NAME + "-zookeeper-0.crt"), is(not(zkCrtBeforeAccident))); + assertThat(kubeClient(NAMESPACE).getSecret(CLUSTER_NAME + "-kafka-brokers").getData().get(CLUSTER_NAME + "-kafka-0.crt"), is(not(kCrtBeforeAccident))); + assertThat(StatefulSetUtils.ssSnapshot(KafkaResources.zookeeperStatefulSetName(CLUSTER_NAME)), is(not(zkPods))); + assertThat(StatefulSetUtils.ssSnapshot(KafkaResources.kafkaStatefulSetName(CLUSTER_NAME)), is(not(kafkaPods))); + + int sentAfter = internalKafkaClient.sendMessagesTls(topicName, NAMESPACE, CLUSTER_NAME, USER_NAME, MESSAGE_COUNT, "TLS"); + assertThat(sentAfter, is(MESSAGE_COUNT)); + } + @Override protected void recreateTestEnv(String coNamespace, List bindingsNamespaces) { super.recreateTestEnv(coNamespace, bindingsNamespaces, Constants.CO_OPERATION_TIMEOUT_SHORT);