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.

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

feat: Add grpc timeouts annotations #11258

Merged
merged 9 commits into from
May 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 2 additions & 4 deletions docs/examples/grpc/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,9 @@ This example demonstrates how to route traffic to a gRPC service through the Ing

### Notes on using response/request streams

> `grpc_read_timeout` and `grpc_send_timeout` will be set as `proxy_read_timeout` and `proxy_send_timeout` when you set backend protocol to `GRPC` or `GRPCS`.

1. If your server only does response streaming and you expect a stream to be open longer than 60 seconds, you will have to change the `grpc_read_timeout` to accommodate this.
2. If your service only does request streaming and you expect a stream to be open longer than 60 seconds, you have to change the
`grpc_send_timeout` and the `client_body_timeout`.
3. If you do both response and request streaming with an open stream longer than 60 seconds, you have to change all three timeouts: `grpc_read_timeout`, `grpc_send_timeout` and `client_body_timeout`.

Values for the timeouts must be specified as e.g. `"1200s"`.

> On the most recent versions of ingress-nginx, changing these timeouts requires using the `nginx.ingress.kubernetes.io/server-snippet` annotation. There are plans for future releases to allow using the Kubernetes annotations to define each timeout separately.
6 changes: 6 additions & 0 deletions docs/user-guide/nginx-configuration/annotations.md
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,12 @@ 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 grpc values will be set and inherited from proxy timeouts:

- [`grpc_connect_timeout=5s`](https://nginx.org/en/docs/http/ngx_http_grpc_module.html#grpc_connect_timeout), from `nginx.ingress.kubernetes.io/proxy-connect-timeout`
- [`grpc_send_timeout=60s`](https://nginx.org/en/docs/http/ngx_http_grpc_module.html#grpc_send_timeout), from `nginx.ingress.kubernetes.io/proxy-send-timeout`
- [`grpc_read_timeout=60s`](https://nginx.org/en/docs/http/ngx_http_grpc_module.html#grpc_read_timeout), from `nginx.ingress.kubernetes.io/proxy-read-timeout`

Note: All timeout values are unitless and in seconds e.g. `nginx.ingress.kubernetes.io/proxy-read-timeout: "120"` sets a valid 120 seconds proxy read timeout.

### Proxy redirect
Expand Down
6 changes: 6 additions & 0 deletions docs/user-guide/nginx-configuration/configmap.md
Original file line number Diff line number Diff line change
Expand Up @@ -1142,14 +1142,20 @@ See NGINX [client_max_body_size](https://nginx.org/en/docs/http/ngx_http_core_mo

Sets the timeout for [establishing a connection with a proxied server](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout). It should be noted that this timeout cannot usually exceed 75 seconds.

It will also set the [grpc_connect_timeout](https://nginx.org/en/docs/http/ngx_http_grpc_module.html#grpc_connect_timeout) for gRPC connections.

## proxy-read-timeout

Sets the timeout in seconds for [reading a response from the proxied server](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout). The timeout is set only between two successive read operations, not for the transmission of the whole response.

It will also set the [grpc_read_timeout](https://nginx.org/en/docs/http/ngx_http_grpc_module.html#grpc_read_timeout) for gRPC connections.

## proxy-send-timeout

Sets the timeout in seconds for [transmitting a request to the proxied server](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout). The timeout is set only between two successive write operations, not for the transmission of the whole request.

It will also set the [grpc_send_timeout](https://nginx.org/en/docs/http/ngx_http_grpc_module.html#grpc_send_timeout) for gRPC connections.

## proxy-buffers-number

Sets the number of the buffer used for [reading the first part of the response](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffers) received from the proxied server. This part usually contains a small response header.
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ require (
)

require (
github.com/Anddd7/pb v0.0.0-20240425032658-369b0f6a404c
github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161 // indirect
github.com/BurntSushi/toml v1.3.2 // indirect
github.com/beorn7/perks v1.0.1 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,8 @@ cloud.google.com/go/workflows v1.10.0/go.mod h1:fZ8LmRmZQWacon9UCX1r/g/DfAXx5VcP
dario.cat/mergo v1.0.0 h1:AGCNq9Evsj31mOgNPcLyXc+4PNABt905YmuqPYYpBWk=
dario.cat/mergo v1.0.0/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk=
dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
github.com/Anddd7/pb v0.0.0-20240425032658-369b0f6a404c h1:uhBf0CHXi7nCFZXxHV7l1cBcYFEEVRK4FYxvm1l9lKg=
github.com/Anddd7/pb v0.0.0-20240425032658-369b0f6a404c/go.mod h1:vYWKbnXd2KAZHUECLPzSE0Er3FgiEmOdPtxwSIRihck=
gioui.org v0.0.0-20210308172011-57750fc8a0a6/go.mod h1:RSH6KIUZ0p2xy5zHDxgAM4zumjgTw83q2ge/PI+yyw8=
git.sr.ht/~sbinet/gg v0.3.1/go.mod h1:KGYtlADtqsqANL9ueOFkWymvzUvLMQllU5Ixo+8v3pc=
github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161 h1:L/gRVlceqvL25UVaW/CKtUDjefjrs0SPonmDGUVOYP0=
Expand Down
7 changes: 7 additions & 0 deletions rootfs/etc/nginx/template/nginx.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpc_connect_timeout {{ $location.Proxy.ConnectTimeout }}s;
grpc_send_timeout {{ $location.Proxy.SendTimeout }}s;
grpc_read_timeout {{ $location.Proxy.ReadTimeout }}s;
Comment on lines +1486 to +1488
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{{ end }}

{{/* Add any additional configuration defined */}}
{{ $location.ConfigurationSnippet }}

Expand Down
94 changes: 91 additions & 3 deletions test/e2e/annotations/grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,13 @@ import (
"fmt"
"strings"

delaypb "github.com/Anddd7/pb/grpcbin"
pb "github.com/moul/pb/grpcbin/go-grpc"
"github.com/onsi/ginkgo/v2"
"github.com/stretchr/testify/assert"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/metadata"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -35,16 +37,17 @@ import (
"k8s.io/ingress-nginx/test/e2e/framework"
)

const echoHost = "echo"
const (
echoHost = "echo"
host = "grpc"
)

var _ = framework.DescribeAnnotation("backend-protocol - GRPC", func() {
f := framework.NewDefaultFramework("grpc", framework.WithHTTPBunEnabled())

ginkgo.It("should use grpc_pass in the configuration file", func() {
f.NewGRPCFortuneTellerDeployment()

host := "grpc"

annotations := map[string]string{
"nginx.ingress.kubernetes.io/backend-protocol": "GRPC",
}
Expand Down Expand Up @@ -259,4 +262,89 @@ var _ = framework.DescribeAnnotation("backend-protocol - GRPC", func() {
metadata := res.GetMetadata()
assert.Equal(ginkgo.GinkgoT(), metadata["content-type"].Values[0], "application/grpc")
})

ginkgo.It("should return OK when request not exceed timeout", func() {
f.NewGRPCBinDelayDeployment()

proxyTimeout := "10"
ingressName := "grpcbin-delay"

annotations := make(map[string]string)
annotations["nginx.ingress.kubernetes.io/backend-protocol"] = "GRPC"
annotations["nginx.ingress.kubernetes.io/proxy-connect-timeout"] = proxyTimeout
annotations["nginx.ingress.kubernetes.io/proxy-send-timeout"] = proxyTimeout
annotations["nginx.ingress.kubernetes.io/proxy-read-timeout"] = proxyTimeout

ing := framework.NewSingleIngress(host, "/", host, f.Namespace, ingressName, 50051, annotations)

f.EnsureIngress(ing)

f.WaitForNginxServer(host,
func(server string) bool {
return strings.Contains(server, fmt.Sprintf("grpc_connect_timeout %ss;", proxyTimeout)) &&
strings.Contains(server, fmt.Sprintf("grpc_send_timeout %ss;", proxyTimeout)) &&
strings.Contains(server, fmt.Sprintf("grpc_read_timeout %ss;", proxyTimeout))
})

conn, err := grpc.Dial(
f.GetNginxIP()+":80",
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithAuthority(host),
)
assert.Nil(ginkgo.GinkgoT(), err, "error creating a connection")
defer conn.Close()

client := delaypb.NewGrpcbinServiceClient(conn)

res, err := client.Unary(context.Background(), &delaypb.UnaryRequest{
Data: "hello",
})
assert.Nil(ginkgo.GinkgoT(), err)

metadata := res.GetResponseAttributes().RequestHeaders
assert.Equal(ginkgo.GinkgoT(), metadata["content-type"], "application/grpc")
assert.Equal(ginkgo.GinkgoT(), metadata[":authority"], host)
})

ginkgo.It("should return Error when request exceed timeout", func() {
f.NewGRPCBinDelayDeployment()

proxyTimeout := "10"
ingressName := "grpcbin-delay"

annotations := make(map[string]string)
annotations["nginx.ingress.kubernetes.io/backend-protocol"] = "GRPC"
annotations["nginx.ingress.kubernetes.io/proxy-connect-timeout"] = proxyTimeout
annotations["nginx.ingress.kubernetes.io/proxy-send-timeout"] = proxyTimeout
annotations["nginx.ingress.kubernetes.io/proxy-read-timeout"] = proxyTimeout

ing := framework.NewSingleIngress(host, "/", host, f.Namespace, ingressName, 50051, annotations)

f.EnsureIngress(ing)

f.WaitForNginxServer(host,
func(server string) bool {
return strings.Contains(server, fmt.Sprintf("grpc_connect_timeout %ss;", proxyTimeout)) &&
strings.Contains(server, fmt.Sprintf("grpc_send_timeout %ss;", proxyTimeout)) &&
strings.Contains(server, fmt.Sprintf("grpc_read_timeout %ss;", proxyTimeout))
})

conn, err := grpc.Dial(
f.GetNginxIP()+":80",
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithAuthority(host),
)
assert.Nil(ginkgo.GinkgoT(), err, "error creating a connection")
defer conn.Close()

client := delaypb.NewGrpcbinServiceClient(conn)

_, err = client.Unary(context.Background(), &delaypb.UnaryRequest{
Data: "hello",
RequestAttributes: &delaypb.RequestAttributes{
Delay: 15,
},
})
assert.Error(ginkgo.GinkgoT(), err)
})
})
109 changes: 109 additions & 0 deletions test/e2e/framework/grpc_delay.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
Copyright 2024 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package framework

import (
"github.com/onsi/ginkgo/v2"
"github.com/stretchr/testify/assert"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/util/intstr"
)

// NewGRPCBinDelayDeployment creates a new single replica
// deployment of the grpcbin image in a particular namespace
func (f *Framework) NewGRPCBinDelayDeployment() {
f.NewNewGRPCBinDelayDeploymentWithReplicas(1)
}

// NewNewGRPCBinDelayDeploymentWithReplicas creates a new deployment of the
// grpcbin image in a particular namespace. Number of replicas is configurable
func (f *Framework) NewNewGRPCBinDelayDeploymentWithReplicas(replicas int32) {
name := "grpcbin-delay"

deployment := &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: f.Namespace,
},
Spec: appsv1.DeploymentSpec{
Replicas: NewInt32(replicas),
Selector: &metav1.LabelSelector{
MatchLabels: map[string]string{
"app": name,
},
},
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
"app": name,
},
},
Spec: corev1.PodSpec{
TerminationGracePeriodSeconds: NewInt64(0),
Containers: []corev1.Container{
{
Name: name,
Image: "ghcr.io/anddd7/grpcbin:v1.0.6",
Env: []corev1.EnvVar{},
Ports: []corev1.ContainerPort{
{
Name: "grpc",
ContainerPort: 50051,
},
},
},
},
},
},
},
}

d := f.EnsureDeployment(deployment)

err := waitForPodsReady(f.KubeClientSet, DefaultTimeout, int(replicas), f.Namespace, &metav1.ListOptions{
LabelSelector: fields.SelectorFromSet(fields.Set(d.Spec.Template.ObjectMeta.Labels)).String(),
})
assert.Nil(ginkgo.GinkgoT(), err, "failed to wait for to become ready")

service := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: f.Namespace,
},
Spec: corev1.ServiceSpec{
Ports: []corev1.ServicePort{
{
Name: "grpc",
Port: 50051,
TargetPort: intstr.FromInt(50051),
Protocol: "TCP",
},
},
Selector: map[string]string{
"app": name,
},
},
}

f.EnsureService(service)

err = WaitForEndpoints(f.KubeClientSet, DefaultTimeout, name, f.Namespace, int(replicas))
assert.Nil(ginkgo.GinkgoT(), err, "waiting for endpoints to become ready")
}