Polishing.

Simplify code. Add since tags. Introduce Lease.fromTimeToLive(…) factory method in favor of Lease.of(…) with empty lease Id. Refactor Lease.isRotatingGenericLease(…) to Lease.hasLeaseId(…). Adopt SecretLeaseContainer to these changes. Add debug logging for leases without lease Id. Extend unit tests. Adjust reference documentation.

Original pull request: gh-95.
See gh-68.

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

@@ -20,8 +20,8 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Set;
+import java.util.Map.Entry;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.ScheduledFuture;
@@ -291,13 +291,6 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 		}
 	}
 
-	private static boolean isRotatingGenericSecret(RequestedSecret requestedSecret,
-			VaultResponseSupport<Map<String, Object>> secrets) {
-		return Mode.ROTATE.equals(requestedSecret.getMode()) && !secrets.isRenewable()
-				&& secrets.getLeaseDuration() > 0
-				&& "".equals(secrets.getLeaseId());
-	}
-
 	private void start(RequestedSecret requestedSecret,
 			LeaseRenewalScheduler renewalScheduler) {
 
@@ -306,12 +299,15 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 		if (secrets != null) {
 
 			Lease lease;
+
 			if (StringUtils.hasText(secrets.getLeaseId())) {
 				lease = Lease.of(secrets.getLeaseId(), secrets.getLeaseDuration(),
 						secrets.isRenewable());
-			} else if (isRotatingGenericSecret(requestedSecret, secrets)) {
-				lease = Lease.of(secrets.getLeaseDuration());
-			} else {
+			}
+			else if (isRotatingGenericSecret(requestedSecret, secrets)) {
+				lease = Lease.fromTimeToLive(secrets.getLeaseDuration());
+			}
+			else {
 				lease = Lease.none();
 			}
 
@@ -320,6 +316,13 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 		}
 	}
 
+	private static boolean isRotatingGenericSecret(RequestedSecret requestedSecret,
+			VaultResponseSupport<Map<String, Object>> secrets) {
+
+		return Mode.ROTATE.equals(requestedSecret.getMode()) && !secrets.isRenewable()
+				&& secrets.getLeaseDuration() > 0;
+	}
+
 	/**
 	 * Stop the {@link SecretLeaseContainer}. Stopping the container will stop lease
 	 * renewal, secrets rotation and event publishing. Active leases are not expired.
@@ -389,7 +392,7 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 					Lease lease = entry.getValue().getLease();
 					entry.getValue().disableScheduleRenewal();
 
-					if (lease != null && !lease.isRotatingGenericLease()) {
+					if (lease != null && lease.hasLeaseId()) {
 						doRevokeLease(entry.getKey(), lease);
 					}
 				}
@@ -408,33 +411,42 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 	void potentiallyScheduleLeaseRenewal(final RequestedSecret requestedSecret,
 			final Lease lease, final LeaseRenewalScheduler leaseRenewal) {
 
-		if (leaseRenewal.isLeaseRenewable(lease)) {
+		if (!leaseRenewal.isLeaseRenewable(lease, requestedSecret)) {
+			return;
+		}
 
-			if (log.isDebugEnabled()) {
-				log.debug(String.format("Lease %s qualified for renewal",
-						lease.getLeaseId()));
+		if (log.isDebugEnabled()) {
+
+			if (lease.hasLeaseId()) {
+				log.debug(String.format("Secret %s with Lease %s qualified for renewal",
+						requestedSecret.getPath(), lease.getLeaseId()));
+			}
+			else {
+				log.debug(String.format(
+						"Secret %s with cache hint is qualified for renewal",
+						requestedSecret.getPath()));
 			}
 
-			leaseRenewal.scheduleRenewal(new RenewLease() {
+		}
 
-				@Override
-				public Lease renewLease(Lease lease) {
+		leaseRenewal.scheduleRenewal(requestedSecret, new RenewLease() {
 
-					Lease newLease = doRenewLease(requestedSecret, lease);
+			@Override
+			public Lease renewLease(Lease lease) {
 
-					if (newLease == null) {
-						return null;
-					}
+				Lease newLease = doRenewLease(requestedSecret, lease);
+
+				if (!Lease.none().equals(newLease)) {
 
 					potentiallyScheduleLeaseRenewal(requestedSecret, newLease,
 							leaseRenewal);
 
 					onAfterLeaseRenewed(requestedSecret, newLease);
-
-					return newLease;
 				}
-			}, lease, getMinRenewalSeconds(), getExpiryThresholdSeconds());
-		}
+
+				return newLease;
+			}
+		}, lease, getMinRenewalSeconds(), getExpiryThresholdSeconds());
 	}
 
 	// -------------------------------------------------------------------------
@@ -468,43 +480,20 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 	 * @param lease the lease.
 	 * @return the new lease or {@literal null} if expired/secret cannot be rotated.
 	 */
-	protected Lease doRenewLease(final RequestedSecret requestedSecret,
-			final Lease lease) {
+	protected Lease doRenewLease(final RequestedSecret requestedSecret, final Lease lease) {
 
 		try {
-			ResponseEntity<Map<String, Object>> entity = operations
-					.doWithSession(new RestOperationsCallback<ResponseEntity<Map<String, Object>>>() {
 
-						@Override
-						@SuppressWarnings("unchecked")
-						public ResponseEntity<Map<String, Object>> doWithRestOperations(
-										RestOperations restOperations) {
-									if (lease.isRotatingGenericLease()) {
-										return (ResponseEntity) restOperations.exchange(
-												requestedSecret.getPath(), HttpMethod.GET,
-												null, Map.class, (Object) null);
-									} else {
-							return (ResponseEntity) restOperations.exchange(
-									"/sys/renew/{leaseId}", HttpMethod.PUT, null, Map.class, lease.getLeaseId());
-									}
-						}
-					});
+			Lease renewed = lease.hasLeaseId() ? renew(lease) : lease;
 
-			Map<String, Object> body = entity.getBody();
-			String leaseId = (String) body.get("lease_id");
-			Number leaseDuration = (Number) body.get("lease_duration");
-			boolean renewable = (Boolean) body.get("renewable");
+			if (!renewed.hasLeaseId() || renewed.getLeaseDuration() == 0
+					|| renewed.getLeaseDuration() < minRenewalSeconds) {
 
-			if (leaseDuration == null || leaseDuration.intValue() < minRenewalSeconds) {
 				onLeaseExpired(requestedSecret, lease);
-				return null;
+				return Lease.none();
 			}
 
-			Lease results = Lease.of(leaseId, leaseDuration.longValue(), renewable);
-			if (results.isRotatingGenericLease()) {
-				onSecretsObtained(requestedSecret, results, body);
-			}
-			return results;
+			return renewed;
 		}
 		catch (HttpStatusCodeException e) {
 
@@ -521,7 +510,31 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 			onError(requestedSecret, lease, e);
 		}
 
-		return null;
+		return Lease.none();
+	}
+
+	private Lease renew(final Lease lease) {
+
+		ResponseEntity<Map<String, Object>> entity = operations
+				.doWithSession(new RestOperationsCallback<ResponseEntity<Map<String, Object>>>() {
+
+					@Override
+					@SuppressWarnings("unchecked")
+					public ResponseEntity<Map<String, Object>> doWithRestOperations(
+							RestOperations restOperations) {
+						return (ResponseEntity) restOperations.exchange(
+								"/sys/renew/{leaseId}", HttpMethod.PUT, null, Map.class,
+								lease.getLeaseId());
+					}
+				});
+
+		Map<String, Object> body = entity.getBody();
+		String leaseId = (String) body.get("lease_id");
+		Number leaseDuration = (Number) body.get("lease_duration");
+		boolean renewable = (Boolean) body.get("renewable");
+
+		return Lease.of(leaseId, leaseDuration != null ? leaseDuration.longValue() : 0,
+				renewable);
 	}
 
 	/**
@@ -553,8 +566,8 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 
 			onBeforeLeaseRevocation(requestedSecret, lease);
 
-			operations.doWithSession(
-					new RestOperationsCallback<ResponseEntity<Map<String, Object>>>() {
+			operations
+					.doWithSession(new RestOperationsCallback<ResponseEntity<Map<String, Object>>>() {
 
 						@Override
 						@SuppressWarnings("unchecked")
@@ -605,20 +618,29 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 		/**
 		 * Schedule {@link Lease} renewal. Previously registered renewal tasks are
 		 * canceled to prevent renewal of stale {@link Lease}s.
+		 * @param requestedSecret the requested secret.
 		 * @param renewLease strategy to renew a {@link Lease}.
 		 * @param lease the current {@link Lease}.
 		 * @param minRenewalSeconds minimum number of seconds before renewing a
 		 * {@link Lease}. This is to prevent too many renewals in a very short timeframe.
 		 * @param expiryThresholdSeconds number of seconds to renew before {@link Lease}.
-		 * expires.
 		 */
-		void scheduleRenewal(final RenewLease renewLease, final Lease lease,
+		void scheduleRenewal(final RequestedSecret requestedSecret,
+				final RenewLease renewLease, final Lease lease,
 				final int minRenewalSeconds, final int expiryThresholdSeconds) {
 
 			if (log.isDebugEnabled()) {
-				log.debug(String.format(
-						"Scheduling renewal for lease %s, lease duration %d",
-						lease.getLeaseId(), lease.getLeaseDuration()));
+				if (lease.hasLeaseId()) {
+					log.debug(String
+							.format("Scheduling renewal for secret %s with lease %s, lease duration %d",
+									requestedSecret.getPath(), lease.getLeaseId(),
+									lease.getLeaseDuration()));
+				}
+				else {
+					log.debug(String
+							.format("Scheduling renewal for secret %s, with cache hint duration %d",
+									requestedSecret.getPath(), lease.getLeaseDuration()));
+				}
 			}
 
 			Lease currentLease = this.currentLeaseRef.get();
@@ -628,36 +650,42 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 				cancelSchedule(currentLease);
 			}
 
-			ScheduledFuture<?> scheduledFuture = taskScheduler.schedule(
-					new Runnable() {
+			Runnable task = new Runnable() {
 
-						@Override
-						public void run() {
+				@Override
+				public void run() {
 
-							try {
+					schedules.remove(lease);
 
-								schedules.remove(lease);
+					if (currentLeaseRef.get() != lease) {
+						log.debug("Current lease has changed. Skipping renewal");
+						return;
+					}
 
-								if (currentLeaseRef.get() != lease) {
-									log.debug("Current lease has changed. Skipping renewal");
-									return;
-								}
-
-								if (log.isDebugEnabled()) {
-									log.debug(String.format("Renewing lease %s",
-											lease.getLeaseId()));
-								}
-
-								currentLeaseRef.compareAndSet(lease,
-										renewLease.renewLease(lease));
-							}
-							catch (Exception e) {
-								log.error(
-										String.format("Cannot renew lease %s",
-												lease.getLeaseId()), e);
-							}
+					if (log.isDebugEnabled()) {
+						if (lease.hasLeaseId()) {
+							log.debug(String.format("Renewing lease %s for secret %s",
+									lease.getLeaseId(), requestedSecret.getPath()));
 						}
-					},
+						else {
+							log.debug(String.format("Renewing secret without lease %s",
+									requestedSecret.getPath()));
+						}
+					}
+
+					try {
+						currentLeaseRef
+								.compareAndSet(lease, renewLease.renewLease(lease));
+					}
+					catch (Exception e) {
+						log.error(String.format("Cannot renew lease %s",
+								lease.getLeaseId()), e);
+					}
+				}
+			};
+
+			ScheduledFuture<?> scheduledFuture = taskScheduler.schedule(
+					task,
 					new OneShotTrigger(getRenewalSeconds(lease, minRenewalSeconds,
 							expiryThresholdSeconds)));
 
@@ -699,9 +727,21 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 					- expiryThresholdSeconds);
 		}
 
-		private boolean isLeaseRenewable(Lease lease) {
-			return lease != null
-					&& (lease.isRenewable() || lease.isRotatingGenericLease());
+		private boolean isLeaseRenewable(Lease lease, RequestedSecret requestedSecret) {
+
+			if (lease == null) {
+				return false;
+			}
+
+			if (lease.isRenewable()) {
+				return true;
+			}
+
+			if (!lease.hasLeaseId() && requestedSecret.getMode() == Mode.ROTATE) {
+				return true;
+			}
+
+			return false;
 		}
 
 		public Lease getLease() {
@@ -751,7 +791,7 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements
 		 * Renew a lease.
 		 *
 		 * @param lease must not be {@literal null}.
-		 * @return the new lease
+		 * @return the new lease.
 		 * @throws VaultException if lease renewal runs into problems
 		 */
 		Lease renewLease(Lease lease) throws VaultException;

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

@@ -16,7 +16,6 @@
 package org.springframework.vault.core.lease.domain;
 
 import org.springframework.util.Assert;
-import org.springframework.util.StringUtils;
 
 /**
  * A lease abstracting the lease Id, duration and its renewability.
@@ -52,19 +51,20 @@ public class Lease {
 	public static Lease of(String leaseId, long leaseDuration, boolean renewable) {
 
 		Assert.hasText(leaseId, "LeaseId must not be empty");
+
 		return new Lease(leaseId, leaseDuration, renewable);
 	}
 
 	/**
-	 * Create a new non-renewable {@link Lease}, with an empty lease ID and
-	 * specified duration.
+	 * Create a new non-renewable {@link Lease}, without a {@code leaseId} and specified
+	 * duration.
 	 *
-	 * @param leaseDuration the lease duration in seconds
+	 * @param leaseDuration the lease duration in seconds.
 	 * @return the created {@link Lease}
+	 * @since 1.1
 	 */
-	public static Lease of(long leaseDuration) {
-
-		return new Lease("", leaseDuration, false);
+	public static Lease fromTimeToLive(long leaseDuration) {
+		return new Lease(null, leaseDuration, false);
 	}
 
 	/**
@@ -76,6 +76,14 @@ public class Lease {
 		return NONE;
 	}
 
+	/**
+	 * @return {@literal true} is the lease is associated with a {@code leaseId}.
+	 * @since 1.1
+	 */
+	public boolean hasLeaseId() {
+		return leaseId != null;
+	}
+
 	/**
 	 * @return the lease Id
 	 */
@@ -91,21 +99,12 @@ public class Lease {
 	}
 
 	/**
-	 *
 	 * @return {@literal true} if the lease is renewable.
 	 */
 	public boolean isRenewable() {
 		return renewable;
 	}
 
-	/**
-	 *
-	 * @return {@literal true} if the lease represents a rotating generic secret.
-	 */
-	public boolean isRotatingGenericLease() {
-		return !renewable && leaseDuration > 0 && StringUtils.isEmpty(leaseId);
-	}
-
 	@Override
 	public boolean equals(Object o) {
 		if (this == o)

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

@@ -187,8 +187,8 @@ public class SecretLeaseContainerUnitTests {
 	@SuppressWarnings("unchecked")
 	public void shouldAcceptSecretsWithRenewableLease() {
 
-		when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class)))
-				.thenReturn(scheduledFuture);
+		when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn(
+				scheduledFuture);
 
 		when(vaultOperations.read(requestedSecret.getPath())).thenReturn(createSecrets());
 
@@ -215,6 +215,43 @@ public class SecretLeaseContainerUnitTests {
 		verify(taskScheduler, times(2)).schedule(captor.capture(), any(Trigger.class));
 	}
 
+	@Test
+	public void shouldRotateGenericSecret() {
+
+		when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn(
+				scheduledFuture);
+
+		when(vaultOperations.read(rotatingGenericSecret.getPath())).thenReturn(
+				createGenericSecrets(Collections.singletonMap("key", (Object) "value")),
+				createGenericSecrets(Collections.singletonMap("foo", (Object) "bar")));
+
+		secretLeaseContainer.addRequestedSecret(rotatingGenericSecret);
+
+		secretLeaseContainer.start();
+
+		ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
+		verify(taskScheduler).schedule(captor.capture(), any(Trigger.class));
+
+		captor.getValue().run();
+		verifyZeroInteractions(scheduledFuture);
+		verify(taskScheduler, times(2)).schedule(captor.capture(), any(Trigger.class));
+
+		ArgumentCaptor<SecretLeaseCreatedEvent> createdEvents = ArgumentCaptor
+				.forClass(SecretLeaseCreatedEvent.class);
+		verify(leaseListenerAdapter, times(3)).onLeaseEvent(createdEvents.capture());
+
+		List<SecretLeaseCreatedEvent> events = createdEvents.getAllValues();
+
+		assertThat(events).hasSize(3);
+		assertThat(events.get(0)).isInstanceOf(SecretLeaseCreatedEvent.class);
+		assertThat(events.get(0).getSecrets()).containsOnlyKeys("key");
+
+		assertThat(events.get(1)).isInstanceOf(SecretLeaseExpiredEvent.class);
+
+		assertThat(events.get(2)).isInstanceOf(SecretLeaseCreatedEvent.class);
+		assertThat(events.get(2).getSecrets()).containsOnlyKeys("foo");
+	}
+
 	@Test
 	public void shouldNotRenewExpiringLease() {
 
@@ -237,8 +274,8 @@ public class SecretLeaseContainerUnitTests {
 	@Test
 	public void shouldNotRotateExpiringLease() {
 
-		when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class)))
-				.thenReturn(scheduledFuture);
+		when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn(
+				scheduledFuture);
 
 		VaultResponse first = createSecrets();
 		VaultResponse second = createSecrets();
@@ -294,8 +331,8 @@ public class SecretLeaseContainerUnitTests {
 	public void shouldPublishRenewalErrors() {
 
 		prepareRenewal();
-		when(vaultOperations.doWithSession(any(RestOperationsCallback.class)))
-				.thenThrow(new HttpClientErrorException(HttpStatus.I_AM_A_TEAPOT));
+		when(vaultOperations.doWithSession(any(RestOperationsCallback.class))).thenThrow(
+				new HttpClientErrorException(HttpStatus.I_AM_A_TEAPOT));
 
 		secretLeaseContainer.start();
 
@@ -406,10 +443,10 @@ public class SecretLeaseContainerUnitTests {
 		verify(vaultOperations).doWithSession(any(RestOperationsCallback.class));
 		verify(scheduledFuture).cancel(false);
 		verify(leaseListenerAdapter).onLeaseEvent(any(SecretLeaseCreatedEvent.class));
-		verify(leaseListenerAdapter)
-				.onLeaseEvent(any(BeforeSecretLeaseRevocationEvent.class));
-		verify(leaseListenerAdapter)
-				.onLeaseEvent(any(AfterSecretLeaseRevocationEvent.class));
+		verify(leaseListenerAdapter).onLeaseEvent(
+				any(BeforeSecretLeaseRevocationEvent.class));
+		verify(leaseListenerAdapter).onLeaseEvent(
+				any(AfterSecretLeaseRevocationEvent.class));
 	}
 
 	@Test
@@ -427,25 +464,20 @@ public class SecretLeaseContainerUnitTests {
 
 		verifyZeroInteractions(taskScheduler);
 
-		verify(leaseListenerAdapter, never())
-				.onLeaseEvent(any(BeforeSecretLeaseRevocationEvent.class));
-		verify(leaseListenerAdapter, never())
-				.onLeaseEvent(any(AfterSecretLeaseRevocationEvent.class));
+		verify(leaseListenerAdapter, never()).onLeaseEvent(
+				any(BeforeSecretLeaseRevocationEvent.class));
+		verify(leaseListenerAdapter, never()).onLeaseEvent(
+				any(AfterSecretLeaseRevocationEvent.class));
 	}
 
 	@Test
-	public void shouldRequestRotatingGenericSecrets() throws Exception {
+	public void shouldRequestRotatingGenericSecrets() {
 
-		when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class)))
-				.thenReturn(scheduledFuture);
+		when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn(
+				scheduledFuture);
 
-		VaultResponse secrets = new VaultResponse();
-		secrets.setLeaseId("");
-		secrets.setRenewable(false);
-		secrets.setLeaseDuration(60);
-		secrets.setData(Collections.singletonMap("key", (Object) "value"));
-
-		when(vaultOperations.read(rotatingGenericSecret.getPath())).thenReturn(secrets);
+		when(vaultOperations.read(rotatingGenericSecret.getPath())).thenReturn(
+				createGenericSecrets());
 
 		secretLeaseContainer.addRequestedSecret(rotatingGenericSecret);
 		secretLeaseContainer.start();
@@ -463,8 +495,8 @@ public class SecretLeaseContainerUnitTests {
 	@SuppressWarnings("unchecked")
 	private void prepareRenewal() {
 
-		when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class)))
-				.thenReturn(scheduledFuture);
+		when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn(
+				scheduledFuture);
 
 		when(vaultOperations.read(requestedSecret.getPath())).thenReturn(createSecrets());
 
@@ -499,4 +531,19 @@ public class SecretLeaseContainerUnitTests {
 
 		return secrets;
 	}
+
+	private VaultResponse createGenericSecrets() {
+		return createGenericSecrets(Collections.singletonMap("key", (Object) "value"));
+	}
+
+	private VaultResponse createGenericSecrets(Map<String, Object> data) {
+
+		VaultResponse secrets = new VaultResponse();
+
+		secrets.setRenewable(false);
+		secrets.setLeaseDuration(100);
+		secrets.setData(data);
+
+		return secrets;
+	}
 }

src/main/asciidoc/reference/getting-started.adoc

@@ -483,7 +483,7 @@ public class AppConfig {
 ----
 ====
 
-NOTE: Secrets obtained from `generic` secret backends are associated with a TTL (`refresh_interval`) but not a lease Id. Spring Vault's ``PropertySource`` is not refreshing/flushing these secrets once the TTL expires despite the requested `Renewal` mode.
+NOTE: Secrets obtained from `generic` secret backends are associated with a TTL (`refresh_interval`) but not a lease Id. Spring Vault's ``PropertySource`` rotates generic secrets when reaching its TTL.
 
 In certain situations, it may not be possible or practical to tightly control
 property source ordering when using `@VaultPropertySource` annotations.