Merge pull request #82 from nginxinc/fix-parsing

Fix configmaps and annotation parsing

Author: pleshakov

nginx-controller/controller/controller.go

@@ -324,46 +324,55 @@ func (lbc *LoadBalancerController) syncCfgm(key string) {
 if serverNamesHashMaxSize, exists := cfgm.Data["server-names-hash-max-size"]; exists {
 cfg.MainServerNamesHashMaxSize = serverNamesHashMaxSize
 }
-HTTP2, err := nginx.GetMapKeyAsBool(cfgm.Data, "http2", cfgm)
-if err != nil && err != nginx.ErrorKeyNotFound {
-glog.Error(err)
-} else {
-cfg.HTTP2 = HTTP2
+if HTTP2, exists, err := nginx.GetMapKeyAsBool(cfgm.Data, "http2", cfgm); exists {
+if err != nil {
+glog.Error(err)
+} else {
+cfg.HTTP2 = HTTP2
+}
 }
 
 // HSTS block
-HSTSErrors := false
-HSTS, err := nginx.GetMapKeyAsBool(cfgm.Data, "hsts", cfgm)
-if err != nil && err != nginx.ErrorKeyNotFound {
-glog.Error(err)
-HSTSErrors = true
-}
-HSTSMaxAge, err := nginx.GetMapKeyAsInt(cfgm.Data, "hsts-max-age", cfgm)
-if err != nil && err != nginx.ErrorKeyNotFound {
-glog.Error(err)
-HSTSErrors = true
-}
-HSTSIncludeSubdomains, err := nginx.GetMapKeyAsBool(cfgm.Data, "hsts-include-subdomains", cfgm)
-if err != nil && err != nginx.ErrorKeyNotFound {
-glog.Error(err)
-HSTSErrors = true
-}
-if HSTSErrors {
-glog.Warningf("Configmap %s/%s: There are configuration issues with hsts annotations, skipping options for all hsts settings", cfgm.GetNamespace(), cfgm.GetName())
-} else {
-cfg.HSTS = HSTS
-cfg.HSTSMaxAge = HSTSMaxAge
-cfg.HSTSIncludeSubdomains = HSTSIncludeSubdomains
+if hsts, exists, err := nginx.GetMapKeyAsBool(cfgm.Data, "hsts", cfgm); exists {
+if err != nil {
+glog.Error(err)
+} else {
+parsingErrors := false
+
+hstsMaxAge, existsMA, err := nginx.GetMapKeyAsInt(cfgm.Data, "hsts-max-age", cfgm)
+if existsMA && err != nil {
+glog.Error(err)
+parsingErrors = true
+}
+hstsIncludeSubdomains, existsIS, err := nginx.GetMapKeyAsBool(cfgm.Data, "hsts-include-subdomains", cfgm)
+if existsIS && err != nil {
+glog.Error(err)
+parsingErrors = true
+}
+
+if parsingErrors {
+glog.Errorf("Configmap %s/%s: There are configuration issues with hsts annotations, skipping options for all hsts settings", cfgm.GetNamespace(), cfgm.GetName())
+} else {
+cfg.HSTS = hsts
+if existsMA {
+cfg.HSTSMaxAge = hstsMaxAge
+}
+if existsIS {
+cfg.HSTSIncludeSubdomains = hstsIncludeSubdomains
+}
+}
+}
 }
 
 if logFormat, exists := cfgm.Data["log-format"]; exists {
 cfg.MainLogFormat = logFormat
 }
-ProxyBuffering, err := nginx.GetMapKeyAsBool(cfgm.Data, "proxy-buffering", cfgm)
-if err != nil && err != nginx.ErrorKeyNotFound {
-glog.Error(err)
-} else {
-cfg.ProxyBuffering = ProxyBuffering
+if proxyBuffering, exists, err := nginx.GetMapKeyAsBool(cfgm.Data, "proxy-buffering", cfgm); exists {
+if err != nil {
+glog.Error(err)
+} else {
+cfg.ProxyBuffering = proxyBuffering
+}
 }
 if proxyBuffers, exists := cfgm.Data["proxy-buffers"]; exists {
 cfg.ProxyBuffers = proxyBuffers

nginx-controller/nginx/configurator.go

@@ -185,42 +185,50 @@ func (cnf *Configurator) createConfig(ingEx *IngressEx) Config {
 if clientMaxBodySize, exists := ingEx.Ingress.Annotations["nginx.org/client-max-body-size"]; exists {
 ingCfg.ClientMaxBodySize = clientMaxBodySize
 }
-HTTP2, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/http2", ingEx.Ingress)
-if err != nil && err != ErrorKeyNotFound {
-glog.Error(err)
-} else {
-ingCfg.HTTP2 = HTTP2
+if HTTP2, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/http2", ingEx.Ingress); exists {
+if err != nil {
+glog.Error(err)
+} else {
+ingCfg.HTTP2 = HTTP2
+}
 }
-ProxyBuffering, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/proxy-buffering", ingEx.Ingress)
-if err != nil && err != ErrorKeyNotFound {
-glog.Error(err)
-} else {
-ingCfg.ProxyBuffering = ProxyBuffering
+if proxyBuffering, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/proxy-buffering", ingEx.Ingress); exists {
+if err != nil {
+glog.Error(err)
+} else {
+ingCfg.ProxyBuffering = proxyBuffering
+}
 }
 
-// HSTS block
-HSTSErrors := false
-HSTS, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/hsts", ingEx.Ingress)
-if err != nil && err != ErrorKeyNotFound {
-glog.Error(err)
-HSTSErrors = true
-}
-HSTSMaxAge, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/hsts-max-age", ingEx.Ingress)
-if err != nil && err != ErrorKeyNotFound {
-glog.Error(err)
-HSTSErrors = true
-}
-HSTSIncludeSubdomains, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/hsts-include-subdomains", ingEx.Ingress)
-if err != nil && err != ErrorKeyNotFound {
-glog.Error(err)
-HSTSErrors = true
-}
-if HSTSErrors {
-glog.Warningf("Ingress %s/%s: There are configuration issues with hsts annotations, skipping annotions for all hsts settings", ingEx.Ingress.GetNamespace(), ingEx.Ingress.GetName())
-} else {
-ingCfg.HSTS = HSTS
-ingCfg.HSTSMaxAge = HSTSMaxAge
-ingCfg.HSTSIncludeSubdomains = HSTSIncludeSubdomains
+if hsts, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/hsts", ingEx.Ingress); exists {
+if err != nil {
+glog.Error(err)
+} else {
+parsingErrors := false
+
+hstsMaxAge, existsMA, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/hsts-max-age", ingEx.Ingress)
+if existsMA && err != nil {
+glog.Error(err)
+parsingErrors = true
+}
+hstsIncludeSubdomains, existsIS, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/hsts-include-subdomains", ingEx.Ingress)
+if existsIS && err != nil {
+glog.Error(err)
+parsingErrors = true
+}
+
+if parsingErrors {
+glog.Errorf("Ingress %s/%s: There are configuration issues with hsts annotations, skipping annotions for all hsts settings", ingEx.Ingress.GetNamespace(), ingEx.Ingress.GetName())
+} else {
+ingCfg.HSTS = hsts
+if existsMA {
+ingCfg.HSTSMaxAge = hstsMaxAge
+}
+if existsIS {
+ingCfg.HSTSIncludeSubdomains = hstsIncludeSubdomains
+}
+}
+}
 }
 
 if proxyBuffers, exists := ingEx.Ingress.Annotations["nginx.org/proxy-buffers"]; exists {

nginx-controller/nginx/convert.go

@@ -1,19 +1,13 @@
 package nginx
 
 import (
-"errors"
 "fmt"
 "strconv"
 
 "k8s.io/kubernetes/pkg/api/meta"
 "k8s.io/kubernetes/pkg/runtime"
 )
 
-var (
-// ErrorKeyNotFound is returned if the key was not found in the map
-ErrorKeyNotFound = errors.New("Key not found in map")
-)
-
 // There seems to be no composite interface in the kubernetes api package,
 // so we have to declare our own.
 type apiObject interface {
@@ -22,25 +16,25 @@ type apiObject interface {
 }
 
 // GetMapKeyAsBool searches the map for the given key and parses the key as bool
-func GetMapKeyAsBool(m map[string]string, key string, context apiObject) (bool, error) {
+func GetMapKeyAsBool(m map[string]string, key string, context apiObject) (bool, bool, error) {
 if str, exists := m[key]; exists {
 b, err := strconv.ParseBool(str)
 if err != nil {
-return false, fmt.Errorf("%s %v/%v '%s' contains invalid bool: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err)
+return false, exists, fmt.Errorf("%s %v/%v '%s' contains invalid bool: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err)
 }
-return b, nil
+return b, exists, nil
 }
-return false, ErrorKeyNotFound
+return false, false, nil
 }
 
 // GetMapKeyAsInt tries to find and parse a key in a map as int64
-func GetMapKeyAsInt(m map[string]string, key string, context apiObject) (int64, error) {
+func GetMapKeyAsInt(m map[string]string, key string, context apiObject) (int64, bool, error) {
 if str, exists := m[key]; exists {
 i, err := strconv.ParseInt(str, 10, 64)
 if err != nil {
-return 0, fmt.Errorf("%s %v/%v '%s' contains invalid integer: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err)
+return 0, exists, fmt.Errorf("%s %v/%v '%s' contains invalid integer: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err)
 }
-return i, nil
+return i, exists, nil
 }
-return 0, ErrorKeyNotFound
+return 0, false, nil
 }

nginx-controller/nginx/convert_test.go

@@ -38,7 +38,10 @@ func TestGetMapKeyAsBool(t *testing.T) {
 "key": "True",
 }
 
-b, err := GetMapKeyAsBool(configMap.Data, "key", &configMap)
+b, exists, err := GetMapKeyAsBool(configMap.Data, "key", &configMap)
+if !exists {
+t.Errorf("The key 'key' must exist in the configMap")
+}
 if err != nil {
 t.Errorf("Unexpected error: %v", err)
 }
@@ -51,9 +54,9 @@ func TestGetMapKeyAsBoolNotFound(t *testing.T) {
 configMap := configMap
 configMap.Data = map[string]string{}
 
-_, err := GetMapKeyAsBool(configMap.Data, "key", &configMap)
-if err != ErrorKeyNotFound {
-t.Errorf("ErrorKeyNotFound was expected, got: %v", err)
+_, exists, _ := GetMapKeyAsBool(configMap.Data, "key", &configMap)
+if exists {
+t.Errorf("The key 'key' must not exist in the configMap")
 }
 }
 
@@ -64,7 +67,7 @@ func TestGetMapKeyAsBoolErrorMessage(t *testing.T) {
 }
 
 // Test with configmap
-_, err := GetMapKeyAsBool(cfgm.Data, "key", &cfgm)
+_, _, err := GetMapKeyAsBool(cfgm.Data, "key", &cfgm)
 if err == nil {
 t.Error("An error was expected")
 }
@@ -78,7 +81,7 @@ func TestGetMapKeyAsBoolErrorMessage(t *testing.T) {
 ingress.Annotations = map[string]string{
 "key": "other_string",
 }
-_, err = GetMapKeyAsBool(ingress.Annotations, "key", &ingress)
+_, _, err = GetMapKeyAsBool(ingress.Annotations, "key", &ingress)
 if err == nil {
 t.Error("An error was expected")
 }
@@ -97,10 +100,13 @@ func TestGetMapKeyAsInt(t *testing.T) {
 "key": "123456789",
 }
 
-i, err := GetMapKeyAsInt(configMap.Data, "key", &configMap)
+i, exists, err := GetMapKeyAsInt(configMap.Data, "key", &configMap)
 if err != nil {
 t.Errorf("Unexpected error: %v", err)
 }
+if !exists {
+t.Errorf("The key 'key' must exist in the configMap")
+}
 var expected int64 = 123456789
 if i != expected {
 t.Errorf("Unexpected return value:\nGot: %v\nExpected: %v", i, expected)
@@ -111,9 +117,9 @@ func TestGetMapKeyAsIntNotFound(t *testing.T) {
 configMap := configMap
 configMap.Data = map[string]string{}
 
-_, err := GetMapKeyAsInt(configMap.Data, "key", &configMap)
-if err != ErrorKeyNotFound {
-t.Errorf("ErrorKeyNotFound was expected, got: %v", err)
+_, exists, _ := GetMapKeyAsInt(configMap.Data, "key", &configMap)
+if exists {
+t.Errorf("The key 'key' must not exist in the configMap")
 }
 }
 
@@ -124,7 +130,7 @@ func TestGetMapKeyAsIntErrorMessage(t *testing.T) {
 }
 
 // Test with configmap
-_, err := GetMapKeyAsInt(cfgm.Data, "key", &cfgm)
+_, _, err := GetMapKeyAsInt(cfgm.Data, "key", &cfgm)
 if err == nil {
 t.Error("An error was expected")
 }
@@ -138,7 +144,7 @@ func TestGetMapKeyAsIntErrorMessage(t *testing.T) {
 ingress.Annotations = map[string]string{
 "key": "other_string",
 }
-_, err = GetMapKeyAsInt(ingress.Annotations, "key", &ingress)
+_, _, err = GetMapKeyAsInt(ingress.Annotations, "key", &ingress)
 if err == nil {
 t.Error("An error was expected")
 }