Merge pull request #4446 from ElvinEfendi/improve-lua-shared-dicts-se…

…tting

lua-shared-dicts improvements, fixes and documentation

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

@@ -148,6 +148,7 @@ The following table shows a configuration option's name, type, and the default v
 |[skip-access-log-urls](#skip-access-log-urls)|[]string|[]string{}|
 |[limit-rate](#limit-rate)|int|0|
 |[limit-rate-after](#limit-rate-after)|int|0|
+|[lua-shared-dicts](#lua-shared-dicts)|string|""|
 |[http-redirect-code](#http-redirect-code)|int|308|
 |[proxy-buffering](#proxy-buffering)|string|"off"|
 |[limit-req-status-code](#limit-req-status-code)|int|503|
@@ -847,6 +848,21 @@ _References:_
 
 Sets the initial amount after which the further transmission of a response to a client will be rate limited.
 
+## lua-shared-dicts
+
+Customize default Lua shared dictionaries or define more. You can use the following syntax to do so:
+
+```
+lua-shared-dicts: "<my dict name>: <my dict size>, [<my dict name>: <my dict size>], ..."
+```
+
+For example following will set default `certificate_data` dictionary to `100M` and will introduce a new dictionary called
+`my_custom_plugin`:
+
+```
+lua-shared-dicts: "certificate_data: 100, my_custom_plugin: 5"
+```
+
 _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/template/configmap.go

@@ -59,11 +59,20 @@ const (
 	globalAuthSnippet         = "global-auth-snippet"
 	globalAuthCacheKey        = "global-auth-cache-key"
 	globalAuthCacheDuration   = "global-auth-cache-duration"
-	luaSharedDicts            = "lua-shared-dicts"
+	luaSharedDictsKey         = "lua-shared-dicts"
 )
 
 var (
-	validRedirectCodes = sets.NewInt([]int{301, 302, 307, 308}...)
+	validRedirectCodes    = sets.NewInt([]int{301, 302, 307, 308}...)
+	defaultLuaSharedDicts = map[string]int{
+		"configuration_data": 20,
+		"certificate_data":   20,
+	}
+)
+
+const (
+	maxAllowedLuaDictSize = 200
+	maxNumberOfLuaDicts   = 100
 )
 
 // ReadConfig obtains the configuration defined by the user merged with the defaults.
@@ -88,20 +97,38 @@ func ReadConfig(src map[string]string) config.Configuration {
 	blockUserAgentList := make([]string, 0)
 	blockRefererList := make([]string, 0)
 	responseHeaders := make([]string, 0)
-	luaSharedDict := make(map[string]int)
+	luaSharedDicts := make(map[string]int)
 
 	//parse lua shared dict values
-	if val, ok := conf[luaSharedDicts]; ok {
-		delete(conf, luaSharedDicts)
+	if val, ok := conf[luaSharedDictsKey]; ok {
+		delete(conf, luaSharedDictsKey)
 		lsd := strings.Split(val, ",")
 		for _, v := range lsd {
 			v = strings.Replace(v, " ", "", -1)
 			results := strings.SplitN(v, ":", 2)
-			val, err := strconv.Atoi(results[1])
+			dictName := results[0]
+			size, err := strconv.Atoi(results[1])
 			if err != nil {
-				klog.Warningf("%v is not a valid lua entry: %v", v, err)
+				klog.Errorf("Ignoring non integer value %v for Lua dictionary %v: %v.", results[1], dictName, err)
+				continue
+			}
+			if size > maxAllowedLuaDictSize {
+				klog.Errorf("Ignoring %v for Lua dictionary %v: maximum size is %v.", size, dictName, maxAllowedLuaDictSize)
+				continue
 			}
-			luaSharedDict[results[0]] = val
+			if len(luaSharedDicts)+1 > maxNumberOfLuaDicts {
+				klog.Errorf("Ignoring %v for Lua dictionary %v: can not configure more than %v dictionaries.",
+					size, dictName, maxNumberOfLuaDicts)
+				continue
+			}
+
+			luaSharedDicts[dictName] = size
+		}
+	}
+	// set default Lua shared dicts
+	for k, v := range defaultLuaSharedDicts {
+		if _, ok := luaSharedDicts[k]; !ok {
+			luaSharedDicts[k] = v
 		}
 	}
 	if val, ok := conf[customHTTPErrors]; ok {
@@ -321,7 +348,7 @@ func ReadConfig(src map[string]string) config.Configuration {
 	to.HideHeaders = hideHeadersList
 	to.ProxyStreamResponses = streamResponses
 	to.DisableIpv6DNS = !ing_net.IsIPv6Enabled()
-	to.LuaSharedDicts = luaSharedDict
+	to.LuaSharedDicts = luaSharedDicts
 
 	config := &mapstructure.DecoderConfig{
 		Metadata:         nil,

internal/ingress/controller/template/configmap_test.go

@@ -124,6 +124,7 @@ func TestMergeConfigMapToStruct(t *testing.T) {
 	}
 
 	def = config.NewDefault()
+	def.LuaSharedDicts = map[string]int{"configuration_data": 20, "certificate_data": 20}
 	def.DisableIpv6DNS = true
 
 	hash, err = hashstructure.Hash(def, &hashstructure.HashOptions{
@@ -142,6 +143,7 @@ func TestMergeConfigMapToStruct(t *testing.T) {
 	}
 
 	def = config.NewDefault()
+	def.LuaSharedDicts = map[string]int{"configuration_data": 20, "certificate_data": 20}
 	def.WhitelistSourceRange = []string{"1.1.1.1/32"}
 	def.DisableIpv6DNS = true
 
@@ -303,50 +305,48 @@ func TestGlobalExternalAuthCacheDurationParsing(t *testing.T) {
 	}
 }
 
-func TestLuaSharedDict(t *testing.T) {
-
+func TestLuaSharedDictsParsing(t *testing.T) {
 	testsCases := []struct {
 		name   string
 		entry  map[string]string
 		expect map[string]int
 	}{
 		{
-			name:   "lua valid entry",
-			entry:  map[string]string{"lua-shared-dicts": "configuration_data:5,certificate_data:5"},
-			expect: map[string]int{"configuration_data": 5, "certificate_data": 5},
+			name:   "default dicts configured when lua-shared-dicts is not set",
+			entry:  make(map[string]string),
+			expect: defaultLuaSharedDicts,
 		},
-
 		{
-			name:   "lua invalid entry",
-			entry:  map[string]string{"lua-shared-dict": "configuration_data:5,certificate_data:5"},
-			expect: map[string]int{},
+			name:   "configuration_data only",
+			entry:  map[string]string{"lua-shared-dicts": "configuration_data:5"},
+			expect: map[string]int{"configuration_data": 5, "certificate_data": 20},
 		},
 		{
-			name:   "lua mixed entry",
-			entry:  map[string]string{"lua-shared-dicts": "configuration_data:10,certificate_data:5"},
-			expect: map[string]int{"configuration_data": 10, "certificate_data": 5},
+			name:   "certificate_data only",
+			entry:  map[string]string{"lua-shared-dicts": "certificate_data: 4"},
+			expect: map[string]int{"configuration_data": 20, "certificate_data": 4},
 		},
 		{
-			name:   "lua valid entry - configuration_data only",
-			entry:  map[string]string{"lua-shared-dicts": "configuration_data:5"},
-			expect: map[string]int{"configuration_data": 5},
+			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, "certificate_data": 20, "my_random_dict": 15, "another_example": 2},
 		},
 		{
-			name:   "lua valid entry certificate_data only",
-			entry:  map[string]string{"lua-shared-dicts": "certificate_data:5"},
-			expect: map[string]int{"certificate_data": 5},
+			name:   "invalid size value should be ignored",
+			entry:  map[string]string{"lua-shared-dicts": "mydict: 10, invalid_dict: 1a"},
+			expect: map[string]int{"configuration_data": 20, "certificate_data": 20, "mydict": 10},
 		},
 		{
-			name:   "lua valid entry certificate_data only",
-			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},
+			name:   "dictionary size can not be larger than 200",
+			entry:  map[string]string{"lua-shared-dicts": "mydict: 10, invalid_dict: 201"},
+			expect: map[string]int{"configuration_data": 20, "certificate_data": 20, "mydict": 10},
 		},
 	}
 
-	for n, tc := range testsCases {
+	for _, tc := range testsCases {
 		cfg := ReadConfig(tc.entry)
 		if !reflect.DeepEqual(cfg.LuaSharedDicts, tc.expect) {
-			t.Errorf("Testing %v. Expected \"%v\" but \"%v\" was returned", n, tc.expect, cfg.LuaSharedDicts)
+			t.Errorf("Testing %v. Expected \"%v\" but \"%v\" was returned", tc.name, tc.expect, cfg.LuaSharedDicts)
 		}
 	}
 }

internal/ingress/controller/template/template.go

@@ -133,36 +133,37 @@ var (
 			}
 			return true
 		},
-		"escapeLiteralDollar":        escapeLiteralDollar,
-		"shouldConfigureLuaRestyWAF": shouldConfigureLuaRestyWAF,
-		"buildLuaSharedDictionaries": buildLuaSharedDictionaries,
-		"buildLocation":              buildLocation,
-		"buildAuthLocation":          buildAuthLocation,
-		"shouldApplyGlobalAuth":      shouldApplyGlobalAuth,
-		"buildAuthResponseHeaders":   buildAuthResponseHeaders,
-		"buildProxyPass":             buildProxyPass,
-		"filterRateLimits":           filterRateLimits,
-		"buildRateLimitZones":        buildRateLimitZones,
-		"buildRateLimit":             buildRateLimit,
-		"configForLua":               configForLua,
-		"locationConfigForLua":       locationConfigForLua,
-		"buildResolvers":             buildResolvers,
-		"buildUpstreamName":          buildUpstreamName,
-		"isLocationInLocationList":   isLocationInLocationList,
-		"isLocationAllowed":          isLocationAllowed,
-		"buildLogFormatUpstream":     buildLogFormatUpstream,
-		"buildDenyVariable":          buildDenyVariable,
-		"getenv":                     os.Getenv,
-		"contains":                   strings.Contains,
-		"hasPrefix":                  strings.HasPrefix,
-		"hasSuffix":                  strings.HasSuffix,
-		"trimSpace":                  strings.TrimSpace,
-		"toUpper":                    strings.ToUpper,
-		"toLower":                    strings.ToLower,
-		"formatIP":                   formatIP,
-		"quote":                      quote,
-		"buildNextUpstream":          buildNextUpstream,
-		"getIngressInformation":      getIngressInformation,
+		"escapeLiteralDollar":             escapeLiteralDollar,
+		"shouldConfigureLuaRestyWAF":      shouldConfigureLuaRestyWAF,
+		"buildLuaSharedDictionaries":      buildLuaSharedDictionaries,
+		"luaConfigurationRequestBodySize": luaConfigurationRequestBodySize,
+		"buildLocation":                   buildLocation,
+		"buildAuthLocation":               buildAuthLocation,
+		"shouldApplyGlobalAuth":           shouldApplyGlobalAuth,
+		"buildAuthResponseHeaders":        buildAuthResponseHeaders,
+		"buildProxyPass":                  buildProxyPass,
+		"filterRateLimits":                filterRateLimits,
+		"buildRateLimitZones":             buildRateLimitZones,
+		"buildRateLimit":                  buildRateLimit,
+		"configForLua":                    configForLua,
+		"locationConfigForLua":            locationConfigForLua,
+		"buildResolvers":                  buildResolvers,
+		"buildUpstreamName":               buildUpstreamName,
+		"isLocationInLocationList":        isLocationInLocationList,
+		"isLocationAllowed":               isLocationAllowed,
+		"buildLogFormatUpstream":          buildLogFormatUpstream,
+		"buildDenyVariable":               buildDenyVariable,
+		"getenv":                          os.Getenv,
+		"contains":                        strings.Contains,
+		"hasPrefix":                       strings.HasPrefix,
+		"hasSuffix":                       strings.HasSuffix,
+		"trimSpace":                       strings.TrimSpace,
+		"toUpper":                         strings.ToUpper,
+		"toLower":                         strings.ToLower,
+		"formatIP":                        formatIP,
+		"quote":                           quote,
+		"buildNextUpstream":               buildNextUpstream,
+		"getIngressInformation":           getIngressInformation,
 		"serverConfig": func(all config.TemplateConfig, server *ingress.Server) interface{} {
 			return struct{ First, Second interface{} }{all, server}
 		},
@@ -236,7 +237,7 @@ func shouldConfigureLuaRestyWAF(disableLuaRestyWAF bool, mode string) bool {
 
 func buildLuaSharedDictionaries(c interface{}, s interface{}, disableLuaRestyWAF bool) string {
 	var out []string
-	// Load config
+
 	cfg, ok := c.(config.Configuration)
 	if !ok {
 		klog.Errorf("expected a 'config.Configuration' type but %T was returned", c)
@@ -247,20 +248,13 @@ func buildLuaSharedDictionaries(c interface{}, s interface{}, disableLuaRestyWAF
 		klog.Errorf("expected an '[]*ingress.Server' type but %T was returned", s)
 		return ""
 	}
-	// check if config contains lua "lua_configuration_data" value otherwise, use default
-	cfgData, ok := cfg.LuaSharedDicts["configuration_data"]
-	if !ok {
-		cfgData = 15
-	}
-	out = append(out, fmt.Sprintf("lua_shared_dict configuration_data %dM", cfgData))
 
-	// check if config contains "lua_certificate_data" value otherwise, use default
-	certData, ok := cfg.LuaSharedDicts["certificate_data"]
-	if !ok {
-		certData = 16
+	for name, size := range cfg.LuaSharedDicts {
+		out = append(out, fmt.Sprintf("lua_shared_dict %s %dM", name, size))
 	}
-	out = append(out, fmt.Sprintf("lua_shared_dict certificate_data %dM", certData))
-	if !disableLuaRestyWAF {
+
+	// TODO: there must be a better place for this
+	if _, ok := cfg.LuaSharedDicts["waf_storage"]; !ok && !disableLuaRestyWAF {
 		luaRestyWAFEnabled := func() bool {
 			for _, server := range servers {
 				for _, location := range server.Locations {
@@ -275,7 +269,24 @@ func buildLuaSharedDictionaries(c interface{}, s interface{}, disableLuaRestyWAF
 			out = append(out, "lua_shared_dict waf_storage 64M")
 		}
 	}
-	return strings.Join(out, ";\n\r") + ";"
+
+	return strings.Join(out, ";\n") + ";\n"
+}
+
+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
+	}
+
+	size := cfg.LuaSharedDicts["configuration_data"]
+	if size < cfg.LuaSharedDicts["certificate_data"] {
+		size = cfg.LuaSharedDicts["certificate_data"]
+	}
+	size = size + 1
+
+	return fmt.Sprintf("%d", size)
 }
 
 // configForLua returns some general configuration as Lua table represented as string

internal/ingress/controller/template/template_test.go

@@ -203,9 +203,12 @@ func TestBuildLuaSharedDictionaries(t *testing.T) {
 	}
 	// returns value from config
 	configuration := buildLuaSharedDictionaries(cfg, servers, false)
-	if !strings.Contains(configuration, "lua_shared_dict configuration_data 10M;\n\rlua_shared_dict certificate_data 20M;") {
+	if !strings.Contains(configuration, "lua_shared_dict configuration_data 10M;\n") {
 		t.Errorf("expected to include 'configuration_data' but got %s", configuration)
 	}
+	if !strings.Contains(configuration, "lua_shared_dict certificate_data 20M;\n") {
+		t.Errorf("expected to include 'certificate_data' but got %s", configuration)
+	}
 	if strings.Contains(configuration, "waf_storage") {
 		t.Errorf("expected to not include 'waf_storage' but got %s", configuration)
 	}
@@ -222,6 +225,19 @@ func TestBuildLuaSharedDictionaries(t *testing.T) {
 	}
 }
 
+func TestLuaConfigurationRequestBodySize(t *testing.T) {
+	cfg := config.Configuration{
+		LuaSharedDicts: map[string]int{
+			"configuration_data": 10, "certificate_data": 20,
+		},
+	}
+
+	size := luaConfigurationRequestBodySize(cfg)
+	if "21" != size {
+		t.Errorf("expected the size to be 20 but got: %v", size)
+	}
+}
+
 func TestFormatIP(t *testing.T) {
 	cases := map[string]struct {
 		Input, Output string

rootfs/etc/nginx/template/nginx.tmpl

@@ -580,9 +580,8 @@ http {
         }
 
         location /configuration {
-            # this should be equals to configuration_data dict
-            client_max_body_size                    10m;
-            client_body_buffer_size                 10m;
+            client_max_body_size                    {{ luaConfigurationRequestBodySize $cfg }}m;
+            client_body_buffer_size                 {{ luaConfigurationRequestBodySize $cfg }}m;
             proxy_buffering                         off;
 
             content_by_lua_block {