feat: propogate local_request_timeout_ms to downstream

[huikang]

Description

Situation: "If an application is configured with a local_request_timeout_ms value, would it be a good idea to auto-propagate this timeout to consumers/downstreams of that service. That way we don't have to configure the timeout in 2 different places. Maybe downstreams would want to timeout sooner. In that case they could specify the timeout and this would just be a way to modify the default timeout value."

• The implementation of this PR is to loop over the upstream proxy services to find the smallest local_request_timeout_ms. If the value is smaller than the default, will propagate to the downstream proxy service.

  • call makeInboundListener after looping all the upstream

• doc is updated

• unit test is added: to make the upstream service to have its own local_request_timeout_ms, a new parameter is used to configure the proxy.Config of the upstream proxy service.

Testing & Reproduction steps

Links

Address #12553

PR Checklist

• updated test coverage
• external facing docs updated
• not a security concern

[mkeeler]

I think it would be good for someone else to take a second look here so I have tagged the consul service mesh team.

My hesitation is that this seems like it could introduce issues with requests to upstreams that have higher timeouts than others.

Take the following scenario:

flowchart TD
   %% Basic States
   otherServices(Other Services)
   subgraph foo
      fooService(foo)
      subgraph fooSidecar [foo sidecar proxy]
         direction TB
         fooInbound(Inbound Listener)
         fooBarUpstream(Bar Upstream)
         fooBazUpstream(Baz Upstream)
      end
   end
   
   subgraph bar
      direction LR
      barService(bar)
      subgraph barSidecar [bar sidecar proxy]
         barInbound(Inbound Listener)
      end
   end
   
   subgraph baz
      direction LR
      bazService(baz)
      subgraph bazSidecar [baz sidecar proxy]
         direction LR         
         bazInbound(Inbound Listener)
      end
   end
   %% classDef default fill:#f9decd,stroke:#e06e2d,stroke-width:4px,color:black;

 %% State Transitions
   otherServices-- Timeout: 5s  -->fooInbound;
   fooInbound-->fooService;
   fooService-->fooBarUpstream;
   fooService-->fooBazUpstream;
   fooBarUpstream-->barInbound;
   fooBazUpstream-->bazInbound;
   barInbound-- Timeout: 60s -->barService
   bazInbound-- Timeout: 5s -->bazService

Loading

Here there are 2 upstreams and the min request timeout for them to their local app is 5s. I think this means that for the flow where some service makes a request to foo which needs to make a request to bar, the timeout may be too short and kill the connections too early.

Ideally we would love the diagram to look more like the following:

flowchart TD
   %% Basic States
   otherServices(Other Services)
   subgraph foo
      fooService(foo)
      subgraph fooSidecar [foo sidecar proxy]
         direction TB
         fooInbound(Inbound Listener)
         fooBarUpstream(Bar Upstream)
         fooBazUpstream(Baz Upstream)
      end
   end
   
   subgraph bar
      direction LR
      barService(bar)
      subgraph barSidecar [bar sidecar proxy]
         barInbound(Inbound Listener)
      end
   end
   
   subgraph baz
      direction LR
      bazService(baz)
      subgraph bazSidecar [baz sidecar proxy]
         direction LR         
         bazInbound(Inbound Listener)
      end
   end

 %% State Transitions
   otherServices-- Timeout: none -->fooInbound;
   fooInbound-->fooService;
   fooService-- Timeout: 60s -->fooBarUpstream;
   fooService-- Timeout: 5s -->fooBazUpstream;
   fooBarUpstream-- Timeout: 60s -->barInbound;
   fooBazUpstream-- Timeout: 5s -->bazInbound;
   barInbound-- Timeout: 60s -->barService
   bazInbound-- Timeout: 5s -->bazService

Loading

Here the timeouts are configured per-upstream so requests from the foo app of the bar service will use the bar services local app timeout.

[github-actions]

This pull request has been automatically flagged for inactivity because it has not been acted upon in the last 60 days. It will be closed if no new activity occurs in the next 30 days. Please feel free to re-open to resurrect the change if you feel this has happened by mistake. Thank you for your contributions.