Publish single rotated on secrets rotation for atomic propertysource updates

We now publish a single SecretLeaseRotatedEvent instead of publishing two events (SecretLeaseExpiredEvent and SecretLeaseCreatedEvent) to atomically update notify listeners such as LeaseAwareVaultPropertySource for atomic updates.

Closes gh-594.

spring-vault-core/src/main/java/org/springframework/vault/core/env/LeaseAwareVaultPropertySource.java

@@ -15,6 +15,8 @@
  */
 package org.springframework.vault.core.env;
 
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
@@ -34,6 +36,7 @@ import org.springframework.vault.core.lease.event.LeaseListenerAdapter;
 import org.springframework.vault.core.lease.event.SecretLeaseCreatedEvent;
 import org.springframework.vault.core.lease.event.SecretLeaseEvent;
 import org.springframework.vault.core.lease.event.SecretLeaseExpiredEvent;
+import org.springframework.vault.core.lease.event.SecretLeaseRotatedEvent;
 import org.springframework.vault.core.lease.event.SecretNotFoundEvent;
 import org.springframework.vault.core.util.PropertyTransformer;
 import org.springframework.vault.core.util.PropertyTransformers;
@@ -233,7 +236,18 @@ public class LeaseAwareVaultPropertySource extends EnumerablePropertySource<Vaul
 		if (leaseEvent instanceof SecretLeaseCreatedEvent) {
 
 			SecretLeaseCreatedEvent created = (SecretLeaseCreatedEvent) leaseEvent;
-			properties.putAll(doTransformProperties(flattenMap(created.getSecrets())));
+
+			Map<String, Object> secrets = doTransformProperties(flattenMap(created.getSecrets()));
+
+			if (leaseEvent instanceof SecretLeaseRotatedEvent) {
+
+				List<String> removedKeys = new ArrayList<>(properties.keySet());
+
+				removedKeys.removeAll(secrets.keySet());
+				removedKeys.forEach(properties::remove);
+			}
+
+			properties.putAll(secrets);
 		}
 	}
 

spring-vault-core/src/main/java/org/springframework/vault/core/lease/SecretLeaseContainer.java

@@ -30,6 +30,7 @@ import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.BiConsumer;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -375,6 +376,16 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements I
 
 	private void start(RequestedSecret requestedSecret, LeaseRenewalScheduler renewalScheduler) {
 
+		doStart(requestedSecret, renewalScheduler, (secrets, lease) -> {
+			onSecretsObtained(requestedSecret, lease, secrets.getRequiredData());
+		}, () -> {
+		});
+	}
+
+	private void doStart(RequestedSecret requestedSecret, LeaseRenewalScheduler renewalScheduler,
+			BiConsumer<VaultResponseSupport<Map<String, Object>>, Lease> callback,
+			Runnable cannotObtainSecretsCallback) {
+
 		VaultResponseSupport<Map<String, Object>> secrets = doGetSecrets(requestedSecret);
 
 		if (secrets != null) {
@@ -399,7 +410,10 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements I
 				scheduleLeaseRotation(requestedSecret, lease, renewalScheduler);
 			}
 
-			onSecretsObtained(requestedSecret, lease, secrets.getRequiredData());
+			callback.accept(secrets, lease);
+		}
+		else {
+			cannotObtainSecretsCallback.run();
 		}
 	}
 
@@ -723,10 +737,13 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements I
 	 */
 	protected void onLeaseExpired(RequestedSecret requestedSecret, Lease lease) {
 
-		super.onLeaseExpired(requestedSecret, lease);
-
 		if (requestedSecret.getMode() == Mode.ROTATE) {
-			start(requestedSecret, this.renewals.get(requestedSecret));
+			doStart(requestedSecret, this.renewals.get(requestedSecret), (secrets, currentLease) -> {
+				onSecretsRotated(requestedSecret, lease, currentLease, secrets.getRequiredData());
+			}, () -> super.onLeaseExpired(requestedSecret, lease));
+		}
+		else {
+			super.onLeaseExpired(requestedSecret, lease);
 		}
 	}
 

spring-vault-core/src/main/java/org/springframework/vault/core/lease/SecretLeaseEventPublisher.java

@@ -27,16 +27,7 @@ import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 import org.springframework.vault.core.lease.domain.Lease;
 import org.springframework.vault.core.lease.domain.RequestedSecret;
-import org.springframework.vault.core.lease.event.AfterSecretLeaseRenewedEvent;
-import org.springframework.vault.core.lease.event.AfterSecretLeaseRevocationEvent;
-import org.springframework.vault.core.lease.event.BeforeSecretLeaseRevocationEvent;
-import org.springframework.vault.core.lease.event.LeaseErrorListener;
-import org.springframework.vault.core.lease.event.LeaseListener;
-import org.springframework.vault.core.lease.event.SecretLeaseCreatedEvent;
-import org.springframework.vault.core.lease.event.SecretLeaseErrorEvent;
-import org.springframework.vault.core.lease.event.SecretLeaseEvent;
-import org.springframework.vault.core.lease.event.SecretLeaseExpiredEvent;
-import org.springframework.vault.core.lease.event.SecretNotFoundEvent;
+import org.springframework.vault.core.lease.event.*;
 
 /**
  * Publisher for {@link SecretLeaseEvent}s.
@@ -111,16 +102,33 @@ public class SecretLeaseEventPublisher implements InitializingBean {
 	 * @param requestedSecret must not be {@literal null}.
 	 * @param lease must not be {@literal null}.
 	 * @param body must not be {@literal null}.
+	 * @see SecretLeaseCreatedEvent
 	 */
 	protected void onSecretsObtained(RequestedSecret requestedSecret, Lease lease, Map<String, Object> body) {
 		dispatch(new SecretLeaseCreatedEvent(requestedSecret, lease, body));
 	}
 
+	/**
+	 * Hook method called when secrets were rotated. The default implementation is to
+	 * notify {@link LeaseListener}. Implementations can override this method in
+	 * subclasses.
+	 * @param requestedSecret must not be {@literal null}.
+	 * @param lease must not be {@literal null}.
+	 * @param body must not be {@literal null}.
+	 * @since 2.3
+	 * @see SecretLeaseRotatedEvent
+	 */
+	protected void onSecretsRotated(RequestedSecret requestedSecret, Lease previousLease, Lease lease,
+			Map<String, Object> body) {
+		dispatch(new SecretLeaseRotatedEvent(requestedSecret, previousLease, lease, body));
+	}
+
 	/**
 	 * Hook method called when secrets were not found. The default implementation is to
 	 * notify {@link LeaseListener}. Implementations can override this method in
 	 * subclasses.
 	 * @param requestedSecret must not be {@literal null}.
+	 * @see SecretNotFoundEvent
 	 */
 	protected void onSecretsNotFound(RequestedSecret requestedSecret) {
 		dispatch(new SecretNotFoundEvent(requestedSecret, Lease.none()));
@@ -132,6 +140,7 @@ public class SecretLeaseEventPublisher implements InitializingBean {
 	 * subclasses.
 	 * @param requestedSecret must not be {@literal null}.
 	 * @param lease must not be {@literal null}.
+	 * @see AfterSecretLeaseRenewedEvent
 	 */
 	protected void onAfterLeaseRenewed(RequestedSecret requestedSecret, Lease lease) {
 		dispatch(new AfterSecretLeaseRenewedEvent(requestedSecret, lease));
@@ -143,6 +152,7 @@ public class SecretLeaseEventPublisher implements InitializingBean {
 	 * this method in subclasses.
 	 * @param requestedSecret must not be {@literal null}.
 	 * @param lease must not be {@literal null}.
+	 * @see BeforeSecretLeaseRevocationEvent
 	 */
 	protected void onBeforeLeaseRevocation(RequestedSecret requestedSecret, Lease lease) {
 		dispatch(new BeforeSecretLeaseRevocationEvent(requestedSecret, lease));
@@ -154,6 +164,7 @@ public class SecretLeaseEventPublisher implements InitializingBean {
 	 * this method in subclasses.
 	 * @param requestedSecret must not be {@literal null}.
 	 * @param lease must not be {@literal null}.
+	 * @see AfterSecretLeaseRevocationEvent
 	 */
 	protected void onAfterLeaseRevocation(RequestedSecret requestedSecret, Lease lease) {
 		dispatch(new AfterSecretLeaseRevocationEvent(requestedSecret, lease));
@@ -165,6 +176,7 @@ public class SecretLeaseEventPublisher implements InitializingBean {
 	 * subclasses.
 	 * @param requestedSecret must not be {@literal null}.
 	 * @param lease must not be {@literal null}.
+	 * @see SecretLeaseExpiredEvent
 	 */
 	protected void onLeaseExpired(RequestedSecret requestedSecret, Lease lease) {
 		dispatch(new SecretLeaseExpiredEvent(requestedSecret, lease));
@@ -177,6 +189,7 @@ public class SecretLeaseEventPublisher implements InitializingBean {
 	 * @param requestedSecret must not be {@literal null}.
 	 * @param lease may be {@literal null}
 	 * @param e the causing exception.
+	 * @see SecretLeaseErrorEvent
 	 */
 	protected void onError(RequestedSecret requestedSecret, @Nullable Lease lease, Exception e) {
 		dispatch(new SecretLeaseErrorEvent(requestedSecret, lease, e));

spring-vault-core/src/main/java/org/springframework/vault/core/lease/event/SecretLeaseCreatedEvent.java

@@ -34,10 +34,11 @@ public class SecretLeaseCreatedEvent extends SecretLeaseEvent {
 	private final Map<String, Object> secrets;
 
 	/**
-	 * Create a new {@link SecretLeaseExpiredEvent} given {@link RequestedSecret},
+	 * Create a new {@link SecretLeaseCreatedEvent} given {@link RequestedSecret},
 	 * {@link Lease} and {@code secrets}.
 	 * @param requestedSecret must not be {@literal null}.
 	 * @param lease must not be {@literal null}.
+	 * @param secrets must not be {@literal null}.
 	 */
 	public SecretLeaseCreatedEvent(RequestedSecret requestedSecret, Lease lease, Map<String, Object> secrets) {
 

spring-vault-core/src/main/java/org/springframework/vault/core/lease/event/SecretLeaseRotatedEvent.java

@@ -0,0 +1,55 @@
+/*
+ * Copyright 2020 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.vault.core.lease.event;
+
+import java.util.Map;
+
+import org.springframework.vault.core.lease.domain.Lease;
+import org.springframework.vault.core.lease.domain.RequestedSecret;
+
+/**
+ * Event published after rotating secrets.
+ *
+ * @author Mark Paluch
+ * @since 2.3
+ */
+public class SecretLeaseRotatedEvent extends SecretLeaseCreatedEvent {
+
+	private final Lease previousLease;
+
+	/**
+	 * Create a new {@link SecretLeaseRotatedEvent} given {@link RequestedSecret},
+	 * {@link Lease} and {@code secrets}.
+	 * @param requestedSecret must not be {@literal null}.
+	 * @param previousLease must not be {@literal null}.
+	 * @param currentLease must not be {@literal null}.
+	 */
+	public SecretLeaseRotatedEvent(RequestedSecret requestedSecret, Lease previousLease, Lease currentLease,
+			Map<String, Object> secrets) {
+
+		super(requestedSecret, currentLease, secrets);
+		this.previousLease = previousLease;
+	}
+
+	public Lease getPreviousLease() {
+		return this.previousLease;
+	}
+
+	public Lease getCurrentLease() {
+		return getLease();
+	}
+
+}

spring-vault-core/src/test/java/org/springframework/vault/core/env/LeaseAwareVaultPropertySourceUnitTests.java

@@ -31,14 +31,13 @@ import org.springframework.vault.core.lease.event.LeaseErrorListener;
 import org.springframework.vault.core.lease.event.LeaseListener;
 import org.springframework.vault.core.lease.event.SecretLeaseCreatedEvent;
 import org.springframework.vault.core.lease.event.SecretLeaseErrorEvent;
+import org.springframework.vault.core.lease.event.SecretLeaseRotatedEvent;
 import org.springframework.vault.core.lease.event.SecretNotFoundEvent;
 import org.springframework.vault.core.util.PropertyTransformers;
 
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.when;
+import static org.assertj.core.api.Assertions.*;
+import static org.mockito.ArgumentMatchers.*;
+import static org.mockito.Mockito.*;
 
 /**
  * Unit tests for {@link LeaseAwareVaultPropertySource}.
@@ -74,6 +73,32 @@ class LeaseAwareVaultPropertySourceUnitTests {
 		assertThat(propertySource.getPropertyNames()).containsOnly("key");
 	}
 
+	@Test
+	void shouldRotateSecrets() {
+
+		RequestedSecret secret = RequestedSecret.renewable("my-path");
+
+		List<LeaseListener> listeners = new ArrayList<>();
+		doAnswer(invocation -> {
+			listeners.add(invocation.getArgument(0));
+			return null;
+		}).when(this.leaseContainer).addLeaseListener(any());
+		when(this.leaseContainer.addRequestedSecret(any())).then(invocation -> {
+
+			listeners.forEach(
+					leaseListener -> leaseListener.onLeaseEvent(new SecretLeaseCreatedEvent(invocation.getArgument(0),
+							Lease.none(), Collections.singletonMap("key", "value"))));
+			return invocation.getArgument(0);
+		});
+
+		LeaseAwareVaultPropertySource propertySource = new LeaseAwareVaultPropertySource(this.leaseContainer, secret);
+
+		listeners.forEach(it -> it.onLeaseEvent(new SecretLeaseRotatedEvent(secret, Lease.none(), Lease.none(),
+				Collections.singletonMap("new-key", "value"))));
+
+		assertThat(propertySource.getPropertyNames()).containsOnly("new-key");
+	}
+
 	@Test
 	void ignoresNotFoundByDefault() {
 

spring-vault-core/src/test/java/org/springframework/vault/core/lease/SecretLeaseContainerUnitTests.java

@@ -45,22 +45,18 @@ import org.springframework.vault.core.lease.event.AfterSecretLeaseRevocationEven
 import org.springframework.vault.core.lease.event.BeforeSecretLeaseRevocationEvent;
 import org.springframework.vault.core.lease.event.LeaseListenerAdapter;
 import org.springframework.vault.core.lease.event.SecretLeaseCreatedEvent;
+import org.springframework.vault.core.lease.event.SecretLeaseErrorEvent;
 import org.springframework.vault.core.lease.event.SecretLeaseEvent;
 import org.springframework.vault.core.lease.event.SecretLeaseExpiredEvent;
+import org.springframework.vault.core.lease.event.SecretLeaseRotatedEvent;
 import org.springframework.vault.core.lease.event.SecretNotFoundEvent;
 import org.springframework.vault.support.LeaseStrategy;
 import org.springframework.vault.support.VaultResponse;
 import org.springframework.web.client.HttpClientErrorException;
 
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.eq;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyNoInteractions;
-import static org.mockito.Mockito.verifyNoMoreInteractions;
-import static org.mockito.Mockito.when;
+import static org.assertj.core.api.Assertions.*;
+import static org.mockito.ArgumentMatchers.*;
+import static org.mockito.Mockito.*;
 
 /**
  * Unit tests for {@link SecretLeaseContainer}.
@@ -327,12 +323,11 @@ class SecretLeaseContainerUnitTests {
 		captor.getValue().run();
 		verify(this.taskScheduler, times(2)).schedule(captor.capture(), any(Trigger.class));
 
-		assertThat(events).hasSize(3);
+		assertThat(events).hasSize(2);
 		assertThat(events.get(0)).isInstanceOf(SecretLeaseCreatedEvent.class);
-		assertThat(events.get(1)).isInstanceOf(SecretLeaseExpiredEvent.class);
-		assertThat(events.get(2)).isInstanceOf(SecretLeaseCreatedEvent.class);
+		assertThat(events.get(1)).isInstanceOf(SecretLeaseRotatedEvent.class);
 
-		SecretLeaseCreatedEvent rotated = (SecretLeaseCreatedEvent) events.get(2);
+		SecretLeaseRotatedEvent rotated = (SecretLeaseRotatedEvent) events.get(1);
 		assertThat(rotated.getSecrets()).containsEntry("key", "value2");
 	}
 
@@ -357,18 +352,52 @@ class SecretLeaseContainerUnitTests {
 		verify(this.taskScheduler, times(2)).schedule(captor.capture(), any(Trigger.class));
 
 		ArgumentCaptor<SecretLeaseEvent> createdEvents = ArgumentCaptor.forClass(SecretLeaseEvent.class);
-		verify(this.leaseListenerAdapter, times(3)).onLeaseEvent(createdEvents.capture());
+		verify(this.leaseListenerAdapter, times(2)).onLeaseEvent(createdEvents.capture());
 
 		List<SecretLeaseEvent> events = createdEvents.getAllValues();
 
-		assertThat(events).hasSize(3);
+		assertThat(events).hasSize(2);
+		assertThat(events.get(0)).isInstanceOf(SecretLeaseCreatedEvent.class);
+		assertThat(((SecretLeaseCreatedEvent) events.get(0)).getSecrets()).containsOnlyKeys("key");
+
+		assertThat(events.get(1)).isInstanceOf(SecretLeaseRotatedEvent.class);
+		assertThat(((SecretLeaseCreatedEvent) events.get(1)).getSecrets()).containsOnlyKeys("foo");
+	}
+
+	@Test
+	void failedRotateShouldEmitExpiredAndErrorEvents() {
+
+		when(this.taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn(this.scheduledFuture);
+
+		when(this.vaultOperations.read(this.rotatingGenericSecret.getPath()))
+				.thenReturn(createGenericSecrets(Collections.singletonMap("key", "value")))
+				.thenThrow(new HttpClientErrorException(HttpStatus.I_AM_A_TEAPOT));
+
+		this.secretLeaseContainer.addRequestedSecret(this.rotatingGenericSecret);
+
+		this.secretLeaseContainer.start();
+
+		ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
+		verify(this.taskScheduler).schedule(captor.capture(), any(Trigger.class));
+
+		captor.getValue().run();
+		verifyNoInteractions(this.scheduledFuture);
+
+		ArgumentCaptor<SecretLeaseEvent> createdEvents = ArgumentCaptor.forClass(SecretLeaseEvent.class);
+		verify(this.leaseListenerAdapter, times(2)).onLeaseEvent(createdEvents.capture());
+
+		ArgumentCaptor<SecretLeaseErrorEvent> errorEvents = ArgumentCaptor.forClass(SecretLeaseErrorEvent.class);
+		verify(this.leaseListenerAdapter).onLeaseError(errorEvents.capture(), any());
+
+		List<SecretLeaseEvent> events = createdEvents.getAllValues();
+
+		assertThat(events).hasSize(2);
 		assertThat(events.get(0)).isInstanceOf(SecretLeaseCreatedEvent.class);
 		assertThat(((SecretLeaseCreatedEvent) events.get(0)).getSecrets()).containsOnlyKeys("key");
 
 		assertThat(events.get(1)).isInstanceOf(SecretLeaseExpiredEvent.class);
 
-		assertThat(events.get(2)).isInstanceOf(SecretLeaseCreatedEvent.class);
-		assertThat(((SecretLeaseCreatedEvent) events.get(2)).getSecrets()).containsOnlyKeys("foo");
+		assertThat(errorEvents.getAllValues()).hasSize(1);
 	}
 
 	@Test
@@ -391,7 +420,7 @@ class SecretLeaseContainerUnitTests {
 		verify(this.taskScheduler, times(2)).schedule(captor.capture(), any(Trigger.class));
 
 		ArgumentCaptor<SecretLeaseEvent> createdEvents = ArgumentCaptor.forClass(SecretLeaseEvent.class);
-		verify(this.leaseListenerAdapter, times(3)).onLeaseEvent(createdEvents.capture());
+		verify(this.leaseListenerAdapter, times(2)).onLeaseEvent(createdEvents.capture());
 	}
 
 	@Test
@@ -437,18 +466,16 @@ class SecretLeaseContainerUnitTests {
 		verify(this.taskScheduler, times(2)).schedule(captor.capture(), any(Trigger.class));
 
 		ArgumentCaptor<SecretLeaseEvent> createdEvents = ArgumentCaptor.forClass(SecretLeaseEvent.class);
-		verify(this.leaseListenerAdapter, times(3)).onLeaseEvent(createdEvents.capture());
+		verify(this.leaseListenerAdapter, times(2)).onLeaseEvent(createdEvents.capture());
 
 		List<SecretLeaseEvent> events = createdEvents.getAllValues();
 
-		assertThat(events).hasSize(3);
+		assertThat(events).hasSize(2);
 		assertThat(events.get(0)).isInstanceOf(SecretLeaseCreatedEvent.class);
 		assertThat(((SecretLeaseCreatedEvent) events.get(0)).getSecrets()).containsOnlyKeys("key");
 
-		assertThat(events.get(1)).isInstanceOf(SecretLeaseExpiredEvent.class);
-
-		assertThat(events.get(2)).isInstanceOf(SecretLeaseCreatedEvent.class);
-		assertThat(((SecretLeaseCreatedEvent) events.get(2)).getSecrets()).containsOnlyKeys("foo");
+		assertThat(events.get(1)).isInstanceOf(SecretLeaseRotatedEvent.class);
+		assertThat(((SecretLeaseCreatedEvent) events.get(1)).getSecrets()).containsOnlyKeys("foo");
 	}
 
 	@Test

src/main/asciidoc/new-features.adoc

@@ -10,6 +10,7 @@
 * Support for `transform` backend (Enterprise Feature).
 * Documentation of <<vault.core.secret-engines,how to use Vault secret backends>>.
 * Login credentials for Kubernetes and PCF authentication are reloaded for each login attempt.
+* `SecretLeaseContainer` publishes `SecretLeaseRotatedEvent` instead of `SecretLeaseExpiredEvent` and `SecretLeaseCreatedEvent` on successful secret rotation.
 
 [[new-features.2-2-0]]
 === What's new in Spring Vault 2.2