Merge branch 'master' of github.com:spinnaker/keel

Author: luispollo

build.gradle.kts

@@ -62,6 +62,7 @@ subprojects {
 
       "testImplementation"("org.junit.platform:junit-platform-runner")
       "testImplementation"("org.junit.jupiter:junit-jupiter-api")
+      "testImplementation"("org.junit.jupiter:junit-jupiter-params")
       "testImplementation"("io.mockk:mockk")
       "testImplementation"("org.jacoco:org.jacoco.ant:0.8.5")
 

keel-clouddriver/src/main/kotlin/com/netflix/spinnaker/keel/clouddriver/CloudDriverCache.kt

@@ -15,6 +15,7 @@
  */
 package com.netflix.spinnaker.keel.clouddriver
 
+import com.netflix.spinnaker.keel.clouddriver.model.Certificate
 import com.netflix.spinnaker.keel.clouddriver.model.Credential
 import com.netflix.spinnaker.keel.clouddriver.model.Network
 import com.netflix.spinnaker.keel.clouddriver.model.SecurityGroupSummary
@@ -32,6 +33,8 @@ interface CloudDriverCache {
   fun credentialBy(name: String): Credential
   fun defaultKeyPairForAccount(account: String) =
     credentialBy(account).attributes["defaultKeyPair"] as String
+  fun certificateByName(name: String): Certificate
+  fun certificateByArn(arn: String): Certificate
 }
 
 class ResourceNotFound(message: String) : IntegrationException(message)

keel-clouddriver/src/main/kotlin/com/netflix/spinnaker/keel/clouddriver/CloudDriverService.kt

@@ -18,6 +18,7 @@ package com.netflix.spinnaker.keel.clouddriver
 import com.netflix.spinnaker.keel.clouddriver.model.ActiveServerGroup
 import com.netflix.spinnaker.keel.clouddriver.model.AmazonLoadBalancer
 import com.netflix.spinnaker.keel.clouddriver.model.ApplicationLoadBalancerModel
+import com.netflix.spinnaker.keel.clouddriver.model.Certificate
 import com.netflix.spinnaker.keel.clouddriver.model.ClassicLoadBalancerModel
 import com.netflix.spinnaker.keel.clouddriver.model.Credential
 import com.netflix.spinnaker.keel.clouddriver.model.DockerImage
@@ -66,10 +67,11 @@ interface CloudDriverService {
     @Header("X-SPINNAKER-USER") user: String = DEFAULT_SERVICE_ACCOUNT
   ): SecurityGroupSummary
 
-  @GET("/networks")
+  @GET("/networks/{cloudProvider}")
   suspend fun listNetworks(
+    @Path("cloudProvider") cloudProvider: String,
     @Header("X-SPINNAKER-USER") user: String = DEFAULT_SERVICE_ACCOUNT
-  ): Map<String, Set<Network>>
+  ): Set<Network>
 
   @GET("/subnets/{cloudProvider}")
   suspend fun listSubnets(
@@ -196,4 +198,7 @@ interface CloudDriverService {
     @Query("entityId") entityId: String,
     @Header("X-SPINNAKER-USER") user: String = DEFAULT_SERVICE_ACCOUNT
   ): List<EntityTags>
+
+  @GET("/certificates/aws")
+  suspend fun getCertificates() : List<Certificate>
 }

keel-clouddriver/src/main/kotlin/com/netflix/spinnaker/keel/clouddriver/MemoryCloudDriverCache.kt

@@ -18,6 +18,7 @@ package com.netflix.spinnaker.keel.clouddriver
 import com.github.benmanes.caffeine.cache.AsyncCache
 import com.github.benmanes.caffeine.cache.AsyncLoadingCache
 import com.netflix.spinnaker.keel.caffeine.CacheFactory
+import com.netflix.spinnaker.keel.clouddriver.model.Certificate
 import com.netflix.spinnaker.keel.clouddriver.model.Credential
 import com.netflix.spinnaker.keel.clouddriver.model.Network
 import com.netflix.spinnaker.keel.clouddriver.model.SecurityGroupSummary
@@ -56,7 +57,6 @@ class MemoryCloudDriverCache(
           }
       }
         .handleNotFound()
-        ?: notFound("Security group with id $id not found in the $account account and $region region")
     }
 
   private val securityGroupsByName: AsyncLoadingCache<Triple<String, String, String>, SecurityGroupSummary> = cacheFactory
@@ -72,34 +72,31 @@ class MemoryCloudDriverCache(
           }
       }
         .handleNotFound()
-        ?: notFound("Security group with name $name not found in the $account account and $region region")
     }
 
   private val networksById: AsyncLoadingCache<String, Network> = cacheFactory
-    .asyncLoadingCache(cacheName = "networksById") { id ->
+    .asyncBulkLoadingCache(cacheName = "networksById") {
       runCatching {
-        cloudDriver.listNetworks(DEFAULT_SERVICE_ACCOUNT)["aws"]
-          ?.firstOrNull { it.id == id }
-          ?.also {
-            networksByName.put(Triple(it.account, it.region, it.name), completedFuture(it))
-          }
+        cloudDriver.listNetworks("aws", DEFAULT_SERVICE_ACCOUNT)
+          .associateBy { it.id }
       }
-        .handleNotFound()
-        ?: notFound("VPC network with id $id not found")
+        .getOrElse { ex ->
+          throw CacheLoadingException("Error loading networksById cache", ex)
+        }
     }
 
   private val networksByName: AsyncLoadingCache<Triple<String, String, String?>, Network> = cacheFactory
-    .asyncLoadingCache(cacheName = "networksByName") { (account, region, name) ->
+    .asyncBulkLoadingCache(cacheName = "networksByName") {
       runCatching {
         cloudDriver
-          .listNetworks(DEFAULT_SERVICE_ACCOUNT)["aws"]
-          ?.firstOrNull { it.name == name && it.account == account && it.region == region }
-          ?.also {
-            networksById.put(it.id, completedFuture(it))
+          .listNetworks("aws", DEFAULT_SERVICE_ACCOUNT)
+          .associateBy {
+            Triple(it.account, it.region, it.name)
           }
       }
-        .handleNotFound()
-        ?: notFound("VPC network named $name not found in $region")
+        .getOrElse { ex ->
+          throw CacheLoadingException("Error loading networksByName cache", ex)
+        }
     }
 
   private data class AvailabilityZoneKey(
@@ -121,7 +118,7 @@ class MemoryCloudDriverCache(
           .toSet()
       }
         .getOrElse { ex ->
-          throw CacheLoadingException("Error loading cache", ex)
+          throw CacheLoadingException("Error loading availabilityZones cache", ex)
         }
     }
 
@@ -133,69 +130,98 @@ class MemoryCloudDriverCache(
         cloudDriver.getCredential(name, DEFAULT_SERVICE_ACCOUNT)
       }
         .handleNotFound()
-        ?: notFound("Credentials with name $name not found")
     }
 
   private val subnetsById: AsyncLoadingCache<String, Subnet> = cacheFactory
-    .asyncLoadingCache(cacheName = "subnetsById") { subnetId ->
+    .asyncBulkLoadingCache(cacheName = "subnetsById") {
       runCatching {
         cloudDriver
           .listSubnets("aws", DEFAULT_SERVICE_ACCOUNT)
-          .find { it.id == subnetId }
+          .associateBy { it.id }
       }
-        .handleNotFound()
-        ?: notFound("Subnet with id $subnetId not found")
+        .getOrElse { ex -> throw CacheLoadingException("Error loading subnetsById cache", ex) }
     }
 
-  private val subnetsByPurpose: AsyncLoadingCache<Triple<String, String, String>, Subnet> = cacheFactory
-    .asyncLoadingCache(cacheName = "subnetsByPurpose") { (account, region, purpose) ->
+  private val subnetsByPurpose: AsyncLoadingCache<Triple<String, String, String?>, Subnet> = cacheFactory
+    .asyncBulkLoadingCache(cacheName = "subnetsByPurpose") {
       runCatching {
         cloudDriver
           .listSubnets("aws", DEFAULT_SERVICE_ACCOUNT)
-          .find { it.account == account && it.region == region && it.purpose == purpose }
+          .associateBy { Triple(it.account, it.region, it.purpose) }
       }
-        .handleNotFound()
-        ?: notFound("Subnet with purpose \"$purpose\" not found in $account:$region")
+        .getOrElse { ex -> throw CacheLoadingException("Error loading subnetsByPurpose cache", ex) }
     }
 
+  private val certificatesByName: AsyncLoadingCache<String, Certificate> =
+    cacheFactory
+      .asyncBulkLoadingCache("certificatesByName") {
+        runCatching {
+          cloudDriver
+            .getCertificates()
+            .associateBy { it.serverCertificateName }
+        }
+          .getOrElse { ex -> throw CacheLoadingException("Error loading certificatesByName cache", ex) }
+      }
+
+  private val certificatesByArn: AsyncLoadingCache<String, Certificate> =
+    cacheFactory
+      .asyncBulkLoadingCache("certificatesByArn") {
+        runCatching {
+          cloudDriver
+            .getCertificates()
+            .associateBy { it.arn }
+        }
+          .getOrElse { ex -> throw CacheLoadingException("Error loading certificatesByArn cache", ex) }
+      }
+
   override fun credentialBy(name: String): Credential =
     runBlocking {
-      credentials.get(name).await()
+      credentials.get(name).await() ?: notFound("Credential with name $name not found")
     }
 
   override fun securityGroupById(account: String, region: String, id: String): SecurityGroupSummary =
     runBlocking {
-      securityGroupsById.get(Triple(account, region, id)).await()
+      securityGroupsById.get(Triple(account, region, id)).await()?: notFound("Security group with id $id not found in $account:$region")
     }
 
   override fun securityGroupByName(account: String, region: String, name: String): SecurityGroupSummary =
     runBlocking {
-      securityGroupsByName.get(Triple(account, region, name)).await()
+      securityGroupsByName.get(Triple(account, region, name)).await()?: notFound("Security group with name $name not found in $account:$region")
     }
 
   override fun networkBy(id: String): Network =
     runBlocking {
-      networksById.get(id).await()
+      networksById.get(id).await() ?: notFound("VPC network with id $id not found")
     }
 
   override fun networkBy(name: String?, account: String, region: String): Network =
     runBlocking {
-      networksByName.get(Triple(account, region, name)).await()
+      networksByName.get(Triple(account, region, name)).await() ?: notFound("VPC network named $name not found in $account:$region")
     }
 
   override fun availabilityZonesBy(account: String, vpcId: String, purpose: String, region: String): Set<String> =
     runBlocking {
-      availabilityZones.get(AvailabilityZoneKey(account, region, vpcId, purpose)).await()
+      availabilityZones.get(AvailabilityZoneKey(account, region, vpcId, purpose)).await() ?: notFound("Availability zone with purpose \"$purpose\" not found in $account:$region")
     }
 
   override fun subnetBy(subnetId: String): Subnet =
     runBlocking {
-      subnetsById.get(subnetId).await()
+      subnetsById.get(subnetId).await() ?: notFound("Subnet with id $subnetId not found")
     }
 
   override fun subnetBy(account: String, region: String, purpose: String): Subnet =
     runBlocking {
-      subnetsByPurpose.get(Triple(account, region, purpose)).await()
+      subnetsByPurpose.get(Triple(account, region, purpose)).await() ?: notFound("Subnet with purpose \"$purpose\" not found in $account:$region")
+    }
+
+  override fun certificateByName(name: String): Certificate =
+    runBlocking {
+      certificatesByName.get(name).await() ?: notFound("Certificate with name $name not found")
+    }
+
+  override fun certificateByArn(arn: String): Certificate =
+    runBlocking {
+      certificatesByArn.get(arn).await() ?: notFound("Certificate with ARN $arn not found")
     }
 }
 

keel-clouddriver/src/main/kotlin/com/netflix/spinnaker/keel/clouddriver/model/Certificate.kt

@@ -0,0 +1,3 @@
+package com.netflix.spinnaker.keel.clouddriver.model
+
+data class Certificate(val serverCertificateName: String, val arn: String)

keel-clouddriver/src/test/kotlin/com/netflix/spinnaker/keel/clouddriver/MemoryCloudDriverCacheTest.kt

@@ -1,6 +1,7 @@
 package com.netflix.spinnaker.keel.clouddriver
 
 import com.netflix.spinnaker.keel.caffeine.TEST_CACHE_FACTORY
+import com.netflix.spinnaker.keel.clouddriver.model.Certificate
 import com.netflix.spinnaker.keel.clouddriver.model.Credential
 import com.netflix.spinnaker.keel.clouddriver.model.Network
 import com.netflix.spinnaker.keel.clouddriver.model.SecurityGroupSummary
@@ -9,6 +10,8 @@ import com.netflix.spinnaker.keel.retrofit.RETROFIT_NOT_FOUND
 import com.netflix.spinnaker.keel.retrofit.RETROFIT_SERVICE_UNAVAILABLE
 import io.mockk.mockk
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.ValueSource
 import strikt.api.expectThat
 import strikt.api.expectThrows
 import strikt.assertions.containsExactly
@@ -51,6 +54,11 @@ internal class MemoryCloudDriverCacheTest {
     Subnet("d", "vpc-3", "prod", "us-west-2", "us-west-2d", "external (vpc3)")
   )
 
+  val certificates = listOf(
+    Certificate("cert-1", "arn:cert-1"),
+    Certificate("cert-2", "arn:cert-2")
+  )
+
   @Test
   fun `security groups are looked up from CloudDriver when accessed by id`() {
     every {
@@ -133,8 +141,8 @@ internal class MemoryCloudDriverCacheTest {
   @Test
   fun `VPC networks are looked up by id from CloudDriver`() {
     every {
-      cloudDriver.listNetworks()
-    } returns mapOf("aws" to vpcs)
+      cloudDriver.listNetworks("aws")
+    } returns vpcs
 
     subject.networkBy("vpc-2").let { vpc ->
       expectThat(vpc) {
@@ -148,8 +156,8 @@ internal class MemoryCloudDriverCacheTest {
   @Test
   fun `an invalid VPC id throws an exception`() {
     every {
-      cloudDriver.listNetworks()
-    } returns mapOf("aws" to vpcs)
+      cloudDriver.listNetworks("aws")
+    } returns vpcs
 
     expectThrows<ResourceNotFound> {
       subject.networkBy("vpc-5")
@@ -159,8 +167,8 @@ internal class MemoryCloudDriverCacheTest {
   @Test
   fun `VPC networks are looked up by name and region from CloudDriver`() {
     every {
-      cloudDriver.listNetworks()
-    } returns mapOf("aws" to vpcs)
+      cloudDriver.listNetworks("aws")
+    } returns vpcs
 
     subject.networkBy("vpcName", "test", "us-west-2").let { vpc ->
       expectThat(vpc.id).isEqualTo("vpc-2")
@@ -170,8 +178,8 @@ internal class MemoryCloudDriverCacheTest {
   @Test
   fun `an invalid VPC name and region throws an exception`() {
     every {
-      cloudDriver.listNetworks()
-    } returns mapOf("aws" to vpcs)
+      cloudDriver.listNetworks("aws")
+    } returns vpcs
 
     expectThrows<ResourceNotFound> {
       subject.networkBy("invalid", "prod", "us-west-2")
@@ -211,4 +219,76 @@ internal class MemoryCloudDriverCacheTest {
       subject.availabilityZonesBy("prod", "vpc-3", "external (vpc3)", "us-west-2")
     ).containsExactly("us-west-2d")
   }
+
+  @ParameterizedTest
+  @ValueSource(strings = ["cert-1", "cert-2"])
+  fun `certificates are looked up from CloudDriver when requested by name`(name: String) {
+    every { cloudDriver.getCertificates() } returns certificates
+
+    expectThat(subject.certificateByName(name))
+      .get { serverCertificateName } isEqualTo name
+  }
+
+  @Test
+  fun `an unknown certificate name throws an exception`() {
+    every { cloudDriver.getCertificates() } returns certificates
+
+    expectThrows<ResourceNotFound> { subject.certificateByName("does-not-exist") }
+  }
+
+  @ParameterizedTest
+  @ValueSource(strings = ["cert-1", "cert-2"])
+  fun `subsequent requests for a certificate by name hit the cache`(name: String) {
+    every { cloudDriver.getCertificates() } returns certificates
+
+    repeat(4) { subject.certificateByName(name) }
+
+    verify(exactly = 1) { cloudDriver.getCertificates() }
+  }
+
+  @Test
+  fun `all certs are cached at once when requested by name`() {
+    every { cloudDriver.getCertificates() } returns certificates
+
+    listOf("cert-1", "cert-2")
+      .forEach(subject::certificateByName)
+
+    verify(exactly = 1) { cloudDriver.getCertificates() }
+  }
+
+  @ParameterizedTest
+  @ValueSource(strings = ["arn:cert-1", "arn:cert-2"])
+  fun `certificates are looked up from CloudDriver when requested by ARN`(arn: String) {
+    every { cloudDriver.getCertificates() } returns certificates
+
+    expectThat(subject.certificateByArn(arn))
+      .get { arn } isEqualTo arn
+  }
+
+  @Test
+  fun `an unknown certificate ARN throws an exception`() {
+    every { cloudDriver.getCertificates() } returns certificates
+
+    expectThrows<ResourceNotFound> { subject.certificateByArn("does-not-exist") }
+  }
+
+  @ParameterizedTest
+  @ValueSource(strings = ["arn:cert-1", "arn:cert-2"])
+  fun `subsequent requests for a certificate by ARN hit the cache`(arn: String) {
+    every { cloudDriver.getCertificates() } returns certificates
+
+    repeat(5) { subject.certificateByArn(arn) }
+
+    verify(exactly = 1) { cloudDriver.getCertificates() }
+  }
+
+  @Test
+  fun `all certs are cached at once when requested by ARN`() {
+    every { cloudDriver.getCertificates() } returns certificates
+
+    listOf("arn:cert-1", "arn:cert-2")
+      .forEach(subject::certificateByArn)
+
+    verify(exactly = 1) { cloudDriver.getCertificates() }
+  }
 }