Address review commentsReview comments lensesio#1

caltangelo · Mar 17, 2023 · d9085ee · d9085ee
1 parent dbc8327
commit d9085ee
Show file tree

Hide file tree

Showing 7 changed files with 39 additions and 53 deletions.
diff --git a/project/Settings.scala b/project/Settings.scala
@@ -66,6 +66,11 @@ object Settings extends Dependencies {
 
   implicit final class AssemblyConfigurator(project: Project) {
 
+    /*
+    Exclude the jar signing files from dependencies.  They come from other jars,
+    and including these would break assembly (due to duplicates) or classloading
+    (due to mismatching jar checksum/signature).
+     */
     val excludeFilePatterns = Set(".MF", ".RSA", ".DSA", ".SF")
 
     def excludeFileFilter(p: String): Boolean =

diff --git a/...vider/src/it/scala/io/lenses/connect/secrets/integration/SecretProviderRestResponse.scala b/...vider/src/it/scala/io/lenses/connect/secrets/integration/SecretProviderRestResponse.scala
@@ -15,9 +15,9 @@ case class SecretProviderRestResponse(
   renewable:     Boolean,
   leaseDuration: BigInt,
   data:          DataResponse,
-  wrapInfo:      Object, // todo what is this?
-  warnings:      Object, // todo
-  auth:          Object, // todo
+  wrapInfo:      Any, // todo what is this?
+  warnings:      Any, // todo
+  auth:          Any, // todo
 )
 
 case object SecretProviderRestResponse {

diff --git a/...r/src/it/scala/io/lenses/connect/secrets/integration/SecretProviderRestResponseTest.scala b/...r/src/it/scala/io/lenses/connect/secrets/integration/SecretProviderRestResponseTest.scala
@@ -1,6 +1,5 @@
 package io.lenses.connect.secrets.integration
 
-import org.scalatest.funspec.AnyFunSpec
 import org.scalatest.funsuite.AnyFunSuite
 import org.scalatest.matchers.should.Matchers
 

diff --git a/secret-provider/src/main/scala/io/lenses/connect/secrets/cache/TtlCache.scala b/secret-provider/src/main/scala/io/lenses/connect/secrets/cache/TtlCache.scala
@@ -5,41 +5,36 @@ import com.typesafe.scalalogging.LazyLogging
 import java.time.Clock
 import scala.collection.concurrent.TrieMap
 
-class TtlCache[V](implicit clock: Clock) extends LazyLogging {
+class TtlCache[V](fnGetMissingValue: String => Either[Throwable, ValueWithTtl[V]])(implicit clock: Clock)
+    extends LazyLogging {
 
   private val cache = TrieMap[String, ValueWithTtl[V]]()
 
-  private def put(
-    key:   String,
-    value: ValueWithTtl[V],
-  ): ValueWithTtl[V] = {
-    cache.put(key, value)
-    value
-  }
+  def cachingWithTtl(
+    key:                String,
+    fnCondition:        V => Boolean                       = _ => true,
+    fnFilterReturnKeys: ValueWithTtl[V] => ValueWithTtl[V] = identity,
+  ): Either[Throwable, ValueWithTtl[V]] = {
+    get(key,
+        Seq(
+          v => v.isAlive,
+          v => fnCondition(v.value),
+        ),
+    ).fold(fnGetMissingValue(key).map(put(key, _)))(Right(_))
+  }.map(fnFilterReturnKeys)
 
   private def get(
     key:          String,
     fnConditions: Seq[ValueWithTtl[V] => Boolean] = Seq(_ => true),
   ): Option[ValueWithTtl[V]] =
     cache.get(key).filter(value => fnConditions.forall(_(value)))
 
-  def cachingWithTtl(
-    key:                String,
-    fnCondition:        V => Boolean                       = _ => true,
-    fnGetMissingValue:  () => Either[Throwable, ValueWithTtl[V]],
-    fnFilterReturnKeys: ValueWithTtl[V] => ValueWithTtl[V] = e => e,
-  ): Either[Throwable, ValueWithTtl[V]] =
-    getCachedValue(key, fnCondition)
-      .orElse(fnGetMissingValue().map(put(key, _)))
-      .map(fnFilterReturnKeys)
-
-  private def getCachedValue(key: String, fnCondition: V => Boolean): Either[Throwable, ValueWithTtl[V]] =
-    get(
-      key,
-      Seq(
-        v => v.isAlive,
-        v => fnCondition(v.value),
-      ),
-    ).toRight(new RuntimeException("No cached value"))
+  private def put(
+    key:   String,
+    value: ValueWithTtl[V],
+  ): ValueWithTtl[V] = {
+    cache.put(key, value)
+    value
+  }
 
 }
diff --git a/secret-provider/src/main/scala/io/lenses/connect/secrets/cache/ValueWithTtl.scala b/secret-provider/src/main/scala/io/lenses/connect/secrets/cache/ValueWithTtl.scala
@@ -16,10 +16,8 @@ case class Ttl(originalTtl: Duration, expiry: OffsetDateTime) {
 }
 
 object Ttl {
-  def apply(ttl: Option[Duration], defaultTtl: Option[Duration])(implicit clock: Clock): Option[Ttl] = {
-    val applicableTtl = Seq(ttl, defaultTtl).flatten.filterNot(_.toMillis == 0).headOption
-    applicableTtl.map(finalTtl => Ttl(finalTtl, ttlToExpiry(finalTtl)))
-  }
+  def apply(ttl: Option[Duration], defaultTtl: Option[Duration])(implicit clock: Clock): Option[Ttl] =
+    ttl.orElse(defaultTtl).map(finalTtl => Ttl(finalTtl, ttlToExpiry(finalTtl)))
 
   def ttlToExpiry(ttl: Duration)(implicit clock: Clock): OffsetDateTime = {
     val offset         = clock.getZone.getRules.getOffset(clock.instant())

diff --git a/secret-provider/src/main/scala/io/lenses/connect/secrets/providers/VaultSecretProvider.scala b/secret-provider/src/main/scala/io/lenses/connect/secrets/providers/VaultSecretProvider.scala
@@ -37,7 +37,7 @@ class VaultSecretProvider() extends ConfigProvider with VaultHelper {
   private var tokenRenewal: Option[AsyncFunctionLoop] = None
 
   private implicit val clock: Clock = Clock.systemDefaultZone()
-  private val cache = new TtlCache[Map[String, String]]()
+  private val cache = new TtlCache[Map[String, String]](k => getSecretValue(k))
 
   def getClient: Option[Vault] = vaultClient
 
@@ -62,33 +62,31 @@ class VaultSecretProvider() extends ConfigProvider with VaultHelper {
 
   // lookup secrets at a path
   override def get(path: String): ConfigData = {
-    logger.info(" -> VaultSecretProvider.get(path: {})", path)
+    logger.debug(" -> VaultSecretProvider.get(path: {})", path)
     val sec = cache
       .cachingWithTtl(
         path,
-        fnGetMissingValue = getSecretValue(path),
       )
       .fold(
         ex => throw ex,
         ConfigDataBuilder(_),
       )
-    logger.info(" <- VaultSecretProvider.get(path: {}, ttl: {})", path, sec.ttl())
+    logger.debug(" <- VaultSecretProvider.get(path: {}, ttl: {})", path, sec.ttl())
     sec
   }
 
   // get secret keys at a path
   override def get(path: String, keys: util.Set[String]): ConfigData = {
-    logger.info(" -> VaultSecretProvider.get(path: {}, keys: {})", path, keys.asScala)
+    logger.debug(" -> VaultSecretProvider.get(path: {}, keys: {})", path, keys.asScala)
     val sec = cache.cachingWithTtl(
       path,
       fnCondition        = s => keys.asScala.subsetOf(s.keySet),
-      fnGetMissingValue  = getSecretValue(path),
       fnFilterReturnKeys = filter(_, keys.asScala.toSet),
     ).fold(
       ex => throw ex,
       ConfigDataBuilder(_),
     )
-    logger.info(" <- VaultSecretProvider.get(path: {}, keys: {}, ttl: {})", path, keys.asScala, sec.ttl())
+    logger.debug(" <- VaultSecretProvider.get(path: {}, keys: {}, ttl: {})", path, keys.asScala, sec.ttl())
     sec
   }
 
@@ -102,8 +100,8 @@ class VaultSecretProvider() extends ConfigProvider with VaultHelper {
     tokenRenewal.foreach(_.close())
 
   // get the secrets and ttl under a path
-  private def getSecretValue(path: String): () => Either[Throwable, ValueWithTtl[Map[String, String]]] = () => {
-    logger.info(s"Looking up value from Vault at [$path]")
+  val getSecretValue: String => Either[Throwable, ValueWithTtl[Map[String, String]]] = path => {
+    logger.debug(s"Looking up value from Vault at [$path]")
     Try(vaultClient.get.logical().read(path)) match {
       case Failure(ex) =>
         failWithEx(s"Failed to fetch secrets from path [$path]", ex)
@@ -115,7 +113,7 @@ class VaultSecretProvider() extends ConfigProvider with VaultHelper {
         failWithEx(s"No secrets found at path [$path]")
       case Success(response) =>
         val ttl =
-          Option(response.getLeaseDuration).map(Duration(_, TimeUnit.SECONDS))
+          Option(response.getLeaseDuration).filterNot(_ == 0L).map(Duration(_, TimeUnit.SECONDS))
         Right(
           ValueWithTtl(ttl, settings.defaultTtl, parseSuccessfulResponse(path, response)),
         )

diff --git a/secret-provider/src/test/scala/io/lenses/connect/secrets/cache/TtlTest.scala b/secret-provider/src/test/scala/io/lenses/connect/secrets/cache/TtlTest.scala
@@ -39,13 +39,4 @@ class TtlTest extends AnyFunSuite with Matchers with OptionValues {
     ttl.value.expiry should be(Instant.EPOCH.plus(10, ChronoUnit.MINUTES).atOffset(toOffset))
   }
 
-  test("zero ttl should be ignored") {
-    val ttl = Ttl(Some(Duration(0, MINUTES)), Some(Duration(5, MINUTES)))
-    ttl.value.originalTtl should be(Duration(5, MINUTES))
-    ttl.value.expiry should be(Instant.EPOCH.plus(5, ChronoUnit.MINUTES).atOffset(toOffset))
-  }
-
-  test("zero ttls should be ignored") {
-    Ttl(Some(Duration(0, MINUTES)), Some(Duration(0, MINUTES))) should be(Option.empty)
-  }
 }