Repository: kubernetes/ingress-nginx
Pull request #5361 (2020-04-13 21:03:05)

Cleanup parsing of annotations with lists
<!--- Provide a general summary of your changes in the Title above --->
<!--- Please don't @-mention people in PR or commit messages (do so in an additional comment). --->

## What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Which issue/s this PR fixes
<!--
(optional, in `fixes #<issue number>` format, will close that issue when PR gets merged):

fixes #
-->

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I've read the [CONTRIBUTION](https://github.com/kubernetes/ingress-nginx/blob/master/CONTRIBUTING.md) guide
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.



Patch:
diff --git a/internal/ingress/controller/template/configmap.go b/internal/ingress/controller/template/configmap.go
index a51af3445..a91204777 100644
--- a/internal/ingress/controller/template/configmap.go
+++ b/internal/ingress/controller/template/configmap.go
@@ -108,7 +108,7 @@ func ReadConfig(src map[string]string) config.Configuration {
 	//parse lua shared dict values
 	if val, ok := conf[luaSharedDictsKey]; ok {
 		delete(conf, luaSharedDictsKey)
-		lsd := strings.Split(val, ",")
+		lsd := splitAndTrimSpace(val, ",")
 		for _, v := range lsd {
 			v = strings.Replace(v, " ", "", -1)
 			results := strings.SplitN(v, ":", 2)
@@ -137,9 +137,10 @@ func ReadConfig(src map[string]string) config.Configuration {
 			luaSharedDicts[k] = v
 		}
 	}
+
 	if val, ok := conf[customHTTPErrors]; ok {
 		delete(conf, customHTTPErrors)
-		for _, i := range strings.Split(val, ",") {
+		for _, i := range splitAndTrimSpace(val, ",") {
 			j, err := strconv.Atoi(i)
 			if err != nil {
 				klog.Warningf("%v is not a valid http code: %v", i, err)
@@ -148,27 +149,32 @@ func ReadConfig(src map[string]string) config.Configuration {
 			}
 		}
 	}
+
 	if val, ok := conf[hideHeaders]; ok {
 		delete(conf, hideHeaders)
-		hideHeadersList = strings.Split(val, ",")
+		hideHeadersList = splitAndTrimSpace(val, ",")
 	}
+
 	if val, ok := conf[skipAccessLogUrls]; ok {
 		delete(conf, skipAccessLogUrls)
-		skipUrls = strings.Split(val, ",")
+		skipUrls = splitAndTrimSpace(val, ",")
 	}
+
 	if val, ok := conf[whitelistSourceRange]; ok {
 		delete(conf, whitelistSourceRange)
-		whiteList = append(whiteList, strings.Split(val, ",")...)
+		whiteList = append(whiteList, splitAndTrimSpace(val, ",")...)
 	}
+
 	if val, ok := conf[proxyRealIPCIDR]; ok {
 		delete(conf, proxyRealIPCIDR)
-		proxyList = append(proxyList, strings.Split(val, ",")...)
+		proxyList = append(proxyList, splitAndTrimSpace(val, ",")...)
 	} else {
 		proxyList = append(proxyList, "0.0.0.0/0")
 	}
+
 	if val, ok := conf[bindAddress]; ok {
 		delete(conf, bindAddress)
-		for _, i := range strings.Split(val, ",") {
+		for _, i := range splitAndTrimSpace(val, ",") {
 			ns := net.ParseIP(i)
 			if ns != nil {
 				if ing_net.IsIPV6(ns) {
@@ -184,15 +190,17 @@ func ReadConfig(src map[string]string) config.Configuration {
 
 	if val, ok := conf[blockCIDRs]; ok {
 		delete(conf, blockCIDRs)
-		blockCIDRList = strings.Split(val, ",")
+		blockCIDRList = splitAndTrimSpace(val, ",")
 	}
+
 	if val, ok := conf[blockUserAgents]; ok {
 		delete(conf, blockUserAgents)
-		blockUserAgentList = strings.Split(val, ",")
+		blockUserAgentList = splitAndTrimSpace(val, ",")
 	}
+
 	if val, ok := conf[blockReferers]; ok {
 		delete(conf, blockReferers)
-		blockRefererList = strings.Split(val, ",")
+		blockRefererList = splitAndTrimSpace(val, ",")
 	}
 
 	if val, ok := conf[httpRedirectCode]; ok {
@@ -250,15 +258,12 @@ func ReadConfig(src map[string]string) config.Configuration {
 		delete(conf, globalAuthResponseHeaders)
 
 		if len(val) != 0 {
-			harr := strings.Split(val, ",")
+			harr := splitAndTrimSpace(val, ",")
 			for _, header := range harr {
-				header = strings.TrimSpace(header)
-				if len(header) > 0 {
-					if !authreq.ValidHeader(header) {
-						klog.Warningf("Global auth location denied - %v.", "invalid headers list")
-					} else {
-						responseHeaders = append(responseHeaders, header)
-					}
+				if !authreq.ValidHeader(header) {
+					klog.Warningf("Global auth location denied - %v.", "invalid headers list")
+				} else {
+					responseHeaders = append(responseHeaders, header)
 				}
 			}
 		}
@@ -267,19 +272,16 @@ func ReadConfig(src map[string]string) config.Configuration {
 
 	if val, ok := conf[globalAuthRequestRedirect]; ok {
 		delete(conf, globalAuthRequestRedirect)
-
 		to.GlobalExternalAuth.RequestRedirect = val
 	}
 
 	if val, ok := conf[globalAuthSnippet]; ok {
 		delete(conf, globalAuthSnippet)
-
 		to.GlobalExternalAuth.AuthSnippet = val
 	}
 
 	if val, ok := conf[globalAuthCacheKey]; ok {
 		delete(conf, globalAuthCacheKey)
-
 		to.GlobalExternalAuth.AuthCacheKey = val
 	}
 
@@ -318,23 +320,17 @@ func ReadConfig(src map[string]string) config.Configuration {
 
 	// Nginx Status whitelist
 	if val, ok := conf[nginxStatusIpv4Whitelist]; ok {
-		whitelist := make([]string, 0)
-		whitelist = append(whitelist, strings.Split(val, ",")...)
-		to.NginxStatusIpv4Whitelist = whitelist
-
+		to.NginxStatusIpv4Whitelist = splitAndTrimSpace(val, ",")
 		delete(conf, nginxStatusIpv4Whitelist)
 	}
-	if val, ok := conf[nginxStatusIpv6Whitelist]; ok {
-		whitelist := make([]string, 0)
-		whitelist = append(whitelist, strings.Split(val, ",")...)
-		to.NginxStatusIpv6Whitelist = whitelist
 
+	if val, ok := conf[nginxStatusIpv6Whitelist]; ok {
+		to.NginxStatusIpv6Whitelist = splitAndTrimSpace(val, ",")
 		delete(conf, nginxStatusIpv6Whitelist)
 	}
 
 	if val, ok := conf[workerProcesses]; ok {
 		to.WorkerProcesses = val
-
 		if val == "auto" {
 			to.WorkerProcesses = strconv.Itoa(runtime.NumCPU())
 		}
@@ -343,11 +339,7 @@ func ReadConfig(src map[string]string) config.Configuration {
 	}
 
 	if val, ok := conf[plugins]; ok {
-		to.Plugins = strings.Split(val, ",")
-		for i := range to.Plugins {
-			to.Plugins[i] = strings.TrimSpace(to.Plugins[i])
-		}
-
+		to.Plugins = splitAndTrimSpace(val, ",")
 		delete(conf, plugins)
 	}
 
@@ -405,3 +397,16 @@ func filterErrors(codes []int) []int {
 
 	return fa
 }
+
+func splitAndTrimSpace(s, sep string) []string {
+	f := func(c rune) bool {
+		return strings.EqualFold(string(c), sep)
+	}
+
+	values := strings.FieldsFunc(s, f)
+	for i := range values {
+		values[i] = strings.TrimSpace(values[i])
+	}
+
+	return values
+}


Test patch:
diff --git a/internal/ingress/controller/template/configmap_test.go b/internal/ingress/controller/template/configmap_test.go
index 6b0d51c6b..2cc13c523 100644
--- a/internal/ingress/controller/template/configmap_test.go
+++ b/internal/ingress/controller/template/configmap_test.go
@@ -358,3 +358,39 @@ func TestLuaSharedDictsParsing(t *testing.T) {
 		}
 	}
 }
+
+func TestSplitAndTrimSpace(t *testing.T) {
+	testsCases := []struct {
+		name   string
+		input  string
+		expect []string
+	}{
+		{
+			name:   "empty string",
+			input:  "",
+			expect: []string{},
+		},
+		{
+			name:   "two elements",
+			input:  "el1,el2",
+			expect: []string{"el1", "el2"},
+		},
+		{
+			name:   "two elements with spaces",
+			input:  " el1, el2",
+			expect: []string{"el1", "el2"},
+		},
+		{
+			name:   "empty elements with spaces",
+			input:  " el1, el2,el3,,",
+			expect: []string{"el1", "el2", "el3"},
+		},
+	}
+
+	for _, tc := range testsCases {
+		data := splitAndTrimSpace(tc.input, ",")
+		if !reflect.DeepEqual(data, tc.expect) {
+			t.Errorf("Testing %v. Expected \"%v\" but \"%v\" was returned", tc.name, tc.expect, data)
+		}
+	}
+}