diff --git a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/config/Front50Configuration.groovy b/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/config/Front50Configuration.groovy index 00ec8b3a56..cfeb30adaa 100644 --- a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/config/Front50Configuration.groovy +++ b/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)) diff --git a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/config/Front50ConfigurationProperties.java b/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/config/Front50ConfigurationProperties.java index 14de35840a..111d8858d4 100644 --- a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/config/Front50ConfigurationProperties.java +++ b/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. + * + *

These properties can be configured in your YAML configuration: + * + *

+ * front50:
+ *   baseUrl: http://front50.example.com
+ *   enabled: true
+ *   useTriggeredByEndpoint: true
+ *   okhttp:
+ *     connectTimeoutMs: 10000
+ *     readTimeoutMs: 60000
+ *     writeTimeoutMs: 60000
+ * 
+ * + *

If not explicitly configured, a fallback chain will be used for timeouts: + * + *

    + *
  1. Use explicit okhttp configuration if present + *
  2. Fall back to global okhttp client configuration + *
  3. Use default fallback values (10s connect, 60s read/write) + *
+ */ @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; + } } } diff --git a/orca-front50/src/main/java/com/netflix/spinnaker/orca/front50/config/Front50Configuration.java b/orca-front50/src/main/java/com/netflix/spinnaker/orca/front50/config/Front50Configuration.java new file mode 100644 index 0000000000..aebec10a32 --- /dev/null +++ b/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 dependentPipelineExecutionListenerAdapter( + DependentPipelineExecutionListener delegate, ExecutionRepository repository) { + return new ExecutionListenerAdapter(delegate, repository); + } +} diff --git a/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/config/Front50ConfigurationSpec.groovy b/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/config/Front50ConfigurationSpec.groovy new file mode 100644 index 0000000000..18f8dee3cc --- /dev/null +++ b/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)") + } +}