expose timeout and retries configurations to users in Route

[lichuqiang]

Expected Behavior

Users can config timeout and retry policy for HTTP requests in route resources.

Actual Behavior

The config items are not exposed to end users.
As both newly introduced ClusterIngress and underlying VirtualService resources support timeout and retry configuration, I think it reasonable to expose them to end users.

/cc @tcnghia for thoughts

/assign

[ZhiminXiang]

For timeout, ideally we want to have timeout per revision. However, because of the limitation of Istio/envoy, currently we are not able to exactly achieve this.

I have a pending (old) PR about using a workaround for setting revision timeout. I did not keep pushing it since I did not get a chance to investigate what is the right way to do an API change.

We could restart this work based on the current ClusterIngress infrastructure.

[tcnghia]

@jonjohnsonjr can advise about the current guideline on API versioning.

[mattmoor]

@k4leung4 is going to pick up the per-Revision timeoutSeconds work.

[tcnghia]

It seems that configuring Timeout based on destination is still not feasible in Envoy (https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/route/route_components.proto#route-routeaction) so it is very unlikely that we can get this working at the Ingress for any of our KIngress implementation (they're all Envoy-based).

[vagababov]

I thought we removed retries?
Can we retire this one?

[richterdavid]

I have a half-done PR for this in progress...

[vagababov]

@tcnghia
What's the status there :)

[markusthoemmes]

Is Envoy's timeout a time-to-first-byte timeout? Will it keep Websocket connections intact forever? Our current timeout handling allows for that.

[richterdavid]

The docs say "Timeout for HTTP requests.", and don't specify behavior further.
https://istio.io/docs/reference/config/networking/virtual-service/#HTTPRoute

I would guess that this ends up being a 504 error, which is what I'm seeing in some of the broken integration tests, which is documented here:
https://tools.ietf.org/html/rfc7231#section-6.6.5
I would guess that this is time-to-last-byte.

If you don't specify v1.Revision.Spec.TimeoutSeconds, this shouldn't change the behavior.

[richterdavid]

I'd appreciate suggestions where and how to make changes to the documentation, samples, and end-to-end/integration tests for this feature.

[vagababov]

For docs: https://github.com/knative/docs/tree/master/docs/serving. Networking doesn't seem to have its own directory now, but rather assortment of files (cc @tcnghia )
Samples are in the same place under samples. Though I don't think we have many feature specific samples. Might be a good start (cc @mattmoor )

For e2e, given it's a CM change there are two ways:

• patch the CM in the e2e test (we're trying to go away from this though)
• patch the CM in the shell script and run the test as a separate suite (https://github.com/knative/serving/blob/master/test/e2e-tests.sh#L86, like this) -- but you'll need to explicitly unpatch once test is over.

Finally, if we're going to permit this feature on a revision basis, then it's much easier, wherein you just create the revision with desired properties and then run standard test (note that if we're doing per-revision settings, then passing explicit timeout is more useful than passing around CM as currently done).

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.