fix(plugins): DeckPluginCache should only download plugins that requi…

…re deck (#1028)

* fix(plugins): DeckPluginCache should only download plugins that require deck

* fix(spring): changes to support spring boot 2.2.4

* fix(plugins): Set spring.applicaiton.name test property in MainSpec

* fix(plugins): Add property to gate-test.yml and some failing specs

build.gradle

@@ -51,6 +51,7 @@ allprojects {
 
       implementation "org.codehaus.groovy:groovy-all"
       implementation "net.logstash.logback:logstash-logback-encoder"
+      implementation "org.jetbrains.kotlin:kotlin-reflect"
       runtimeOnly "org.springframework.boot:spring-boot-properties-migrator"
       testRuntime "org.springframework.boot:spring-boot-properties-migrator"
 

gate-deck-plugins/src/main/kotlin/com/netflix/spinnaker/gate/plugins/DeckPluginCache.kt

@@ -18,9 +18,7 @@ package com.netflix.spinnaker.gate.plugins
 import com.netflix.spectator.api.Id
 import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.kork.plugins.bundle.PluginBundleExtractor
-import com.netflix.spinnaker.kork.plugins.update.PluginUpdateService
 import com.netflix.spinnaker.kork.plugins.update.SpinnakerUpdateManager
-import org.pf4j.update.PluginInfo
 import org.slf4j.LoggerFactory
 import org.springframework.scheduling.annotation.Scheduled
 import java.nio.file.Files
@@ -29,15 +27,9 @@ import java.nio.file.StandardCopyOption
 
 /**
  * Responsible for keeping an up-to-date cache of all plugins that Deck needs to know about.
- *
- * TODO(rz): Currently downloads all plugins and inspects the extracted files to determine if a plugin has deck assets.
- *  This is pretty inefficient - would be better to have a metadata file shipped alongside a bundle.
- * TODO(rz): Downloads all plugins to an isolated temp directory, so if Gate itself has plugins, this cache will be
- *  downloading an intersection of plugins twice.
  */
 class DeckPluginCache(
   private val updateManager: SpinnakerUpdateManager,
-  private val updateService: PluginUpdateService,
   private val pluginBundleExtractor: PluginBundleExtractor,
   private val registry: Registry
 ) {
@@ -70,16 +62,12 @@ class DeckPluginCache(
       updateManager.refresh()
 
       val newCache = updateManager.plugins
-        .map {
-          val plugin = DeckPluginVersion(it.id, selectPluginVersion(it))
-          val path = getOrDownload(plugin.id, plugin.version)
-          PluginCacheEntry(plugin, path)
-        }
-        .filter {
-          // TODO(rz): Once bundles support manifest artifacts, we can just inspect those
-          //  for a deck plugin rather than downloading everything and seeing if a
-          //  index.js file exists.
-          it.path.resolve("index.js").toFile().exists()
+        .mapNotNull { pluginInfo ->
+          updateManager.getLastPluginRelease(pluginInfo.id, DECK_REQUIREMENT)?.let { lastRelease ->
+            val plugin = DeckPluginVersion(pluginInfo.id, lastRelease.version)
+            val path = getOrDownload(plugin.id, plugin.version)
+            PluginCacheEntry(plugin, path)
+          }
         }
 
       cache.removeIf { !newCache.contains(it) }
@@ -109,7 +97,7 @@ class DeckPluginCache(
       registry.timer(downloadDurationId.withPluginTags(pluginId, pluginVersion)).record {
         log.info("Downloading plugin '$pluginId@$pluginVersion'")
         val deckPluginPath = pluginBundleExtractor.extractService(
-          updateService.download(pluginId, pluginVersion),
+          updateManager.downloadPluginRelease(pluginId, pluginVersion),
           "deck"
         )
 
@@ -124,10 +112,6 @@ class DeckPluginCache(
     return cachePath
   }
 
-  private fun selectPluginVersion(pluginInfo: PluginInfo): String {
-    return updateManager.getLastPluginRelease(pluginInfo.id).version
-  }
-
   private fun Id.withPluginTags(pluginId: String, version: String): Id =
     withTags("pluginId", pluginId, "version", version)
 
@@ -142,5 +126,6 @@ class DeckPluginCache(
 
   companion object {
     internal val CACHE_ROOT_PATH = Files.createTempDirectory("downloaded-plugin-cache")
+    internal const val DECK_REQUIREMENT = "deck"
   }
 }

gate-deck-plugins/src/main/kotlin/com/netflix/spinnaker/gate/plugins/DeckPluginConfiguration.kt

@@ -17,7 +17,6 @@ package com.netflix.spinnaker.gate.plugins
 
 import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.kork.plugins.bundle.PluginBundleExtractor
-import com.netflix.spinnaker.kork.plugins.update.PluginUpdateService
 import com.netflix.spinnaker.kork.plugins.update.SpinnakerUpdateManager
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty
 import org.springframework.context.annotation.Bean
@@ -33,10 +32,9 @@ open class DeckPluginConfiguration {
   @Bean
   open fun deckPluginCache(
     updateManager: SpinnakerUpdateManager,
-    updateService: PluginUpdateService,
     registry: Registry
   ): DeckPluginCache =
-    DeckPluginCache(updateManager, updateService, PluginBundleExtractor(), registry)
+    DeckPluginCache(updateManager, PluginBundleExtractor(), registry)
 
   @Bean
   open fun deckPluginService(

gate-deck-plugins/src/main/kotlin/com/netflix/spinnaker/gate/plugins/DeckPluginsController.kt

@@ -33,7 +33,7 @@ class DeckPluginsController(
 ) {
 
   @ApiOperation(value = "Retrieve a plugin manifest")
-  @GetMapping
+  @GetMapping("/plugin-manifest.json")
   fun getPluginManifest(): List<DeckPluginVersion> {
     return deckPluginService.getPluginsManifests()
   }

gate-deck-plugins/src/test/kotlin/com/netflix/spinnaker/gate/plugins/DeckPluginCacheTest.kt

@@ -18,7 +18,6 @@ package com.netflix.spinnaker.gate.plugins
 import com.netflix.spectator.api.NoopRegistry
 import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.kork.plugins.bundle.PluginBundleExtractor
-import com.netflix.spinnaker.kork.plugins.update.PluginUpdateService
 import com.netflix.spinnaker.kork.plugins.update.SpinnakerUpdateManager
 import dev.minutest.junit.JUnit5Minutests
 import dev.minutest.rootContext
@@ -59,10 +58,9 @@ class DeckPluginCacheTest : JUnit5Minutests {
 
   private inner class Fixture {
     val updateManager: SpinnakerUpdateManager = mockk(relaxed = true)
-    val updateService: PluginUpdateService = mockk(relaxed = true)
     val pluginBundleExtractor: PluginBundleExtractor = mockk(relaxed = true)
     val registry: Registry = NoopRegistry()
-    val subject = DeckPluginCache(updateManager, updateService, pluginBundleExtractor, registry)
+    val subject = DeckPluginCache(updateManager, pluginBundleExtractor, registry)
 
     init {
       val plugins = listOf(
@@ -93,13 +91,13 @@ class DeckPluginCacheTest : JUnit5Minutests {
       every { updateManager.plugins } returns plugins
 
       val pluginIdSlot = slot<String>()
-      every { updateManager.getLastPluginRelease(capture(pluginIdSlot)) } answers {
+      every { updateManager.getLastPluginRelease(capture(pluginIdSlot), "deck") } answers {
         PluginInfo.PluginRelease().apply {
           version = plugins.find { it.id == pluginIdSlot.captured }!!.releases.last().version
         }
       }
 
-      every { updateService.download(any(), any()) } returns Paths.get("/dev/null")
+      every { updateManager.downloadPluginRelease(any(), any()) } returns Paths.get("/dev/null")
 
       every { pluginBundleExtractor.extractService(any(), any()) } answers {
         val temp = Files.createTempDirectory("downloaded-plugins")

gate-web/gate-web.gradle

@@ -59,6 +59,7 @@ dependencies {
   testImplementation "com.squareup.retrofit:retrofit-mock"
   testImplementation "org.springframework.security:spring-security-test"
   testImplementation "org.springframework.security:spring-security-ldap"
+  testImplementation "org.springframework.security:spring-security-oauth2-jose"
   testImplementation "com.unboundid:unboundid-ldapsdk"
   testImplementation "com.netflix.spinnaker.kork:kork-jedis-test"
 

gate-web/src/main/groovy/com/netflix/spinnaker/gate/Main.groovy

@@ -17,7 +17,7 @@
 package com.netflix.spinnaker.gate
 
 import com.netflix.spinnaker.hystrix.spectator.HystrixSpectatorConfig
-import org.springframework.boot.actuate.autoconfigure.ldap.LdapHealthIndicatorAutoConfiguration
+import org.springframework.boot.actuate.autoconfigure.ldap.LdapHealthContributorAutoConfiguration
 import org.springframework.boot.autoconfigure.EnableAutoConfiguration
 import org.springframework.boot.autoconfigure.groovy.template.GroovyTemplateAutoConfiguration
 import org.springframework.boot.autoconfigure.gson.GsonAutoConfiguration
@@ -33,7 +33,9 @@ import org.springframework.scheduling.annotation.EnableAsync
 @EnableConfigurationProperties
 @Import([HystrixSpectatorConfig])
 @ComponentScan(["com.netflix.spinnaker.gate", "com.netflix.spinnaker.config"])
-@EnableAutoConfiguration(exclude = [GroovyTemplateAutoConfiguration, GsonAutoConfiguration, LdapHealthIndicatorAutoConfiguration])
+@EnableAutoConfiguration(exclude = [GroovyTemplateAutoConfiguration,
+  GsonAutoConfiguration,
+  LdapHealthContributorAutoConfiguration])
 class Main {
 
   static final Map<String, String> DEFAULT_PROPS = [

gate-web/src/main/groovy/com/netflix/spinnaker/gate/health/RedisRelaxedHealthIndicator.groovy

@@ -27,7 +27,7 @@ import org.springframework.stereotype.Component
 import org.springframework.util.Assert
 import redis.clients.jedis.Jedis
 import redis.clients.jedis.JedisPool
-import redis.clients.util.Pool
+import redis.clients.jedis.util.Pool
 
 import java.util.concurrent.atomic.AtomicReference
 import java.util.function.ToDoubleFunction

gate-web/src/test/groovy/com/netflix/spinnaker/gate/FunctionalSpec.groovy

@@ -41,7 +41,6 @@ import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
 import com.netflix.spinnaker.kork.dynamicconfig.SpringDynamicConfigService
 import com.netflix.spinnaker.kork.web.exceptions.GenericExceptionHandlers
 import org.springframework.boot.SpringApplication
-import org.springframework.boot.actuate.autoconfigure.ldap.LdapHealthIndicatorAutoConfiguration
 import org.springframework.boot.autoconfigure.EnableAutoConfiguration
 import org.springframework.boot.autoconfigure.groovy.template.GroovyTemplateAutoConfiguration
 import org.springframework.boot.autoconfigure.gson.GsonAutoConfiguration
@@ -254,7 +253,7 @@ class FunctionalSpec extends Specification {
   }
 
   @Order(10)
-  @EnableAutoConfiguration(exclude = [GroovyTemplateAutoConfiguration, GsonAutoConfiguration, LdapHealthIndicatorAutoConfiguration])
+  @EnableAutoConfiguration(exclude = [GroovyTemplateAutoConfiguration, GsonAutoConfiguration])
   private static class FunctionalConfiguration extends WebSecurityConfigurerAdapter {
 
     @Bean

gate-web/src/test/groovy/com/netflix/spinnaker/gate/security/ldap/LdapAuthSpec.groovy

@@ -48,7 +48,7 @@ import static org.springframework.security.test.web.servlet.request.SecurityMock
 @Slf4j
 @GateSystemTest
 @SpringBootTest(
-    properties = ['ldap.enabled=true'])
+    properties = ['ldap.enabled=true', 'spring.application.name=gate'])
 @ContextConfiguration(
   classes = [LdapSsoConfig, Main, LdapTestConfig, RedisTestConfig],
   initializers = YamlFileApplicationContextInitializer

gate-web/src/test/groovy/com/netflix/spinnaker/gate/swagger/GenerateSwaggerSpec.groovy

@@ -25,7 +25,9 @@ import static org.springframework.test.web.servlet.request.MockMvcRequestBuilder
   classes = [Main],
   initializers = YamlFileApplicationContextInitializer
 )
-@TestPropertySource(properties = [ "services.kayenta.enabled=true","services.kayenta.canary-config-store=true" ]) // Enable Controllers we want to document in the spec here.
+@TestPropertySource(
+  properties = [ "services.kayenta.enabled=true","services.kayenta.canary-config-store=true",
+    "spring.application.name=gate" ]) // Enable Controllers we want to document in the spec here.
 class GenerateSwaggerSpec extends Specification {
 
   @Autowired

gate-web/src/test/resources/basic-auth.properties

@@ -1,3 +1,4 @@
 security.basicform.enabled=true
 spring.security.user.name=basic-user
 spring.security.user.password=basic-password
+spring.application.name=gate

gate-web/src/test/resources/gate-test.yml

@@ -1,3 +1,7 @@
+spring:
+  application:
+    name: gate
+
 services:
   clouddriver.baseUrl: "http://localhost:7002"
 

gradle.properties

@@ -1,7 +1,7 @@
 #Thu Jan 23 21:05:49 UTC 2020
-fiatVersion=1.12.0
+fiatVersion=1.13.0
 enablePublishing=false
 spinnakerGradleVersion=7.0.1
-korkVersion=7.12.0
+korkVersion=7.13.1
 includeProviders=basic,iap,ldap,oauth2,saml,x509
 org.gradle.parallel=true