allow kb granularity for lua shared dicts (#6750)

Update internal/ingress/controller/template/configmap.go

Co-authored-by: Ricardo Katz <rikatz@users.noreply.github.com>

Co-authored-by: Ricardo Katz <rikatz@users.noreply.github.com>

docs/user-guide/nginx-configuration/configmap.md

@@ -1046,6 +1046,12 @@ For example following will set default `certificate_data` dictionary to `100M` a
 lua-shared-dicts: "certificate_data: 100, my_custom_plugin: 5"
 ```
 
+You can optionally set a size unit to allow for kilobyte-granularity. Allowed units are 'm' or 'k' (case-insensitive), and it defaults to MB if no unit is provided. Here is a similar example, but the `my_custom_plugin` dict is only 512KB.
+
+```
+lua-shared-dicts: "certificate_data: 100, my_custom_plugin: 512k"
+```
+
 _References:_
 [http://nginx.org/en/docs/http/ngx_http_core_module.html#limit_rate_after](http://nginx.org/en/docs/http/ngx_http_core_module.html#limit_rate_after)
 

internal/ingress/controller/nginx.go

@@ -229,7 +229,7 @@ type NGINXController struct {
 	// runningConfig contains the running configuration in the Backend
 	runningConfig *ingress.Configuration
 
-	t ngx_template.TemplateWriter
+	t ngx_template.Writer
 
 	resolver []net.IP
 

internal/ingress/controller/template/configmap.go

@@ -19,6 +19,7 @@ package template
 import (
 	"fmt"
 	"net"
+	"regexp"
 	"strconv"
 	"strings"
 	"time"
@@ -67,21 +68,22 @@ const (
 
 var (
 	validRedirectCodes    = sets.NewInt([]int{301, 302, 307, 308}...)
+	dictSizeRegex         = regexp.MustCompile(`^(\d+)([kKmM])?$`)
 	defaultLuaSharedDicts = map[string]int{
-		"configuration_data":            20,
+		"configuration_data":            20480,
-		"certificate_data":              20,
+		"certificate_data":              20480,
-		"balancer_ewma":                 10,
+		"balancer_ewma":                 10240,
-		"balancer_ewma_last_touched_at": 10,
+		"balancer_ewma_last_touched_at": 10240,
-		"balancer_ewma_locks":           1,
+		"balancer_ewma_locks":           1024,
-		"certificate_servers":           5,
+		"certificate_servers":           5120,
-		"ocsp_response_cache":           5, // keep this same as certificate_servers
+		"ocsp_response_cache":           5120, // keep this same as certificate_servers
-		"global_throttle_cache":         10,
+		"global_throttle_cache":         10240,
 	}
 	defaultGlobalAuthRedirectParam = "rd"
 )
 
 const (
-	maxAllowedLuaDictSize = 200
+	maxAllowedLuaDictSize = 204800
 	maxNumberOfLuaDicts   = 100
 )
 
@@ -117,18 +119,18 @@ func ReadConfig(src map[string]string) config.Configuration {
 			v = strings.Replace(v, " ", "", -1)
 			results := strings.SplitN(v, ":", 2)
 			dictName := results[0]
-			size, err := strconv.Atoi(results[1])
+			size := dictStrToKb(results[1])
-			if err != nil {
+			if size < 0 {
-				klog.Errorf("Ignoring non integer value %v for Lua dictionary %v: %v.", results[1], dictName, err)
+				klog.Errorf("Ignoring poorly formatted value %v for Lua dictionary %v", results[1], dictName)
 				continue
 			}
 			if size > maxAllowedLuaDictSize {
-				klog.Errorf("Ignoring %v for Lua dictionary %v: maximum size is %v.", size, dictName, maxAllowedLuaDictSize)
+				klog.Errorf("Ignoring %v for Lua dictionary %v: maximum size is %vk.", results[1], dictName, maxAllowedLuaDictSize)
 				continue
 			}
 			if len(luaSharedDicts)+1 > maxNumberOfLuaDicts {
 				klog.Errorf("Ignoring %v for Lua dictionary %v: can not configure more than %v dictionaries.",
-					size, dictName, maxNumberOfLuaDicts)
+					results[1], dictName, maxNumberOfLuaDicts)
 				continue
 			}
 
@@ -427,3 +429,22 @@ func splitAndTrimSpace(s, sep string) []string {
 
 	return values
 }
+
+func dictStrToKb(sizeStr string) int {
+	sizeMatch := dictSizeRegex.FindStringSubmatch(sizeStr)
+	if sizeMatch == nil {
+		return -1
+	}
+	size, _ := strconv.Atoi(sizeMatch[1]) // validated already with regex
+	if sizeMatch[2] == "" || strings.ToLower(sizeMatch[2]) == "m" {
+		size *= 1024
+	}
+	return size
+}
+
+func dictKbToStr(size int) string {
+	if size%1024 == 0 {
+		return fmt.Sprintf("%dM", size/1024)
+	}
+	return fmt.Sprintf("%dK", size)
+}

internal/ingress/controller/template/configmap_test.go

@@ -344,27 +344,32 @@ func TestLuaSharedDictsParsing(t *testing.T) {
 		{
 			name:   "configuration_data only",
 			entry:  map[string]string{"lua-shared-dicts": "configuration_data:5"},
-			expect: map[string]int{"configuration_data": 5},
+			expect: map[string]int{"configuration_data": 5120},
 		},
 		{
 			name:   "certificate_data only",
 			entry:  map[string]string{"lua-shared-dicts": "certificate_data: 4"},
-			expect: map[string]int{"certificate_data": 4},
+			expect: map[string]int{"certificate_data": 4096},
 		},
 		{
 			name:   "custom dicts",
 			entry:  map[string]string{"lua-shared-dicts": "configuration_data:   10, my_random_dict:15 ,   another_example:2"},
-			expect: map[string]int{"configuration_data": 10, "my_random_dict": 15, "another_example": 2},
+			expect: map[string]int{"configuration_data": 10240, "my_random_dict": 15360, "another_example": 2048},
 		},
 		{
 			name:   "invalid size value should be ignored",
-			entry:  map[string]string{"lua-shared-dicts": "mydict: 10, invalid_dict: 1a"},
+			entry:  map[string]string{"lua-shared-dicts": "mydict: 10, invalid_dict: 1a, bad_mb_dict:10mb"},
-			expect: map[string]int{"mydict": 10},
+			expect: map[string]int{"mydict": 10240},
 		},
 		{
 			name:   "dictionary size can not be larger than 200",
-			entry:  map[string]string{"lua-shared-dicts": "mydict: 10, invalid_dict: 201"},
+			entry:  map[string]string{"lua-shared-dicts": "mydict: 10, invalid_dict: 201, invalid_kb: 204801k"},
-			expect: map[string]int{"mydict": 10},
+			expect: map[string]int{"mydict": 10240},
+		},
+		{
+			name:   "specified units are interpreted properly",
+			entry:  map[string]string{"lua-shared-dicts": "kb_dict_a: 512k, mb_dict_a: 30m, kb_dict_b:16K, mb_dict_b:4M"},
+			expect: map[string]int{"kb_dict_a": 512, "mb_dict_a": 30720, "kb_dict_b": 16, "mb_dict_b": 4096},
 		},
 	}
 
@@ -418,3 +423,76 @@ func TestSplitAndTrimSpace(t *testing.T) {
 		}
 	}
 }
+
+func TestDictStrToKb(t *testing.T) {
+	testCases := []struct {
+		name   string
+		input  string
+		expect int
+	}{
+		{
+			name:   "unitless int size converted to kb",
+			input:  "50",
+			expect: 51200,
+		},
+		{
+			name:   "lowercase k accepted",
+			input:  "512k",
+			expect: 512,
+		},
+		{
+			name:   "uppercase K accepted",
+			input:  "512K",
+			expect: 512,
+		},
+		{
+			name:   "lowercase m accepted",
+			input:  "10m",
+			expect: 10240,
+		},
+		{
+			name:   "uppercase M accepted",
+			input:  "10M",
+			expect: 10240,
+		},
+		{
+			name:   "trailing characters fail",
+			input:  "50kb",
+			expect: -1,
+		},
+		{
+			name:   "leading characters fail",
+			input:  " 50k",
+			expect: -1,
+		},
+	}
+	for _, tc := range testCases {
+		if size := dictStrToKb(tc.input); size != tc.expect {
+			t.Errorf("Testing %v. Expected \"%v\" but \"%v\" was returned", tc.name, tc.expect, size)
+		}
+	}
+}
+
+func TestDictKbToStr(t *testing.T) {
+	testCases := []struct {
+		name   string
+		input  int
+		expect string
+	}{
+		{
+			name:   "mod 1024 reports as M",
+			input:  5120,
+			expect: "5M",
+		},
+		{
+			name:   "non-mod 1024 reports as K",
+			input:  5001,
+			expect: "5001K",
+		},
+	}
+	for _, tc := range testCases {
+		if sizeStr := dictKbToStr(tc.input); sizeStr != tc.expect {
+			t.Errorf("Testing %v. Expected \"%v\" but \"%v\" was returned", tc.name, tc.expect, sizeStr)
+		}
+	}
+}

internal/ingress/controller/template/template.go

@@ -60,8 +60,8 @@ const (
 	stateComment
 )
 
-// TemplateWriter is the interface to render a template
+// Writer is the interface to render a template
-type TemplateWriter interface {
+type Writer interface {
 	Write(conf config.TemplateConfig) ([]byte, error)
 }
 
@@ -329,7 +329,8 @@ func buildLuaSharedDictionaries(c interface{}, s interface{}) string {
 	}
 
 	for name, size := range cfg.LuaSharedDicts {
-		out = append(out, fmt.Sprintf("lua_shared_dict %s %dM", name, size))
+		sizeStr := dictKbToStr(size)
+		out = append(out, fmt.Sprintf("lua_shared_dict %s %s", name, sizeStr))
 	}
 
 	sort.Strings(out)
@@ -341,16 +342,16 @@ func luaConfigurationRequestBodySize(c interface{}) string {
 	cfg, ok := c.(config.Configuration)
 	if !ok {
 		klog.Errorf("expected a 'config.Configuration' type but %T was returned", c)
-		return "100" // just a default number
+		return "100M" // just a default number
 	}
 
 	size := cfg.LuaSharedDicts["configuration_data"]
 	if size < cfg.LuaSharedDicts["certificate_data"] {
 		size = cfg.LuaSharedDicts["certificate_data"]
 	}
-	size = size + 1
+	size = size + 1024
 
-	return fmt.Sprintf("%d", size)
+	return dictKbToStr(size)
 }
 
 // configForLua returns some general configuration as Lua table represented as string

internal/ingress/controller/template/template_test.go

@@ -214,7 +214,7 @@ func TestBuildLuaSharedDictionaries(t *testing.T) {
 	// config lua dict
 	cfg := config.Configuration{
 		LuaSharedDicts: map[string]int{
-			"configuration_data": 10, "certificate_data": 20,
+			"configuration_data": 10240, "certificate_data": 20480,
 		},
 	}
 	actual := buildLuaSharedDictionaries(cfg, invalidType)
@@ -255,13 +255,13 @@ func TestBuildLuaSharedDictionaries(t *testing.T) {
 func TestLuaConfigurationRequestBodySize(t *testing.T) {
 	cfg := config.Configuration{
 		LuaSharedDicts: map[string]int{
-			"configuration_data": 10, "certificate_data": 20,
+			"configuration_data": 10240, "certificate_data": 20480,
 		},
 	}
 
 	size := luaConfigurationRequestBodySize(cfg)
-	if size != "21" {
+	if size != "21M" {
-		t.Errorf("expected the size to be 20 but got: %v", size)
+		t.Errorf("expected the size to be 21M but got: %v", size)
 	}
 }
 

rootfs/etc/nginx/template/nginx.tmpl

@@ -676,8 +676,8 @@ http {
         }
 
         location /configuration {
-            client_max_body_size                    {{ luaConfigurationRequestBodySize $cfg }}m;
+            client_max_body_size                    {{ luaConfigurationRequestBodySize $cfg }};
-            client_body_buffer_size                 {{ luaConfigurationRequestBodySize $cfg }}m;
+            client_body_buffer_size                 {{ luaConfigurationRequestBodySize $cfg }};
             proxy_buffering                         off;
 
             content_by_lua_block {