Fix(orca-front50): front50 timeout fallback

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

@@ -28,8 +28,11 @@ import com.netflix.spinnaker.orca.front50.spring.DependentPipelineExecutionListe
 import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository
 import com.netflix.spinnaker.orca.retrofit.RetrofitConfiguration
 import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog
+import com.netflix.spinnaker.okhttp.OkHttpClientConfigurationProperties
 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 +62,8 @@ import static retrofit.Endpoints.newFixedEndpoint
 @ConditionalOnExpression('${front50.enabled:true}')
 class Front50Configuration {
 
+  private static final Logger log = LoggerFactory.getLogger(Front50Configuration.class)
+
   @Autowired
   OkHttpClientProvider clientProvider
 
@@ -74,17 +79,41 @@ 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();
+  Front50Service front50Service(
+      Endpoint front50Endpoint,
+      ObjectMapper mapper,
+      Front50ConfigurationProperties front50ConfigurationProperties,
+      OkHttpClientConfigurationProperties okHttpClientConfigurationProperties) {
+
+    // Get base client with global configuration
+    OkHttpClient baseClient = clientProvider.getClient(new DefaultServiceEndpoint("front50", front50Endpoint.getUrl()))
+    OkHttpClient.Builder builder = baseClient.newBuilder()
+
+    // Apply global timeouts first
+    builder.connectTimeout(okHttpClientConfigurationProperties.getConnectTimeoutMs(), TimeUnit.MILLISECONDS)
+    builder.readTimeout(okHttpClientConfigurationProperties.getReadTimeoutMs(), TimeUnit.MILLISECONDS)
+
+    // Override with Front50-specific timeouts if explicitly defined
+    if (front50ConfigurationProperties.okhttp?.connectTimeoutMs != null) {
+      log.debug("Using front50-specific connect timeout: {}ms", front50ConfigurationProperties.okhttp.connectTimeoutMs)
+      builder.connectTimeout(front50ConfigurationProperties.okhttp.connectTimeoutMs, TimeUnit.MILLISECONDS)
+    }
+
+    if (front50ConfigurationProperties.okhttp?.readTimeoutMs != null) {
+      log.debug("Using front50-specific read timeout: {}ms", front50ConfigurationProperties.okhttp.readTimeoutMs)
+      builder.readTimeout(front50ConfigurationProperties.okhttp.readTimeoutMs, TimeUnit.MILLISECONDS)
+    }
+
+    if (front50ConfigurationProperties.okhttp?.writeTimeoutMs != null) {
+      log.debug("Using front50-specific write timeout: {}ms", front50ConfigurationProperties.okhttp.writeTimeoutMs)
+      builder.writeTimeout(front50ConfigurationProperties.okhttp.writeTimeoutMs, TimeUnit.MILLISECONDS)
+    }
+
+    // Create and return the service
     new RestAdapter.Builder()
       .setRequestInterceptor(spinnakerRequestInterceptor)
       .setEndpoint(front50Endpoint)
-      .setClient(new Ok3Client(okHttpClient))
+      .setClient(new Ok3Client(builder.build()))
       .setLogLevel(retrofitLogLevel)
       .setLog(new RetrofitSlf4jLog(Front50Service))
       .setConverter(new JacksonConverter(mapper))

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,33 @@ 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 120 seconds (120000ms) */
+    private Long readTimeoutMs = 120000L;
+
+    /** Write timeout in milliseconds. Default is 60 seconds (60000ms) */
+    private Long writeTimeoutMs = 60000L;
 
-    int writeTimeoutMs = 10000;
+    /** Connection timeout in milliseconds. Default is 5 seconds (5000ms) */
+    private Long connectTimeoutMs = 5000L;
 
-    int connectTimeoutMs = 10000;
+    /**
+     * Checks if this instance has any custom timeout configuration.
+     *
+     * @return true if any timeout is non-default, false otherwise
+     */
+    public boolean hasCustomTimeouts() {
+      // Compare with default values to determine if explicit config was provided
+      return readTimeoutMs != 120000L || writeTimeoutMs != 60000L || connectTimeoutMs != 5000L;
+    }
   }
 }

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

@@ -0,0 +1,139 @@
+/*
+ * Copyright 2014 Netflix, 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 static retrofit.Endpoints.newFixedEndpoint;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.jakewharton.retrofit.Ok3Client;
+import com.netflix.spinnaker.config.DefaultServiceEndpoint;
+import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler;
+import com.netflix.spinnaker.okhttp.OkHttpClientConfigurationProperties;
+import com.netflix.spinnaker.orca.events.ExecutionEvent;
+import com.netflix.spinnaker.orca.events.ExecutionListenerAdapter;
+import com.netflix.spinnaker.orca.front50.Front50Service;
+import com.netflix.spinnaker.orca.front50.spring.DependentPipelineExecutionListener;
+import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository;
+import com.netflix.spinnaker.orca.retrofit.RetrofitConfiguration;
+import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog;
+import java.util.concurrent.TimeUnit;
+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;
+import org.springframework.context.ApplicationListener;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.ComponentScan;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Import;
+import retrofit.Endpoint;
+import retrofit.RequestInterceptor;
+import retrofit.RestAdapter;
+import retrofit.converter.JacksonConverter;
+
+@Configuration
+@Import(RetrofitConfiguration.class)
+@ComponentScan({
+  "com.netflix.spinnaker.orca.front50.pipeline",
+  "com.netflix.spinnaker.orca.front50.tasks",
+  "com.netflix.spinnaker.orca.front50"
+})
+@EnableConfigurationProperties(Front50ConfigurationProperties.class)
+@ConditionalOnExpression("${front50.enabled:true}")
+public class Front50Configuration {
+
+  private static final Logger log = LoggerFactory.getLogger(Front50Configuration.class);
+
+  @Autowired private OkHttpClientProvider clientProvider;
+
+  @Autowired private RestAdapter.LogLevel retrofitLogLevel;
+
+  @Autowired private RequestInterceptor spinnakerRequestInterceptor;
+
+  @Bean
+  public Endpoint front50Endpoint(Front50ConfigurationProperties front50ConfigurationProperties) {
+    return newFixedEndpoint(front50ConfigurationProperties.getBaseUrl());
+  }
+
+  @Bean
+  public Front50Service front50Service(
+      Endpoint front50Endpoint,
+      ObjectMapper mapper,
+      Front50ConfigurationProperties front50ConfigurationProperties,
+      OkHttpClientConfigurationProperties okHttpClientConfigurationProperties) {
+
+    // Get base client with global configuration
+    OkHttpClient baseClient =
+        clientProvider.getClient(new DefaultServiceEndpoint("front50", front50Endpoint.getUrl()));
+    OkHttpClient.Builder builder = baseClient.newBuilder();
+
+    // Apply global timeouts first
+    builder.connectTimeout(
+        okHttpClientConfigurationProperties.getConnectTimeoutMs(), TimeUnit.MILLISECONDS);
+    builder.readTimeout(
+        okHttpClientConfigurationProperties.getReadTimeoutMs(), TimeUnit.MILLISECONDS);
+
+    // Override with Front50-specific timeouts if explicitly defined
+    if (front50ConfigurationProperties.getOkhttp() != null
+        && front50ConfigurationProperties.getOkhttp().getConnectTimeoutMs() != null) {
+      log.debug(
+          "Using front50-specific connect timeout: {}ms",
+          front50ConfigurationProperties.getOkhttp().getConnectTimeoutMs());
+      builder.connectTimeout(
+          front50ConfigurationProperties.getOkhttp().getConnectTimeoutMs(), TimeUnit.MILLISECONDS);
+    }
+
+    if (front50ConfigurationProperties.getOkhttp() != null
+        && front50ConfigurationProperties.getOkhttp().getReadTimeoutMs() != null) {
+      log.debug(
+          "Using front50-specific read timeout: {}ms",
+          front50ConfigurationProperties.getOkhttp().getReadTimeoutMs());
+      builder.readTimeout(
+          front50ConfigurationProperties.getOkhttp().getReadTimeoutMs(), TimeUnit.MILLISECONDS);
+    }
+
+    if (front50ConfigurationProperties.getOkhttp() != null
+        && front50ConfigurationProperties.getOkhttp().getWriteTimeoutMs() != null) {
+      log.debug(
+          "Using front50-specific write timeout: {}ms",
+          front50ConfigurationProperties.getOkhttp().getWriteTimeoutMs());
+      builder.writeTimeout(
+          front50ConfigurationProperties.getOkhttp().getWriteTimeoutMs(), TimeUnit.MILLISECONDS);
+    }
+
+    // Create and return the service
+    return new RestAdapter.Builder()
+        .setRequestInterceptor(spinnakerRequestInterceptor)
+        .setEndpoint(front50Endpoint)
+        .setClient(new Ok3Client(builder.build()))
+        .setLogLevel(retrofitLogLevel)
+        .setLog(new RetrofitSlf4jLog(Front50Service.class))
+        .setConverter(new JacksonConverter(mapper))
+        .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
+        .build()
+        .create(Front50Service.class);
+  }
+
+  @Bean
+  public ApplicationListener<ExecutionEvent> dependentPipelineExecutionListenerAdapter(
+      DependentPipelineExecutionListener delegate, ExecutionRepository repository) {
+    return new ExecutionListenerAdapter(delegate, repository);
+  }
+}

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

@@ -0,0 +1,111 @@
+/*
+ * 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.okhttp.OkHttpClientConfigurationProperties
+import retrofit.Endpoint
+import spock.lang.Specification
+import spock.lang.Subject
+import spock.lang.Unroll
+
+import java.util.concurrent.TimeUnit
+
+/**
+ * Tests for Front50 timeout configuration
+ */
+class Front50ConfigurationSpec extends Specification {
+
+  @Subject
+  Front50Configuration front50Configuration = new Front50Configuration()
+
+  /**
+   * Verifies that the default timeout values in Front50ConfigurationProperties match
+   * those in OkHttpClientConfigurationProperties to ensure backward compatibility
+   */
+  def "default timeout values should match OkHttpClientConfigurationProperties"() {
+    given:
+    OkHttpClientConfigurationProperties globalProps = new OkHttpClientConfigurationProperties()
+    Front50ConfigurationProperties.OkHttpConfigurationProperties front50Props = 
+      new Front50ConfigurationProperties.OkHttpConfigurationProperties()
+
+    expect:
+    front50Props.connectTimeoutMs == globalProps.connectTimeoutMs
+    front50Props.readTimeoutMs == globalProps.readTimeoutMs
+  }
+
+  /**
+   * Verifies that the front50Service method accepts OkHttpClientConfigurationProperties
+   * as a parameter, which is necessary for the timeout fallback mechanism to work
+   */
+  def "front50Service method should accept OkHttpClientConfigurationProperties parameter"() {
+    given:
+    def method = Front50Configuration.class.getDeclaredMethod(
+        "front50Service", 
+        Endpoint.class, 
+        ObjectMapper.class, 
+        Front50ConfigurationProperties.class,
+        OkHttpClientConfigurationProperties.class)
+
+    expect:
+    method != null
+  }
+
+  /**
+   * Verifies that hasCustomTimeouts correctly identifies when custom timeouts exist
+   */
+  @Unroll
+  def "hasCustomTimeouts should return #expected when #description"() {
+    given:
+    def props = new Front50ConfigurationProperties.OkHttpConfigurationProperties()
+    props.connectTimeoutMs = connectTimeout
+    props.readTimeoutMs = readTimeout
+
+    expect:
+    props.hasCustomTimeouts() == expected
+
+    where:
+    connectTimeout | readTimeout | expected | description
+    5000L          | 120000L     | false    | "using default values"
+    10000L         | 120000L     | true     | "only connect timeout changed"
+    5000L          | 30000L      | true     | "only read timeout changed"
+    10000L         | 30000L      | true     | "both timeouts changed"
+  }
+
+  /**
+   * This test verifies that the Front50Configuration implementation follows the correct pattern
+   * for timeout fallback by examining the source code.
+   */
+  def "front50Service uses the correct timeout fallback pattern"() {
+    given:
+    def sourceFile = new File("/Users/shlomodaari/armory/spinnaker-oss-services/orca/orca-front50/src/main/java/com/netflix/spinnaker/orca/front50/config/Front50Configuration.java")
+    def sourceCode = sourceFile.exists() ? sourceFile.text : null
+
+    expect:
+    sourceCode != null
+
+    // First apply global timeouts
+    sourceCode.contains("builder.connectTimeout(okHttpClientConfigurationProperties.getConnectTimeoutMs(), TimeUnit.MILLISECONDS)")
+    sourceCode.contains("builder.readTimeout(okHttpClientConfigurationProperties.getReadTimeoutMs(), TimeUnit.MILLISECONDS)")
+
+    // Then conditionally override with Front50-specific timeouts if defined
+    sourceCode.contains("front50ConfigurationProperties.getOkhttp() != null && front50ConfigurationProperties.getOkhttp().getConnectTimeoutMs() != null")
+    sourceCode.contains("builder.connectTimeout(front50ConfigurationProperties.getOkhttp().getConnectTimeoutMs(), TimeUnit.MILLISECONDS)")
+    sourceCode.contains("front50ConfigurationProperties.getOkhttp() != null && front50ConfigurationProperties.getOkhttp().getReadTimeoutMs() != null")
+    sourceCode.contains("builder.readTimeout(front50ConfigurationProperties.getOkhttp().getReadTimeoutMs(), TimeUnit.MILLISECONDS)")
+  }
+}