Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from Apr 1, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa06f03
Accidentally removed CA should trigger crt recreation (and rolling up…
sknot-rh Mar 27, 2020
59a459e
comments
sknot-rh Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ModelUtils.java
View file
Open in desktop
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() || (clusterCa.isExpiring(secret, keyCertName + ".crt") && isMaintenanceTimeWindowsSatisfied)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am reading the logic correctly, might it make sense to put isMaintenanceTimeWindowsSatisfied first, i.e

if (isMaintenanceTimeWindowsSatisfied && ...

As the prior three checks dont matter if this is false?

Copy link
Member Author

@sknot-rh sknot-rh Mar 30, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isMaintenanceTimeWindowsSatisfied is tied just for this part of condition: (clusterCa.isExpiring(secret, keyCertName + ".crt") && isMaintenanceTimeWindowsSatisfied)
However it makes sense to move is as the first one in this scope. Good catch.

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
10 changes: 9 additions & 1 deletion operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java
View file
Open in desktop
Expand Up @@ -387,8 +387,12 @@ protected Map<String, CertAndKey> maybeCopyOrGenerateCerts(
reasons.add("certificate is expiring");
}

if (renewalType == 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 @@ -737,6 +741,10 @@ public boolean keyReplaced() {
return renewalType == RenewalType.REPLACE_KEY;
}

public boolean keyCreated() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isKeyCreated would be slightly more idiomatic, but given the other similar methods are not using is I can understand why you matched the convention

return renewalType == RenewalType.CREATE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly stylistic but I believe .equals is generally preferred to == as we can choose to override how we compare objects and can avoid object hashes not matching https://stackoverflow.com/questions/7520432/what-is-the-difference-between-and-equals-in-java
99% of the time it doesn't matter but thought it was worth a mention.

}

private int removeExpiredCerts(Map<String, String> newData) {
Iterator<Map.Entry<String, String>> iter = newData.entrySet().iterator();
List<String> removed = new ArrayList<>();
Expand Down
36 changes: 36 additions & 0 deletions systemtest/src/test/java/io/strimzi/systemtest/RollingUpdateST.java
View file
Open in desktop
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,41 @@ 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 testAccidentallyRemovedCaTriggersRollingUpdate() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be overly nit-picky, but the cause of why it is removed could be seen as not part of the 'test'
I would opt to have it be called something like

testClusterCaRemovedTriggersRollingUpdate

and then have a documented comment explaining the context of this test is it being accidentally removed.

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();

String userName = "alice";
KafkaUser user = KafkaUserResource.tlsUser(CLUSTER_NAME, userName).done();
sknot-rh marked this conversation as resolved.
Show resolved Hide resolved

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, userName, 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");
kubeClient(NAMESPACE).deleteSecret(KafkaResources.clusterCaKeySecretName(CLUSTER_NAME));
sknot-rh marked this conversation as resolved.
Show resolved Hide resolved
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, userName, 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