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
 			}
-			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

@@ -303,50 +303,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:   "lua invalid entry",
-			entry:  map[string]string{"lua-shared-dict": "configuration_data:5,certificate_data:5"},
-			expect: map[string]int{},
+			name:   "default dicts configured when lua-shared-dicts is not set",
+			entry:  make(map[string]string),
+			expect: defaultLuaSharedDicts,
 		},
 		{
-			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:   "lua valid entry - configuration_data only",
+			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": 5, "certificate_data": 20},
 		},
 		{
-			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:   "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 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:   "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:   "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

@@ -236,7 +236,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 +247,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 +268,8 @@ 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"
 }
 
 // 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)
 	}

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;"))
 	})
 })