Merge pull request #274 from Dean-Coakley/lb-method

Change the default load balancing method to least_conn

Author: pleshakov

examples/customization/README.md

@@ -39,7 +39,7 @@ The table below summarizes all of the options. For some of them, there are examp
 | N/A | `http-snippets` | Sets a custom snippet in http context. | N/A | |
 | `nginx.org/location-snippets` | `location-snippets` | Sets a custom snippet in location context. | N/A | |
 | `nginx.org/server-snippets` | `server-snippets` | Sets a custom snippet in server context. | N/A | |
-| `nginx.org/lb-method` | `lb-method` | Sets the [load balancing method](https://www.nginx.com/resources/admin-guide/load-balancer/#method). The default `""` specifies the round-robin method. | `""` | |
+| `nginx.org/lb-method` | `lb-method` | Sets 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"`. | `"least_conn"` | |
 | `nginx.org/listen-ports` | N/A | Configures HTTP ports that NGINX will listen on. | `[80]` | |
 | `nginx.org/listen-ports-ssl` | N/A | Configures HTTPS ports that NGINX will listen on. | `[443]` | |
 | N/A | `worker-processes` | Sets the value of the [worker_processes](http://nginx.org/en/docs/ngx_core_module.html#worker_processes) directive. | `auto` | |
@@ -64,7 +64,7 @@ The table below summarizes all of the options. For some of them, there are examp
 
 1. Make sure that you specify the configmaps resource to use when you start an Ingress controller.
 For example, `-nginx-configmaps=default/nginx-config`, where we specify
-the config map to use with the following format: `<namespace>/<name>`. 
+the config map to use with the following format: `<namespace>/<name>`.
 
 1. Create a configmaps file with the name *nginx-config.yaml* and set the values
 that make sense for your setup:

examples/customization/nginx-config.yaml

@@ -45,6 +45,7 @@ data:
     if ($new_uri) {
         rewrite ^ $new_uri permanent;
     }
+  lb-method: "round_robin" # default is least_conn. Sets the load balancing method for upstreams. See https://docs.nginx.com/nginx/admin-guide/load-balancer/http-load-balancer/#choosing-a-load-balancing-method
   location-snippets: | # No default. Pipe is used for multiple line snippets. Make sure the snippet is not a default value, in order to avoid duplication.
     proxy_temp_path  /var/nginx/proxy_temp;
     charset  koi8-r;

nginx-controller/controller/controller.go

@@ -34,10 +34,11 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
 
+	"sort"
+
 	api_v1 "k8s.io/api/core/v1"
 	extensions "k8s.io/api/extensions/v1beta1"
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"sort"
 )
 
 const (
@@ -449,7 +450,19 @@ func (lbc *LoadBalancerController) syncCfgm(task Task) {
 		}
 
 		if lbMethod, exists := cfgm.Data["lb-method"]; exists {
-			cfg.LBMethod = lbMethod
+			if lbc.nginxPlus {
+				if parsedMethod, err := nginx.ParseLBMethodForPlus(lbMethod); err != nil {
+					glog.Errorf("Configmap %s/%s: Invalid value for the lb-method key: got %q: %v", cfgm.GetNamespace(), cfgm.GetName(), lbMethod, err)
+				} else {
+					cfg.LBMethod = parsedMethod
+				}
+			} else {
+				if parsedMethod, err := nginx.ParseLBMethod(lbMethod); err != nil {
+					glog.Errorf("Configmap %s/%s: Invalid value for the lb-method key: got %q: %v", cfgm.GetNamespace(), cfgm.GetName(), lbMethod, err)
+				} else {
+					cfg.LBMethod = parsedMethod
+				}
+			}
 		}
 
 		if proxyConnectTimeout, exists := cfgm.Data["proxy-connect-timeout"]; exists {

nginx-controller/nginx/config.go

@@ -73,5 +73,6 @@ func NewDefaultConfig() *Config {
 		SSLPorts:                   []int{443},
 		MaxFails:                   1,
 		FailTimeout:                "10s",
+		LBMethod:                   "least_conn",
 	}
 }

nginx-controller/nginx/configurator.go

@@ -280,7 +280,19 @@ func (cnf *Configurator) createConfig(ingEx *IngressEx) Config {
 
 	//Override from annotation
 	if lbMethod, exists := ingEx.Ingress.Annotations["nginx.org/lb-method"]; exists {
-		ingCfg.LBMethod = lbMethod
+		if cnf.isPlus() {
+			if parsedMethod, err := ParseLBMethodForPlus(lbMethod); err != nil {
+				glog.Errorf("Ingress %s/%s: Invalid value for the nginx.org/lb-method: got %q: %v", ingEx.Ingress.GetNamespace(), ingEx.Ingress.GetName(), lbMethod, err)
+			} else {
+				ingCfg.LBMethod = parsedMethod
+			}
+		} else {
+			if parsedMethod, err := ParseLBMethod(lbMethod); err != nil {
+				glog.Errorf("Ingress %s/%s: Invalid value for the nginx.org/lb-method: got %q: %v", ingEx.Ingress.GetNamespace(), ingEx.Ingress.GetName(), lbMethod, err)
+			} else {
+				ingCfg.LBMethod = parsedMethod
+			}
+		}
 	}
 
 	if serverTokens, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/server-tokens", ingEx.Ingress); exists {

nginx-controller/nginx/extensions.go

@@ -0,0 +1,61 @@
+package nginx
+
+import (
+	"fmt"
+	"strings"
+)
+
+// ParseLBMethod parses method and matches it to a corresponding load balancing method in NGINX. An error is returned if method is not valid
+func ParseLBMethod(method string) (string, error) {
+	method = strings.TrimSpace(method)
+	if method == "round_robin" {
+		return "", nil
+	}
+	if strings.HasPrefix(method, "hash") {
+		method, err := validateHashLBMethod(method)
+		return method, err
+	}
+
+	if method == "least_conn" || method == "ip_hash" {
+		return method, nil
+	}
+	return "", fmt.Errorf("Invalid load balancing method: %q", method)
+}
+
+var nginxPlusLBValidInput = map[string]bool{
+	"least_time":                    true,
+	"last_byte":                     true,
+	"least_conn":                    true,
+	"ip_hash":                       true,
+	"least_time header":             true,
+	"least_time last_byte":          true,
+	"least_time header inflight":    true,
+	"least_time last_byte inflight": true,
+}
+
+// ParseLBMethodForPlus parses method and matches it to a corresponding load balancing method in NGINX Plus. An error is returned if method is not valid
+func ParseLBMethodForPlus(method string) (string, error) {
+	method = strings.TrimSpace(method)
+	if method == "round_robin" {
+		return "", nil
+	}
+	if strings.HasPrefix(method, "hash") {
+		method, err := validateHashLBMethod(method)
+		return method, err
+	}
+
+	if _, exists := nginxPlusLBValidInput[method]; exists {
+		return method, nil
+	}
+	return "", fmt.Errorf("Invalid load balancing method: %q", method)
+}
+
+func validateHashLBMethod(method string) (string, error) {
+	keyWords := strings.Split(method, " ")
+	if keyWords[0] == "hash" {
+		if len(keyWords) == 2 || len(keyWords) == 3 && keyWords[2] == "consistent" {
+			return method, nil
+		}
+	}
+	return "", fmt.Errorf("Invalid load balancing method: %q", method)
+}

nginx-controller/nginx/extensions_test.go

@@ -0,0 +1,83 @@
+package nginx
+
+import "testing"
+
+func TestParseLBMethod(t *testing.T) {
+	var testsWithValidInput = []struct {
+		input    string
+		expected string
+	}{
+		{"least_conn", "least_conn"},
+		{"round_robin", ""},
+		{"ip_hash", "ip_hash"},
+		{"hash $request_id", "hash $request_id"},
+		{"hash $request_id consistent", "hash $request_id consistent"},
+	}
+
+	var invalidInput = []string{
+		"",
+		"blabla",
+		"least_time header",
+		"hash123",
+		"hash $request_id conwrongspelling",
+	}
+
+	for _, test := range testsWithValidInput {
+		result, err := ParseLBMethod(test.input)
+		if err != nil {
+			t.Errorf("TestParseLBMethod(%q) returned an error for valid input", test.input)
+		}
+
+		if result != test.expected {
+			t.Errorf("TestParseLBMethod(%q) returned %q expected %q", test.input, result, test.expected)
+		}
+	}
+
+	for _, input := range invalidInput {
+		_, err := ParseLBMethod(input)
+		if err == nil {
+			t.Errorf("TestParseLBMethod(%q) does not return an error for invalid input", input)
+		}
+	}
+}
+
+func TestParseLBMethodForPlus(t *testing.T) {
+	var testsWithValidInput = []struct {
+		input    string
+		expected string
+	}{
+		{"least_conn", "least_conn"},
+		{"round_robin", ""},
+		{"ip_hash", "ip_hash"},
+		{"hash $request_id", "hash $request_id"},
+		{"least_time header", "least_time header"},
+		{"least_time last_byte", "least_time last_byte"},
+		{"least_time header inflight", "least_time header inflight"},
+		{"least_time last_byte inflight", "least_time last_byte inflight"},
+	}
+
+	var invalidInput = []string{
+		"",
+		"blabla",
+		"hash123",
+		"least_time inflight header",
+	}
+
+	for _, test := range testsWithValidInput {
+		result, err := ParseLBMethodForPlus(test.input)
+		if err != nil {
+			t.Errorf("TestParseLBMethod(%q) returned an error for valid input", test.input)
+		}
+
+		if result != test.expected {
+			t.Errorf("TestParseLBMethod(%q) returned %q expected %q", test.input, result, test.expected)
+		}
+	}
+
+	for _, input := range invalidInput {
+		_, err := ParseLBMethodForPlus(input)
+		if err == nil {
+			t.Errorf("TestParseLBMethod(%q) does not return an error for invalid input", input)
+		}
+	}
+}