Support client_body_timeout annotation

[mneverov]

I'd like nginx.ingress.kubernetes.io/client-body-timeout annotation to be propagated to nginx server config.
The feature for grpc timeout annotations was implemented in #11250.
As it stated in the documentation the client_body_timeout must be provided if an app does request or both request and response streaming.
Currently, the only way to configure the client_body_timeout for nginx is by using server snippet.

No.

No.

[k8s-ci-robot]

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[longwuyuan]

@mneverov thanks for posting this. Your expectation seems absolutely valid.

There is shortage of resources. If you have interest to implement the annotation, please discuss in the ingress-nginx-dev channel of the Kubernetes slack. Also you can join community meetings.

But I also request you to study why client-body-timeout is a configMap option currently https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/configmap/#client-body-timeout . It will take me some reading and testing to get to know this directive as I am not a developer. What is needed to be confirmed is why you can not use it from the configMap

https://nginx.org/en/docs/http/ngx_http_core_module.html#client_body_timeout

[mneverov]

/assign

[longwuyuan]

Also, just FYI, the snippets feature is targeted for deprecation so there is no future for snippets.

[mneverov]

Also, just FYI, the snippets feature is targeted for deprecation so there is no future for snippets.

yes, thanks. That was the reason for raising this issue

[longwuyuan]

Ah ok. thanks.
So in that case I am curious if you already know why you can not use the configMap to set client-body-timeout.

[mneverov]

Ah ok. thanks. So in that case I am curious if you already know why you can not use the configMap to set client-body-timeout.

Will check, thanks

[mneverov]

@longwuyuan thanks for your suggestion. When client-body-timeout is configured in the configmap it is propagated to nginx http (global context). I was not aware of this and it covers my use case.
OTOH, when it is specified in the server-snippet the timeout is rendered inside a server context, i.e. it is local to the created Ingress.
Please let me know if the implementation still make sense as there is a workaround (global timeout).

[longwuyuan]

From user experience point of view, because of the other timeouts as annotations, it will be nice to have client-body-timeout also as annotation. But that is just user-experience.

The nginx docs is clear that the directive is accepted in all contexts, the perspective of a webserver vs a reverseproxy vs a ingress-controller comes into play. I will need to test the impact of using configMap vs server-snippet vs configuration-snippet. On top of that if there was a annotation existing, it would likely be implemented to be used in the location context.

Now that above part is theory. The practical side is there are no other demands on this directive, in the way that you are requiring, most likely because there are no open issues for timeout on both (request as well as response) streaming.

So because there is nothing broken and because there are no resources available to work on this issue, and configMap works for you, my suggestion is best not to pursue this any further now.

If a developer gets time, then they will comment and commit time so it can/will be taken up then.

Thanks you very much for the feedback as I was thinking that there is no workaround.

[longwuyuan]

/assign
/close

Closing because configMap key for client-body-timeout is a existing workaround.

[k8s-ci-robot]

@longwuyuan: Closing this issue.

In response to this:

/assign
/close

Closing because configMap key for client-body-timeout is a existing workaround.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[mneverov]

@longwuyuan thanks for the suggested workaround and prompt reply

[mneverov]

@longwuyuan I'd like to resurrect the issue. The reason is as we discussed -- the timeout specified in the ConfigMap is set as global in nginx for all ingresses. It appears we have quite a few of them and setting global timeout is not an option. The current workaround is switch back to server-snippet

[longwuyuan]

ok.
You can type /reopen here.
The developer guide section of docs has 2 sub-sections that may help if needed https://kubernetes.github.io/ingress-nginx/developer-guide/getting-started/

[mneverov]

/reopen

[k8s-ci-robot]

@mneverov: Reopened this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[mneverov]

@longwuyuan I found that there is already client-body-buffere-size annotation, i.e. annotations related to client_body_* and client_header_* configurations. I can create a separate package client and add the new annotation there, OTOH I see that annotations are bundled by functionality (auth, proxy, etc), so I would like to do a refactoring: rename package clientbodybuffersize to client and create client.Config struct that contains both fields (BodyBufferSize and BodyTimeout).
It will make this PR bigger (separated commits for the refactoring and actual implementation), but cleaner and aligned with the rest of the codebase. Other optioin is I can create separate PRs. WDYT?

[longwuyuan]

I am not a developer so please wait for other comments.

cc @rikatz @tao12345666333 @ElvinEfendi I think this issue and the suggested solution (PR) above, may need your comments, because its about a annotation that should have existed but is not.

@mneverov i prefer the single bigger PR (is it possible squash commits !) as that means one place to track. But as said, please wait for comments from others.