conditional retry condition in retry policy

[yxue]

Description:

envoy/api/envoy/config/route/v3/route_components.proto

Line 884 in abb1f96

message RetryPolicy {

defines retry policy for HTTP request. It doesn't support configure different retry policies for different HTTP methods. Users might want to retry on connection failure for all methods but 5xx for idempotent methods only. I think the retry policy should provide such function to allow configure different retry policies for different HTTP methods. More generally, the retry policy should be able to support conditional retry condition.

Relevant Links:
istio/istio#13851

[mattklein123]

At a certain point we probably need to call it quits on adding more and more specific capabilities to the HTTP policy and just make it pluggable. WDYT? cc @snowp

[snowp]

Making the retry policy configurable makes sense to me, we could just pull out the relevant functions from RetryState into a pluggable class. We'd have it be instantiated optionally once per downstream with the request headers (to allow it to parse the retry headers) and then it can just handle the "should this response/reset reason be retried" part that RetryStateImpl is doing today.

This would also make it easier to define composite rules, since we could have those implementation instantiate nested retry policies and delegate the decision to those.

[yxue]

I opened a draft PR for making retry policy pluggable. @snowp @mattklein123 could you please take a look if I am on the right track? I think we can have more discussion based on the current implementation.

1. In the PR, I added a pluggable class which contains a function

  bool shouldRetryHeader(const Http::HeaderMap& request_header, const Http::HeaderMap& response_headers)

2. Add the pluggable class to the RetryStateImpl which is similar to RetryPrioity and RetryHostPredicate

3. Add a header retry demo ResponseHeaderRetryHeader which is currently a noop implementation.