Merge pull request #3474 from aledbf/improve-parsing

Improve parsing of annotations and use of Ingress wrapper

internal/ingress/annotations/authreq/main.go

@@ -123,9 +123,6 @@ func (a authReq) Parse(ing *extensions.Ingress) (interface{}, error) {
 	if err != nil {
 		return nil, err
 	}
-	if urlString == "" {
-		return nil, ing_errors.NewLocationDenied("an empty string is not a valid URL")
-	}
 
 	authURL, err := url.Parse(urlString)
 	if err != nil {

internal/ingress/annotations/authreq/main_test.go

@@ -105,6 +105,10 @@ func TestAnnotations(t *testing.T) {
 			}
 			continue
 		}
+		if err != nil {
+			t.Errorf("%v: unexpected error: %v", test.title, err)
+		}
+
 		u, ok := i.(*Config)
 		if !ok {
 			t.Errorf("%v: expected an External type", test.title)

internal/ingress/annotations/authtls/main.go

@@ -87,52 +87,45 @@ type authTLS struct {
 // Parse parses the annotations contained in the ingress
 // rule used to use a Certificate as authentication method
 func (a authTLS) Parse(ing *extensions.Ingress) (interface{}, error) {
+	var err error
+	config := &Config{}
 
 	tlsauthsecret, err := parser.GetStringAnnotation("auth-tls-secret", ing)
 	if err != nil {
 		return &Config{}, err
 	}
 
-	if tlsauthsecret == "" {
-		return &Config{}, ing_errors.NewLocationDenied("an empty string is not a valid secret name")
-	}
-
 	_, _, err = k8s.ParseNameNS(tlsauthsecret)
 	if err != nil {
 		return &Config{}, ing_errors.NewLocationDenied(err.Error())
 	}
 
-	tlsVerifyClient, err := parser.GetStringAnnotation("auth-tls-verify-client", ing)
-	if err != nil || !authVerifyClientRegex.MatchString(tlsVerifyClient) {
-		tlsVerifyClient = defaultAuthVerifyClient
-	}
-
-	tlsdepth, err := parser.GetIntAnnotation("auth-tls-verify-depth", ing)
-	if err != nil || tlsdepth == 0 {
-		tlsdepth = defaultAuthTLSDepth
-	}
-
 	authCert, err := a.r.GetAuthCertificate(tlsauthsecret)
 	if err != nil {
 		e := errors.Wrap(err, "error obtaining certificate")
 		return &Config{}, ing_errors.LocationDenied{Reason: e}
 	}
+	config.AuthSSLCert = *authCert
 
-	errorpage, err := parser.GetStringAnnotation("auth-tls-error-page", ing)
-	if err != nil || errorpage == "" {
-		errorpage = ""
+	config.VerifyClient, err = parser.GetStringAnnotation("auth-tls-verify-client", ing)
+	if err != nil || !authVerifyClientRegex.MatchString(config.VerifyClient) {
+		config.VerifyClient = defaultAuthVerifyClient
+	}
+
+	config.ValidationDepth, err = parser.GetIntAnnotation("auth-tls-verify-depth", ing)
+	if err != nil || config.ValidationDepth == 0 {
+		config.ValidationDepth = defaultAuthTLSDepth
+	}
+
+	config.ErrorPage, err = parser.GetStringAnnotation("auth-tls-error-page", ing)
+	if err != nil {
+		config.ErrorPage = ""
 	}
 
-	passCert, err := parser.GetBoolAnnotation("auth-tls-pass-certificate-to-upstream", ing)
+	config.PassCertToUpstream, err = parser.GetBoolAnnotation("auth-tls-pass-certificate-to-upstream", ing)
 	if err != nil {
-		passCert = false
+		config.PassCertToUpstream = false
 	}
 
-	return &Config{
-		AuthSSLCert:        *authCert,
-		VerifyClient:       tlsVerifyClient,
-		ValidationDepth:    tlsdepth,
-		ErrorPage:          errorpage,
-		PassCertToUpstream: passCert,
-	}, nil
+	return config, nil
 }

internal/ingress/annotations/connection/main_test.go

@@ -38,7 +38,6 @@ func TestParse(t *testing.T) {
 		annotations map[string]string
 		expected    *Config
 	}{
-		{map[string]string{annotation: ""}, &Config{Enabled: true, Header: ""}},
 		{map[string]string{annotation: "keep-alive"}, &Config{Enabled: true, Header: "keep-alive"}},
 		{map[string]string{}, &Config{Enabled: false}},
 		{nil, &Config{Enabled: false}},

internal/ingress/annotations/cors/main.go

@@ -97,43 +97,39 @@ func (c1 *Config) Equal(c2 *Config) bool {
 // Parse parses the annotations contained in the ingress
 // rule used to indicate if the location/s should allows CORS
 func (c cors) Parse(ing *extensions.Ingress) (interface{}, error) {
-	corsenabled, err := parser.GetBoolAnnotation("enable-cors", ing)
+	var err error
+	config := &Config{}
+
+	config.CorsEnabled, err = parser.GetBoolAnnotation("enable-cors", ing)
 	if err != nil {
-		corsenabled = false
+		config.CorsEnabled = false
 	}
 
-	corsalloworigin, err := parser.GetStringAnnotation("cors-allow-origin", ing)
-	if err != nil || corsalloworigin == "" || !corsOriginRegex.MatchString(corsalloworigin) {
-		corsalloworigin = "*"
+	config.CorsAllowOrigin, err = parser.GetStringAnnotation("cors-allow-origin", ing)
+	if err != nil || !corsOriginRegex.MatchString(config.CorsAllowOrigin) {
+		config.CorsAllowOrigin = "*"
 	}
 
-	corsallowheaders, err := parser.GetStringAnnotation("cors-allow-headers", ing)
-	if err != nil || corsallowheaders == "" || !corsHeadersRegex.MatchString(corsallowheaders) {
-		corsallowheaders = defaultCorsHeaders
+	config.CorsAllowHeaders, err = parser.GetStringAnnotation("cors-allow-headers", ing)
+	if err != nil || !corsHeadersRegex.MatchString(config.CorsAllowHeaders) {
+		config.CorsAllowHeaders = defaultCorsHeaders
 	}
 
-	corsallowmethods, err := parser.GetStringAnnotation("cors-allow-methods", ing)
-	if err != nil || corsallowmethods == "" || !corsMethodsRegex.MatchString(corsallowmethods) {
-		corsallowmethods = defaultCorsMethods
+	config.CorsAllowMethods, err = parser.GetStringAnnotation("cors-allow-methods", ing)
+	if err != nil || !corsMethodsRegex.MatchString(config.CorsAllowMethods) {
+		config.CorsAllowMethods = defaultCorsMethods
 	}
 
-	corsallowcredentials, err := parser.GetBoolAnnotation("cors-allow-credentials", ing)
+	config.CorsAllowCredentials, err = parser.GetBoolAnnotation("cors-allow-credentials", ing)
 	if err != nil {
-		corsallowcredentials = true
+		config.CorsAllowCredentials = true
 	}
 
-	corsmaxage, err := parser.GetIntAnnotation("cors-max-age", ing)
+	config.CorsMaxAge, err = parser.GetIntAnnotation("cors-max-age", ing)
 	if err != nil {
-		corsmaxage = defaultCorsMaxAge
+		config.CorsMaxAge = defaultCorsMaxAge
 	}
 
-	return &Config{
-		CorsEnabled:          corsenabled,
-		CorsAllowOrigin:      corsalloworigin,
-		CorsAllowHeaders:     corsallowheaders,
-		CorsAllowMethods:     corsallowmethods,
-		CorsAllowCredentials: corsallowcredentials,
-		CorsMaxAge:           corsmaxage,
-	}, nil
+	return config, nil
 
 }

internal/ingress/annotations/influxdb/main.go

@@ -43,40 +43,37 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation {
 
 // Parse parses the annotations to look for InfluxDB configurations
 func (c influxdb) Parse(ing *extensions.Ingress) (interface{}, error) {
-	influxdbEnabled, err := parser.GetBoolAnnotation("enable-influxdb", ing)
+	var err error
+	config := &Config{}
+
+	config.InfluxDBEnabled, err = parser.GetBoolAnnotation("enable-influxdb", ing)
 	if err != nil {
-		influxdbEnabled = false
+		config.InfluxDBEnabled = false
 	}
 
-	influxdbMeasurement, err := parser.GetStringAnnotation("influxdb-measurement", ing)
+	config.InfluxDBMeasurement, err = parser.GetStringAnnotation("influxdb-measurement", ing)
 	if err != nil {
-		influxdbMeasurement = "default"
+		config.InfluxDBMeasurement = "default"
 	}
 
-	influxdbPort, err := parser.GetStringAnnotation("influxdb-port", ing)
+	config.InfluxDBPort, err = parser.GetStringAnnotation("influxdb-port", ing)
 	if err != nil {
 		// This is not the default 8086 port but the port usually used to expose
 		// influxdb in UDP, the module uses UDP to talk to influx via the line protocol.
-		influxdbPort = "8089"
+		config.InfluxDBPort = "8089"
 	}
 
-	influxdbHost, err := parser.GetStringAnnotation("influxdb-host", ing)
+	config.InfluxDBHost, err = parser.GetStringAnnotation("influxdb-host", ing)
 	if err != nil {
-		influxdbHost = "127.0.0.1"
+		config.InfluxDBHost = "127.0.0.1"
 	}
 
-	influxdbServerName, err := parser.GetStringAnnotation("influxdb-server-name", ing)
+	config.InfluxDBServerName, err = parser.GetStringAnnotation("influxdb-server-name", ing)
 	if err != nil {
-		influxdbServerName = "nginx-ingress"
+		config.InfluxDBServerName = "nginx-ingress"
 	}
 
-	return &Config{
-		InfluxDBEnabled:     influxdbEnabled,
-		InfluxDBMeasurement: influxdbMeasurement,
-		InfluxDBPort:        influxdbPort,
-		InfluxDBHost:        influxdbHost,
-		InfluxDBServerName:  influxdbServerName,
-	}, nil
+	return config, nil
 }
 
 // Equal tests for equality between two Config types

internal/ingress/annotations/log/main.go

@@ -54,15 +54,18 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation {
 // Parse parses the annotations contained in the ingress
 // rule used to indicate if the location/s should enable logs
 func (l log) Parse(ing *extensions.Ingress) (interface{}, error) {
-	accessEnabled, err := parser.GetBoolAnnotation("enable-access-log", ing)
+	var err error
+	config := &Config{}
+
+	config.Access, err = parser.GetBoolAnnotation("enable-access-log", ing)
 	if err != nil {
-		accessEnabled = true
+		config.Access = true
 	}
 
-	rewriteEnabled, err := parser.GetBoolAnnotation("enable-rewrite-log", ing)
+	config.Rewrite, err = parser.GetBoolAnnotation("enable-rewrite-log", ing)
 	if err != nil {
-		rewriteEnabled = false
+		config.Rewrite = false
 	}
 
-	return &Config{Access: accessEnabled, Rewrite: rewriteEnabled}, nil
+	return config, nil
 }

internal/ingress/annotations/luarestywaf/main.go

@@ -86,43 +86,38 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation {
 // used to indicate if the location/s contains a fragment of
 // configuration to be included inside the paths of the rules
 func (a luarestywaf) Parse(ing *extensions.Ingress) (interface{}, error) {
+	var err error
+	config := &Config{}
+
 	mode, err := parser.GetStringAnnotation("lua-resty-waf", ing)
 	if err != nil {
 		return &Config{}, err
 	}
 
-	mode = strings.ToUpper(mode)
-	if _, ok := luaRestyWAFModes[mode]; !ok {
+	config.Mode = strings.ToUpper(mode)
+	if _, ok := luaRestyWAFModes[config.Mode]; !ok {
 		return &Config{}, errors.NewInvalidAnnotationContent("lua-resty-waf", mode)
 	}
 
-	debug, _ := parser.GetBoolAnnotation("lua-resty-waf-debug", ing)
+	config.Debug, _ = parser.GetBoolAnnotation("lua-resty-waf-debug", ing)
 
 	ignoredRuleSetsStr, _ := parser.GetStringAnnotation("lua-resty-waf-ignore-rulesets", ing)
-	ignoredRuleSets := strings.FieldsFunc(ignoredRuleSetsStr, func(c rune) bool {
+	config.IgnoredRuleSets = strings.FieldsFunc(ignoredRuleSetsStr, func(c rune) bool {
 		strC := string(c)
 		return strC == "," || strC == " "
 	})
 
 	// TODO(elvinefendi) maybe validate the ruleset string here
-	extraRulesetString, _ := parser.GetStringAnnotation("lua-resty-waf-extra-rules", ing)
+	config.ExtraRulesetString, _ = parser.GetStringAnnotation("lua-resty-waf-extra-rules", ing)
 
-	scoreThreshold, _ := parser.GetIntAnnotation("lua-resty-waf-score-threshold", ing)
+	config.ScoreThreshold, _ = parser.GetIntAnnotation("lua-resty-waf-score-threshold", ing)
 
-	allowUnknownContentTypes, _ := parser.GetBoolAnnotation("lua-resty-waf-allow-unknown-content-types", ing)
+	config.AllowUnknownContentTypes, _ = parser.GetBoolAnnotation("lua-resty-waf-allow-unknown-content-types", ing)
 
-	processMultipartBody, err := parser.GetBoolAnnotation("lua-resty-waf-process-multipart-body", ing)
+	config.ProcessMultipartBody, err = parser.GetBoolAnnotation("lua-resty-waf-process-multipart-body", ing)
 	if err != nil {
-		processMultipartBody = true
+		config.ProcessMultipartBody = true
 	}
 
-	return &Config{
-		Mode:                     mode,
-		Debug:                    debug,
-		IgnoredRuleSets:          ignoredRuleSets,
-		ExtraRulesetString:       extraRulesetString,
-		ScoreThreshold:           scoreThreshold,
-		AllowUnknownContentTypes: allowUnknownContentTypes,
-		ProcessMultipartBody:     processMultipartBody,
-	}, nil
+	return config, nil
 }

internal/ingress/annotations/modsecurity/main.go

@@ -66,31 +66,28 @@ type modSecurity struct {
 // Parse parses the annotations contained in the ingress
 // rule used to enable ModSecurity in a particular location
 func (a modSecurity) Parse(ing *extensions.Ingress) (interface{}, error) {
+	var err error
+	config := &Config{}
 
-	enableModSecurity, err := parser.GetBoolAnnotation("enable-modsecurity", ing)
+	config.Enable, err = parser.GetBoolAnnotation("enable-modsecurity", ing)
 	if err != nil {
-		enableModSecurity = false
+		config.Enable = false
 	}
 
-	owaspRules, err := parser.GetBoolAnnotation("enable-owasp-core-rules", ing)
+	config.OWASPRules, err = parser.GetBoolAnnotation("enable-owasp-core-rules", ing)
 	if err != nil {
-		owaspRules = false
+		config.OWASPRules = false
 	}
 
-	transactionID, err := parser.GetStringAnnotation("modsecurity-transaction-id", ing)
+	config.TransactionID, err = parser.GetStringAnnotation("modsecurity-transaction-id", ing)
 	if err != nil {
-		transactionID = ""
+		config.TransactionID = ""
 	}
 
-	snippet, err := parser.GetStringAnnotation("modsecurity-snippet", ing)
+	config.Snippet, err = parser.GetStringAnnotation("modsecurity-snippet", ing)
 	if err != nil {
-		snippet = ""
+		config.Snippet = ""
 	}
 
-	return Config{
-		Enable:        enableModSecurity,
-		OWASPRules:    owaspRules,
-		TransactionID: transactionID,
-		Snippet:       snippet,
-	}, nil
+	return config, nil
 }

internal/ingress/annotations/modsecurity/main_test.go

@@ -70,7 +70,8 @@ func TestParse(t *testing.T) {
 	for _, testCase := range testCases {
 		ing.SetAnnotations(testCase.annotations)
 		result, _ := ap.Parse(ing)
-		if result != testCase.expected {
+		config := result.(*Config)
+		if !config.Equal(&testCase.expected) {
 			t.Errorf("expected %v but returned %v, annotations: %s", testCase.expected, result, testCase.annotations)
 		}
 	}

internal/ingress/annotations/parser/main.go

@@ -52,6 +52,10 @@ func (a ingAnnotations) parseBool(name string) (bool, error) {
 func (a ingAnnotations) parseString(name string) (string, error) {
 	val, ok := a[name]
 	if ok {
+		if len(val) == 0 {
+			return "", errors.NewInvalidAnnotationContent(name, val)
+		}
+
 		return val, nil
 	}
 	return "", errors.ErrMissingAnnotations
@@ -97,6 +101,7 @@ func GetStringAnnotation(name string, ing *extensions.Ingress) (string, error) {
 	if err != nil {
 		return "", err
 	}
+
 	return ingAnnotations(ing.GetAnnotations()).parseString(v)
 }
 

internal/ingress/annotations/parser/main_test.go

@@ -79,7 +79,7 @@ func TestGetStringAnnotation(t *testing.T) {
 
 	_, err := GetStringAnnotation("", nil)
 	if err == nil {
-		t.Errorf("expected error but retuned nil")
+		t.Errorf("expected error but none returned")
 	}
 
 	tests := []struct {
@@ -91,6 +91,7 @@ func TestGetStringAnnotation(t *testing.T) {
 	}{
 		{"valid - A", "string", "A", "A", false},
 		{"valid - B", "string", "B", "B", false},
+		{"empty", "string", "", "", true},
 	}
 
 	data := map[string]string{}
@@ -102,7 +103,7 @@ func TestGetStringAnnotation(t *testing.T) {
 		s, err := GetStringAnnotation(test.field, ing)
 		if test.expErr {
 			if err == nil {
-				t.Errorf("%v: expected error but retuned nil", test.name)
+				t.Errorf("%v: expected error but none returned", test.name)
 			}
 			continue
 		}