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

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 {
+	if val, ok := conf[luaSharedDictsKey]; ok {
-		delete(conf, luaSharedDicts)
+		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
 			}
-			luaSharedDict[results[0]] = val
+			if size > maxAllowedLuaDictSize {
+				klog.Errorf("Ignoring %v for Lua dictionary %v: maximum size is %v.", size, 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)
+				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",
+			name:   "default dicts configured when lua-shared-dicts is not set",
-			entry:  map[string]string{"lua-shared-dicts": "configuration_data:5,certificate_data:5"},
+			entry:  make(map[string]string),
-			expect: map[string]int{"configuration_data": 5, "certificate_data": 5},
+			expect: defaultLuaSharedDicts,
-		},
-
-		{
-			name:   "lua invalid entry",
-			entry:  map[string]string{"lua-shared-dict": "configuration_data:5,certificate_data:5"},
-			expect: map[string]int{},
 		},
 		{
-			name:   "lua mixed entry",
+			name:   "configuration_data only",
-			entry:  map[string]string{"lua-shared-dicts": "configuration_data:10,certificate_data:5"},
-			expect: map[string]int{"configuration_data": 10, "certificate_data": 5},
-		},
-		{
-			name:   "lua valid entry - 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": 5, "certificate_data": 20},
 		},
 		{
-			name:   "lua valid entry certificate_data only",
+			name:   "certificate_data only",
-			entry:  map[string]string{"lua-shared-dicts": "certificate_data:5"},
+			entry:  map[string]string{"lua-shared-dicts": "certificate_data: 4"},
-			expect: map[string]int{"certificate_data": 5},
+			expect: map[string]int{"configuration_data": 20, "certificate_data": 4},
 		},
 		{
-			name:   "lua valid entry certificate_data only",
+			name:   "custom dicts",
-			entry:  map[string]string{"lua-shared-dicts": "configuration_data:10, my_random_dict:15,another_example:2"},
+			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": 10, "certificate_data": 20, "my_random_dict": 15, "another_example": 2},
+		},
+		{
+			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:   "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,
+		"escapeLiteralDollar":             escapeLiteralDollar,
-		"shouldConfigureLuaRestyWAF": shouldConfigureLuaRestyWAF,
+		"shouldConfigureLuaRestyWAF":      shouldConfigureLuaRestyWAF,
-		"buildLuaSharedDictionaries": buildLuaSharedDictionaries,
+		"buildLuaSharedDictionaries":      buildLuaSharedDictionaries,
-		"buildLocation":              buildLocation,
+		"luaConfigurationRequestBodySize": luaConfigurationRequestBodySize,
-		"buildAuthLocation":          buildAuthLocation,
+		"buildLocation":                   buildLocation,
-		"shouldApplyGlobalAuth":      shouldApplyGlobalAuth,
+		"buildAuthLocation":               buildAuthLocation,
-		"buildAuthResponseHeaders":   buildAuthResponseHeaders,
+		"shouldApplyGlobalAuth":           shouldApplyGlobalAuth,
-		"buildProxyPass":             buildProxyPass,
+		"buildAuthResponseHeaders":        buildAuthResponseHeaders,
-		"filterRateLimits":           filterRateLimits,
+		"buildProxyPass":                  buildProxyPass,
-		"buildRateLimitZones":        buildRateLimitZones,
+		"filterRateLimits":                filterRateLimits,
-		"buildRateLimit":             buildRateLimit,
+		"buildRateLimitZones":             buildRateLimitZones,
-		"configForLua":               configForLua,
+		"buildRateLimit":                  buildRateLimit,
-		"locationConfigForLua":       locationConfigForLua,
+		"configForLua":                    configForLua,
-		"buildResolvers":             buildResolvers,
+		"locationConfigForLua":            locationConfigForLua,
-		"buildUpstreamName":          buildUpstreamName,
+		"buildResolvers":                  buildResolvers,
-		"isLocationInLocationList":   isLocationInLocationList,
+		"buildUpstreamName":               buildUpstreamName,
-		"isLocationAllowed":          isLocationAllowed,
+		"isLocationInLocationList":        isLocationInLocationList,
-		"buildLogFormatUpstream":     buildLogFormatUpstream,
+		"isLocationAllowed":               isLocationAllowed,
-		"buildDenyVariable":          buildDenyVariable,
+		"buildLogFormatUpstream":          buildLogFormatUpstream,
-		"getenv":                     os.Getenv,
+		"buildDenyVariable":               buildDenyVariable,
-		"contains":                   strings.Contains,
+		"getenv":                          os.Getenv,
-		"hasPrefix":                  strings.HasPrefix,
+		"contains":                        strings.Contains,
-		"hasSuffix":                  strings.HasSuffix,
+		"hasPrefix":                       strings.HasPrefix,
-		"trimSpace":                  strings.TrimSpace,
+		"hasSuffix":                       strings.HasSuffix,
-		"toUpper":                    strings.ToUpper,
+		"trimSpace":                       strings.TrimSpace,
-		"toLower":                    strings.ToLower,
+		"toUpper":                         strings.ToUpper,
-		"formatIP":                   formatIP,
+		"toLower":                         strings.ToLower,
-		"quote":                      quote,
+		"formatIP":                        formatIP,
-		"buildNextUpstream":          buildNextUpstream,
+		"quote":                           quote,
-		"getIngressInformation":      getIngressInformation,
+		"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
+	for name, size := range cfg.LuaSharedDicts {
-	certData, ok := cfg.LuaSharedDicts["certificate_data"]
+		out = append(out, fmt.Sprintf("lua_shared_dict %s %dM", name, size))
-	if !ok {
-		certData = 16
 	}
-	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                    {{ luaConfigurationRequestBodySize $cfg }}m;
-            client_max_body_size                    10m;
+            client_body_buffer_size                 {{ luaConfigurationRequestBodySize $cfg }}m;
-            client_body_buffer_size                 10m;
             proxy_buffering                         off;
 
             content_by_lua_block {

test/e2e/lua/dynamic_configuration.go

@@ -62,30 +62,6 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() {
 		})
 	})
 
-	Context("Lua shared dict", func() {
-		It("update config", func() {
-
-			host := "foo.com"
-			ingress := framework.NewSingleIngress(host, "/", host, f.Namespace, "http-svc", 80, nil)
-			f.EnsureIngress(ingress)
-
-			lkey := "lua-shared-dicts"
-			lval := "configuration_data:100,certificate_data:200"
-
-			By("update shared dict")
-
-			f.UpdateNginxConfigMapData(lkey, lval)
-
-			var nginxConfig string
-			f.WaitForNginxConfiguration(func(cfg string) bool {
-				nginxConfig = cfg
-				return true
-			})
-
-			Expect(strings.ContainsAny(nginxConfig, "configuration_data:100M,certificate_data:200M"), true)
-		})
-	})
-
 	Context("when only backends change", func() {
 		It("handles endpoints only changes", func() {
 			var nginxConfig string

test/e2e/settings/lua_shared_dicts.go

@@ -17,14 +17,13 @@ limitations under the License.
 package settings
 
 import (
-	"strings"
-
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
 	"k8s.io/ingress-nginx/test/e2e/framework"
 )
 
 var _ = framework.IngressNginxDescribe("LuaSharedDict", func() {
-
 	f := framework.NewDefaultFramework("lua-shared-dicts")
 	host := "lua-shared-dicts"
 
@@ -35,13 +34,20 @@ var _ = framework.IngressNginxDescribe("LuaSharedDict", func() {
 	AfterEach(func() {
 	})
 
-	It("update lua shared dict", func() {
+	It("configures lua shared dicts", func() {
 		ingress := framework.NewSingleIngress(host, "/", host, f.Namespace, "http-svc", 80, nil)
 		f.EnsureIngress(ingress)
-		By("update shared dict")
+
-		f.UpdateNginxConfigMapData("lua-shared-dicts", "configuration_data:123,certificate_data:456")
+		f.UpdateNginxConfigMapData("lua-shared-dicts", "configuration_data:60,certificate_data:300, my_dict: 15 , invalid: 1a")
+
+		ngxCfg := ""
 		f.WaitForNginxConfiguration(func(cfg string) bool {
-			return strings.Contains(cfg, "lua_shared_dict configuration_data 123M; lua_shared_dict certificate_data 456M;")
+			ngxCfg = cfg
+			return true
 		})
+
+		Expect(ngxCfg).Should(ContainSubstring("lua_shared_dict configuration_data 60M;"))
+		Expect(ngxCfg).Should(ContainSubstring("lua_shared_dict certificate_data 20M;"))
+		Expect(ngxCfg).Should(ContainSubstring("lua_shared_dict my_dict 15M;"))
 	})
 })