Skip to content

Commit

Permalink
Accidentally removed CA should trigger crt recreation (and rolling up…
Browse files Browse the repository at this point in the history 
…date) (strimzi#2756)

* Accidentally removed CA should trigger crt recreation (and rolling update)

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* comments

Signed-off-by: Stanislav Knot <sknot@redhat.com>
  • Loading branch information
@sknot-rh @klalafaryan
sknot-rh authored and klalafaryan committed Oct 21, 2020
1 parent 943d0e4 commit 1bbf58c
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 9 deletions.
4 changes: 2 additions & 2 deletions cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ModelUtils.java
View file
Expand Up @@ -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);
Expand Down
22 changes: 15 additions & 7 deletions operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java
View file
Expand Up @@ -387,8 +387,12 @@ protected Map<String, CertAndKey> 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);
Expand Down Expand Up @@ -552,7 +556,7 @@ public void createRenewOrReplace(String namespace, String clusterName, Map<Strin
Map<String, String> 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"));
Expand All @@ -561,7 +565,7 @@ public void createRenewOrReplace(String namespace, String clusterName, Map<Strin
Map<String, String> 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"));
Expand Down Expand Up @@ -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 {}.",
Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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<String, String> newData) {
Expand Down
37 changes: 37 additions & 0 deletions systemtest/src/test/java/io/strimzi/systemtest/RollingUpdateST.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String, String> kafkaPods = StatefulSetUtils.ssSnapshot(KafkaResources.kafkaStatefulSetName(CLUSTER_NAME));
Map<String, String> 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<String> bindingsNamespaces) {
super.recreateTestEnv(coNamespace, bindingsNamespaces, Constants.CO_OPERATION_TIMEOUT_SHORT);
Expand Down

0 comments on commit 1bbf58c

Please sign in to comment.