From 4f6c1f8af1232fab49bcde3318e9886743b7e704 Mon Sep 17 00:00:00 2001 From: Dean Coakley Date: Tue, 25 Jun 2019 15:17:21 +0100 Subject: [PATCH 1/4] Add timeouts support in Upstreams vs/vsr --- Makefile | 4 +- docs/virtualserver-and-virtualserverroute.md | 30 ++++--- internal/configs/config_params.go | 2 + internal/configs/ingress.go | 1 + internal/configs/ingress_test.go | 6 +- internal/configs/version1/config.go | 1 + internal/configs/version2/config.go | 10 ++- .../version2/nginx-plus.virtualserver.tmpl | 7 +- .../configs/version2/nginx.virtualserver.tmpl | 3 +- internal/configs/version2/templates_test.go | 5 ++ internal/configs/virtualserver.go | 78 +++++++++++-------- internal/configs/virtualserver_test.go | 12 +-- pkg/apis/configuration/v1alpha1/types.go | 15 ++-- .../configuration/validation/validation.go | 5 ++ .../validation/validation_test.go | 5 +- 15 files changed, 116 insertions(+), 68 deletions(-) diff --git a/Makefile b/Makefile index 599c992575..96af249050 100644 --- a/Makefile +++ b/Makefile @@ -29,9 +29,9 @@ lint: test: ifeq ($(BUILD_IN_CONTAINER),1) - $(DOCKER_RUN) $(GOLANG_CONTAINER) go test ./... + $(DOCKER_RUN) $(GOLANG_CONTAINER) go test -race ./... else - go test ./... + go test -race ./... endif certificate-and-key: diff --git a/docs/virtualserver-and-virtualserverroute.md b/docs/virtualserver-and-virtualserverroute.md index d60628e826..b9c9e35077 100644 --- a/docs/virtualserver-and-virtualserverroute.md +++ b/docs/virtualserver-and-virtualserverroute.md @@ -4,7 +4,7 @@ The VirtualServer and VirtualServerRoute resources are new load balancing config This document is the reference documentation for the resources. To see additional examples of using the resources for specific use cases, go to the [examples-of-custom-resources](../examples-of-custom-resources) folder. -**Feature Status**: The VirtualServer and VirtualServerRoute resources are available as a preview feature: it is suitable for experimenting and testing; however, it must be used with caution in production environments. Additionally, while the feature is in preview, we might introduce some backward-incompatible changes to the resources specification in the next releases. +**Feature Status**: The VirtualServer and VirtualServerRoute resources are available as a preview feature: it is suitable for experimenting and testing; however, it must be used with caution in production environments. Additionally, while the feature is in preview, we might introduce some backward-incompatible changes to the resources specification in the next releases. ## Contents - [VirtualServer and VirtualServerRoute Resources](#VirtualServer-and-VirtualServerRoute-Resources) @@ -25,7 +25,7 @@ This document is the reference documentation for the resources. To see additiona - [Validation](#Validation) - [Customization via ConfigMap](#Customization-via-ConfigMap) -## Prerequisites +## Prerequisites The VirtualServer and VirtualServerRoute resources are disabled by default. Make sure to follow Step 1.4 of the [installation](installation.md) doc during the installation process to enable the resources. @@ -124,7 +124,7 @@ VirtualServerRoute: apiVersion: k8s.nginx.org/v1alpha1 kind: VirtualServerRoute metadata: - name: coffee + name: coffee namespace: coffee-ns spec: host: cafe.example.com @@ -132,7 +132,7 @@ spec: - name: latte service: latte-svc port: 80 - - name: espresso + - name: espresso service: espresso-svc port: 80 subroutes: @@ -179,7 +179,10 @@ port: 80 lb-method: round_robin fail-timeout: 10s max-fails: 1 -``` +connect-timeout: 30s +read-timeout: 30s +send-timeout: 30s +``` | Field | Description | Type | Required | | ----- | ----------- | ---- | -------- | @@ -189,6 +192,9 @@ max-fails: 1 | `lb-method` | The load [balancing method](https://docs.nginx.com/nginx/admin-guide/load-balancer/http-load-balancer/#choosing-a-load-balancing-method). To use the round-robin method, specify `round_robin`. The default is specified in the `lb-method` ConfigMap key. | `string` | No | | `fail-timeout` | The time during which the specified number of unsuccessful attempts to communicate with an upstream server should happen to consider the server unavailable. See the [fail_timeout](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#fail_timeout) parameter of the server directive. The default is set in the `fail-timeout` ConfigMap key. | `string` | No | | `max-fails` | The number of unsuccessful attempts to communicate with an upstream server that should happen in the duration set by the `fail-timeout` to consider the server unavailable. See the [max_fails](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#max_fails) parameter of the server directive. The default is set in the `max-fails` ConfgMap key. | `int` | No | +`connect-timeout` | The timeout for establishing a connection with an upstream server. See the [proxy_connect_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout) directive. The default is specified in the `proxy-connect-timeout` ConfigMap key. | `string` | No +`read-timeout` | The timeout for reading a response from an upstream server. See the [proxy_read_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout) directive. The default is specified in the `proxy-read-timeout` ConfigMap key. | `string` | No +`send-timeout` | The timeout for transmitting a request to an upstream server. See the [proxy_send_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout) directive. The default is 60s. | `string` | No ### Split @@ -210,12 +216,12 @@ splits: ### Rules -The rules defines a set of content-based routing rules in a route or subroute. +The rules defines a set of content-based routing rules in a route or subroute. In the example below, NGINX routes requests with the path `/coffee` to different upstreams based on the value of the cookie `user`: * `user=john` -> `coffee-future` * `user=bob` -> `coffee-deprecated` -* If the cookie is not set or not equal to either `john` or `bob`, NGINX routes to `coffee-stable` +* If the cookie is not set or not equal to either `john` or `bob`, NGINX routes to `coffee-stable` ```yaml path: /coffee @@ -233,8 +239,8 @@ rules: ``` In the next example, NGINX routes requests based on the value of the built-in [`$request_method` variable](http://nginx.org/en/docs/http/ngx_http_core_module.html#var_request_method), which represents the HTTP method of a request: -* all POST requests -> `coffee-post` -* all non-POST requests -> `coffee` +* all POST requests -> `coffee-post` +* all non-POST requests -> `coffee` ```yaml path: /coffee @@ -242,7 +248,7 @@ rules: conditions: - variable: $request_method matches: - - values: + - values: - POST upstream: coffee-post defaultUpstream: coffee @@ -311,9 +317,9 @@ In the kubectl get and similar commands, you can also use the short name `vs` in Working with VirtualServerRoute resources is analogous. In the kubectl commands, use `virtualserverroute` or the short name `vsr`. -### Validation +### Validation -The Ingress Controller validates VirtualServer and VirtualServerRoute resources. If a resource is invalid, the Ingress Controller will reject it. +The Ingress Controller validates VirtualServer and VirtualServerRoute resources. If a resource is invalid, the Ingress Controller will reject it. You can check if the Ingress Controller successfully applied the configuration for a VirtualServer. For our example `cafe` VirtualServer, we can run: ``` diff --git a/internal/configs/config_params.go b/internal/configs/config_params.go index fa3fda4198..d4ce98a373 100644 --- a/internal/configs/config_params.go +++ b/internal/configs/config_params.go @@ -8,6 +8,7 @@ type ConfigParams struct { ServerTokens string ProxyConnectTimeout string ProxyReadTimeout string + ProxySendTimeout string ClientMaxBodySize string HTTP2 bool RedirectToHTTPS bool @@ -91,6 +92,7 @@ func NewDefaultConfigParams() *ConfigParams { ServerTokens: "on", ProxyConnectTimeout: "60s", ProxyReadTimeout: "60s", + ProxySendTimeout: "60s", ClientMaxBodySize: "1m", SSLRedirect: true, MainServerNamesHashMaxSize: "512", diff --git a/internal/configs/ingress.go b/internal/configs/ingress.go index a87ad5b2fd..4274448113 100644 --- a/internal/configs/ingress.go +++ b/internal/configs/ingress.go @@ -235,6 +235,7 @@ func createLocation(path string, upstream version1.Upstream, cfg *ConfigParams, Upstream: upstream, ProxyConnectTimeout: cfg.ProxyConnectTimeout, ProxyReadTimeout: cfg.ProxyReadTimeout, + ProxySendTimeout: cfg.ProxySendTimeout, ClientMaxBodySize: cfg.ClientMaxBodySize, Websocket: websocket, Rewrite: rewrite, diff --git a/internal/configs/ingress_test.go b/internal/configs/ingress_test.go index cc1b415434..ac957be713 100644 --- a/internal/configs/ingress_test.go +++ b/internal/configs/ingress_test.go @@ -5,7 +5,7 @@ import ( "testing" "github.com/nginxinc/kubernetes-ingress/internal/configs/version1" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/api/extensions/v1beta1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -161,6 +161,7 @@ func createExpectedConfigForCafeIngressEx() version1.IngressNginxConfig { Upstream: coffeeUpstream, ProxyConnectTimeout: "60s", ProxyReadTimeout: "60s", + ProxySendTimeout: "60s", ClientMaxBodySize: "1m", ProxyBuffering: true, }, @@ -169,6 +170,7 @@ func createExpectedConfigForCafeIngressEx() version1.IngressNginxConfig { Upstream: teaUpstream, ProxyConnectTimeout: "60s", ProxyReadTimeout: "60s", + ProxySendTimeout: "60s", ClientMaxBodySize: "1m", ProxyBuffering: true, }, @@ -510,6 +512,7 @@ func createExpectedConfigForMergeableCafeIngress() version1.IngressNginxConfig { Upstream: coffeeUpstream, ProxyConnectTimeout: "60s", ProxyReadTimeout: "60s", + ProxySendTimeout: "60s", ClientMaxBodySize: "1m", ProxyBuffering: true, MinionIngress: &version1.Ingress{ @@ -526,6 +529,7 @@ func createExpectedConfigForMergeableCafeIngress() version1.IngressNginxConfig { Upstream: teaUpstream, ProxyConnectTimeout: "60s", ProxyReadTimeout: "60s", + ProxySendTimeout: "60s", ClientMaxBodySize: "1m", ProxyBuffering: true, MinionIngress: &version1.Ingress{ diff --git a/internal/configs/version1/config.go b/internal/configs/version1/config.go index 0d245f3eb5..ef45e40dca 100644 --- a/internal/configs/version1/config.go +++ b/internal/configs/version1/config.go @@ -105,6 +105,7 @@ type Location struct { Upstream Upstream ProxyConnectTimeout string ProxyReadTimeout string + ProxySendTimeout string ClientMaxBodySize string Websocket bool Rewrite string diff --git a/internal/configs/version2/config.go b/internal/configs/version2/config.go index a477abe6b8..9828adaa28 100644 --- a/internal/configs/version2/config.go +++ b/internal/configs/version2/config.go @@ -11,9 +11,12 @@ type VirtualServerConfig struct { // Upstream defines an upstream. type Upstream struct { - Name string - Servers []UpstreamServer - LBMethod string + Name string + Servers []UpstreamServer + LBMethod string + ProxyConnectTimeout string + ProxyReadTimeout string + ProxySendTimeout string } // UpstreamServer defines an upstream server. @@ -53,6 +56,7 @@ type Location struct { Snippets []string ProxyConnectTimeout string ProxyReadTimeout string + ProxySendTimeout string ClientMaxBodySize string ProxyMaxTempFileSize string ProxyBuffering bool diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 56a9deef52..96cd05f75f 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -35,7 +35,7 @@ server { listen 80{{ if $s.ProxyProtocol }} proxy_protocol{{ end }}; server_name {{ $s.ServerName }}; - + {{ with $ssl := $s.SSL }} listen 443 ssl{{ if $ssl.HTTP2 }} http2{{ end }}{{ if $s.ProxyProtocol }} proxy_protocol{{ end }}; @@ -45,11 +45,11 @@ server { {{ if $ssl.Ciphers }} ssl_ciphers {{ $ssl.Ciphers }}; {{ end }} - + {{ if $ssl.RedirectToHTTPS }} if ($scheme = http) { return 301 https://$host$request_uri; - } + } {{ end }} {{ end }} @@ -90,6 +90,7 @@ server { proxy_connect_timeout {{ $l.ProxyConnectTimeout }}; proxy_read_timeout {{ $l.ProxyReadTimeout }}; + proxy_send_timeout {{ $l.ProxySendTimeout }}; client_max_body_size {{ $l.ClientMaxBodySize }}; {{ if $l.ProxyMaxTempFileSize }} diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index 456f41a4d1..96cd05f75f 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -90,6 +90,7 @@ server { proxy_connect_timeout {{ $l.ProxyConnectTimeout }}; proxy_read_timeout {{ $l.ProxyReadTimeout }}; + proxy_send_timeout {{ $l.ProxySendTimeout }}; client_max_body_size {{ $l.ClientMaxBodySize }}; {{ if $l.ProxyMaxTempFileSize }} @@ -116,7 +117,7 @@ server { proxy_set_header X-Forwarded-Host $host; proxy_set_header X-Forwarded-Port $server_port; proxy_set_header X-Forwarded-Proto $scheme; - + proxy_pass {{ $l.ProxyPass }}; } {{ end }} diff --git a/internal/configs/version2/templates_test.go b/internal/configs/version2/templates_test.go index 256a11ddb2..9117baccf0 100644 --- a/internal/configs/version2/templates_test.go +++ b/internal/configs/version2/templates_test.go @@ -117,6 +117,7 @@ var virtualServerCfg = VirtualServerConfig{ Snippets: []string{"# location snippet"}, ProxyConnectTimeout: "30s", ProxyReadTimeout: "31s", + ProxySendTimeout: "32s", ClientMaxBodySize: "1m", ProxyBuffering: true, ProxyBuffers: "8 4k", @@ -128,6 +129,7 @@ var virtualServerCfg = VirtualServerConfig{ Path: "@loc0", ProxyConnectTimeout: "30s", ProxyReadTimeout: "31s", + ProxySendTimeout: "32s", ClientMaxBodySize: "1m", ProxyPass: "http://coffee-v1", }, @@ -135,6 +137,7 @@ var virtualServerCfg = VirtualServerConfig{ Path: "@loc1", ProxyConnectTimeout: "30s", ProxyReadTimeout: "31s", + ProxySendTimeout: "32s", ClientMaxBodySize: "1m", ProxyPass: "http://coffee-v2", }, @@ -142,6 +145,7 @@ var virtualServerCfg = VirtualServerConfig{ Path: "@match_loc_0", ProxyConnectTimeout: "30s", ProxyReadTimeout: "31s", + ProxySendTimeout: "32s", ClientMaxBodySize: "1m", ProxyPass: "http://coffee-v2", }, @@ -149,6 +153,7 @@ var virtualServerCfg = VirtualServerConfig{ Path: "@match_loc_default", ProxyConnectTimeout: "30s", ProxyReadTimeout: "31s", + ProxySendTimeout: "32s", ClientMaxBodySize: "1m", ProxyPass: "http://coffee-v1", }, diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 0b44832e6c..7d42421e9d 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -81,9 +81,22 @@ func (namer *variableNamer) GetNameForVariableForRulesRouteMainMap(rulesIndex in return fmt.Sprintf("$vs_%s_rules_%d", namer.safeNsName, rulesIndex) } +func getUpstream(upstreamName string, upstreams []conf_v1alpha1.Upstream) conf_v1alpha1.Upstream { + for _, u := range upstreams { + if u.Name == upstreamName { + return u + } + } + return conf_v1alpha1.Upstream{} +} + func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileName string, baseCfgParams *ConfigParams, isPlus bool) version2.VirtualServerConfig { ssl := generateSSLConfig(virtualServerEx.VirtualServer.Spec.TLS, tlsPemFileName, baseCfgParams) + // oUpstreams keeps track of conf_v1alpha1.Upstreams as they are created + // necessary to know what Location references which Upstream + var oUpstreams []conf_v1alpha1.Upstream + virtualServerUpstreamNamer := newUpstreamNamerForVirtualServer(virtualServerEx.VirtualServer) var upstreams []version2.Upstream @@ -94,6 +107,7 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam endpointsKey := GenerateEndpointsKey(virtualServerEx.VirtualServer.Namespace, u.Service, u.Port) ups := generateUpstream(upstreamName, u, virtualServerEx.Endpoints[endpointsKey], isPlus, baseCfgParams) upstreams = append(upstreams, ups) + oUpstreams = append(oUpstreams, u) } // generate upstreams for each VirtualServerRoute for _, vsr := range virtualServerEx.VirtualServerRoutes { @@ -103,6 +117,7 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam endpointsKey := GenerateEndpointsKey(vsr.Namespace, u.Service, u.Port) ups := generateUpstream(upstreamName, u, virtualServerEx.Endpoints[endpointsKey], isPlus, baseCfgParams) upstreams = append(upstreams, ups) + oUpstreams = append(oUpstreams, u) } } @@ -123,13 +138,13 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam } if len(r.Splits) > 0 { - splitCfg := generateSplitRouteConfig(r, virtualServerUpstreamNamer, variableNamer, len(splitClients), baseCfgParams) + splitCfg := generateSplitRouteConfig(r, virtualServerUpstreamNamer, oUpstreams, variableNamer, len(splitClients), baseCfgParams) splitClients = append(splitClients, splitCfg.SplitClient) locations = append(locations, splitCfg.Locations...) internalRedirectLocations = append(internalRedirectLocations, splitCfg.InternalRedirectLocation) } else if r.Rules != nil { - rulesRouteCfg := generateRulesRouteConfig(r, virtualServerUpstreamNamer, variableNamer, rulesRoutes, baseCfgParams) + rulesRouteCfg := generateRulesRouteConfig(r, virtualServerUpstreamNamer, oUpstreams, variableNamer, rulesRoutes, baseCfgParams) maps = append(maps, rulesRouteCfg.Maps...) locations = append(locations, rulesRouteCfg.Locations...) @@ -138,7 +153,8 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam rulesRoutes++ } else { upstreamName := virtualServerUpstreamNamer.GetNameForUpstream(r.Upstream) - loc := generateLocation(r.Path, upstreamName, baseCfgParams) + upstream := getUpstream(r.Upstream, oUpstreams) + loc := generateLocation(r.Path, upstreamName, upstream, baseCfgParams) locations = append(locations, loc) } } @@ -148,13 +164,13 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam upstreamNamer := newUpstreamNamerForVirtualServerRoute(virtualServerEx.VirtualServer, vsr) for _, r := range vsr.Spec.Subroutes { if len(r.Splits) > 0 { - splitCfg := generateSplitRouteConfig(r, upstreamNamer, variableNamer, len(splitClients), baseCfgParams) + splitCfg := generateSplitRouteConfig(r, upstreamNamer, oUpstreams, variableNamer, len(splitClients), baseCfgParams) splitClients = append(splitClients, splitCfg.SplitClient) locations = append(locations, splitCfg.Locations...) internalRedirectLocations = append(internalRedirectLocations, splitCfg.InternalRedirectLocation) } else if r.Rules != nil { - rulesRouteCfg := generateRulesRouteConfig(r, upstreamNamer, variableNamer, rulesRoutes, baseCfgParams) + rulesRouteCfg := generateRulesRouteConfig(r, upstreamNamer, oUpstreams, variableNamer, rulesRoutes, baseCfgParams) maps = append(maps, rulesRouteCfg.Maps...) locations = append(locations, rulesRouteCfg.Locations...) @@ -163,7 +179,8 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam rulesRoutes++ } else { upstreamName := upstreamNamer.GetNameForUpstream(r.Upstream) - loc := generateLocation(r.Path, upstreamName, baseCfgParams) + upstream := getUpstream(r.Upstream, oUpstreams) + loc := generateLocation(r.Path, upstreamName, upstream, baseCfgParams) locations = append(locations, loc) } } @@ -216,13 +233,11 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, endp upsServers = append(upsServers, s) } - ups := version2.Upstream{ + return version2.Upstream{ Name: upstreamName, Servers: upsServers, LBMethod: generateLBMethod(upstream.LBMethod, cfgParams.LBMethod), } - - return ups } func generateLBMethod(method string, defaultMethod string) string { @@ -231,7 +246,6 @@ func generateLBMethod(method string, defaultMethod string) string { } else if method == "round_robin" { return "" } - return method } @@ -249,12 +263,13 @@ func generatePositiveIntFromPointer(n *int, defaultN int) int { return *n } -func generateLocation(path string, upstreamName string, cfgParams *ConfigParams) version2.Location { - loc := version2.Location{ +func generateLocation(path string, upstreamName string, upstream conf_v1alpha1.Upstream, cfgParams *ConfigParams) version2.Location { + return version2.Location{ Path: path, Snippets: cfgParams.LocationSnippets, - ProxyConnectTimeout: cfgParams.ProxyConnectTimeout, - ProxyReadTimeout: cfgParams.ProxyReadTimeout, + ProxyConnectTimeout: generateTime(upstream.ProxyConnectTimeout, cfgParams.ProxyConnectTimeout), + ProxyReadTimeout: generateTime(upstream.ProxyReadTimeout, cfgParams.ProxyReadTimeout), + ProxySendTimeout: generateTime(upstream.ProxySendTimeout, cfgParams.ProxySendTimeout), ClientMaxBodySize: cfgParams.ClientMaxBodySize, ProxyMaxTempFileSize: cfgParams.ProxyMaxTempFileSize, ProxyBuffering: cfgParams.ProxyBuffering, @@ -262,7 +277,6 @@ func generateLocation(path string, upstreamName string, cfgParams *ConfigParams) ProxyBufferSize: cfgParams.ProxyBufferSize, ProxyPass: fmt.Sprintf("http://%v", upstreamName), } - return loc } type splitRouteCfg struct { @@ -271,7 +285,7 @@ type splitRouteCfg struct { InternalRedirectLocation version2.InternalRedirectLocation } -func generateSplitRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstreamNamer, variableNamer *variableNamer, index int, cfgParams *ConfigParams) splitRouteCfg { +func generateSplitRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstreamNamer, oUpstreams []conf_v1alpha1.Upstream, variableNamer *variableNamer, index int, cfgParams *ConfigParams) splitRouteCfg { splitClientVarName := variableNamer.GetNameForSplitClientVariable(index) // Generate a SplitClient @@ -297,7 +311,8 @@ func generateSplitRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstream for i, s := range route.Splits { path := fmt.Sprintf("@splits_%d_split_%d", index, i) upstreamName := upstreamNamer.GetNameForUpstream(s.Upstream) - loc := generateLocation(path, upstreamName, cfgParams) + upstream := getUpstream(upstreamName, oUpstreams) + loc := generateLocation(path, upstreamName, upstream, cfgParams) locations = append(locations, loc) } @@ -320,7 +335,7 @@ type rulesRouteCfg struct { InternalRedirectLocation version2.InternalRedirectLocation } -func generateRulesRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstreamNamer, variableNamer *variableNamer, index int, cfgParams *ConfigParams) rulesRouteCfg { +func generateRulesRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstreamNamer, oUpstreams []conf_v1alpha1.Upstream, variableNamer *variableNamer, index int, cfgParams *ConfigParams) rulesRouteCfg { // Generate maps var maps []version2.Map @@ -378,14 +393,17 @@ func generateRulesRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstream for i, m := range route.Rules.Matches { path := fmt.Sprintf("@rules_%d_match_%d", index, i) upstreamName := upstreamNamer.GetNameForUpstream(m.Upstream) - loc := generateLocation(path, upstreamName, cfgParams) + upstream := getUpstream(m.Upstream, oUpstreams) + loc := generateLocation(path, upstreamName, upstream, cfgParams) locations = append(locations, loc) } - // Generate defaultUpsteam location + // Generate defaultUpstream location path := fmt.Sprintf("@rules_%d_default", index) upstreamName := upstreamNamer.GetNameForUpstream(route.Rules.DefaultUpstream) - loc := generateLocation(path, upstreamName, cfgParams) + upstream := getUpstream(route.Rules.DefaultUpstream, oUpstreams) + + loc := generateLocation(path, upstreamName, upstream, cfgParams) locations = append(locations, loc) // Generate an InternalRedirectLocation to the location defined by the main map variable @@ -401,14 +419,14 @@ func generateRulesRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstream } } -var specialMapParameters = map[string]bool{ - "default": true, - "hostnames": true, - "include": true, - "volatile": true, -} - func generateValueForRulesRouteMap(matchedValue string) (value string, isNegative bool) { + var specialMapParameters = map[string]bool{ + "default": true, + "hostnames": true, + "include": true, + "volatile": true, + } + if len(matchedValue) == 0 { return `""`, false } @@ -521,11 +539,9 @@ func createUpstreamServersForPlus(virtualServerEx *VirtualServerEx) map[string][ } func createUpstreamServersConfig(baseCfgParams *ConfigParams) nginx.ServerConfig { - cfg := nginx.ServerConfig{ + return nginx.ServerConfig{ MaxFails: baseCfgParams.MaxFails, FailTimeout: baseCfgParams.FailTimeout, SlowStart: baseCfgParams.SlowStart, } - - return cfg } diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index c26b192dca..aac344db62 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -4,10 +4,8 @@ import ( "reflect" "testing" - "github.com/nginxinc/kubernetes-ingress/internal/nginx" - "github.com/nginxinc/kubernetes-ingress/internal/configs/version2" - + "github.com/nginxinc/kubernetes-ingress/internal/nginx" conf_v1alpha1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1alpha1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -803,6 +801,7 @@ func TestGenerateLocation(t *testing.T) { cfgParams := ConfigParams{ ProxyConnectTimeout: "30s", ProxyReadTimeout: "31s", + ProxySendTimeout: "32s", ClientMaxBodySize: "1m", ProxyMaxTempFileSize: "1024m", ProxyBuffering: true, @@ -818,6 +817,7 @@ func TestGenerateLocation(t *testing.T) { Snippets: []string{"# location snippet"}, ProxyConnectTimeout: "30s", ProxyReadTimeout: "31s", + ProxySendTimeout: "32s", ClientMaxBodySize: "1m", ProxyMaxTempFileSize: "1024m", ProxyBuffering: true, @@ -826,7 +826,7 @@ func TestGenerateLocation(t *testing.T) { ProxyPass: "http://test-upstream", } - result := generateLocation(path, upstreamName, &cfgParams) + result := generateLocation(path, upstreamName, conf_v1alpha1.Upstream{}, &cfgParams) if !reflect.DeepEqual(result, expected) { t.Errorf("generateLocation() returned %v but expected %v", result, expected) } @@ -1074,7 +1074,7 @@ func TestGenerateSplitRouteConfig(t *testing.T) { cfgParams := ConfigParams{} - result := generateSplitRouteConfig(route, upstreamNamer, variableNamer, index, &cfgParams) + result := generateSplitRouteConfig(route, upstreamNamer, []conf_v1alpha1.Upstream{}, variableNamer, index, &cfgParams) if !reflect.DeepEqual(result, expected) { t.Errorf("generateSplitRouteConfig() returned %v but expected %v", result, expected) } @@ -1286,7 +1286,7 @@ func TestGenerateRulesRouteConfig(t *testing.T) { cfgParams := ConfigParams{} - result := generateRulesRouteConfig(route, upstreamNamer, variableNamer, index, &cfgParams) + result := generateRulesRouteConfig(route, upstreamNamer, []conf_v1alpha1.Upstream{}, variableNamer, index, &cfgParams) if !reflect.DeepEqual(result, expected) { t.Errorf("generateRulesRouteConfig() returned \n%v but expected \n%v", result, expected) } diff --git a/pkg/apis/configuration/v1alpha1/types.go b/pkg/apis/configuration/v1alpha1/types.go index 2f9ba0820c..87c40a95f2 100644 --- a/pkg/apis/configuration/v1alpha1/types.go +++ b/pkg/apis/configuration/v1alpha1/types.go @@ -25,12 +25,15 @@ type VirtualServerSpec struct { // Upstream defines an upstream. type Upstream struct { - Name string `json:"name"` - Service string `json:"service"` - Port uint16 `json:"port"` - LBMethod string `json:"lb-method"` - FailTimeout string `json:"fail-timeout"` - MaxFails *int `json:"max-fails"` + Name string `json:"name"` + Service string `json:"service"` + Port uint16 `json:"port"` + LBMethod string `json:"lb-method"` + FailTimeout string `json:"fail-timeout"` + MaxFails *int `json:"max-fails"` + ProxyConnectTimeout string `json:"connect-timeout"` + ProxyReadTimeout string `json:"read-timeout"` + ProxySendTimeout string `json:"send-timeout"` } // Route defines a route. diff --git a/pkg/apis/configuration/validation/validation.go b/pkg/apis/configuration/validation/validation.go index ba40f5792b..51dc6daefe 100644 --- a/pkg/apis/configuration/validation/validation.go +++ b/pkg/apis/configuration/validation/validation.go @@ -138,6 +138,11 @@ func validateUpstreams(upstreams []v1alpha1.Upstream, fieldPath *field.Path, isP } allErrs = append(allErrs, validateServiceName(u.Service, idxPath.Child("service"))...) + + allErrs = append(allErrs, validateTime(u.ProxyConnectTimeout, idxPath.Child("connect-timeout"))...) + allErrs = append(allErrs, validateTime(u.ProxyReadTimeout, idxPath.Child("read-timeout"))...) + allErrs = append(allErrs, validateTime(u.ProxySendTimeout, idxPath.Child("send-timeout"))...) + allErrs = append(allErrs, validateUpstreamLBMethod(u.LBMethod, idxPath.Child("lb-method"), isPlus)...) allErrs = append(allErrs, validateTime(u.FailTimeout, idxPath.Child("fail-timeout"))...) allErrs = append(allErrs, validatePositiveIntOrZero(u.MaxFails, idxPath.Child("max-fails"))...) diff --git a/pkg/apis/configuration/validation/validation_test.go b/pkg/apis/configuration/validation/validation_test.go index 35ff81c893..3cbea5f319 100644 --- a/pkg/apis/configuration/validation/validation_test.go +++ b/pkg/apis/configuration/validation/validation_test.go @@ -3,10 +3,9 @@ package validation import ( "testing" - "k8s.io/apimachinery/pkg/util/sets" - "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1alpha1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" ) @@ -1160,7 +1159,7 @@ func TestIsValidMatchValue(t *testing.T) { validValues := []string{ "abc", "123", - `\" + `\" abc\"`, `\"`, } From 1449fe5f694bb19b479f4b4e4e9706512f7ce4bc Mon Sep 17 00:00:00 2001 From: Dean Coakley Date: Thu, 4 Jul 2019 11:53:46 +0100 Subject: [PATCH 2/4] Implement PR feedback --- Makefile | 4 +- docs/virtualserver-and-virtualserverroute.md | 6 +- internal/configs/ingress.go | 1 - internal/configs/ingress_test.go | 6 +- internal/configs/version1/config.go | 1 - internal/configs/version2/config.go | 9 +-- internal/configs/virtualserver.go | 56 ++++++++----------- internal/configs/virtualserver_test.go | 4 +- .../validation/validation_test.go | 3 +- 9 files changed, 37 insertions(+), 53 deletions(-) diff --git a/Makefile b/Makefile index 96af249050..599c992575 100644 --- a/Makefile +++ b/Makefile @@ -29,9 +29,9 @@ lint: test: ifeq ($(BUILD_IN_CONTAINER),1) - $(DOCKER_RUN) $(GOLANG_CONTAINER) go test -race ./... + $(DOCKER_RUN) $(GOLANG_CONTAINER) go test ./... else - go test -race ./... + go test ./... endif certificate-and-key: diff --git a/docs/virtualserver-and-virtualserverroute.md b/docs/virtualserver-and-virtualserverroute.md index b9c9e35077..cf7a9dc8f3 100644 --- a/docs/virtualserver-and-virtualserverroute.md +++ b/docs/virtualserver-and-virtualserverroute.md @@ -192,9 +192,9 @@ send-timeout: 30s | `lb-method` | The load [balancing method](https://docs.nginx.com/nginx/admin-guide/load-balancer/http-load-balancer/#choosing-a-load-balancing-method). To use the round-robin method, specify `round_robin`. The default is specified in the `lb-method` ConfigMap key. | `string` | No | | `fail-timeout` | The time during which the specified number of unsuccessful attempts to communicate with an upstream server should happen to consider the server unavailable. See the [fail_timeout](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#fail_timeout) parameter of the server directive. The default is set in the `fail-timeout` ConfigMap key. | `string` | No | | `max-fails` | The number of unsuccessful attempts to communicate with an upstream server that should happen in the duration set by the `fail-timeout` to consider the server unavailable. See the [max_fails](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#max_fails) parameter of the server directive. The default is set in the `max-fails` ConfgMap key. | `int` | No | -`connect-timeout` | The timeout for establishing a connection with an upstream server. See the [proxy_connect_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout) directive. The default is specified in the `proxy-connect-timeout` ConfigMap key. | `string` | No -`read-timeout` | The timeout for reading a response from an upstream server. See the [proxy_read_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout) directive. The default is specified in the `proxy-read-timeout` ConfigMap key. | `string` | No -`send-timeout` | The timeout for transmitting a request to an upstream server. See the [proxy_send_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout) directive. The default is 60s. | `string` | No +`connect-timeout` | The timeout for establishing a connection with an upstream server. See the [proxy_connect_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout) directive. The default is specified in the `proxy-connect-timeout` ConfigMap key. | `string` | No +`read-timeout` | The timeout for reading a response from an upstream server. See the [proxy_read_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout) directive. The default is specified in the `proxy-read-timeout` ConfigMap key. | `string` | No +`send-timeout` | The timeout for transmitting a request to an upstream server. See the [proxy_send_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout) directive. The default is `60s`. | `string` | No ### Split diff --git a/internal/configs/ingress.go b/internal/configs/ingress.go index 4274448113..a87ad5b2fd 100644 --- a/internal/configs/ingress.go +++ b/internal/configs/ingress.go @@ -235,7 +235,6 @@ func createLocation(path string, upstream version1.Upstream, cfg *ConfigParams, Upstream: upstream, ProxyConnectTimeout: cfg.ProxyConnectTimeout, ProxyReadTimeout: cfg.ProxyReadTimeout, - ProxySendTimeout: cfg.ProxySendTimeout, ClientMaxBodySize: cfg.ClientMaxBodySize, Websocket: websocket, Rewrite: rewrite, diff --git a/internal/configs/ingress_test.go b/internal/configs/ingress_test.go index ac957be713..cc1b415434 100644 --- a/internal/configs/ingress_test.go +++ b/internal/configs/ingress_test.go @@ -5,7 +5,7 @@ import ( "testing" "github.com/nginxinc/kubernetes-ingress/internal/configs/version1" - v1 "k8s.io/api/core/v1" + "k8s.io/api/core/v1" "k8s.io/api/extensions/v1beta1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -161,7 +161,6 @@ func createExpectedConfigForCafeIngressEx() version1.IngressNginxConfig { Upstream: coffeeUpstream, ProxyConnectTimeout: "60s", ProxyReadTimeout: "60s", - ProxySendTimeout: "60s", ClientMaxBodySize: "1m", ProxyBuffering: true, }, @@ -170,7 +169,6 @@ func createExpectedConfigForCafeIngressEx() version1.IngressNginxConfig { Upstream: teaUpstream, ProxyConnectTimeout: "60s", ProxyReadTimeout: "60s", - ProxySendTimeout: "60s", ClientMaxBodySize: "1m", ProxyBuffering: true, }, @@ -512,7 +510,6 @@ func createExpectedConfigForMergeableCafeIngress() version1.IngressNginxConfig { Upstream: coffeeUpstream, ProxyConnectTimeout: "60s", ProxyReadTimeout: "60s", - ProxySendTimeout: "60s", ClientMaxBodySize: "1m", ProxyBuffering: true, MinionIngress: &version1.Ingress{ @@ -529,7 +526,6 @@ func createExpectedConfigForMergeableCafeIngress() version1.IngressNginxConfig { Upstream: teaUpstream, ProxyConnectTimeout: "60s", ProxyReadTimeout: "60s", - ProxySendTimeout: "60s", ClientMaxBodySize: "1m", ProxyBuffering: true, MinionIngress: &version1.Ingress{ diff --git a/internal/configs/version1/config.go b/internal/configs/version1/config.go index ef45e40dca..0d245f3eb5 100644 --- a/internal/configs/version1/config.go +++ b/internal/configs/version1/config.go @@ -105,7 +105,6 @@ type Location struct { Upstream Upstream ProxyConnectTimeout string ProxyReadTimeout string - ProxySendTimeout string ClientMaxBodySize string Websocket bool Rewrite string diff --git a/internal/configs/version2/config.go b/internal/configs/version2/config.go index 9828adaa28..5ff90ad681 100644 --- a/internal/configs/version2/config.go +++ b/internal/configs/version2/config.go @@ -11,12 +11,9 @@ type VirtualServerConfig struct { // Upstream defines an upstream. type Upstream struct { - Name string - Servers []UpstreamServer - LBMethod string - ProxyConnectTimeout string - ProxyReadTimeout string - ProxySendTimeout string + Name string + Servers []UpstreamServer + LBMethod string } // UpstreamServer defines an upstream server. diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 7d42421e9d..322a147df1 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -81,21 +81,12 @@ func (namer *variableNamer) GetNameForVariableForRulesRouteMainMap(rulesIndex in return fmt.Sprintf("$vs_%s_rules_%d", namer.safeNsName, rulesIndex) } -func getUpstream(upstreamName string, upstreams []conf_v1alpha1.Upstream) conf_v1alpha1.Upstream { - for _, u := range upstreams { - if u.Name == upstreamName { - return u - } - } - return conf_v1alpha1.Upstream{} -} - func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileName string, baseCfgParams *ConfigParams, isPlus bool) version2.VirtualServerConfig { ssl := generateSSLConfig(virtualServerEx.VirtualServer.Spec.TLS, tlsPemFileName, baseCfgParams) - // oUpstreams keeps track of conf_v1alpha1.Upstreams as they are created + // crUpstreams keeps track of conf_v1alpha1.Upstreams as they are created // necessary to know what Location references which Upstream - var oUpstreams []conf_v1alpha1.Upstream + crUpstreams := make(map[string]conf_v1alpha1.Upstream) virtualServerUpstreamNamer := newUpstreamNamerForVirtualServer(virtualServerEx.VirtualServer) @@ -107,7 +98,7 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam endpointsKey := GenerateEndpointsKey(virtualServerEx.VirtualServer.Namespace, u.Service, u.Port) ups := generateUpstream(upstreamName, u, virtualServerEx.Endpoints[endpointsKey], isPlus, baseCfgParams) upstreams = append(upstreams, ups) - oUpstreams = append(oUpstreams, u) + crUpstreams[upstreamName] = u } // generate upstreams for each VirtualServerRoute for _, vsr := range virtualServerEx.VirtualServerRoutes { @@ -117,7 +108,7 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam endpointsKey := GenerateEndpointsKey(vsr.Namespace, u.Service, u.Port) ups := generateUpstream(upstreamName, u, virtualServerEx.Endpoints[endpointsKey], isPlus, baseCfgParams) upstreams = append(upstreams, ups) - oUpstreams = append(oUpstreams, u) + crUpstreams[upstreamName] = u } } @@ -138,13 +129,13 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam } if len(r.Splits) > 0 { - splitCfg := generateSplitRouteConfig(r, virtualServerUpstreamNamer, oUpstreams, variableNamer, len(splitClients), baseCfgParams) + splitCfg := generateSplitRouteConfig(r, virtualServerUpstreamNamer, crUpstreams, variableNamer, len(splitClients), baseCfgParams) splitClients = append(splitClients, splitCfg.SplitClient) locations = append(locations, splitCfg.Locations...) internalRedirectLocations = append(internalRedirectLocations, splitCfg.InternalRedirectLocation) } else if r.Rules != nil { - rulesRouteCfg := generateRulesRouteConfig(r, virtualServerUpstreamNamer, oUpstreams, variableNamer, rulesRoutes, baseCfgParams) + rulesRouteCfg := generateRulesRouteConfig(r, virtualServerUpstreamNamer, crUpstreams, variableNamer, rulesRoutes, baseCfgParams) maps = append(maps, rulesRouteCfg.Maps...) locations = append(locations, rulesRouteCfg.Locations...) @@ -153,10 +144,11 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam rulesRoutes++ } else { upstreamName := virtualServerUpstreamNamer.GetNameForUpstream(r.Upstream) - upstream := getUpstream(r.Upstream, oUpstreams) + upstream := crUpstreams[upstreamName] loc := generateLocation(r.Path, upstreamName, upstream, baseCfgParams) locations = append(locations, loc) } + } // generate config for subroutes of each VirtualServerRoute @@ -164,13 +156,13 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam upstreamNamer := newUpstreamNamerForVirtualServerRoute(virtualServerEx.VirtualServer, vsr) for _, r := range vsr.Spec.Subroutes { if len(r.Splits) > 0 { - splitCfg := generateSplitRouteConfig(r, upstreamNamer, oUpstreams, variableNamer, len(splitClients), baseCfgParams) + splitCfg := generateSplitRouteConfig(r, upstreamNamer, crUpstreams, variableNamer, len(splitClients), baseCfgParams) splitClients = append(splitClients, splitCfg.SplitClient) locations = append(locations, splitCfg.Locations...) internalRedirectLocations = append(internalRedirectLocations, splitCfg.InternalRedirectLocation) } else if r.Rules != nil { - rulesRouteCfg := generateRulesRouteConfig(r, upstreamNamer, oUpstreams, variableNamer, rulesRoutes, baseCfgParams) + rulesRouteCfg := generateRulesRouteConfig(r, upstreamNamer, crUpstreams, variableNamer, rulesRoutes, baseCfgParams) maps = append(maps, rulesRouteCfg.Maps...) locations = append(locations, rulesRouteCfg.Locations...) @@ -179,7 +171,7 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam rulesRoutes++ } else { upstreamName := upstreamNamer.GetNameForUpstream(r.Upstream) - upstream := getUpstream(r.Upstream, oUpstreams) + upstream := crUpstreams[upstreamName] loc := generateLocation(r.Path, upstreamName, upstream, baseCfgParams) locations = append(locations, loc) } @@ -285,7 +277,7 @@ type splitRouteCfg struct { InternalRedirectLocation version2.InternalRedirectLocation } -func generateSplitRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstreamNamer, oUpstreams []conf_v1alpha1.Upstream, variableNamer *variableNamer, index int, cfgParams *ConfigParams) splitRouteCfg { +func generateSplitRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstreamNamer, crUpstreams map[string]conf_v1alpha1.Upstream, variableNamer *variableNamer, index int, cfgParams *ConfigParams) splitRouteCfg { splitClientVarName := variableNamer.GetNameForSplitClientVariable(index) // Generate a SplitClient @@ -311,7 +303,7 @@ func generateSplitRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstream for i, s := range route.Splits { path := fmt.Sprintf("@splits_%d_split_%d", index, i) upstreamName := upstreamNamer.GetNameForUpstream(s.Upstream) - upstream := getUpstream(upstreamName, oUpstreams) + upstream := crUpstreams[upstreamName] loc := generateLocation(path, upstreamName, upstream, cfgParams) locations = append(locations, loc) } @@ -335,7 +327,8 @@ type rulesRouteCfg struct { InternalRedirectLocation version2.InternalRedirectLocation } -func generateRulesRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstreamNamer, oUpstreams []conf_v1alpha1.Upstream, variableNamer *variableNamer, index int, cfgParams *ConfigParams) rulesRouteCfg { +func generateRulesRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstreamNamer, crUpstreams map[string]conf_v1alpha1.Upstream, + variableNamer *variableNamer, index int, cfgParams *ConfigParams) rulesRouteCfg { // Generate maps var maps []version2.Map @@ -393,7 +386,7 @@ func generateRulesRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstream for i, m := range route.Rules.Matches { path := fmt.Sprintf("@rules_%d_match_%d", index, i) upstreamName := upstreamNamer.GetNameForUpstream(m.Upstream) - upstream := getUpstream(m.Upstream, oUpstreams) + upstream := crUpstreams[upstreamName] loc := generateLocation(path, upstreamName, upstream, cfgParams) locations = append(locations, loc) } @@ -401,8 +394,7 @@ func generateRulesRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstream // Generate defaultUpstream location path := fmt.Sprintf("@rules_%d_default", index) upstreamName := upstreamNamer.GetNameForUpstream(route.Rules.DefaultUpstream) - upstream := getUpstream(route.Rules.DefaultUpstream, oUpstreams) - + upstream := crUpstreams[upstreamName] loc := generateLocation(path, upstreamName, upstream, cfgParams) locations = append(locations, loc) @@ -419,14 +411,14 @@ func generateRulesRouteConfig(route conf_v1alpha1.Route, upstreamNamer *upstream } } -func generateValueForRulesRouteMap(matchedValue string) (value string, isNegative bool) { - var specialMapParameters = map[string]bool{ - "default": true, - "hostnames": true, - "include": true, - "volatile": true, - } +var specialMapParameters = map[string]bool{ + "default": true, + "hostnames": true, + "include": true, + "volatile": true, +} +func generateValueForRulesRouteMap(matchedValue string) (value string, isNegative bool) { if len(matchedValue) == 0 { return `""`, false } diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index aac344db62..cdb3e10ef1 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -1074,7 +1074,7 @@ func TestGenerateSplitRouteConfig(t *testing.T) { cfgParams := ConfigParams{} - result := generateSplitRouteConfig(route, upstreamNamer, []conf_v1alpha1.Upstream{}, variableNamer, index, &cfgParams) + result := generateSplitRouteConfig(route, upstreamNamer, map[string]conf_v1alpha1.Upstream{}, variableNamer, index, &cfgParams) if !reflect.DeepEqual(result, expected) { t.Errorf("generateSplitRouteConfig() returned %v but expected %v", result, expected) } @@ -1286,7 +1286,7 @@ func TestGenerateRulesRouteConfig(t *testing.T) { cfgParams := ConfigParams{} - result := generateRulesRouteConfig(route, upstreamNamer, []conf_v1alpha1.Upstream{}, variableNamer, index, &cfgParams) + result := generateRulesRouteConfig(route, upstreamNamer, map[string]conf_v1alpha1.Upstream{}, variableNamer, index, &cfgParams) if !reflect.DeepEqual(result, expected) { t.Errorf("generateRulesRouteConfig() returned \n%v but expected \n%v", result, expected) } diff --git a/pkg/apis/configuration/validation/validation_test.go b/pkg/apis/configuration/validation/validation_test.go index 3cbea5f319..9505ff34ab 100644 --- a/pkg/apis/configuration/validation/validation_test.go +++ b/pkg/apis/configuration/validation/validation_test.go @@ -3,9 +3,10 @@ package validation import ( "testing" + "k8s.io/apimachinery/pkg/util/sets" + "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1alpha1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" ) From 03e2089f2a3322f9392c957bace565ef4b44749a Mon Sep 17 00:00:00 2001 From: Dean Coakley Date: Thu, 4 Jul 2019 12:00:44 +0100 Subject: [PATCH 3/4] Fix comment to be consistent. Unstage validation test --- internal/configs/virtualserver.go | 4 ++-- pkg/apis/configuration/validation/validation_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 322a147df1..9a97bf5857 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -84,8 +84,8 @@ func (namer *variableNamer) GetNameForVariableForRulesRouteMainMap(rulesIndex in func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileName string, baseCfgParams *ConfigParams, isPlus bool) version2.VirtualServerConfig { ssl := generateSSLConfig(virtualServerEx.VirtualServer.Spec.TLS, tlsPemFileName, baseCfgParams) - // crUpstreams keeps track of conf_v1alpha1.Upstreams as they are created - // necessary to know what Location references which Upstream + // crUpstreams maps an UpstreamName to it's conf_v1alpha1.Upstreams as they are created + // necessary for generateLocation to know what Upstream each Location references crUpstreams := make(map[string]conf_v1alpha1.Upstream) virtualServerUpstreamNamer := newUpstreamNamerForVirtualServer(virtualServerEx.VirtualServer) diff --git a/pkg/apis/configuration/validation/validation_test.go b/pkg/apis/configuration/validation/validation_test.go index 9505ff34ab..35ff81c893 100644 --- a/pkg/apis/configuration/validation/validation_test.go +++ b/pkg/apis/configuration/validation/validation_test.go @@ -1160,7 +1160,7 @@ func TestIsValidMatchValue(t *testing.T) { validValues := []string{ "abc", "123", - `\" + `\" abc\"`, `\"`, } From 6c274c8a0d8c17160a9cbeab16059c68c0a45251 Mon Sep 17 00:00:00 2001 From: Dean Coakley Date: Thu, 4 Jul 2019 12:03:41 +0100 Subject: [PATCH 4/4] Typo --- internal/configs/virtualserver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 9a97bf5857..1209958850 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -84,7 +84,7 @@ func (namer *variableNamer) GetNameForVariableForRulesRouteMainMap(rulesIndex in func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileName string, baseCfgParams *ConfigParams, isPlus bool) version2.VirtualServerConfig { ssl := generateSSLConfig(virtualServerEx.VirtualServer.Spec.TLS, tlsPemFileName, baseCfgParams) - // crUpstreams maps an UpstreamName to it's conf_v1alpha1.Upstreams as they are created + // crUpstreams maps an UpstreamName to its conf_v1alpha1.Upstream as they are generated // necessary for generateLocation to know what Upstream each Location references crUpstreams := make(map[string]conf_v1alpha1.Upstream)