Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support grpc_read_timeout and grpc_send_timeout in annotations #11250

Closed
Anddd7 opened this issue Apr 11, 2024 · 6 comments · Fixed by #11258
Closed

Support grpc_read_timeout and grpc_send_timeout in annotations #11250

Anddd7 opened this issue Apr 11, 2024 · 6 comments · Fixed by #11258
Assignees
@longwuyuan
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@Anddd7
Copy link
Contributor

Anddd7 commented Apr 11, 2024

What do you want to happen?

We want to configure the grpc_read_timeout and grpc_send_timeout with annotations on ingress.

Since the server-snippet has some potential risk, we have to migrate all snippets to common annotations.
https://github.com/search?q=repo%3Akubernetes%2Fingress-nginx+path%3A%2F%5Einternal%5C%2Fingress%5C%2Fannotations%5C%2F%2F+parser.AnnotationRiskCritical&type=code

Is there currently another issue associated with this?

#2475, but it's inactive and closed.

Does it require a particular kubernetes version?

No

Solutions

1. Add new annotations, e.g.

...
annotations:
  nginx.ingress.kubernetes.io/grpc_read_timeout: 3600
  nginx.ingress.kubernetes.io/grpc_send_timeout: 3600
...

2. Set grpc_read_timeout and grpc_send_timeout with the same value in proxy_read_timeout

Similar like nginx-ingress-controller, just set grpc with proxy settings by default
@Anddd7 Anddd7 added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 11, 2024
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Apr 11, 2024
@Anddd7
Copy link
Contributor Author

Anddd7 commented Apr 11, 2024

i'd like to contribute if it's ready to implement

@longwuyuan
Copy link
Contributor

/triage accepted

  • I think a PR will be reviewed because it will offset using snippets
  • But need to improve he accuracy of reference because that link is a nginx webserver and reverseproxy link and this is a go+lua implementation of ingress API. But yeah it is obviously originating in nginx upstream
  • Do you want to submit a PR
  • There is even a video on how to create new annotation in the docs on the developer's help page

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 11, 2024
@longwuyuan
Copy link
Contributor

/assign

@Anddd7 Anddd7 mentioned this issue Apr 13, 2024
Merged
6 tasks
@Anddd7
Copy link
Contributor Author

Anddd7 commented Apr 13, 2024 •
edited

@longwuyuan yes, i have read the docs and read to create a PR.

I refer to exsiting proxy settings to create 3 annotations for grpc timeout, please take a look, #11258

I'll refine the documentation later.

the upstream nginx is already support this, https://nginx.org/en/docs/http/ngx_http_grpc_module.html#grpc_connect_timeout.

@strongjz
Copy link
Member

/priority backlog

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority labels Apr 25, 2024
@github-actions GitHub Actions
Copy link

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

@github-actions github-actions bot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@longwuyuan longwuyuan

Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
[sig-network] Ingress-Nginx
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: Add grpc timeouts annotations Anddd7/ingress-nginx
4 participants
@strongjz @longwuyuan @k8s-ci-robot @Anddd7