Fix(orca-front50): front50 timeout fallback

orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/config/Front50Configuration.groovy

@@ -30,6 +30,8 @@ import com.netflix.spinnaker.orca.retrofit.RetrofitConfiguration
 import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog
 import groovy.transform.CompileStatic
 import okhttp3.OkHttpClient
+import org.slf4j.Logger
+import org.slf4j.LoggerFactory
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression
 import org.springframework.boot.context.properties.EnableConfigurationProperties
@@ -59,6 +61,13 @@ import static retrofit.Endpoints.newFixedEndpoint
 @ConditionalOnExpression('${front50.enabled:true}')
 class Front50Configuration {
 
+  private static final Logger log = LoggerFactory.getLogger(Front50Configuration.class)
+
+  // Default timeout values if no other configuration is provided
+  private static final Long DEFAULT_READ_TIMEOUT_MS = 60000L    // 60 seconds
+  private static final Long DEFAULT_WRITE_TIMEOUT_MS = 60000L   // 60 seconds
+  private static final Long DEFAULT_CONNECT_TIMEOUT_MS = 10000L // 10 seconds
+
   @Autowired
   OkHttpClientProvider clientProvider
 
@@ -75,23 +84,91 @@ class Front50Configuration {
 
   @Bean
   Front50Service front50Service(Endpoint front50Endpoint, ObjectMapper mapper, Front50ConfigurationProperties front50ConfigurationProperties) {
-    OkHttpClient okHttpClient = clientProvider.getClient(new DefaultServiceEndpoint("front50", front50Endpoint.getUrl()));
-    okHttpClient = okHttpClient.newBuilder()
-        .readTimeout(front50ConfigurationProperties.okhttp.readTimeoutMs, TimeUnit.MILLISECONDS)
-        .writeTimeout(front50ConfigurationProperties.okhttp.writeTimeoutMs, TimeUnit.MILLISECONDS)
-        .connectTimeout(front50ConfigurationProperties.okhttp.connectTimeoutMs, TimeUnit.MILLISECONDS)
-        .build();
+    // Get base client with global configuration
+    OkHttpClient baseClient = clientProvider.getClient(new DefaultServiceEndpoint("front50", front50Endpoint.getUrl()))
+
+    // Configure client with appropriate timeouts
+    OkHttpClient configuredClient = configureTimeouts(baseClient, front50ConfigurationProperties)
+
+    // Create and return the service
     new RestAdapter.Builder()
       .setRequestInterceptor(spinnakerRequestInterceptor)
       .setEndpoint(front50Endpoint)
-      .setClient(new Ok3Client(okHttpClient))
+      .setClient(new Ok3Client(configuredClient))
       .setLogLevel(retrofitLogLevel)
       .setLog(new RetrofitSlf4jLog(Front50Service))
       .setConverter(new JacksonConverter(mapper))
       .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
       .build()
       .create(Front50Service)
   }
+
+  /**
+   * Configures an OkHttpClient with appropriate timeout settings using fallback chain:
+   * 1. Use explicit Front50 timeouts if configured
+   * 2. Fall back to global client timeouts
+   * 3. Fall back to default timeout values
+   */
+  private OkHttpClient configureTimeouts(OkHttpClient baseClient, Front50ConfigurationProperties props) {
+    OkHttpClient.Builder builder = baseClient.newBuilder()
+
+    // Apply the timeouts following the fallback chain
+    long readTimeout = getEffectiveTimeout(
+        props.okhttp?.readTimeoutMs, 
+        baseClient.readTimeoutMillis(), 
+        DEFAULT_READ_TIMEOUT_MS,
+        "read")
+
+    long writeTimeout = getEffectiveTimeout(
+        props.okhttp?.writeTimeoutMs, 
+        baseClient.writeTimeoutMillis(), 
+        DEFAULT_WRITE_TIMEOUT_MS,
+        "write")
+
+    long connectTimeout = getEffectiveTimeout(
+        props.okhttp?.connectTimeoutMs, 
+        baseClient.connectTimeoutMillis(), 
+        DEFAULT_CONNECT_TIMEOUT_MS,
+        "connect")
+
+    // Apply effective timeouts to builder
+    builder.readTimeout(readTimeout, TimeUnit.MILLISECONDS)
+           .writeTimeout(writeTimeout, TimeUnit.MILLISECONDS)
+           .connectTimeout(connectTimeout, TimeUnit.MILLISECONDS)
+
+    return builder.build()
+  }
+
+  /**
+   * Returns the effective timeout by following the fallback chain:
+   * 1. Explicit config from Front50ConfigProperties (if different from default)
+   * 2. Global client config 
+   * 3. Default value
+   */
+  private long getEffectiveTimeout(Integer explicitTimeout, long globalTimeout, long defaultTimeout, String timeoutType) {
+    // First check if the explicit timeout is non-null and has been explicitly configured
+    // to something other than the default
+    if (explicitTimeout != null) {
+      // Check if the value differs from default configuration values
+      boolean isDefaultReadValue = explicitTimeout == 60000 && timeoutType == "read"
+      boolean isDefaultWriteValue = explicitTimeout == 60000 && timeoutType == "write"
+      boolean isDefaultConnectValue = explicitTimeout == 10000 && timeoutType == "connect"
+
+      // Only log and use explicit timeout if it's not simply the default value
+      if (!isDefaultReadValue && !isDefaultWriteValue && !isDefaultConnectValue) {
+        log.debug("Using explicit Front50 {} timeout: {}ms", timeoutType, explicitTimeout)
+        return explicitTimeout.longValue()
+      }
+    }
+
+    if (globalTimeout > 0) {
+      log.debug("Using global {} timeout: {}ms", timeoutType, globalTimeout)
+      return globalTimeout 
+    } else {
+      log.debug("Using default {} timeout: {}ms", timeoutType, defaultTimeout)
+      return defaultTimeout
+    }
+  }
 
   @Bean
   ApplicationListener<ExecutionEvent> dependentPipelineExecutionListenerAdapter(DependentPipelineExecutionListener delegate, ExecutionRepository repository) {

orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/config/Front50ConfigurationProperties.java

@@ -19,6 +19,30 @@
 import lombok.Data;
 import org.springframework.boot.context.properties.ConfigurationProperties;
 
+/**
+ * Configuration properties for Front50 service.
+ *
+ * <p>These properties can be configured in your YAML configuration:
+ *
+ * <pre>
+ * front50:
+ *   baseUrl: http://front50.example.com
+ *   enabled: true
+ *   useTriggeredByEndpoint: true
+ *   okhttp:
+ *     connectTimeoutMs: 10000
+ *     readTimeoutMs: 60000
+ *     writeTimeoutMs: 60000
+ * </pre>
+ *
+ * <p>If not explicitly configured, a fallback chain will be used for timeouts:
+ *
+ * <ol>
+ *   <li>Use explicit okhttp configuration if present
+ *   <li>Fall back to global okhttp client configuration
+ *   <li>Use default fallback values (10s connect, 60s read/write)
+ * </ol>
+ */
 @Data
 @ConfigurationProperties("front50")
 public class Front50ConfigurationProperties {
@@ -34,14 +58,50 @@ public class Front50ConfigurationProperties {
    */
   boolean useTriggeredByEndpoint = true;
 
+  /** HTTP client configuration for connecting to Front50 service */
   OkHttpConfigurationProperties okhttp = new OkHttpConfigurationProperties();
 
+  /**
+   * Configuration properties for the OkHttp client connecting to Front50. These will only be used
+   * if explicitly set in the configuration. Otherwise, global client timeouts will be used as
+   * fallback.
+   */
   @Data
   public static class OkHttpConfigurationProperties {
-    int readTimeoutMs = 10000;
+    /** Read timeout in milliseconds. Default is 60 seconds (60000ms) */
+    private Integer readTimeoutMs = 60000;
+
+    /** Write timeout in milliseconds. Default is 60 seconds (60000ms) */
+    private Integer writeTimeoutMs = 60000;
+
+    /** Connection timeout in milliseconds. Default is 10 seconds (10000ms) */
+    private Integer connectTimeoutMs = 10000;
+
+    /**
+     * Checks if read timeout is explicitly configured with a non-default value.
+     *
+     * @return true if read timeout is configured with a non-default value
+     */
+    public boolean hasReadTimeoutConfig() {
+      return readTimeoutMs != null && readTimeoutMs != 60000;
+    }
 
-    int writeTimeoutMs = 10000;
+    /**
+     * Checks if write timeout is explicitly configured with a non-default value.
+     *
+     * @return true if write timeout is configured with a non-default value
+     */
+    public boolean hasWriteTimeoutConfig() {
+      return writeTimeoutMs != null && writeTimeoutMs != 60000;
+    }
 
-    int connectTimeoutMs = 10000;
+    /**
+     * Checks if connect timeout is explicitly configured with a non-default value.
+     *
+     * @return true if connect timeout is configured with a non-default value
+     */
+    public boolean hasConnectTimeoutConfig() {
+      return connectTimeoutMs != null && connectTimeoutMs != 10000;
+    }
   }
 }

orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/config/Front50ConfigurationSpec.groovy

@@ -0,0 +1,143 @@
+/*
+ * Copyright 2024 Armory, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.orca.front50.config
+
+import com.fasterxml.jackson.databind.ObjectMapper
+import com.netflix.spinnaker.config.DefaultServiceEndpoint
+import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider
+import okhttp3.OkHttpClient
+import org.slf4j.Logger
+import retrofit.Endpoint
+import retrofit.RequestInterceptor
+import retrofit.RestAdapter
+import spock.lang.Specification
+import spock.lang.Subject
+import spock.lang.Unroll
+
+import java.lang.reflect.Method
+import java.util.concurrent.TimeUnit
+
+/**
+ * Tests the timeout fallback chain logic in the Front50Configuration class.
+ */
+class Front50ConfigurationSpec extends Specification {
+
+  OkHttpClientProvider clientProvider = Mock()
+  RequestInterceptor requestInterceptor = Mock()
+  Endpoint endpoint = Mock()
+
+  @Subject
+  Front50Configuration front50Configuration
+
+  // Access private getEffectiveTimeout method via reflection
+  private Method getEffectiveTimeoutMethod
+
+  // Default timeout values from Front50Configuration
+  private static final long DEFAULT_READ_TIMEOUT_MS = 60000    // 60 seconds
+  private static final long DEFAULT_WRITE_TIMEOUT_MS = 60000   // 60 seconds
+  private static final long DEFAULT_CONNECT_TIMEOUT_MS = 10000 // 10 seconds
+
+  def setup() {
+    front50Configuration = new Front50Configuration()
+    front50Configuration.clientProvider = clientProvider
+    front50Configuration.spinnakerRequestInterceptor = requestInterceptor
+
+    // Use reflection to access the private getEffectiveTimeout method
+    getEffectiveTimeoutMethod = Front50Configuration.class.getDeclaredMethod(
+        "getEffectiveTimeout", 
+        Integer.class, 
+        long.class, 
+        long.class, 
+        String.class)
+    getEffectiveTimeoutMethod.setAccessible(true)
+  }
+
+  @Unroll
+  def "getEffectiveTimeout should use #description when explicitTimeout=#explicitTimeout, globalTimeout=#globalTimeout, timeoutType=#timeoutType"() {
+    given:
+    when:
+    long result = getEffectiveTimeoutMethod.invoke(
+        front50Configuration, 
+        explicitTimeout, 
+        globalTimeout, 
+        defaultTimeout,
+        timeoutType)
+
+    then:
+    result == expectedTimeout
+
+    where:
+    description               | explicitTimeout | globalTimeout | defaultTimeout | timeoutType || expectedTimeout
+    "custom explicit timeout" | 30000           | 20000         | 60000          | "read"      || 30000
+    "global timeout - read"   | 60000           | 20000         | 60000          | "read"      || 20000  // Default read timeout is ignored
+    "global timeout - write"  | 60000           | 20000         | 60000          | "write"     || 20000  // Default write timeout is ignored
+    "global timeout - connect"| 10000           | 20000         | 60000          | "connect"   || 20000  // Default connect timeout is ignored
+    "default timeout"         | null            | 0             | 60000          | "read"      || 60000
+    "zero explicit timeout"   | 0               | 20000         | 60000          | "read"      || 0       // Zero is treated as explicitly configured
+  }
+
+  @Unroll
+  def "configureTimeouts should handle different combinations of configurations"() {
+    given:
+    Front50ConfigurationProperties props = new Front50ConfigurationProperties()
+    if (explicitTimeoutsSet) {
+      Front50ConfigurationProperties.OkHttpConfigurationProperties okHttpProps = 
+          new Front50ConfigurationProperties.OkHttpConfigurationProperties()
+      okHttpProps.setReadTimeoutMs(readTimeoutMs)
+      okHttpProps.setWriteTimeoutMs(writeTimeoutMs)
+      okHttpProps.setConnectTimeoutMs(connectTimeoutMs)
+      props.setOkhttp(okHttpProps)
+    }
+
+    OkHttpClient baseClient = new OkHttpClient.Builder()
+        .readTimeout(globalReadTimeoutMs, TimeUnit.MILLISECONDS)
+        .writeTimeout(globalWriteTimeoutMs, TimeUnit.MILLISECONDS)
+        .connectTimeout(globalConnectTimeoutMs, TimeUnit.MILLISECONDS)
+        .build()
+
+    OkHttpClient configuredClient = new OkHttpClient.Builder().build()
+
+    clientProvider.getClient(_ as DefaultServiceEndpoint) >> baseClient
+    endpoint.getUrl() >> "http://front50.example.com"
+
+    // Use reflection to access the private configureTimeouts method
+    Method configureTimeoutsMethod = Front50Configuration.class.getDeclaredMethod(
+        "configureTimeouts", 
+        OkHttpClient.class, 
+        Front50ConfigurationProperties.class)
+    configureTimeoutsMethod.setAccessible(true)
+
+    when:
+    configuredClient = configureTimeoutsMethod.invoke(
+        front50Configuration, 
+        baseClient, 
+        props) as OkHttpClient
+
+    then:
+    configuredClient.readTimeoutMillis() == expectedReadTimeoutMs
+    configuredClient.writeTimeoutMillis() == expectedWriteTimeoutMs
+    configuredClient.connectTimeoutMillis() == expectedConnectTimeoutMs
+
+    where:
+    explicitTimeoutsSet | readTimeoutMs | writeTimeoutMs | connectTimeoutMs | globalReadTimeoutMs | globalWriteTimeoutMs | globalConnectTimeoutMs || expectedReadTimeoutMs | expectedWriteTimeoutMs | expectedConnectTimeoutMs
+    true                | 30000         | 35000          | 5000             | 20000               | 25000                | 15000                  || 30000                 | 35000                 | 5000
+    true                | null          | 35000          | null             | 20000               | 25000                | 15000                  || 20000                 | 35000                 | 15000
+    true                | null          | null           | null             | 20000               | 25000                | 15000                  || 20000                 | 25000                 | 15000
+    false               | null          | null           | null             | 20000               | 25000                | 15000                  || 20000                 | 25000                 | 15000
+    false               | null          | null           | null             | 0                   | 0                    | 0                      || DEFAULT_READ_TIMEOUT_MS | DEFAULT_WRITE_TIMEOUT_MS | DEFAULT_CONNECT_TIMEOUT_MS
+  }
+}