From 0405fc8383f474e969f268b41317d0d77ddb5fc1 Mon Sep 17 00:00:00 2001 From: Zhimin Xiang Date: Tue, 24 Jul 2018 14:40:34 -0700 Subject: [PATCH] Set 60 seconds as the timeout of route (#1667) * Set a long default timeout for route * Only use one timeout DefaultRouteTimeout in Route --- .../route/resources/virtual_service.go | 14 +- .../route/resources/virtual_service_test.go | 28 +++- pkg/controller/route/route_test.go | 32 ++++- test/crd.go | 20 +++ test/e2e/e2e.go | 16 +++ test/e2e/service_to_service_test.go | 126 ++++++++++++++++++ test/e2e/test_images/httpproxy/Dockerfile | 6 + test/e2e/test_images/httpproxy/httpproxy.go | 71 ++++++++++ 8 files changed, 302 insertions(+), 11 deletions(-) create mode 100644 test/e2e/service_to_service_test.go create mode 100644 test/e2e/test_images/httpproxy/Dockerfile create mode 100644 test/e2e/test_images/httpproxy/httpproxy.go diff --git a/pkg/controller/route/resources/virtual_service.go b/pkg/controller/route/resources/virtual_service.go index a2cf90fa5244..7a19962f341d 100644 --- a/pkg/controller/route/resources/virtual_service.go +++ b/pkg/controller/route/resources/virtual_service.go @@ -48,7 +48,7 @@ const ( IstioTimeoutHackHeaderKey = "x-envoy-upstream-rq-timeout-ms" IstioTimeoutHackHeaderValue = "0" - DefaultActivatorTimeout = "60s" + DefaultRouteTimeout = "60s" ) // MakeVirtualService creates an Istio VirtualService to set up routing rules. Such VirtualService specifies @@ -142,6 +142,10 @@ func makeVirtualServiceRoute(domains []string, ns string, targets []traffic.Revi route := v1alpha3.HTTPRoute{ Match: matches, Route: weights, + Timeout: DefaultRouteTimeout, + AppendHeaders: map[string]string{ + IstioTimeoutHackHeaderKey: IstioTimeoutHackHeaderValue, + }, } // Add traffic rules for activator. return addActivatorRoutes(&route, ns, inactive) @@ -189,12 +193,8 @@ func addActivatorRoutes(r *v1alpha3.HTTPRoute, ns string, inactive []traffic.Rev }, Weight: totalInactivePercent, }) - r.AppendHeaders = map[string]string{ - controller.GetRevisionHeaderName(): maxInactiveTarget.RevisionName, - controller.GetRevisionHeaderNamespace(): ns, - IstioTimeoutHackHeaderKey: IstioTimeoutHackHeaderValue, - } - r.Timeout = DefaultActivatorTimeout + r.AppendHeaders[controller.GetRevisionHeaderName()] = maxInactiveTarget.RevisionName + r.AppendHeaders[controller.GetRevisionHeaderNamespace()] = ns return r } diff --git a/pkg/controller/route/resources/virtual_service_test.go b/pkg/controller/route/resources/virtual_service_test.go index d2627089a38b..48253ea82c17 100644 --- a/pkg/controller/route/resources/virtual_service_test.go +++ b/pkg/controller/route/resources/virtual_service_test.go @@ -122,6 +122,10 @@ func TestMakeVirtualServiceSpec_CorrectRoutes(t *testing.T) { }, Weight: 100, }}, + Timeout: DefaultRouteTimeout, + AppendHeaders: map[string]string{ + IstioTimeoutHackHeaderKey: IstioTimeoutHackHeaderValue, + }, }, { Match: []v1alpha3.HTTPMatchRequest{{ Authority: &v1alpha3.StringMatch{Exact: "v1.domain.com"}, @@ -133,6 +137,10 @@ func TestMakeVirtualServiceSpec_CorrectRoutes(t *testing.T) { }, Weight: 100, }}, + Timeout: DefaultRouteTimeout, + AppendHeaders: map[string]string{ + IstioTimeoutHackHeaderKey: IstioTimeoutHackHeaderValue, + }, }} routes := MakeVirtualService(r, &traffic.TrafficConfig{Targets: targets}).Spec.Http if diff := cmp.Diff(expected, routes); diff != "" { @@ -205,6 +213,10 @@ func TestMakeVirtualServiceRoute_Vanilla(t *testing.T) { }, Weight: 100, }}, + Timeout: DefaultRouteTimeout, + AppendHeaders: map[string]string{ + IstioTimeoutHackHeaderKey: IstioTimeoutHackHeaderValue, + }, } if diff := cmp.Diff(&expected, route); diff != "" { t.Errorf("Unexpected route (-want +got): %v", diff) @@ -241,6 +253,10 @@ func TestMakeVirtualServiceRoute_ZeroPercentTarget(t *testing.T) { }, Weight: 100, }}, + Timeout: DefaultRouteTimeout, + AppendHeaders: map[string]string{ + IstioTimeoutHackHeaderKey: IstioTimeoutHackHeaderValue, + }, } if diff := cmp.Diff(&expected, route); diff != "" { t.Errorf("Unexpected route (-want +got): %v", diff) @@ -284,6 +300,10 @@ func TestMakeVirtualServiceRoute_TwoTargets(t *testing.T) { }, Weight: 10, }}, + Timeout: DefaultRouteTimeout, + AppendHeaders: map[string]string{ + IstioTimeoutHackHeaderKey: IstioTimeoutHackHeaderValue, + }, } if diff := cmp.Diff(&expected, route); diff != "" { t.Errorf("Unexpected route (-want +got): %v", diff) @@ -321,7 +341,7 @@ func TestMakeVirtualServiceRoute_VanillaScaledToZero(t *testing.T) { "Knative-Serving-Namespace": "test-ns", IstioTimeoutHackHeaderKey: IstioTimeoutHackHeaderValue, }, - Timeout: DefaultActivatorTimeout, + Timeout: DefaultRouteTimeout, } if diff := cmp.Diff(&expected, route); diff != "" { t.Errorf("Unexpected route (-want +got): %v", diff) @@ -364,7 +384,7 @@ func TestMakeVirtualServiceRoute_TwoInactiveTargets(t *testing.T) { "Knative-Serving-Namespace": "test-ns", IstioTimeoutHackHeaderKey: IstioTimeoutHackHeaderValue, }, - Timeout: DefaultActivatorTimeout, + Timeout: DefaultRouteTimeout, } if diff := cmp.Diff(&expected, route); diff != "" { t.Errorf("Unexpected route (-want +got): %v", diff) @@ -402,6 +422,10 @@ func TestMakeVirtualServiceRoute_ZeroPercentNamedTargetScaledToZero(t *testing.T }, Weight: 100, }}, + Timeout: DefaultRouteTimeout, + AppendHeaders: map[string]string{ + IstioTimeoutHackHeaderKey: IstioTimeoutHackHeaderValue, + }, } if diff := cmp.Diff(&expected, route); diff != "" { t.Errorf("Unexpected route (-want +got): %v", diff) diff --git a/pkg/controller/route/route_test.go b/pkg/controller/route/route_test.go index 3fd57f858f2e..89b9f793193f 100644 --- a/pkg/controller/route/route_test.go +++ b/pkg/controller/route/route_test.go @@ -319,7 +319,7 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) { ctrl.GetRevisionHeaderNamespace(): testNamespace, resources.IstioTimeoutHackHeaderKey: resources.IstioTimeoutHackHeaderValue, }, - Timeout: resources.DefaultActivatorTimeout, + Timeout: resources.DefaultRouteTimeout, }}, } if diff := cmp.Diff(expectedSpec, vs.Spec); diff != "" { @@ -406,6 +406,10 @@ func TestCreateRouteWithMultipleTargets(t *testing.T) { }, Weight: 10, }}, + Timeout: resources.DefaultRouteTimeout, + AppendHeaders: map[string]string{ + resources.IstioTimeoutHackHeaderKey: resources.IstioTimeoutHackHeaderValue, + }, }}, } if diff := cmp.Diff(expectedSpec, vs.Spec); diff != "" { @@ -493,7 +497,7 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) { ctrl.GetRevisionHeaderNamespace(): testNamespace, resources.IstioTimeoutHackHeaderKey: resources.IstioTimeoutHackHeaderValue, }, - Timeout: resources.DefaultActivatorTimeout, + Timeout: resources.DefaultRouteTimeout, }}, } if diff := cmp.Diff(expectedSpec, vs.Spec); diff != "" { @@ -595,6 +599,10 @@ func TestCreateRouteWithDuplicateTargets(t *testing.T) { }, Weight: 50, }}, + Timeout: resources.DefaultRouteTimeout, + AppendHeaders: map[string]string{ + resources.IstioTimeoutHackHeaderKey: resources.IstioTimeoutHackHeaderValue, + }, }, { Match: []v1alpha3.HTTPMatchRequest{{Authority: &v1alpha3.StringMatch{Exact: "test-revision-1." + domain}}}, Route: []v1alpha3.DestinationWeight{{ @@ -604,6 +612,10 @@ func TestCreateRouteWithDuplicateTargets(t *testing.T) { }, Weight: 100, }}, + Timeout: resources.DefaultRouteTimeout, + AppendHeaders: map[string]string{ + resources.IstioTimeoutHackHeaderKey: resources.IstioTimeoutHackHeaderValue, + }, }, { Match: []v1alpha3.HTTPMatchRequest{{Authority: &v1alpha3.StringMatch{Exact: "test-revision-2." + domain}}}, Route: []v1alpha3.DestinationWeight{{ @@ -613,6 +625,10 @@ func TestCreateRouteWithDuplicateTargets(t *testing.T) { }, Weight: 100, }}, + Timeout: resources.DefaultRouteTimeout, + AppendHeaders: map[string]string{ + resources.IstioTimeoutHackHeaderKey: resources.IstioTimeoutHackHeaderValue, + }, }}, } if diff := cmp.Diff(expectedSpec, vs.Spec); diff != "" { @@ -699,6 +715,10 @@ func TestCreateRouteWithNamedTargets(t *testing.T) { }, Weight: 50, }}, + Timeout: resources.DefaultRouteTimeout, + AppendHeaders: map[string]string{ + resources.IstioTimeoutHackHeaderKey: resources.IstioTimeoutHackHeaderValue, + }, }, { Match: []v1alpha3.HTTPMatchRequest{{Authority: &v1alpha3.StringMatch{Exact: "bar." + domain}}}, Route: []v1alpha3.DestinationWeight{{ @@ -708,6 +728,10 @@ func TestCreateRouteWithNamedTargets(t *testing.T) { }, Weight: 100, }}, + Timeout: resources.DefaultRouteTimeout, + AppendHeaders: map[string]string{ + resources.IstioTimeoutHackHeaderKey: resources.IstioTimeoutHackHeaderValue, + }, }, { Match: []v1alpha3.HTTPMatchRequest{{Authority: &v1alpha3.StringMatch{Exact: "foo." + domain}}}, Route: []v1alpha3.DestinationWeight{{ @@ -717,6 +741,10 @@ func TestCreateRouteWithNamedTargets(t *testing.T) { }, Weight: 100, }}, + Timeout: resources.DefaultRouteTimeout, + AppendHeaders: map[string]string{ + resources.IstioTimeoutHackHeaderKey: resources.IstioTimeoutHackHeaderValue, + }, }}, } if diff := cmp.Diff(expectedSpec, vs.Spec); diff != "" { diff --git a/test/crd.go b/test/crd.go index 06bda039d253..0b413c0631cd 100644 --- a/test/crd.go +++ b/test/crd.go @@ -101,6 +101,26 @@ func Configuration(namespace string, names ResourceNames, imagePath string) *v1a } } +func ConfigurationWithEnvVars(namespace string, names ResourceNames, imagePath string, envVars []corev1.EnvVar) *v1alpha1.Configuration { + + return &v1alpha1.Configuration{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: names.Config, + }, + Spec: v1alpha1.ConfigurationSpec{ + RevisionTemplate: v1alpha1.RevisionTemplateSpec{ + Spec: v1alpha1.RevisionSpec{ + Container: corev1.Container{ + Image: imagePath, + Env: envVars, + }, + }, + }, + }, + } +} + func ConfigurationWithBuild(namespace string, names ResourceNames, build *buildv1alpha1.BuildSpec, imagePath string) *v1alpha1.Configuration { return &v1alpha1.Configuration{ ObjectMeta: metav1.ObjectMeta{ diff --git a/test/e2e/e2e.go b/test/e2e/e2e.go index d7fd7eb9ef7b..6bb5dd8ad4ae 100644 --- a/test/e2e/e2e.go +++ b/test/e2e/e2e.go @@ -6,6 +6,7 @@ import ( "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" // Mysteriously required to support GCP auth (required by k8s libs). // Apparently just importing it is enough. @_@ side effects @_@. // https://github.com/kubernetes/client-go/issues/242 @@ -72,3 +73,18 @@ func CreateRouteAndConfig(clients *test.Clients, logger *zap.SugaredLogger, imag test.Route(test.Flags.Namespace, names)) return names, err } + +func CreateRouteAndConfigWithEnv(clients *test.Clients, logger *zap.SugaredLogger, imagePath string, envVars []corev1.EnvVar) (test.ResourceNames, error) { + var names test.ResourceNames + names.Config = test.AppendRandomString(configName, logger) + names.Route = test.AppendRandomString(routeName, logger) + + _, err := clients.Configs.Create( + test.ConfigurationWithEnvVars(test.Flags.Namespace, names, imagePath, envVars)) + if err != nil { + return test.ResourceNames{}, err + } + _, err = clients.Routes.Create( + test.Route(test.Flags.Namespace, names)) + return names, err +} \ No newline at end of file diff --git a/test/e2e/service_to_service_test.go b/test/e2e/service_to_service_test.go new file mode 100644 index 000000000000..01948099b8fd --- /dev/null +++ b/test/e2e/service_to_service_test.go @@ -0,0 +1,126 @@ +// +build e2e + +/* +Copyright 2018 The Knative 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 e2e + +import ( + "fmt" + "net/http" + "strings" + "testing" + + "github.com/knative/serving/test" + "github.com/knative/serving/test/spoof" + + "go.uber.org/zap" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + corev1 "k8s.io/api/core/v1" +) + +var ( + clients *test.Clients + logger *zap.SugaredLogger +) +const ( + targetHostEnv = "TARGET_HOST" + helloworldResponse = "Hello World! How about some tasty noodles?" +) + +func createTargetHostEnvVars(routeName string, t *testing.T) []corev1.EnvVar { + helloWorldRoute, err := clients.Routes.Get(routeName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get Route of helloworld app: %v", err) + } + internalDomain := helloWorldRoute.Status.DomainInternal + logger.Infof("helloworld internal domain is %v.", internalDomain) + return []corev1.EnvVar{{ + Name: targetHostEnv, + Value: internalDomain, + }} +} + +func sendRequest(resolvableDomain bool, domain string) (*spoof.Response, error) { + logger.Infof("The domain of request is %s.", domain) + client, err := spoof.New(clients.Kube, logger, domain, resolvableDomain) + if err != nil { + return nil, err + } + + req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://%s", domain), nil) + if err != nil { + return nil, err + } + return client.Do(req) +} + +// In this test, we set up two apps: helloworld and httpproxy. +// helloworld is a simple app that displays a plaintext string. +// httpproxy is a proxy that redirects request to internal service of helloworld app +// with FQDN {route}.{namespace}.svc.cluster.local. +// The expected result is that the request sent to httpproxy app is successfully redirected +// to helloworld app. +func TestServiceToServiceCall(t *testing.T) { + logger = test.GetContextLogger("TestServiceToServiceCall") + clients = Setup(t) + + // Set up helloworld app. + helloWorldImagePath := strings.Join([]string{test.Flags.DockerRepo, "helloworld"}, "/") + logger.Infof("Creating a Route and Configuration for helloworld test app.") + helloWorldNames, err := CreateRouteAndConfig(clients, logger, helloWorldImagePath) + if err != nil { + t.Fatalf("Failed to create Route and Configuration: %v", err) + } + test.CleanupOnInterrupt(func() { TearDown(clients, helloWorldNames, logger) }, logger) + defer TearDown(clients, helloWorldNames, logger) + if err := test.WaitForRouteState(clients.Routes, helloWorldNames.Route, test.IsRouteReady, "RouteIsReady"); err != nil { + t.Fatalf("The Route %s was not marked as Ready to serve traffic: %v", helloWorldNames.Route, err) + } + + // Set up httpproxy app. + httpProxyImagePath := strings.Join([]string{test.Flags.DockerRepo, "httpproxy"}, "/") + logger.Infof("Creating a Route and Configuration for httpproxy test app.") + envVars := createTargetHostEnvVars(helloWorldNames.Route, t) + httpProxyNames, err := CreateRouteAndConfigWithEnv(clients, logger, httpProxyImagePath, envVars) + if err != nil { + t.Fatalf("Failed to create Route and Configuration: %v", err) + } + test.CleanupOnInterrupt(func() { TearDown(clients, httpProxyNames, logger) }, logger) + defer TearDown(clients, httpProxyNames, logger) + if err := test.WaitForRouteState(clients.Routes, httpProxyNames.Route, test.IsRouteReady, "RouteIsReady"); err != nil { + t.Fatalf("The Route %s was not marked as Ready to serve traffic: %v", httpProxyNames.Route, err) + } + httpProxyRoute, err := clients.Routes.Get(httpProxyNames.Route, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get Route %s: %v", httpProxyNames.Route, err) + } + if err = test.WaitForEndpointState(clients.Kube, logger, test.Flags.ResolvableDomain, httpProxyRoute.Status.Domain, test.MatchesAny, "HttpProxy"); err != nil { + t.Fatalf("Failed to start endpoint of httpproxy: %v", err) + } + logger.Info("httpproxy is ready.") + + + // Send request to httpproxy to trigger the http call from httpproxy Pod to internal service of helloworld app. + response, err := sendRequest(test.Flags.ResolvableDomain, httpProxyRoute.Status.Domain) + if err != nil { + t.Fatalf("Failed to send request to httpproxy. %v", err) + } + // We expect the response from httpproxy is equal to the response from htlloworld + if helloworldResponse != strings.TrimSpace(string(response.Body)) { + t.Fatalf("The httpproxy response %s is not equal to helloworld response %s.", string(response.Body), helloworldResponse) + } +} diff --git a/test/e2e/test_images/httpproxy/Dockerfile b/test/e2e/test_images/httpproxy/Dockerfile new file mode 100644 index 000000000000..55bb46647161 --- /dev/null +++ b/test/e2e/test_images/httpproxy/Dockerfile @@ -0,0 +1,6 @@ +FROM golang:latest +RUN mkdir /app +ADD . /app/ +WORKDIR /app +RUN go build -o httpproxy . +CMD ["/app/httpproxy"] diff --git a/test/e2e/test_images/httpproxy/httpproxy.go b/test/e2e/test_images/httpproxy/httpproxy.go new file mode 100644 index 000000000000..c58cb51babe5 --- /dev/null +++ b/test/e2e/test_images/httpproxy/httpproxy.go @@ -0,0 +1,71 @@ +/* +Copyright 2018 The Knative 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 + https://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 main + +import ( + "fmt" + "flag" + "log" + "os" + + "net/http" + "net/http/httputil" + "net/url" +) + +const ( + targetHostEnv = "TARGET_HOST" +) + +var ( + httpProxy *httputil.ReverseProxy + targetHost string +) + +func handler(w http.ResponseWriter, r *http.Request) { + log.Print("Http proxy received a request.") + // Reverse proxy does not automatically reset the Host header. + // We need to manually reset it. + r.Host = getTargetHostEnv() + httpProxy.ServeHTTP(w, r) + return +} + +func getTargetHostEnv() string { + value := os.Getenv(targetHostEnv) + if value == "" { + log.Fatalf("No env %v provided.", targetHostEnv) + } + return value +} + +func initialHttpProxy(proxyUrl string) *httputil.ReverseProxy { + target, err := url.Parse(proxyUrl) + if err != nil { + log.Fatalf("Failed to parse url %v", proxyUrl) + } + return httputil.NewSingleHostReverseProxy(target) +} + +func main() { + flag.Parse() + log.Print("Http Proxy app started.") + + targetHost = getTargetHostEnv() + targetUrl := fmt.Sprintf("http://%s", targetHost) + httpProxy = initialHttpProxy(targetUrl) + + http.HandleFunc("/", handler) + http.ListenAndServe(":8080", nil) +}