Fix(orca-front50): front50 timeout fallback #4851

shlomodaari · 2025-04-02T22:14:58Z

Issue

PR #4773 (Pipeline config batch update) introduced intermittent timeout failures in pipeline operations. After investigation, we discovered that the PR implemented its own timeout handling that bypassed the global OkHttp client configuration:

It used hardcoded and relatively short timeout values (10 seconds) for read/write/connect operations
These timeouts were insufficient for batch pipeline operations under load
Authentication failures occurred when timeout was too short for authorization checks to complete

Solution
This PR implements a robust timeout fallback chain that maintains backward compatibility while fixing the timeout issues:

First priority: Use explicit Front50-specific timeouts when configured in YAML
Second priority: Fall back to global OkHttp client timeouts (from Kork)
Last resort: Use conservative default values (60s read/write, 10s connect)

Implementation Details

Added clear fallback chain logic in Front50Configuration.groovy
Enhanced Front50ConfigurationProperties.java with better defaults and documentation
Added hasCustomTimeouts() helper method to detect explicit configuration
Added detailed DEBUG logging for which timeout source is being used
Added comprehensive unit tests for the fallback chain logic

Testing

Verified all unit tests pass, including batch pipeline operations
Confirmed the fallback chain correctly selects timeouts based on priority
Ensured changes don't break existing functionality

This change addresses intermittent timeout issues during pipeline operations by: - Adding a robust timeout fallback chain for Front50 HTTP client - Establishing clear priority: explicit config → global → default - Setting safer default timeouts (60s read/write, 10s connect) - Adding detailed logging to track timeout sources - Adding tests to verify the timeout fallback logic

- Update default read/write timeouts to 60s in Front50ConfigurationProperties - Add hasCustomTimeouts() helper method to detect explicit configuration - Add comprehensive documentation in Front50ConfigurationProperties - Improve logging when custom timeouts are detected - Ensure consistent default values across all classes

- Convert primitive int fields to Integer objects to properly handle null values - Improve type consistency using Long constants and explicit conversions - Add logic to distinguish between explicitly configured values and default values in properties - Rewrite tests to directly test actual implementation via reflection - Add test cases for default value detection and edge cases

dbyron-sf · 2025-04-02T23:46:42Z

I'd like @kirangodishala to take a look at this before merging.

kirangodishala · 2025-04-03T03:34:08Z

@shlomodaari - the timeout values are configurable at present. 10s is the default value but using the following configuration, you can set them to any desired value. Have you tried this?

front50:
   okhttp:
      readTimeoutMs: <> 
      writeTimeoutMs: <>
      connectTimeoutMs: <>

shlomodaari · 2025-04-03T03:51:15Z

Hi @kirangodishala,
Yes, we're aware of the configuration options. The issue we discovered is more subtle:

After investigating intermittent auth failures with batch pipeline updates, we identified that the configuration wasn't being reliably applied in all code paths. This PR fixes that by:

Creating a clear fallback chain:
- First tries user-specified Front50 config
- Then falls back to global OkHttp settings
- Finally uses safe defaults (60s read/write, 10s connect)
Properly detecting explicit configuration vs default values to prevent scenarios where default values accidentally override global settings
Making behavior consistent across all Front50 client operations rather than having different behaviors depending on code path

The 10-second default was especially problematic for batch operations with multiple pipelines. This implementation maintains backward compatibility while ensuring explicitly configured timeouts are always honored.

I've added detailed Javadoc comments to better document this behavior, which should have been documented from the beginning. This PR ensures that existing configuration works as expected and behaves predictably.

shlomodaari · 2025-04-03T03:54:01Z

Yes, we're aware of the configuration options, but we weren't aware of how they were implemented until we had an incident.

The change in PR #4773 wasn't documented as a breaking change in the release notes. We only discovered these configuration options after experiencing the incident, and upon investigation, we found that the 10-second defaults were actually overriding our global OkHttp client timeouts (which were higher), causing intermittent auth failures with batch pipeline updates.

This implementation maintains your changes but makes them more suitable and efficient. It ensures backward compatibility while preventing timeouts from accidentally overriding longer global timeout values.

I've added detailed Javadoc comments to better document this behavior.

dbyron-sf · 2025-04-03T04:00:54Z

The change in PR #4773 wasn't documented as a breaking change in the release notes.

@shlomodaari Can you make a PR to the release notes to add something about this please?

shlomodaari · 2025-04-03T04:02:28Z

@dbyron-sf I definitely will create a PR tomorrow during my morning :)

kirangodishala · 2025-04-03T07:38:49Z

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

+      boolean isDefaultReadValue = explicitTimeout == 60000 && timeoutType == "read"
+      boolean isDefaultWriteValue = explicitTimeout == 60000 && timeoutType == "write"
+      boolean isDefaultConnectValue = explicitTimeout == 10000 && timeoutType == "connect"


Instead of hardcoded values here, using the constants defined (i.e. DEFAULT_READ_TIMEOUT_MS etc) would be more meaningful I guess.

I address this issue :)
Let me know if anything else should be addressed @kirangodishala

I still don't understand enough about the issue. I need to see that release note. What was the behavior before? What is the new behavior? Would we be better off reverting something? This PR is filled with so many hard-coded magic numbers and special logic that I'd really love to find a different way.

The PR #4773 introduced batch pipeline updates which is valuable functionality. Our changes preserve this functionality while ensuring reliable timeout handling.

What our implementation does:

Creates a clear, documented fallback chain for timeout values

Properly respects both explicit and global configuration

Maintains backward compatibility for all users

Adds proper logging to help debugging

The approach we've taken uses the existing configuration capabilities while making the behavior more consistent and predictable. The special logic is necessary to handle different configuration scenarios while ensuring optimal performance.

With our implementation, users have flexibility to:

Configure Front50-specific timeouts when needed

Rely on global configuration when appropriate

Benefit from sensible defaults otherwise

The tests we've added verify all these scenarios work correctly. We believe this implementation strikes the right balance between functionality, reliability, and backward compatibility.

I don't see any issue with not going forward with this PR and let's make sure next if any breaking change let's at least add some tests! Thank you!
Working on the release notes in the next hour.

@shlomodaari Not sure why spinnakerbot closed spinnaker/spinnaker.io#504. I re-opened it. Can you take a look at spinnaker/spinnaker.io#504 (comment)?

Yes, I will take a look at this. Do we have an ETA for reviewing this PR as well?

OK, thanks for updating spinnaker/spinnaker.io#504. I think I understand it now. One benefit of that PR is that it lets people configure different timeouts when orca communicates with front50 than the "generic" ok-http-client timeouts, but I agree it was a breaking change, and it'd be great to make it less breaking, or not breaking at all.

We could help one class of users (the ones that didn't specify ok-http-client timeouts / used the defaults there), but making the defaults in Front50ConfigurationProperties.OkHttpConfigurationProperties match the defaults in OkHttpClientConfigurationProperties.

If we did that, users that specified something for ok-http-client still see a change in behavior. But isn't it relatively straightforward to fix that? By adding an OkHttpClientConfigurationProperties argument to Front50Configuration.front50Service and doing something like f12cd1f, I think we'd get there.

Thanks for your feedback on spinnaker/spinnaker.io#504!

In PR #4851, we've implemented a solution that handles all three user scenarios with clear priority order:

If a user has default OkHttp settings (no custom configuration):

The default OkHttp client settings (60s connect, 300s read) are applied to Front50

This maintains the behavior for users who didn't customize anything

If a user has custom global OkHttp settings:

These custom global settings are applied to Front50

For example, if they configured ok-http-client.readTimeoutMs=90000, Front50 will use that value

This fixes the regression for users who relied on global settings

If a user has Front50-specific settings configured:

The Front50-specific settings override any global settings (default or custom)

For example, if they set front50.okHttp.readTimeoutMs=120000, that takes precedence

This preserves the new functionality

The priority order is clear:

Front50-specific settings (highest priority)

Custom global OkHttp settings (middle priority)

Default OkHttp settings (lowest priority)

This approach ensures backward compatibility while preserving the ability to have service-specific configuration when needed.

This PR is too complicated/brittle. I'd like to see a PR that has no additional "magic numbers" in it. It still seems to be that we could pull this off with two commits:

change the defaults in Front50ConfigurationProperties.OkHttpConfigurationProperties to match the defaults in OkHttpClientConfigurationProperties

add an OkHttpClientConfigurationProperties argument to Front50Configuration.front50Service and do something like f12cd1f. As in, if the front50-specific properties are configured, use them. Otherwise, fall back to ok-http-client.

spinnakerbot · 2025-04-03T16:45:16Z

The following commits need their title changed:

943fd62: enhance: Improve Front50 timeout configuration consistency

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

…andling - Use constants instead of hardcoded values for default timeout comparisons - Use Long type consistently instead of Integer for better compatibility with Kork - Update tests to match the type changes

…efaults with OkHttpClientConfigurationProperties

dbyron-sf · 2025-04-23T19:33:34Z

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

+/**
+ * Tests for Front50 timeout configuration
+ */
+class Front50ConfigurationSpec extends Specification {


Would you mind converting this to java please. The less groovy code we have in the world, the better.

Yes, let me know if it looks better now :)

Still looks like groovy to me...

dbyron-sf · 2025-04-23T21:45:10Z

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

 import lombok.Data;
 import org.springframework.boot.context.properties.ConfigurationProperties;

+/**


I can't decide whether I think this comment is helpful enough vs. the chances of it getting stale. Since these are the higher-precedence properties, I'm pretty tempted to ditch the comment.

dbyron-sf · 2025-04-23T21:46:04Z

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

  public static class OkHttpConfigurationProperties {
-    int readTimeoutMs = 10000;
+    /** Read timeout in milliseconds. Default is 120 seconds (120000ms) */
+    private Long readTimeoutMs = 120000L;


Suggested change

private Long readTimeoutMs = 120000L;

private long readTimeoutMs = 120000L;

dbyron-sf · 2025-04-23T21:46:15Z

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


-    int writeTimeoutMs = 10000;
+    /** Connection timeout in milliseconds. Default is 5 seconds (5000ms) */
+    private Long connectTimeoutMs = 5000L;


Suggested change

private Long connectTimeoutMs = 5000L;

private long connectTimeoutMs = 5000L;

dbyron-sf · 2025-04-23T21:46:41Z

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

+     *
+     * @return true if any timeout is non-default, false otherwise
+     */
+    public boolean hasCustomTimeouts() {


Don't think we need this method.

dbyron-sf · 2025-04-23T21:51:53Z

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

+    builder.readTimeout(okHttpClientConfigurationProperties.getReadTimeoutMs(), TimeUnit.MILLISECONDS)
+
+    // Override with Front50-specific timeouts if explicitly defined
+    if (front50ConfigurationProperties.okhttp?.connectTimeoutMs != null) {


Even with capital-L long, I don't think this way of checking works, since the properties have default values. I think we gotta do the environment.containsProperty thing. There's a failing test, but I'm not sure it's failing because of this...so perhaps we need some more test coverage like WebhookConfigurationTest.

For me the code is easier to follow using local variables and then calling the builder once, like:

long readTimeoutMs = (environment.containsProperty("webhook.readTimeoutMs") || environment.containsProperty("webhook.read-timeout-ms")) ? webhookProperties.getReadTimeoutMs() : okHttpClientConfigurationProperties.getReadTimeoutMs(); requestFactory.setReadTimeout(Math.toIntExact(readTimeoutMs));

shlomodaari added 2 commits April 2, 2025 14:57

shlomodaari mentioned this pull request Apr 2, 2025

fix(okhttpconfig): increase the defaul timeout due to low number #4849

Open

kirangodishala reviewed Apr 3, 2025

View reviewed changes

fix(orca-front50): Address code review feedback for Front50 timeout h…

3ea06c5

…andling - Use constants instead of hardcoded values for default timeout comparisons - Use Long type consistently instead of Integer for better compatibility with Kork - Update tests to match the type changes

shlomodaari changed the title ~~Fix/front50 timeout fallback~~ Fix(orca-front50): front50 timeout fallback Apr 3, 2025

shlomodaari added 2 commits April 23, 2025 12:01

refactor(front50): Simplify Front50 timeout configuration and align d…

28cad45

…efaults with OkHttpClientConfigurationProperties

refactor(front50): Simplify Front50 timeout configuration and align d…

25c9a38

…efaults with OkHttpClientConfigurationProperties

dbyron-sf reviewed Apr 23, 2025

View reviewed changes

shlomodaari added 2 commits April 23, 2025 12:44

refactor(front50): Convert Front50Configuration from Groovy to Java

05e309c

chore: Remove accidentally committed local config file

109c125

dbyron-sf reviewed Apr 23, 2025

View reviewed changes

dbyron-sf mentioned this pull request May 5, 2025

adding breaking change information for orca spinnaker/spinnaker.io#504

Closed

	private Long readTimeoutMs = 120000L;
	private long readTimeoutMs = 120000L;

	private Long connectTimeoutMs = 5000L;
	private long connectTimeoutMs = 5000L;

Fix(orca-front50): front50 timeout fallback #4851

Are you sure you want to change the base?

Fix(orca-front50): front50 timeout fallback #4851

Uh oh!

Conversation

shlomodaari commented Apr 2, 2025

Uh oh!

dbyron-sf commented Apr 2, 2025

Uh oh!

kirangodishala commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shlomodaari commented Apr 3, 2025

Uh oh!

shlomodaari commented Apr 3, 2025

Uh oh!

dbyron-sf commented Apr 3, 2025

Uh oh!

shlomodaari commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spinnakerbot commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kirangodishala commented Apr 3, 2025 •

edited

Loading