router: add API for retry policy extension

[yxue]

Signed-off-by: Yan Xue yxyan@google.com

1. add retry policy extension field.
2. add utility for converting any to factory config.

Description: add field for retry policy extension; add utility for converting any to factory config
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
#Issue #9946

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10369 was opened by yxue.

see: more, trace.

[yxue]

cc @kyessenov for utility change

[snowp]

Looks pretty good, just a couple comments. Thanks for working on this!

[snowp]

this description is pretty confusing for someone that doesn't know how the extensions usually work, maybe it should mention that this also defined what implementation is being instantiated?

[htuch]

I'm confused. RetryPolicy already has a typed config Any. Surely any implementation-specific extension to this should be nested in there?

[yxue]

RetryPolicy already has a typed config Any

I am not sure if this is true. I didn't find the field with typed Any

envoy/api/envoy/api/v2/route/route_components.proto

Lines 981 to 1075 in d1a36f1

	message RetryPolicy {
	message RetryPriority {
	string name = 1 [(validate.rules).string = {min_bytes: 1}];
	oneof config_type {
	google.protobuf.Struct config = 2 [deprecated = true];
	google.protobuf.Any typed_config = 3;
	}
	}
	message RetryHostPredicate {
	string name = 1 [(validate.rules).string = {min_bytes: 1}];
	oneof config_type {
	google.protobuf.Struct config = 2 [deprecated = true];
	google.protobuf.Any typed_config = 3;
	}
	}
	message RetryBackOff {
	// Specifies the base interval between retries. This parameter is required and must be greater
	// than zero. Values less than 1 ms are rounded up to 1 ms.
	// See :ref:`config_http_filters_router_x-envoy-max-retries` for a discussion of Envoy's
	// back-off algorithm.
	google.protobuf.Duration base_interval = 1 [(validate.rules).duration = {
	required: true
	gt {}
	}];
	// Specifies the maximum interval between retries. This parameter is optional, but must be
	// greater than or equal to the `base_interval` if set. The default is 10 times the
	// `base_interval`. See :ref:`config_http_filters_router_x-envoy-max-retries` for a discussion
	// of Envoy's back-off algorithm.
	google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}];
	}
	// Specifies the conditions under which retry takes place. These are the same
	// conditions documented for :ref:`config_http_filters_router_x-envoy-retry-on` and
	// :ref:`config_http_filters_router_x-envoy-retry-grpc-on`.
	string retry_on = 1;
	// Specifies the allowed number of retries. This parameter is optional and
	// defaults to 1. These are the same conditions documented for
	// :ref:`config_http_filters_router_x-envoy-max-retries`.
	google.protobuf.UInt32Value num_retries = 2;
	// Specifies a non-zero upstream timeout per retry attempt. This parameter is optional. The
	// same conditions documented for
	// :ref:`config_http_filters_router_x-envoy-upstream-rq-per-try-timeout-ms` apply.
	//
	// .. note::
	//
	// If left unspecified, Envoy will use the global
	// :ref:`route timeout <envoy_api_field_route.RouteAction.timeout>` for the request.
	// Consequently, when using a :ref:`5xx <config_http_filters_router_x-envoy-retry-on>` based
	// retry policy, a request that times out will not be retried as the total timeout budget
	// would have been exhausted.
	google.protobuf.Duration per_try_timeout = 3;
	// Specifies an implementation of a RetryPriority which is used to determine the
	// distribution of load across priorities used for retries. Refer to
	// :ref:`retry plugin configuration <arch_overview_http_retry_plugins>` for more details.
	RetryPriority retry_priority = 4;
	// Specifies a collection of RetryHostPredicates that will be consulted when selecting a host
	// for retries. If any of the predicates reject the host, host selection will be reattempted.
	// Refer to :ref:`retry plugin configuration <arch_overview_http_retry_plugins>` for more
	// details.
	repeated RetryHostPredicate retry_host_predicate = 5;
	// The maximum number of times host selection will be reattempted before giving up, at which
	// point the host that was last selected will be routed to. If unspecified, this will default to
	// retrying once.
	int64 host_selection_retry_max_attempts = 6;
	// HTTP status codes that should trigger a retry in addition to those specified by retry_on.
	repeated uint32 retriable_status_codes = 7;
	// Specifies parameters that control retry back off. This parameter is optional, in which case the
	// default base interval is 25 milliseconds or, if set, the current value of the
	// `upstream.base_retry_backoff_ms` runtime parameter. The default maximum interval is 10 times
	// the base interval. The documentation for :ref:`config_http_filters_router_x-envoy-max-retries`
	// describes Envoy's back-off algorithm.
	RetryBackOff retry_back_off = 8;
	// HTTP response headers that trigger a retry if present in the response. A retry will be
	// triggered if any of the header matches match the upstream response headers.
	// The field is only consulted if 'retriable-headers' retry policy is active.
	repeated HeaderMatcher retriable_headers = 9;
	// HTTP headers which must be present in the request for retries to be attempted.
	repeated HeaderMatcher retriable_request_headers = 10;
	}

[htuch]

Ditto. What does "See the supported retry policy implementations for further documentation." mean?

[yxue]

Removed. I mean users can refer to retry policy extensions configuration for details since here is just an Any typed field.

[htuch]

Putting myself in the shoes of new user, this API documentation is unclear, as it's a magic step to using the field. There are no docs or example of how to use the field. It's unclear if existing RetryPolicy can be embedded here. Also, presumably this is logically a oneof with retry_policy? If not, and it's some extension and to be used with RetryPolicy, again, pretty confusing, this is counter to the way the rest of the API operates.

As API shepherd I'd like to see a clean design here for making retry policies a first class extension point.

[yxue]

@htuch the typed_config should exclude from the current retry_policy but this comment points out that we should use a separate field. I am going to add the check in the code to make sure only one of them can be specified.

[htuch]

Thanks. The API no makes sense. I would also explain in the retry_policy docs that the converse holds. Also, we should add an example retry exension with typed config to the docs. It's OK to take that as an AI for our next PR.

[yxue]

I would also explain in the retry_policy docs that the converse holds.

I'd like to add the exclusion comment in the retry_policy field but this field is marked as hidden and I cannot refer the field in the doc. Maybe I should just put the field there without reference?

I planned to add the retry policy extension example in the following PRs.

[htuch]

OK, this is fine for now.

[htuch]

/lgtm api

[htuch]

@snowp can you merge if all looks good to you?

[snowp]

LGTM, thanks!

[euroelessar]

Hi folks, this pull request broke master:

envoy/api/v2/route/route_components.proto:170:51: Field number 19 has already been used in "envoy.api.v2.route.VirtualHost" by field "include_attempt_count_in_response".

[yxue]

@euroelessar thanks! There is a quick fix here #10426

[euroelessar]

@yxue great, thanks!