feat: expose HTTP timeout on AIServiceBackend #384
Conversation
Signed-off-by: Loong <[email protected]>
|
I will add tests if basic logic is OK. @yuzisun please take a look. |
Signed-off-by: Loong <[email protected]>
daixiang0
changed the title
Signed-off-by: Loong <[email protected]>
Signed-off-by: Loong <[email protected]>
| @@ -675,3 +680,14 @@ func backendSecurityPolicyVolumeName(ruleIndex, backendRefIndex int, name string | |||
| func backendSecurityMountPath(backendSecurityPolicyKey string) string { | |||
| return fmt.Sprintf("%s/%s", mountedExtProcSecretPath, backendSecurityPolicyKey) | |||
| } | |||
|
|
|||
| func defaultTimeout() *gwapiv1.HTTPRouteTimeouts { | |||
There was a problem hiding this comment.
Do not find a good timing to set default value, put here for discussion.
There was a problem hiding this comment.
This is the default timeout for the default route, for the other routes it is still going to default to 15s which is the envoy default timeout if not specified.
There was a problem hiding this comment.
@mathetake Do we really need this default route?
There was a problem hiding this comment.
the / is unfortunately necessary as without it, any request will result in 404; envoy router executes before extproc once at the very first phase of request handling, and 404 will be returned before it reaches extproc. However, in reality it should never be used since if the extproc router doesn’t find any match, extproc will return 404 immediate response, not routing to default backend.
so to deal with it in the PR, i don’t think we need to do anything- just leave it as default without setting
my comment from slack; tldr is the default route is an implementation detail that shouldn't be used or have any impact on the behavior users expect. So i would suggest to just remove this defaultTimeout and leave it nil and make it default to the envoy default
There was a problem hiding this comment.
let's remove defaultTimeout and I think we are good to go
mathetake
changed the title
mathetake
changed the title
Commit Message
This adds timeouts field on AIServiceBackend to allow the configuration of timeouts per backend.
Related Issues/PRs (if applicable)
Fixes #361