Skip to content

Commit

Permalink
Factored out a MockedCa for unit tests
Browse files Browse the repository at this point in the history 
Trivial methods refactoring

Signed-off-by: Paolo Patierno <[email protected]>
  • Loading branch information
@ppatierno
ppatierno committed Jan 25, 2022
1 parent 3448225 commit dd9e48b
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 195 deletions.
10 changes: 5 additions & 5 deletions cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ClusterCa.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,12 +248,12 @@ protected String caCertGenerationAnnotation() {

@SuppressWarnings("BooleanExpressionComplexity")
@Override
protected boolean isCaCertGenerationChanged() {
protected boolean hasCaCertGenerationChanged() {
// at least one Secret has a different cluster CA certificate thumbprint.
// it is useful when a renewal cluster CA certificate process needs to be recovered after an operator crash
return isCaCertGenerationChanged(zkNodesSecret) || isCaCertGenerationChanged(brokersSecret) ||
isCaCertGenerationChanged(entityTopicOperatorSecret) || isCaCertGenerationChanged(entityUserOperatorSecret) ||
isCaCertGenerationChanged(kafkaExporterSecret) || isCaCertGenerationChanged(cruiseControlSecret) ||
isCaCertGenerationChanged(clusterOperatorSecret);
return hasCaCertGenerationChanged(zkNodesSecret) || hasCaCertGenerationChanged(brokersSecret) ||
hasCaCertGenerationChanged(entityTopicOperatorSecret) || hasCaCertGenerationChanged(entityUserOperatorSecret) ||
hasCaCertGenerationChanged(kafkaExporterSecret) || hasCaCertGenerationChanged(cruiseControlSecret) ||
hasCaCertGenerationChanged(clusterOperatorSecret);
}
}
2 changes: 1 addition & 1 deletion cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ModelUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ public static Secret buildSecret(Reconciliation reconciliation, ClusterCa cluste
} else {
if (clusterCa.keyCreated() || clusterCa.certRenewed() ||
(isMaintenanceTimeWindowsSatisfied && clusterCa.isExpiring(secret, keyCertName + ".crt")) ||
clusterCa.isCaCertGenerationChanged(secret)) {
clusterCa.hasCaCertGenerationChanged(secret)) {
reasons.add("certificate needs to be renewed");
shouldBeRegenerated = true;
}
Expand Down
240 changes: 72 additions & 168 deletions cluster-operator/src/test/java/io/strimzi/operator/cluster/model/CaRenewalTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@

import io.fabric8.kubernetes.api.model.Secret;
import io.fabric8.kubernetes.api.model.SecretBuilder;
import io.strimzi.api.kafka.model.CertificateExpirationPolicy;
import io.strimzi.certs.CertAndKey;
import io.strimzi.certs.CertManager;
import io.strimzi.certs.Subject;
import io.strimzi.operator.common.PasswordGenerator;
import io.strimzi.operator.common.Reconciliation;
import io.strimzi.test.annotations.ParallelSuite;
import io.strimzi.test.annotations.ParallelTest;
Expand All @@ -30,43 +33,7 @@
public class CaRenewalTest {
@ParallelTest
public void renewalOfStatefulSetCertificatesWithNullSecret() throws IOException {
Ca mockedCa = new Ca(Reconciliation.DUMMY_RECONCILIATION, null, null, null, null, null, null, null, 2, 1, true, null) {
private AtomicInteger invocationCount = new AtomicInteger(0);

@Override
public boolean certRenewed() {
return false;
}

@Override
public boolean isExpiring(Secret secret, String certKey) {
return false;
}

@Override
protected CertAndKey generateSignedCert(Subject subject,
File csrFile, File keyFile, File certFile, File keyStoreFile) throws IOException {
int index = invocationCount.getAndIncrement();

return new CertAndKey(
("new-key" + index).getBytes(),
("new-cert" + index).getBytes(),
("new-truststore" + index).getBytes(),
("new-keystore" + index).getBytes(),
"new-password" + index
);
}

@Override
protected boolean isCaCertGenerationChanged() {
return false;
}

@Override
protected String caCertGenerationAnnotation() {
return null;
}
};
Ca mockedCa = new MockedCa(Reconciliation.DUMMY_RECONCILIATION, null, null, null, null, null, null, null, 2, 1, true, null);

int replicas = 3;
Function<Integer, Subject> subjectFn = i -> new Subject.Builder().build();
Expand Down Expand Up @@ -97,43 +64,8 @@ protected String caCertGenerationAnnotation() {

@ParallelTest
public void renewalOfStatefulSetCertificatesWithCaRenewal() throws IOException {
Ca mockedCa = new Ca(Reconciliation.DUMMY_RECONCILIATION, null, null, null, null, null, null, null, 2, 1, true, null) {
private AtomicInteger invocationCount = new AtomicInteger(0);

@Override
public boolean certRenewed() {
return true;
}

@Override
public boolean isExpiring(Secret secret, String certKey) {
return false;
}

@Override
protected CertAndKey generateSignedCert(Subject subject,
File csrFile, File keyFile, File certFile, File keyStoreFile) throws IOException {
int index = invocationCount.getAndIncrement();

return new CertAndKey(
("new-key" + index).getBytes(),
("new-cert" + index).getBytes(),
("new-truststore" + index).getBytes(),
("new-keystore" + index).getBytes(),
"new-password" + index
);
}

@Override
protected boolean isCaCertGenerationChanged() {
return false;
}

@Override
protected String caCertGenerationAnnotation() {
return null;
}
};
MockedCa mockedCa = new MockedCa(Reconciliation.DUMMY_RECONCILIATION, null, null, null, null, null, null, null, 2, 1, true, null);
mockedCa.setCertRenewed(true);

Secret initialSecret = new SecretBuilder()
.withNewMetadata()
Expand Down Expand Up @@ -183,53 +115,8 @@ protected String caCertGenerationAnnotation() {

@ParallelTest
public void renewalOfStatefulSetCertificatesDelayedRenewalInWindow() throws IOException {
Ca mockedCa = new Ca(Reconciliation.DUMMY_RECONCILIATION, null, null, null, null, null, null, null, 2, 1, true, null) {
private AtomicInteger invocationCount = new AtomicInteger(0);

@Override
public boolean certRenewed() {
return false;
}

@Override
public boolean isExpiring(Secret secret, String certKey) {
return true;
}

@Override
protected boolean certSubjectChanged(CertAndKey certAndKey, Subject desiredSubject, String podName) {
return false;
}

@Override
public X509Certificate getAsX509Certificate(Secret secret, String key) {
return null;
}

@Override
protected CertAndKey generateSignedCert(Subject subject,
File csrFile, File keyFile, File certFile, File keyStoreFile) throws IOException {
int index = invocationCount.getAndIncrement();

return new CertAndKey(
("new-key" + index).getBytes(),
("new-cert" + index).getBytes(),
("new-truststore" + index).getBytes(),
("new-keystore" + index).getBytes(),
"new-password" + index
);
}

@Override
protected boolean isCaCertGenerationChanged() {
return false;
}

@Override
protected String caCertGenerationAnnotation() {
return null;
}
};
MockedCa mockedCa = new MockedCa(Reconciliation.DUMMY_RECONCILIATION, null, null, null, null, null, null, null, 2, 1, true, null);
mockedCa.setCertExpiring(true);

Secret initialSecret = new SecretBuilder()
.withNewMetadata()
Expand Down Expand Up @@ -279,53 +166,8 @@ protected String caCertGenerationAnnotation() {

@ParallelTest
public void renewalOfStatefulSetCertificatesDelayedRenewalOutsideWindow() throws IOException {
Ca mockedCa = new Ca(Reconciliation.DUMMY_RECONCILIATION, null, null, null, null, null, null, null, 2, 1, true, null) {
private AtomicInteger invocationCount = new AtomicInteger(0);

@Override
public boolean certRenewed() {
return false;
}

@Override
public boolean isExpiring(Secret secret, String certKey) {
return true;
}

@Override
protected boolean certSubjectChanged(CertAndKey certAndKey, Subject desiredSubject, String podName) {
return false;
}

@Override
public X509Certificate getAsX509Certificate(Secret secret, String key) {
return null;
}

@Override
protected CertAndKey generateSignedCert(Subject subject,
File csrFile, File keyFile, File certFile, File keyStoreFile) throws IOException {
int index = invocationCount.getAndIncrement();

return new CertAndKey(
("new-key" + index).getBytes(),
("new-cert" + index).getBytes(),
("new-truststore" + index).getBytes(),
("new-keystore" + index).getBytes(),
"new-password" + index
);
}

@Override
protected boolean isCaCertGenerationChanged() {
return false;
}

@Override
protected String caCertGenerationAnnotation() {
return null;
}
};
MockedCa mockedCa = new MockedCa(Reconciliation.DUMMY_RECONCILIATION, null, null, null, null, null, null, null, 2, 1, true, null);
mockedCa.setCertExpiring(true);

Secret initialSecret = new SecretBuilder()
.withNewMetadata()
Expand Down Expand Up @@ -372,4 +214,66 @@ protected String caCertGenerationAnnotation() {
assertThat(new String(newCerts.get("pod2").keyStore()), is("old-keystore"));
assertThat(newCerts.get("pod2").storePassword(), is("old-password"));
}

public class MockedCa extends Ca {
private boolean isCertRenewed;
private boolean isCertExpiring;
private AtomicInteger invocationCount = new AtomicInteger(0);

public MockedCa(Reconciliation reconciliation, CertManager certManager, PasswordGenerator passwordGenerator, String commonName, String caCertSecretName, Secret caCertSecret, String caKeySecretName, Secret caKeySecret, int validityDays, int renewalDays, boolean generateCa, CertificateExpirationPolicy policy) {
super(reconciliation, certManager, passwordGenerator, commonName, caCertSecretName, caCertSecret, caKeySecretName, caKeySecret, validityDays, renewalDays, generateCa, policy);
}

@Override
public boolean certRenewed() {
return isCertRenewed;
}

@Override
public boolean isExpiring(Secret secret, String certKey) {
return isCertExpiring;
}

@Override
protected boolean certSubjectChanged(CertAndKey certAndKey, Subject desiredSubject, String podName) {
return false;
}

@Override
public X509Certificate getAsX509Certificate(Secret secret, String key) {
return null;
}

@Override
protected CertAndKey generateSignedCert(Subject subject,
File csrFile, File keyFile, File certFile, File keyStoreFile) throws IOException {
int index = invocationCount.getAndIncrement();

return new CertAndKey(
("new-key" + index).getBytes(),
("new-cert" + index).getBytes(),
("new-truststore" + index).getBytes(),
("new-keystore" + index).getBytes(),
"new-password" + index
);
}

@Override
protected boolean hasCaCertGenerationChanged() {
return false;
}

@Override
protected String caCertGenerationAnnotation() {
return null;
}

public void setCertRenewed(boolean certRenewed) {
isCertRenewed = certRenewed;
}

public void setCertExpiring(boolean certExpiring) {
isCertExpiring = certExpiring;
}
}
}
22 changes: 3 additions & 19 deletions operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.security.KeyStoreException;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.cert.Certificate;
import java.security.cert.CertificateException;
Expand Down Expand Up @@ -515,7 +514,7 @@ public void createRenewOrReplace(String namespace, String clusterName, Map<Strin
if (!generateCa) {
certData = caCertSecret != null ? caCertSecret.getData() : emptyMap();
keyData = caKeySecret != null ? singletonMap(CA_KEY, caKeySecret.getData().get(CA_KEY)) : emptyMap();
renewalType = isCaCertGenerationChanged() ? RenewalType.REPLACE_KEY : RenewalType.NOOP;
renewalType = hasCaCertGenerationChanged() ? RenewalType.REPLACE_KEY : RenewalType.NOOP;
caCertsRemoved = false;
} else {
this.renewalType = shouldCreateOrRenew(currentCert, namespace, clusterName, maintenanceWindowSatisfied);
Expand Down Expand Up @@ -729,21 +728,6 @@ public byte[] currentCaKey() {
return decoder.decode(caKeySecret().getData().get(CA_KEY));
}

/**
* @return The base64 encoded thumbprint of the current CA certificate.
*/
public String currentCaCertThumbprint() {
try {
X509Certificate cert = Ca.x509Certificate(currentCaCertBytes());
byte[] signature = MessageDigest.getInstance("SHA-256").digest(cert.getEncoded());
return Base64.getEncoder().encodeToString(signature);
} catch (CertificateException e) {
throw new RuntimeException("Failed to decode Cluster CA certificate", e);
} catch (NoSuchAlgorithmException e) {
throw new RuntimeException("Failed to get certificate signature of Cluster CA", e);
}
}

/**
* True if the last call to {@link #createRenewOrReplace(String, String, Map, Map, Map, OwnerReference, boolean)}
* resulted in expired certificates being removed from the CA {@code Secret}.
Expand Down Expand Up @@ -1047,7 +1031,7 @@ private void renewCaCert(Subject subject, Map<String, String> certData) {
* @return if the current (cluster or clients) CA certificate generation is changed compared to the the one
* brought on Secrets containing certificates signed by that CA (i.e ZooKeeper nodes, Kafka brokers, ...)
*/
protected abstract boolean isCaCertGenerationChanged();
protected abstract boolean hasCaCertGenerationChanged();

/**
* It checks if the current (cluster or clients) CA certificate generation is changed compared to the one
Expand All @@ -1057,7 +1041,7 @@ private void renewCaCert(Subject subject, Map<String, String> certData) {
* @return if the current (cluster or clients) CA certificate generation is changed compared to the one
* brought by the corresponding annotation on the provided Secret
*/
protected boolean isCaCertGenerationChanged(Secret secret) {
protected boolean hasCaCertGenerationChanged(Secret secret) {
if (secret != null) {
String caCertGenerationAnno = Optional.ofNullable(secret.getMetadata().getAnnotations())
.map(annotations -> annotations.get(caCertGenerationAnnotation()))
Expand Down
4 changes: 2 additions & 2 deletions operator-common/src/main/java/io/strimzi/operator/cluster/model/ClientsCa.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ protected String caCertGenerationAnnotation() {
}

@Override
protected boolean isCaCertGenerationChanged() {
return isCaCertGenerationChanged(brokersSecret);
protected boolean hasCaCertGenerationChanged() {
return hasCaCertGenerationChanged(brokersSecret);
}

@Override
Expand Down

0 comments on commit dd9e48b

Please sign in to comment.