From 497246f8ba8064bf9be8587c8bd7a8e27b4784ec Mon Sep 17 00:00:00 2001 From: Manuel Alejandro de Brito Fontes Date: Sun, 2 Dec 2018 15:35:12 -0300 Subject: [PATCH] Annotations cannot being empty --- internal/ingress/annotations/authreq/main.go | 3 --- .../ingress/annotations/authreq/main_test.go | 4 ++++ internal/ingress/annotations/authtls/main.go | 6 +----- .../annotations/connection/main_test.go | 1 - internal/ingress/annotations/cors/main.go | 6 +++--- internal/ingress/annotations/parser/main.go | 5 +++++ .../ingress/annotations/parser/main_test.go | 5 +++-- .../annotations/sessionaffinity/main.go | 4 ++-- internal/ingress/controller/store/store.go | 20 +++++++++---------- 9 files changed, 28 insertions(+), 26 deletions(-) diff --git a/internal/ingress/annotations/authreq/main.go b/internal/ingress/annotations/authreq/main.go index 7cca4cad6..e6aebedc4 100644 --- a/internal/ingress/annotations/authreq/main.go +++ b/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 { diff --git a/internal/ingress/annotations/authreq/main_test.go b/internal/ingress/annotations/authreq/main_test.go index 6af03dae8..c9aebc678 100644 --- a/internal/ingress/annotations/authreq/main_test.go +++ b/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) diff --git a/internal/ingress/annotations/authtls/main.go b/internal/ingress/annotations/authtls/main.go index f3d965f44..cad3cc3e8 100644 --- a/internal/ingress/annotations/authtls/main.go +++ b/internal/ingress/annotations/authtls/main.go @@ -95,10 +95,6 @@ func (a authTLS) Parse(ing *extensions.Ingress) (interface{}, error) { 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()) @@ -122,7 +118,7 @@ func (a authTLS) Parse(ing *extensions.Ingress) (interface{}, error) { } config.ErrorPage, err = parser.GetStringAnnotation("auth-tls-error-page", ing) - if err != nil || config.ErrorPage == "" { + if err != nil { config.ErrorPage = "" } diff --git a/internal/ingress/annotations/connection/main_test.go b/internal/ingress/annotations/connection/main_test.go index 1900fc29d..67448ad9d 100644 --- a/internal/ingress/annotations/connection/main_test.go +++ b/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}}, diff --git a/internal/ingress/annotations/cors/main.go b/internal/ingress/annotations/cors/main.go index 683f1c69e..ff61aeb74 100644 --- a/internal/ingress/annotations/cors/main.go +++ b/internal/ingress/annotations/cors/main.go @@ -106,17 +106,17 @@ func (c cors) Parse(ing *extensions.Ingress) (interface{}, error) { } config.CorsAllowOrigin, err = parser.GetStringAnnotation("cors-allow-origin", ing) - if err != nil || config.CorsAllowOrigin == "" || !corsOriginRegex.MatchString(config.CorsAllowOrigin) { + if err != nil || !corsOriginRegex.MatchString(config.CorsAllowOrigin) { config.CorsAllowOrigin = "*" } config.CorsAllowHeaders, err = parser.GetStringAnnotation("cors-allow-headers", ing) - if err != nil || config.CorsAllowHeaders == "" || !corsHeadersRegex.MatchString(config.CorsAllowHeaders) { + if err != nil || !corsHeadersRegex.MatchString(config.CorsAllowHeaders) { config.CorsAllowHeaders = defaultCorsHeaders } config.CorsAllowMethods, err = parser.GetStringAnnotation("cors-allow-methods", ing) - if err != nil || config.CorsAllowMethods == "" || !corsMethodsRegex.MatchString(config.CorsAllowMethods) { + if err != nil || !corsMethodsRegex.MatchString(config.CorsAllowMethods) { config.CorsAllowMethods = defaultCorsMethods } diff --git a/internal/ingress/annotations/parser/main.go b/internal/ingress/annotations/parser/main.go index f167e83b6..2012354dc 100644 --- a/internal/ingress/annotations/parser/main.go +++ b/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) } diff --git a/internal/ingress/annotations/parser/main_test.go b/internal/ingress/annotations/parser/main_test.go index f65fbdbad..6b6365a96 100644 --- a/internal/ingress/annotations/parser/main_test.go +++ b/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 } diff --git a/internal/ingress/annotations/sessionaffinity/main.go b/internal/ingress/annotations/sessionaffinity/main.go index 0c110bd00..f408f2708 100644 --- a/internal/ingress/annotations/sessionaffinity/main.go +++ b/internal/ingress/annotations/sessionaffinity/main.go @@ -86,7 +86,7 @@ func (a affinity) cookieAffinityParse(ing *extensions.Ingress) *Cookie { cookie := &Cookie{} cookie.Name, err = parser.GetStringAnnotation(annotationAffinityCookieName, ing) - if err != nil || cookie.Name == "" { + if err != nil { glog.V(3).Infof("Ingress %v: No value found in annotation %v. Using the default %v", ing.Name, annotationAffinityCookieName, defaultAffinityCookieName) cookie.Name = defaultAffinityCookieName } @@ -110,7 +110,7 @@ func (a affinity) cookieAffinityParse(ing *extensions.Ingress) *Cookie { } cookie.Path, err = parser.GetStringAnnotation(annotationAffinityCookiePath, ing) - if err != nil || cookie.Path == "" { + if err != nil { glog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieMaxAge) } diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 4b0a1ef16..1d90688d1 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -310,7 +310,7 @@ func New(checkOCSP bool, } recorder.Eventf(ing, corev1.EventTypeNormal, "CREATE", fmt.Sprintf("Ingress %s/%s", ing.Namespace, ing.Name)) - store.extractAnnotations(ing) + store.syncIngress(ing) store.updateSecretIngressMap(ing) store.syncSecrets(ing) @@ -365,7 +365,7 @@ func New(checkOCSP bool, recorder.Eventf(curIng, corev1.EventTypeNormal, "UPDATE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name)) } - store.extractAnnotations(curIng) + store.syncIngress(curIng) store.updateSecretIngressMap(curIng) store.syncSecrets(curIng) @@ -394,7 +394,7 @@ func New(checkOCSP bool, glog.Errorf("could not find Ingress %v in local store", ingKey) continue } - store.extractAnnotations(ing) + store.syncIngress(ing) store.syncSecrets(ing) } updateCh.In() <- Event{ @@ -421,7 +421,7 @@ func New(checkOCSP bool, glog.Errorf("could not find Ingress %v in local store", ingKey) continue } - store.extractAnnotations(ing) + store.syncIngress(ing) store.syncSecrets(ing) } updateCh.In() <- Event{ @@ -460,7 +460,7 @@ func New(checkOCSP bool, glog.Errorf("could not find Ingress %v in local store", ingKey) continue } - store.extractAnnotations(ing) + store.syncIngress(ing) } updateCh.In() <- Event{ Type: DeleteEvent, @@ -530,7 +530,7 @@ func New(checkOCSP bool, glog.Errorf("could not find Ingress %v in local store: %v", key, err) continue } - store.extractAnnotations(ing) + store.syncIngress(ing) } updateCh.In() <- Event{ @@ -588,9 +588,9 @@ func New(checkOCSP bool, return store } -// extractAnnotations parses ingress annotations converting the value of the -// annotation to a go struct and also information about the referenced secrets -func (s *k8sStore) extractAnnotations(ing *extensions.Ingress) { +// syncIngress parses ingress annotations converting the value of the +// annotation to a go struct +func (s *k8sStore) syncIngress(ing *extensions.Ingress) { key := k8s.MetaNamespaceKey(ing) glog.V(3).Infof("updating annotations information for ingress %v", key) @@ -665,7 +665,7 @@ func (s *k8sStore) updateSecretIngressMap(ing *extensions.Ingress) { // 'namespace/name' key from the given annotation name. func objectRefAnnotationNsKey(ann string, ing *extensions.Ingress) (string, error) { annValue, err := parser.GetStringAnnotation(ann, ing) - if annValue == "" { + if err != nil { return "", err }