-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from all commits
2db4731
b43137b
7ade80e
044d125
41b01a5
6eb6949
ae589ed
fbf77e3
ee204a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be interesting to change the data type from 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like as comment in issue
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }} | ||
|
||
|
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") | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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