Skip to content

Commit 229d1cb

Browse files
committed
fix: publish reload state atomically, skip store failures
A failed reload no longer leaves new ACME clients with the old certificate cache: both are published only after loading succeeds. Store failures in the renewal loop are logged and retried next cycle instead of burning attempts as CA rejections.
1 parent d57bbc2 commit 229d1cb

3 files changed

Lines changed: 20 additions & 11 deletions

File tree

carapace-server/src/main/java/org/carapaceproxy/server/certificates/DynamicCertificatesManager.java

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@
6767
import org.carapaceproxy.cluster.GroupMembershipHandler;
6868
import org.carapaceproxy.configstore.CertificateData;
6969
import org.carapaceproxy.configstore.ConfigurationStore;
70+
import org.carapaceproxy.configstore.ConfigurationStoreException;
7071
import org.carapaceproxy.core.HttpProxyServer;
7172
import org.carapaceproxy.core.RuntimeServerConfiguration;
7273
import org.carapaceproxy.server.config.ConfigurationNotValidException;
@@ -180,9 +181,11 @@ public synchronized void reloadConfiguration(RuntimeServerConfiguration configur
180181
loadOrCreateAcmeUserKeyPair(provider.name()), provider.url(), provider.kid(), provider.hmac()
181182
));
182183
}
183-
acmeClients = Map.copyOf(clients);
184184
domainsCheckerIPAddresses = configuration.getDomainsCheckerIPAddresses();
185-
loadCertificates(configuration.getCertificates());
185+
final var newCertificates = loadCertificates(configuration.getCertificates());
186+
// published together once loading succeeded: a failed reload must not leave new clients with the old cache
187+
acmeClients = Map.copyOf(clients);
188+
this.certificates = newCertificates;
186189
period = configuration.getDynamicCertificatesManagerPeriod();
187190
if (scheduledFuture != null) {
188191
scheduledFuture.cancel(true);
@@ -219,7 +222,7 @@ private ACMEClient acmeClientFor(CertificateData cert) {
219222
return client;
220223
}
221224

222-
private void loadCertificates(Map<String, SSLCertificateConfiguration> certificates) throws ConfigurationNotValidException {
225+
private Map<String, CertificateData> loadCertificates(Map<String, SSLCertificateConfiguration> certificates) throws ConfigurationNotValidException {
223226
try {
224227
final var _certificates = new ConcurrentHashMap<String, CertificateData>();
225228
for (Entry<String, SSLCertificateConfiguration> e : certificates.entrySet()) {
@@ -238,7 +241,7 @@ private void loadCertificates(Map<String, SSLCertificateConfiguration> certifica
238241
));
239242
}
240243
}
241-
this.certificates = _certificates; // only certificates/domains specified in the config have to be managed.
244+
return _certificates; // only certificates/domains specified in the config have to be managed.
242245
} catch (GeneralSecurityException | MalformedURLException e) {
243246
throw new DynamicCertificatesManagerException("Unable to load dynamic certificates configuration.", e);
244247
}
@@ -392,6 +395,9 @@ private void certificatesLifecycle() {
392395
store.saveCertificate(cert);
393396
flushCache = true;
394397
}
398+
} catch (ConfigurationStoreException ex) {
399+
// a store failure is infra-transient: retried at the next cycle, not counted as a CA rejection
400+
LOG.error("Store failure while handling dynamic certificate for domain {}", domain, ex);
395401
} catch (AcmeException | IOException | GeneralSecurityException | RuntimeException ex) {
396402
// RuntimeException included on purpose, as an escaped one would silently cancel the scheduled task;
397403
// this would kill the renewal loop for every certificate

carapace-server/src/test/java/org/carapaceproxy/server/certificates/CertificatesTest.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -492,6 +492,7 @@ public void testFailedConfigurationApplyRestoresCertificateData() throws Excepti
492492
CertificateData restored = store.loadCertificateForDomain("localhost2");
493493
assertEquals(previous.getChain(), restored.getChain());
494494
assertEquals(previous.getState(), restored.getState());
495+
assertEquals(previous.getProvider(), restored.getProvider());
495496
}
496497

497498
@Test

carapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.java

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@
6161
import org.carapaceproxy.cluster.impl.NullGroupMembershipHandler;
6262
import org.carapaceproxy.configstore.CertificateData;
6363
import org.carapaceproxy.configstore.ConfigurationStore;
64+
import org.carapaceproxy.configstore.ConfigurationStoreException;
6465
import org.carapaceproxy.configstore.PropertiesConfigurationStore;
6566
import org.carapaceproxy.core.HttpProxyServer;
6667
import org.carapaceproxy.core.Listeners;
@@ -736,19 +737,20 @@ public void testCertificateProviderRouting() throws Exception {
736737
}
737738

738739
@Test
739-
@Parameters({"stale", "transient"})
740+
@Parameters({"stale", "transient", "store"})
740741
public void testPendingOrderPollFailure(String failureCase) throws Exception {
741742
// ACME mocking: the pending order cannot be polled back from the CA
742743
ACMEClient ac = mock(ACMEClient.class);
743744
Login login = mock(Login.class);
744745
Order order = mock(Order.class);
745746
when(login.bindOrder(any())).thenReturn(order);
746747
when(ac.getLogin()).thenReturn(login);
747-
doThrow(failureCase.equals("transient")
748-
? new AcmeNetworkException(new IOException("connection reset"))
749-
// e.g., the order belongs to a different CA after a provider change
750-
: new AcmeException("unknown order")
751-
).when(ac).checkResponseForOrder(any());
748+
doThrow(switch (failureCase) {
749+
case "transient" -> new AcmeNetworkException(new IOException("connection reset"));
750+
case "store" -> new ConfigurationStoreException(new IOException("db down"));
751+
// e.g., the order belongs to a different CA after a provider change
752+
default -> new AcmeException("unknown order");
753+
}).when(ac).checkResponseForOrder(any());
752754

753755
HttpProxyServer parent = mock(HttpProxyServer.class);
754756
when(parent.getListeners()).thenReturn(mock(Listeners.class));
@@ -783,7 +785,7 @@ public void testPendingOrderPollFailure(String failureCase) throws Exception {
783785
man.run();
784786
assertCertificateState(domain, WAITING, 1, man);
785787
} else {
786-
// state untouched and nothing persisted, will be retried at the next cycle
788+
// transient ACME and store failures alike: state untouched and nothing persisted, retried at the next cycle
787789
assertCertificateState(domain, ORDERING, 0, man);
788790
verify(store, never()).saveCertificate(any());
789791
}

0 commit comments

Comments
 (0)