fix concurrent reads during rotation with expiration

jakelandis · Sep 11, 2023 · c84815f · c84815f
1 parent 323be40
commit c84815f
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 10 deletions.
diff --git a/server/src/main/java/org/elasticsearch/common/settings/RotatableSecret.java b/server/src/main/java/org/elasticsearch/common/settings/RotatableSecret.java
@@ -100,15 +100,21 @@ private void checkExpired() {
         }
         try {
             if (expired) {
-                stamp = stampedLock.tryConvertToWriteLock(stamp);
-                if (stamp == 0) {
-                    // block until we can acquire the write lock
+                long stampUpgrade = stampedLock.tryConvertToWriteLock(stamp);
+                if (stampUpgrade == 0) {
+                    // upgrade failed so we need to manually unlock the read lock and grab the write lock
+                    stampedLock.unlockRead(stamp);
                     stamp = stampedLock.writeLock();
+                    expired = secrets.prior != null && secrets.priorValidTill.isBefore(Instant.now()); // check again since we had to unlock
+                } else {
+                    stamp = stampUpgrade;
                 }
                 needToUnlock = true;
-                SecureString prior = secrets.prior;
-                secrets = new Secrets(secrets.current, null, Instant.EPOCH);
-                prior.close(); // zero out the memory
+                if (expired) {
+                    SecureString prior = secrets.prior;
+                    secrets = new Secrets(secrets.current, null, Instant.EPOCH);
+                    prior.close(); // zero out the memory
+                }
             }
         } finally {
             if (needToUnlock) { // only unlock if we acquired a read or write lock

diff --git a/server/src/test/java/org/elasticsearch/common/settings/RotatableSecretTests.java b/server/src/test/java/org/elasticsearch/common/settings/RotatableSecretTests.java
@@ -97,25 +97,31 @@ public void testConcurrentReadWhileLocked() throws Exception {
         assertEquals(secret1, rotatableSecret.getSecrets().current());
         assertNull(rotatableSecret.getSecrets().prior());
 
+        boolean expired = randomBoolean();
         CountDownLatch latch = new CountDownLatch(1);
         TimeValue mockGracePeriod = mock(TimeValue.class);  // use a mock to force a long rotation to exercise the concurrency
         when(mockGracePeriod.getMillis()).then((Answer<Long>) invocation -> {
             latch.await();
-            return Long.MAX_VALUE;
+            return expired ? 0L : Long.MAX_VALUE;
         });
 
         // start writer thread
         Thread t1 = new Thread(() -> rotatableSecret.rotate(secret2, mockGracePeriod));
         t1.start();
         assertBusy(() -> assertEquals(Thread.State.WAITING, t1.getState())); // waiting on countdown latch, holds write lock
+        assertTrue(rotatableSecret.isWriteLocked());
 
         // start reader threads
         int readers = randomIntBetween(0, 16);
         Set<Thread> readerThreads = new HashSet<>(readers);
-        for (int i = 0; i <= readers; i++) {
+        for (int i = 0; i < readers; i++) {
             Thread t = new Thread(() -> {
                 if (randomBoolean()) { // either matches or isSet can block
-                    assertTrue(rotatableSecret.matches(secret1));
+                    if (expired) {
+                        assertFalse(rotatableSecret.matches(secret1));
+                    } else {
+                        assertTrue(rotatableSecret.matches(secret1));
+                    }
                     assertTrue(rotatableSecret.matches(secret2));
                 } else {
                     assertTrue(rotatableSecret.isSet());
@@ -130,12 +136,12 @@ public void testConcurrentReadWhileLocked() throws Exception {
         assertTrue(rotatableSecret.isWriteLocked());
         latch.countDown(); // let thread1 finish, which also unblocks the reader threads
         assertBusy(() -> assertEquals(Thread.State.TERMINATED, t1.getState())); // done with work
-        assertFalse(rotatableSecret.isWriteLocked());
         for (Thread t : readerThreads) {
             assertBusy(() -> assertEquals(Thread.State.TERMINATED, t.getState())); // done with work
             t.join();
         }
         t1.join();
+        assertFalse(rotatableSecret.isWriteLocked());
     }
 
     public void testConcurrentRotations() throws Exception {