fix(misc): Updates to use retrofit service provider impl. (#1293)

* fix(misc): Updates to use retrofit service provider impl fix(misc): test config * fix(misc): Make kork-retrofit as runtime dep only * fix(misc): Make kork-retrofit as runtime dep only * fix(misc): Make kork-retrofit as runtime dep only * fix(misc): move to setter injection * fix(misc): Make kork-retrofit as runtime dep only * fix(misc): trying test autoconfig option to wire up dep * fix(misc): update auto config * fix(misc): add qualifier for bean * fix(misc): update dependencies * fix(misc): explicit depends on * fix(misc): add test logging * fix(misc): test config * fix(misc): test config * fix(misc): test config * fix(misc): test config * fix(misc): test config * fix(misc): test * fix(misc): add flag to dump auto config report * fix(misc): add flag to dump auto config report * fix(misc): fix config * fix(misc): fix config * fix(misc): fix config * fix(misc): test * fix(misc): test * fix(test): test fix * fix(misc): fix config Co-authored-by: Adam Jordens <adam@jordens.org>
spinnaker · Aug 14, 2020 · 794159c · 794159c
1 parent 9fc9087
commit 794159c
Show file tree

Hide file tree

Showing 10 changed files with 71 additions and 81 deletions.
diff --git a/gate-web/gate-web.gradle b/gate-web/gate-web.gradle
@@ -26,6 +26,7 @@ dependencies {
 
   implementation "com.squareup.retrofit:retrofit"
   implementation "com.squareup.retrofit:converter-jackson"
+  implementation "com.jakewharton.retrofit:retrofit1-okhttp3-client"
   implementation "com.fasterxml.jackson.dataformat:jackson-dataformat-yaml"
   implementation "com.fasterxml.jackson.datatype:jackson-datatype-jsr310"
 
@@ -44,7 +45,6 @@ dependencies {
   implementation 'commons-io:commons-io'
   implementation 'org.springframework.session:spring-session-data-redis'
   implementation "de.huxhorn.sulky:de.huxhorn.sulky.ulid"
-  implementation "com.jakewharton.retrofit:retrofit1-okhttp3-client:1.1.0"
   implementation "org.apache.commons:commons-lang3"
 
   implementation "org.springframework:spring-web"
@@ -71,6 +71,7 @@ dependencies {
   testImplementation "org.springframework.security:spring-security-oauth2-jose"
   testImplementation "com.unboundid:unboundid-ldapsdk"
   testImplementation "com.netflix.spinnaker.kork:kork-jedis-test"
+  testRuntimeOnly "com.netflix.spinnaker.kork:kork-retrofit"
 
   // Add each included authz provider as a runtime dependency
   gradle.includedProviderProjects.each {
@@ -98,4 +99,5 @@ test {
       }
     }
   }
+  useJUnitPlatform()
 }
diff --git a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/GateConfig.groovy b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/GateConfig.groovy
@@ -20,7 +20,6 @@ import com.fasterxml.jackson.databind.DeserializationFeature
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.fasterxml.jackson.dataformat.yaml.YAMLMapper
 import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule
-import com.jakewharton.retrofit.Ok3Client
 import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.config.DefaultServiceEndpoint
 import com.netflix.spinnaker.config.OkHttp3ClientConfiguration
@@ -37,9 +36,9 @@ import com.netflix.spinnaker.gate.converters.YamlHttpMessageConverter
 import com.netflix.spinnaker.gate.filters.RequestLoggingFilter
 import com.netflix.spinnaker.gate.plugins.deck.DeckPluginConfiguration
 import com.netflix.spinnaker.gate.plugins.web.PluginWebConfiguration
-import com.netflix.spinnaker.gate.retrofit.Slf4jRetrofitLogger
 import com.netflix.spinnaker.gate.services.EurekaLookupService
 import com.netflix.spinnaker.gate.services.internal.*
+import com.netflix.spinnaker.kork.client.ServiceClientProvider
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
 import com.netflix.spinnaker.kork.web.context.AuthenticatedRequestContextProvider
 import com.netflix.spinnaker.kork.web.context.RequestContextProvider
@@ -66,14 +65,10 @@ import org.springframework.http.converter.json.AbstractJackson2HttpMessageConver
 import org.springframework.security.web.context.AbstractSecurityWebApplicationInitializer
 import org.springframework.session.data.redis.config.ConfigureRedisAction
 import org.springframework.session.data.redis.config.annotation.web.http.RedisHttpSessionConfiguration
-import org.springframework.stereotype.Component
 import org.springframework.util.CollectionUtils
 import org.springframework.web.client.RestTemplate
 import redis.clients.jedis.JedisPool
 import retrofit.Endpoint
-import retrofit.RequestInterceptor
-import retrofit.RestAdapter
-import retrofit.converter.JacksonConverter
 
 import javax.servlet.*
 import java.util.concurrent.ExecutorService
@@ -88,16 +83,22 @@ import static retrofit.Endpoints.newFixedEndpoint
 @Import([PluginsAutoConfiguration, DeckPluginConfiguration, PluginWebConfiguration])
 class GateConfig extends RedisHttpSessionConfiguration {
 
+  private ServiceClientProvider serviceClientProvider
+
   @Value('${server.session.timeout-in-seconds:3600}')
   void setSessionTimeout(int maxInactiveIntervalInSeconds) {
     super.setMaxInactiveIntervalInSeconds(maxInactiveIntervalInSeconds)
   }
 
-  @Value('${retrofit.logLevel:BASIC}')
-  String retrofitLogLevel
+  @Autowired
+  void setServiceClientProvider(@Qualifier('serviceClientProvider') ServiceClientProvider serviceClientProvider) {
+    this.serviceClientProvider = serviceClientProvider
+  }
 
   @Autowired
-  RequestInterceptor spinnakerRequestInterceptor
+  GateConfig(@Value('${server.session.timeout-in-seconds:3600}') int maxInactiveIntervalInSeconds) {
+    super.setMaxInactiveIntervalInSeconds(maxInactiveIntervalInSeconds)
+  }
 
   /**
    * This pool is used for the rate limit storage, as opposed to the JedisConnectionFactory, which
@@ -172,52 +173,50 @@ class GateConfig extends RedisHttpSessionConfiguration {
   }
 
   @Bean
-  OrcaServiceSelector orcaServiceSelector(OkHttpClientProvider clientProvider, RequestContextProvider contextProvider) {
-    return new OrcaServiceSelector(createClientSelector("orca", OrcaService, clientProvider), contextProvider)
+  OrcaServiceSelector orcaServiceSelector(RequestContextProvider contextProvider) {
+    return new OrcaServiceSelector(createClientSelector("orca", OrcaService), contextProvider)
   }
 
   @Bean
   @Primary
-  FiatService fiatService(OkHttpClientProvider clientProvider) {
+  FiatService fiatService() {
     // always create the fiat service even if 'services.fiat.enabled' is 'false' (it can be enabled dynamically)
-    createClient "fiat", FiatService, clientProvider, null, true
+    createClient "fiat", FiatService, null, true
   }
 
   @Bean
-  ExtendedFiatService extendedFiatService(OkHttpClientProvider clientProvider) {
+  ExtendedFiatService extendedFiatService() {
     // always create the fiat service even if 'services.fiat.enabled' is 'false' (it can be enabled dynamically)
-    createClient "fiat", ExtendedFiatService, clientProvider, null, true
+    createClient "fiat", ExtendedFiatService,  null, true
   }
 
   @Bean
   @ConditionalOnProperty("services.fiat.config.dynamic-endpoints.login")
-  FiatService fiatLoginService(OkHttpClientProvider clientProvider) {
+  FiatService fiatLoginService() {
     // always create the fiat service even if 'services.fiat.enabled' is 'false' (it can be enabled dynamically)
-    createClient "fiat", FiatService, clientProvider, "login", true
+    createClient "fiat", FiatService,  "login", true
   }
 
 
   @Bean
-  Front50Service front50Service(OkHttpClientProvider clientProvider) {
-    createClient "front50", Front50Service, clientProvider
+  Front50Service front50Service() {
+    createClient "front50", Front50Service
   }
 
   @Bean
-  ClouddriverService clouddriverService(OkHttpClientProvider clientProvider) {
-    createClient "clouddriver", ClouddriverService, clientProvider
+  ClouddriverService clouddriverService() {
+    createClient "clouddriver", ClouddriverService
   }
 
   @Bean
   @ConditionalOnProperty("services.keel.enabled")
   KeelService keelService(OkHttpClientProvider clientProvider) {
-    createClient "keel", KeelService, clientProvider
+    createClient "keel", KeelService
   }
 
   @Bean
   ClouddriverServiceSelector clouddriverServiceSelector(ClouddriverService defaultClouddriverService,
-                                                        OkHttpClientProvider clientProvider,
                                                         DynamicConfigService dynamicConfigService,
-                                                        DynamicRoutingConfigProperties properties,
                                                         RequestContextProvider contextProvider
   ) {
     if (serviceConfiguration.getService("clouddriver").getConfig().containsKey("dynamicEndpoints")) {
@@ -240,79 +239,77 @@ class GateConfig extends RedisHttpSessionConfiguration {
 
       List<ServiceSelector> selectors = []
       endpoints.each { sourceApp, url ->
-        def service = buildService("clouddriver", clientProvider, ClouddriverService, newFixedEndpoint(url))
+        def service = buildService("clouddriver",  ClouddriverService, newFixedEndpoint(url))
         selectors << new ByUserOriginSelector(service, 2, ['origin': (Object) sourceApp])
       }
 
       return new ClouddriverServiceSelector(
         new SelectableService(selectors + defaultSelector), dynamicConfigService, contextProvider)
     }
 
-    SelectableService selectableService = createClientSelector("clouddriver", ClouddriverService, clientProvider)
+    SelectableService selectableService = createClientSelector("clouddriver", ClouddriverService)
     return new ClouddriverServiceSelector(selectableService, dynamicConfigService, contextProvider)
   }
 
   //---- semi-optional components:
   @Bean
   @ConditionalOnProperty('services.rosco.enabled')
-  RoscoService roscoService(OkHttpClientProvider clientProvider) {
-    createClient "rosco", RoscoService, clientProvider
+  RoscoService roscoService() {
+    createClient "rosco", RoscoService
   }
 
   @Bean
   @ConditionalOnProperty('services.rosco.enabled')
-  RoscoServiceSelector roscoServiceSelector(OkHttpClientProvider clientProvider, RoscoService defaultService) {
+  RoscoServiceSelector roscoServiceSelector(RoscoService defaultService) {
     return new RoscoServiceSelector(
-      createClientSelector("rosco", RoscoService, clientProvider),
+      createClientSelector("rosco", RoscoService),
       defaultService
     )
   }
 
   //---- optional backend components:
   @Bean
   @ConditionalOnProperty('services.echo.enabled')
-  EchoService echoService(OkHttpClientProvider clientProvider) {
-    createClient "echo", EchoService, clientProvider
+  EchoService echoService() {
+    createClient "echo", EchoService
   }
 
   @Bean
   @ConditionalOnProperty('services.igor.enabled')
-  IgorService igorService(OkHttpClientProvider clientProvider) {
-    createClient "igor", IgorService, clientProvider
+  IgorService igorService() {
+    createClient "igor", IgorService
   }
 
   @Bean
   @ConditionalOnProperty('services.mine.enabled')
-  MineService mineService(OkHttpClientProvider clientProvider) {
-    createClient "mine", MineService, clientProvider
+  MineService mineService() {
+    createClient "mine", MineService
   }
 
   @Bean
   @ConditionalOnProperty('services.kayenta.enabled')
-  KayentaService kayentaService(OkHttpClientProvider clientProvider,
-                                OkHttpClientConfigurationProperties props,
+  KayentaService kayentaService(OkHttpClientConfigurationProperties props,
                                 OkHttp3MetricsInterceptor interceptor,
                                 @Value('${services.kayenta.externalhttps:false}') boolean kayentaExternalHttps) {
     if (kayentaExternalHttps) {
       def noSslCustomizationProps = props.clone()
       noSslCustomizationProps.keyStore = null
       noSslCustomizationProps.trustStore = null
       def okHttpClient = new OkHttp3ClientConfiguration(noSslCustomizationProps, interceptor).create().build()
-      createClient "kayenta", KayentaService, clientProvider
+      createClient "kayenta", KayentaService
     } else {
-      createClient "kayenta", KayentaService, clientProvider
+      createClient "kayenta", KayentaService
     }
   }
 
   @Bean
   @ConditionalOnProperty('services.swabbie.enabled')
-  SwabbieService swabbieService(OkHttpClientProvider clientProvider) {
-    createClient("swabbie", SwabbieService, clientProvider)
+  SwabbieService swabbieService() {
+    createClient("swabbie", SwabbieService)
   }
 
   private <T> T createClient(String serviceName,
                              Class<T> type,
-                             OkHttpClientProvider clientProvider,
                              String dynamicName = null,
                              boolean forceEnabled = false) {
     Service service = serviceConfiguration.getService(serviceName)
@@ -326,28 +323,21 @@ class GateConfig extends RedisHttpSessionConfiguration {
 
     Endpoint endpoint = serviceConfiguration.getServiceEndpoint(serviceName, dynamicName)
 
-    buildService(serviceName, clientProvider, type, endpoint)
+    buildService(serviceName, type, endpoint)
   }
 
-  private <T> T buildService(String serviceName, OkHttpClientProvider clientProvider, Class<T> type, Endpoint endpoint) {
+  private <T> T buildService(String serviceName, Class<T> type, Endpoint endpoint) {
     // New role providers break deserialization if this is not enabled.
     ObjectMapper objectMapper = new ObjectMapper()
       .enable(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_AS_NULL)
       .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
       .registerModule(new JavaTimeModule())
 
-    new RestAdapter.Builder()
-      .setRequestInterceptor(spinnakerRequestInterceptor)
-      .setEndpoint(endpoint)
-      .setClient(new Ok3Client(clientProvider.getClient(new DefaultServiceEndpoint(serviceName, endpoint.url))))
-      .setConverter(new JacksonConverter(objectMapper))
-      .setLogLevel(RestAdapter.LogLevel.valueOf(retrofitLogLevel))
-      .setLog(new Slf4jRetrofitLogger(type))
-      .build()
-      .create(type)
+    serviceClientProvider.getService(type, new DefaultServiceEndpoint(serviceName, endpoint.url), objectMapper)
+
   }
 
-  private <T> SelectableService createClientSelector(String serviceName, Class<T> type, OkHttpClientProvider clientProvider) {
+  private <T> SelectableService createClientSelector(String serviceName, Class<T> type) {
     Service service = serviceConfiguration.getService(serviceName)
     if (CollectionUtils.isEmpty(service?.getBaseUrls())) {
       throw new IllegalArgumentException("Unknown service ${serviceName} requested of type ${type}")
@@ -358,7 +348,6 @@ class GateConfig extends RedisHttpSessionConfiguration {
         def selector = new DefaultServiceSelector(
           buildService(
             serviceName,
-            clientProvider,
             type,
             newFixedEndpoint(it.baseUrl)),
           it.priority,

diff --git a/...rc/main/groovy/com/netflix/spinnaker/gate/health/DownstreamServicesHealthIndicator.groovy b/...rc/main/groovy/com/netflix/spinnaker/gate/health/DownstreamServicesHealthIndicator.groovy
@@ -17,12 +17,11 @@
 
 package com.netflix.spinnaker.gate.health
 
-import com.jakewharton.retrofit.Ok3Client
 import com.netflix.spinnaker.config.DefaultServiceEndpoint
-import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider
 import com.netflix.spinnaker.gate.config.Service
 import com.netflix.spinnaker.gate.config.ServiceConfiguration
 import com.netflix.spinnaker.gate.services.internal.HealthCheckableService
+import com.netflix.spinnaker.kork.client.ServiceClientProvider
 import com.netflix.spinnaker.security.AuthenticatedRequest
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
@@ -32,14 +31,11 @@ import org.springframework.scheduling.annotation.Scheduled
 import org.springframework.stereotype.Component
 import org.springframework.web.context.request.RequestContextHolder
 import org.springframework.web.context.request.ServletRequestAttributes
-import retrofit.RestAdapter
 import retrofit.RetrofitError
 
 import java.util.concurrent.atomic.AtomicBoolean
 import java.util.concurrent.atomic.AtomicReference
 
-import static retrofit.Endpoints.newFixedEndpoint
-
 @Component
 @Slf4j
 class DownstreamServicesHealthIndicator extends AbstractHealthIndicator {
@@ -49,7 +45,7 @@ class DownstreamServicesHealthIndicator extends AbstractHealthIndicator {
   AtomicBoolean skipDownstreamServiceChecks = new AtomicBoolean(false)
 
   @Autowired
-  DownstreamServicesHealthIndicator(OkHttpClientProvider clientProvider,
+  DownstreamServicesHealthIndicator(ServiceClientProvider serviceProvider,
                                     ServiceConfiguration serviceConfiguration) {
     this(
       serviceConfiguration.services.findResults { String name, Service service ->
@@ -58,11 +54,8 @@ class DownstreamServicesHealthIndicator extends AbstractHealthIndicator {
         }
 
         return [
-          name, new RestAdapter.Builder()
-          .setEndpoint(newFixedEndpoint(service.baseUrl))
-          .setClient(new Ok3Client(clientProvider.getClient(new DefaultServiceEndpoint(name, service.baseUrl))))
-          .build()
-          .create(HealthCheckableService)
+          name,
+          serviceProvider.getService(HealthCheckableService, new DefaultServiceEndpoint(name, service.baseUrl))
         ]
       }.collectEntries()
     )

diff --git a/gate-web/src/test/groovy/com/netflix/spinnaker/gate/MainSpec.java b/gate-web/src/test/groovy/com/netflix/spinnaker/gate/MainSpec.java
@@ -1,15 +1,24 @@
 package com.netflix.spinnaker.gate;
 
-import org.junit.Test;
-import org.junit.runner.RunWith;
+import com.netflix.spinnaker.kork.client.ServiceClientProvider;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.test.context.ActiveProfiles;
 import org.springframework.test.context.TestPropertySource;
-import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
+import org.springframework.test.context.junit.jupiter.SpringExtension;
 
-@RunWith(SpringJUnit4ClassRunner.class)
+@ExtendWith(SpringExtension.class)
 @SpringBootTest(classes = {Main.class})
+@ActiveProfiles("test")
 @TestPropertySource(properties = {"spring.config.location=classpath:gate-test.yml"})
 public class MainSpec {
+
+  @Autowired ServiceClientProvider serviceClientProvider;
+
   @Test
-  public void startupTest() {}
+  public void startupTest() {
+    assert serviceClientProvider != null;
+  }
 }
diff --git a/.../src/test/groovy/com/netflix/spinnaker/gate/config/GateCorsAllowedOriginConfigSpec.groovy b/.../src/test/groovy/com/netflix/spinnaker/gate/config/GateCorsAllowedOriginConfigSpec.groovy
@@ -32,7 +32,7 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.
 @AutoConfigureMockMvc
 @SpringBootTest(classes = Main)
 @ActiveProfiles('alloworigincors')
-@TestPropertySource(properties = ["spring.config.location=classpath:gate-test.yml"])
+@TestPropertySource(properties = ["spring.config.location=classpath:gate-test.yml", "retrofit.enabled=true"])
 class GateCorsAllowedOriginConfigSpec extends Specification {
 
   @Autowired

diff --git a/gate-web/src/test/groovy/com/netflix/spinnaker/gate/config/GateCorsRegexConfigSpec.groovy b/gate-web/src/test/groovy/com/netflix/spinnaker/gate/config/GateCorsRegexConfigSpec.groovy
@@ -32,7 +32,7 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.
 @AutoConfigureMockMvc
 @SpringBootTest(classes = Main)
 @ActiveProfiles('regexcors')
-@TestPropertySource(properties = ["spring.config.location=classpath:gate-test.yml"])
+@TestPropertySource(properties = ["spring.config.location=classpath:gate-test.yml", "retrofit.enabled=true"])
 class GateCorsRegexConfigSpec extends Specification {
 
   @Autowired