feat: Add grpc timeouts annotations #11258

Anddd7 · 2024-04-13T12:39:05Z

What this PR does / why we need it:

This PR adds support for grpc timeout settings via annotations. To avoid introducing security issues with server-snippets, we need a more direct way to configure the nginx grpc module.

Types of changes

New feature (non-breaking change which adds functionality)

Which issue/s this PR fixes

fixes #11250

How Has This Been Tested?

with unit tests and e2e tests

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added unit and/or e2e tests to cover my changes.
All new and existing tests passed.

k8s-ci-robot · 2024-04-13T12:39:13Z

Welcome @Anddd7!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-04-13T12:39:14Z

Hi @Anddd7. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

netlify · 2024-04-13T12:39:21Z

✅ Deploy Preview for kubernetes-ingress-nginx ready!

Name	Link
🔨 Latest commit	`ee204a7`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/66383e1d9a365e0008f87fd2
😎 Deploy Preview	https://deploy-preview-11258--kubernetes-ingress-nginx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Anddd7 · 2024-04-13T13:07:43Z

@Anddd7 TODO add docs for annoations and configmap

longwuyuan · 2024-04-13T15:48:35Z

@Anddd7 thanks for this contribution. It helps.

I am not a developer. So can you help me locate where you are creating the pod that runs the grpc server for test backend

Anddd7 · 2024-04-14T03:55:19Z

@longwuyuan to avoid bringing big changes, i set the grpc timeouts with existing proxy timeouts when backend protocol is 'grpc/grpcs'. so we can simply reuse proxy-send-timeout - just set backend-protocol=grpc. https://github.com/kubernetes/ingress-nginx/pull/11258/files#diff-cbf382cc05c9f274b5db56a581b335dba8ecb80fd96a8f1a6a068b2594c9b1caR1484

I add 2 more e2e test in test/e2e/annotations/grpc.go, to ensure grpc timeouts are rendered correctly. There are 3 existing tests that will create grpc server/client covered grpc connection test.

rikatz · 2024-04-15T21:44:07Z

docs/user-guide/nginx-configuration/annotations.md

@@ -700,6 +700,14 @@ In some scenarios is required to have different values. To allow this we provide
 - `nginx.ingress.kubernetes.io/proxy-next-upstream-tries`
 - `nginx.ingress.kubernetes.io/proxy-request-buffering`

+If you indicate [Backend Protocol](#backend-protocol) as `GRPC` or `GRPCS`, the following values will be applied to gRPC connection as well:


can you define the defaults or point to the docs of them please :) users may not know what {{ proxy_connect_timeout }} means. Thanks!!!

Or maybe just point to nginx docs to the defaults and say they can be configured somewhere else

Yeah, please point to the docs.nginx. thanks https://nginx.org/en/docs/http/ngx_http_grpc_module.html#grpc_send_timeout

sure, i added the default values (same as proxy_*_timout) and nginx docs

rikatz · 2024-04-15T21:46:22Z

/ok-to-test
/approve

So far looks good to me. I am wondering if we want different annotations for gRPC, but it does make sense to follow the same proxy-timeout from backend.

@longwuyuan I will leave the lgtm to you, after fixing the docs

rikatz · 2024-04-15T21:47:08Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -1481,6 +1481,13 @@ stream {
 proxy_next_upstream_timeout {{ $location.Proxy.NextUpstreamTimeout }};
 proxy_next_upstream_tries {{ $location.Proxy.NextUpstreamTries }};

+ {{ if or (eq $location.BackendProtocol "GRPC") (eq $location.BackendProtocol "GRPCS") }}
+ # Grpc settings


question: what happens if the user does not set the proxy configuration timeout? I can't remember if there are defaults for connecttimeout, sendtimeout, etc.

The proxy default timeouts (5s,60s,60s) is configured in defuault backend in

https://github.com/Anddd7/ingress-nginx/blob/5badb96d909242e02eb3f0d95372c2a298ec5325/internal/ingress/controller/config/config.go#L863-L867

rikatz · 2024-04-15T21:47:37Z

/approve cancel

@Anddd7 just one question on defaults, maybe we just want to set the values if they are defined on annotations, otherwise leave as is.

This would avoid breaking changes (I'm not sure if the default proxy_timeout and others defaults are the same for gRPC, if you can just double check it I appreciate it)

Anddd7 · 2024-04-25T05:39:33Z

/retest-required

Anddd7 · 2024-04-26T00:24:10Z

Hi @tao12345666333 , i have created e2e tests to verify grpc timeout, but pull-ingress-nginx-codegen check is always failed. Could you help to review and resolve it?

tao12345666333 · 2024-04-26T00:24:59Z

sure

Anddd7 · 2024-05-06T05:17:54Z

all tests passed after rebase, feel free to review and comment

tao12345666333 · 2024-05-07T22:00:49Z

Thank you 🙏

I have added this to my list, will finish review this week.

msfidelis · 2024-05-18T13:20:10Z

rootfs/etc/nginx/template/nginx.tmpl

+ grpc_connect_timeout {{ $location.Proxy.ConnectTimeout }}s;
+ grpc_send_timeout {{ $location.Proxy.SendTimeout }}s;
+ grpc_read_timeout {{ $location.Proxy.ReadTimeout }}s;


It would be interesting to change the data type from Int to time.Duration, allowing the configuration to be done in milliseconds instead only of in seconds.

Even more interesting would be to have explicit configurations for gRPC backends instead of reusing the global proxy settings. Only use the global ones if the specific ones are not set.

Like as comment in issue

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

https://github.com/kubernetes/ingress-nginx/blob/main/internal/ingress/annotations/proxy/main.go#L29-L31

Changing the default unit may not be something that should be included in this PR. Because modifying the time unit will break many existing configurations.

@msfidelis Would you want to create an issue for tracking this?

tao12345666333

Thank you for your contributions!

/lgtm

k8s-ci-robot · 2024-05-26T16:34:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Anddd7, tao12345666333, YannickZ

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tao12345666333]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 13, 2024

k8s-ci-robot requested review from puerco and strongjz April 13, 2024 12:39

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 13, 2024

k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 13, 2024

k8s-ci-robot added needs-priority size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 13, 2024

Anddd7 mentioned this pull request Apr 13, 2024

Support grpc_read_timeout and grpc_send_timeout in annotations #11250

Closed

Anddd7 force-pushed the add-grpc-timeouts branch from 2297425 to f1aed2b Compare April 14, 2024 03:07

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/docs and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 14, 2024

Anddd7 changed the title ~~WIP - feat: Add grpc timeouts annotations~~ feat: Add grpc timeouts annotations Apr 14, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 14, 2024

rikatz reviewed Apr 15, 2024

View reviewed changes

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2024

strongjz requested review from tao12345666333 and rikatz April 24, 2024 14:30

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 25, 2024

Anddd7 force-pushed the add-grpc-timeouts branch from 630e50a to ad35805 Compare April 25, 2024 13:08

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2024

Anddd7 added 9 commits May 6, 2024 10:16

✨ feat: add grpc timeouts with proxy settings if backend is grpc

2db4731

📝 docs: Documentation only changes

b43137b

🐛 fix: uppercase for protocol

7ade80e

📝 docs: grpc timeouts example

044d125

📝 docs: add links and default values for proxy timeout

41b01a5

🧪 test: add e2e test for timeout

6eb6949

🐛 fix: upgrade to 1.0.6 to fix nil pointer

ae589ed

🐛 fix: lint

fbf77e3

🧪 test: trigger ci

ee204a7

Anddd7 force-pushed the add-grpc-timeouts branch from 156aef3 to ee204a7 Compare May 6, 2024 02:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2024

msfidelis reviewed May 18, 2024

View reviewed changes

YannickZ approved these changes May 23, 2024

View reviewed changes

tao12345666333 approved these changes May 26, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2024

k8s-ci-robot merged commit d0e9934 into kubernetes:main May 26, 2024
27 checks passed

Anddd7 deleted the add-grpc-timeouts branch May 27, 2024 00:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add grpc timeouts annotations #11258

feat: Add grpc timeouts annotations #11258

Anddd7 commented Apr 13, 2024 •

edited

k8s-ci-robot commented Apr 13, 2024

k8s-ci-robot commented Apr 13, 2024

netlify bot commented Apr 13, 2024 •

edited

Anddd7 commented Apr 13, 2024

longwuyuan commented Apr 13, 2024

Anddd7 commented Apr 14, 2024 •

edited

rikatz Apr 15, 2024

rikatz Apr 15, 2024

longwuyuan Apr 16, 2024 •

edited

Anddd7 Apr 16, 2024

rikatz commented Apr 15, 2024

rikatz Apr 15, 2024

Anddd7 Apr 16, 2024

rikatz commented Apr 15, 2024 •

edited

Anddd7 commented Apr 25, 2024

Anddd7 commented Apr 26, 2024

tao12345666333 commented Apr 26, 2024

Anddd7 commented May 6, 2024

tao12345666333 commented May 7, 2024

msfidelis May 18, 2024

msfidelis May 18, 2024

tao12345666333 May 26, 2024

tao12345666333 May 26, 2024

tao12345666333 left a comment

k8s-ci-robot commented May 26, 2024

feat: Add grpc timeouts annotations #11258

feat: Add grpc timeouts annotations #11258

Conversation

Anddd7 commented Apr 13, 2024 • edited

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Apr 13, 2024

k8s-ci-robot commented Apr 13, 2024

netlify bot commented Apr 13, 2024 • edited

✅ Deploy Preview for kubernetes-ingress-nginx ready!

Anddd7 commented Apr 13, 2024

longwuyuan commented Apr 13, 2024

Anddd7 commented Apr 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longwuyuan Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rikatz commented Apr 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rikatz commented Apr 15, 2024 • edited

Anddd7 commented Apr 25, 2024

Anddd7 commented Apr 26, 2024

tao12345666333 commented Apr 26, 2024

Anddd7 commented May 6, 2024

tao12345666333 commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tao12345666333 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 26, 2024

Anddd7 commented Apr 13, 2024 •

edited

netlify bot commented Apr 13, 2024 •

edited

Anddd7 commented Apr 14, 2024 •

edited

longwuyuan Apr 16, 2024 •

edited

rikatz commented Apr 15, 2024 •

edited