From 67808c0ed8d4cce8de5ebce742e72cfc4a7d04fe Mon Sep 17 00:00:00 2001 From: Manuel Alejandro de Brito Fontes Date: Fri, 30 Nov 2018 19:56:11 -0300 Subject: [PATCH 1/3] Improve parsing of annotations and use of Ingress wrapper --- internal/ingress/annotations/authtls/main.go | 41 +++--- internal/ingress/annotations/cors/main.go | 42 +++--- internal/ingress/annotations/influxdb/main.go | 31 ++-- internal/ingress/annotations/log/main.go | 13 +- .../ingress/annotations/luarestywaf/main.go | 31 ++-- .../ingress/annotations/modsecurity/main.go | 25 ++-- .../annotations/modsecurity/main_test.go | 3 +- internal/ingress/annotations/proxy/main.go | 97 +++++++------ internal/ingress/annotations/rewrite/main.go | 41 +++--- .../annotations/sessionaffinity/main.go | 37 +++-- .../controller/store/ingress_annotation.go | 12 +- internal/ingress/controller/store/store.go | 105 ++++++-------- .../ingress/controller/store/store_test.go | 134 ++++++++---------- 13 files changed, 278 insertions(+), 334 deletions(-) diff --git a/internal/ingress/annotations/authtls/main.go b/internal/ingress/annotations/authtls/main.go index f498dfb80..f3d965f44 100644 --- a/internal/ingress/annotations/authtls/main.go +++ b/internal/ingress/annotations/authtls/main.go @@ -87,6 +87,8 @@ 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 { @@ -102,37 +104,32 @@ func (a authTLS) Parse(ing *extensions.Ingress) (interface{}, error) { 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 } - passCert, err := parser.GetBoolAnnotation("auth-tls-pass-certificate-to-upstream", ing) + 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 == "" { + config.ErrorPage = "" + } + + 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 } diff --git a/internal/ingress/annotations/cors/main.go b/internal/ingress/annotations/cors/main.go index b882018ae..683f1c69e 100644 --- a/internal/ingress/annotations/cors/main.go +++ b/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 || config.CorsAllowOrigin == "" || !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 || config.CorsAllowHeaders == "" || !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 || config.CorsAllowMethods == "" || !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 } diff --git a/internal/ingress/annotations/influxdb/main.go b/internal/ingress/annotations/influxdb/main.go index d8125ec44..4f1565693 100644 --- a/internal/ingress/annotations/influxdb/main.go +++ b/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 diff --git a/internal/ingress/annotations/log/main.go b/internal/ingress/annotations/log/main.go index d744c3c2f..8235f2c1f 100644 --- a/internal/ingress/annotations/log/main.go +++ b/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 } diff --git a/internal/ingress/annotations/luarestywaf/main.go b/internal/ingress/annotations/luarestywaf/main.go index c29a97e9c..39ac19382 100644 --- a/internal/ingress/annotations/luarestywaf/main.go +++ b/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 } diff --git a/internal/ingress/annotations/modsecurity/main.go b/internal/ingress/annotations/modsecurity/main.go index a5058bb68..33a825e6a 100644 --- a/internal/ingress/annotations/modsecurity/main.go +++ b/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 } diff --git a/internal/ingress/annotations/modsecurity/main_test.go b/internal/ingress/annotations/modsecurity/main_test.go index 44d51194e..691c0b82a 100644 --- a/internal/ingress/annotations/modsecurity/main_test.go +++ b/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) } } diff --git a/internal/ingress/annotations/proxy/main.go b/internal/ingress/annotations/proxy/main.go index bfe7e4bba..b8844ed60 100644 --- a/internal/ingress/annotations/proxy/main.go +++ b/internal/ingress/annotations/proxy/main.go @@ -103,72 +103,75 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation { // ParseAnnotations parses the annotations contained in the ingress // rule used to configure upstream check parameters func (a proxy) Parse(ing *extensions.Ingress) (interface{}, error) { - defBackend := a.r.GetDefaultBackend() - ct, err := parser.GetIntAnnotation("proxy-connect-timeout", ing) + config := &Config{} + + var err error + + config.ConnectTimeout, err = parser.GetIntAnnotation("proxy-connect-timeout", ing) if err != nil { - ct = defBackend.ProxyConnectTimeout + config.ConnectTimeout = defBackend.ProxyConnectTimeout } - st, err := parser.GetIntAnnotation("proxy-send-timeout", ing) + config.SendTimeout, err = parser.GetIntAnnotation("proxy-send-timeout", ing) if err != nil { - st = defBackend.ProxySendTimeout + config.SendTimeout = defBackend.ProxySendTimeout } - rt, err := parser.GetIntAnnotation("proxy-read-timeout", ing) + config.ReadTimeout, err = parser.GetIntAnnotation("proxy-read-timeout", ing) if err != nil { - rt = defBackend.ProxyReadTimeout + config.ReadTimeout = defBackend.ProxyReadTimeout } - bufs, err := parser.GetStringAnnotation("proxy-buffer-size", ing) - if err != nil || bufs == "" { - bufs = defBackend.ProxyBufferSize - } - - cp, err := parser.GetStringAnnotation("proxy-cookie-path", ing) - if err != nil || cp == "" { - cp = defBackend.ProxyCookiePath - } - - cd, err := parser.GetStringAnnotation("proxy-cookie-domain", ing) - if err != nil || cd == "" { - cd = defBackend.ProxyCookieDomain - } - - bs, err := parser.GetStringAnnotation("proxy-body-size", ing) - if err != nil || bs == "" { - bs = defBackend.ProxyBodySize - } - - nu, err := parser.GetStringAnnotation("proxy-next-upstream", ing) - if err != nil || nu == "" { - nu = defBackend.ProxyNextUpstream - } - - nut, err := parser.GetIntAnnotation("proxy-next-upstream-tries", ing) + config.BufferSize, err = parser.GetStringAnnotation("proxy-buffer-size", ing) if err != nil { - nut = defBackend.ProxyNextUpstreamTries + config.BufferSize = defBackend.ProxyBufferSize } - rb, err := parser.GetStringAnnotation("proxy-request-buffering", ing) - if err != nil || rb == "" { - rb = defBackend.ProxyRequestBuffering + config.CookiePath, err = parser.GetStringAnnotation("proxy-cookie-path", ing) + if err != nil { + config.CookiePath = defBackend.ProxyCookiePath } - prf, err := parser.GetStringAnnotation("proxy-redirect-from", ing) - if err != nil || prf == "" { - prf = defBackend.ProxyRedirectFrom + config.CookieDomain, err = parser.GetStringAnnotation("proxy-cookie-domain", ing) + if err != nil { + config.CookieDomain = defBackend.ProxyCookieDomain } - prt, err := parser.GetStringAnnotation("proxy-redirect-to", ing) - if err != nil || rb == "" { - prt = defBackend.ProxyRedirectTo + config.BodySize, err = parser.GetStringAnnotation("proxy-body-size", ing) + if err != nil { + config.BodySize = defBackend.ProxyBodySize } - pb, err := parser.GetStringAnnotation("proxy-buffering", ing) - if err != nil || pb == "" { - pb = defBackend.ProxyBuffering + config.NextUpstream, err = parser.GetStringAnnotation("proxy-next-upstream", ing) + if err != nil { + config.NextUpstream = defBackend.ProxyNextUpstream } - return &Config{bs, ct, st, rt, bufs, cd, cp, nu, nut, prf, prt, rb, pb}, nil + config.NextUpstreamTries, err = parser.GetIntAnnotation("proxy-next-upstream-tries", ing) + if err != nil { + config.NextUpstreamTries = defBackend.ProxyNextUpstreamTries + } + + config.RequestBuffering, err = parser.GetStringAnnotation("proxy-request-buffering", ing) + if err != nil { + config.RequestBuffering = defBackend.ProxyRequestBuffering + } + + config.ProxyRedirectFrom, err = parser.GetStringAnnotation("proxy-redirect-from", ing) + if err != nil { + config.ProxyRedirectFrom = defBackend.ProxyRedirectFrom + } + + config.ProxyRedirectTo, err = parser.GetStringAnnotation("proxy-redirect-to", ing) + if err != nil { + config.ProxyRedirectTo = defBackend.ProxyRedirectTo + } + + config.ProxyBuffering, err = parser.GetStringAnnotation("proxy-buffering", ing) + if err != nil { + config.ProxyBuffering = defBackend.ProxyBuffering + } + + return config, nil } diff --git a/internal/ingress/annotations/rewrite/main.go b/internal/ingress/annotations/rewrite/main.go index acbe55650..bafbbdd9c 100644 --- a/internal/ingress/annotations/rewrite/main.go +++ b/internal/ingress/annotations/rewrite/main.go @@ -87,27 +87,24 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation { // ParseAnnotations parses the annotations contained in the ingress // rule used to rewrite the defined paths func (a rewrite) Parse(ing *extensions.Ingress) (interface{}, error) { - rt, _ := parser.GetStringAnnotation("rewrite-target", ing) - sslRe, err := parser.GetBoolAnnotation("ssl-redirect", ing) - if err != nil { - sslRe = a.r.GetDefaultBackend().SSLRedirect - } - fSslRe, err := parser.GetBoolAnnotation("force-ssl-redirect", ing) - if err != nil { - fSslRe = a.r.GetDefaultBackend().ForceSSLRedirect - } - abu, _ := parser.GetBoolAnnotation("add-base-url", ing) - bus, _ := parser.GetStringAnnotation("base-url-scheme", ing) - ar, _ := parser.GetStringAnnotation("app-root", ing) - ur, _ := parser.GetBoolAnnotation("use-regex", ing) + var err error + config := &Config{} - return &Config{ - Target: rt, - AddBaseURL: abu, - BaseURLScheme: bus, - SSLRedirect: sslRe, - ForceSSLRedirect: fSslRe, - AppRoot: ar, - UseRegex: ur, - }, nil + config.Target, _ = parser.GetStringAnnotation("rewrite-target", ing) + config.SSLRedirect, err = parser.GetBoolAnnotation("ssl-redirect", ing) + if err != nil { + config.SSLRedirect = a.r.GetDefaultBackend().SSLRedirect + } + + config.ForceSSLRedirect, err = parser.GetBoolAnnotation("force-ssl-redirect", ing) + if err != nil { + config.ForceSSLRedirect = a.r.GetDefaultBackend().ForceSSLRedirect + } + + config.AddBaseURL, _ = parser.GetBoolAnnotation("add-base-url", ing) + config.BaseURLScheme, _ = parser.GetStringAnnotation("base-url-scheme", ing) + config.AppRoot, _ = parser.GetStringAnnotation("app-root", ing) + config.UseRegex, _ = parser.GetBoolAnnotation("use-regex", ing) + + return config, nil } diff --git a/internal/ingress/annotations/sessionaffinity/main.go b/internal/ingress/annotations/sessionaffinity/main.go index 2f9b7e41c..0c110bd00 100644 --- a/internal/ingress/annotations/sessionaffinity/main.go +++ b/internal/ingress/annotations/sessionaffinity/main.go @@ -81,42 +81,39 @@ type Cookie struct { // cookieAffinityParse gets the annotation values related to Cookie Affinity // It also sets default values when no value or incorrect value is found func (a affinity) cookieAffinityParse(ing *extensions.Ingress) *Cookie { + var err error + cookie := &Cookie{} - sn, err := parser.GetStringAnnotation(annotationAffinityCookieName, ing) - if err != nil || sn == "" { + cookie.Name, err = parser.GetStringAnnotation(annotationAffinityCookieName, ing) + if err != nil || cookie.Name == "" { glog.V(3).Infof("Ingress %v: No value found in annotation %v. Using the default %v", ing.Name, annotationAffinityCookieName, defaultAffinityCookieName) - sn = defaultAffinityCookieName + cookie.Name = defaultAffinityCookieName } - cookie.Name = sn - sh, err := parser.GetStringAnnotation(annotationAffinityCookieHash, ing) - - if err != nil || !affinityCookieHashRegex.MatchString(sh) { + cookie.Hash, err = parser.GetStringAnnotation(annotationAffinityCookieHash, ing) + if err != nil || !affinityCookieHashRegex.MatchString(cookie.Hash) { glog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Setting it to default %v", ing.Name, annotationAffinityCookieHash, defaultAffinityCookieHash) - sh = defaultAffinityCookieHash + cookie.Hash = defaultAffinityCookieHash } - cookie.Hash = sh - se, err := parser.GetStringAnnotation(annotationAffinityCookieExpires, ing) - if err != nil || !affinityCookieExpiresRegex.MatchString(se) { + cookie.Expires, err = parser.GetStringAnnotation(annotationAffinityCookieExpires, ing) + if err != nil || !affinityCookieExpiresRegex.MatchString(cookie.Expires) { glog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieExpires) - se = "" + cookie.Expires = "" } - cookie.Expires = se - sm, err := parser.GetStringAnnotation(annotationAffinityCookieMaxAge, ing) - if err != nil || !affinityCookieExpiresRegex.MatchString(sm) { + cookie.MaxAge, err = parser.GetStringAnnotation(annotationAffinityCookieMaxAge, ing) + if err != nil || !affinityCookieExpiresRegex.MatchString(cookie.MaxAge) { glog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieMaxAge) - sm = "" + cookie.MaxAge = "" } - cookie.MaxAge = sm - sp, err := parser.GetStringAnnotation(annotationAffinityCookiePath, ing) - if err != nil || sp == "" { + cookie.Path, err = parser.GetStringAnnotation(annotationAffinityCookiePath, ing) + if err != nil || cookie.Path == "" { glog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieMaxAge) } - cookie.Path = sp + return cookie } diff --git a/internal/ingress/controller/store/ingress_annotation.go b/internal/ingress/controller/store/ingress_annotation.go index 0a9d4011c..c9c596d1e 100644 --- a/internal/ingress/controller/store/ingress_annotation.go +++ b/internal/ingress/controller/store/ingress_annotation.go @@ -18,16 +18,16 @@ package store import ( "k8s.io/client-go/tools/cache" - "k8s.io/ingress-nginx/internal/ingress/annotations" + "k8s.io/ingress-nginx/internal/ingress" ) -// IngressAnnotationsLister makes a Store that lists annotations in Ingress rules. -type IngressAnnotationsLister struct { +// IngressWithAnnotationsLister makes a Store that lists Ingress rules with annotations already parsed +type IngressWithAnnotationsLister struct { cache.Store } -// ByKey returns the Ingress annotations matching key in the local Ingress annotations Store. -func (il IngressAnnotationsLister) ByKey(key string) (*annotations.Ingress, error) { +// ByKey returns the Ingress with annotations matching key in the local store or an error +func (il IngressWithAnnotationsLister) ByKey(key string) (*ingress.Ingress, error) { i, exists, err := il.GetByKey(key) if err != nil { return nil, err @@ -35,5 +35,5 @@ func (il IngressAnnotationsLister) ByKey(key string) (*annotations.Ingress, erro if !exists { return nil, NotExistsError(key) } - return i.(*annotations.Ingress), nil + return i.(*ingress.Ingress), nil } diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 72ec71392..e77b6c097 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -73,9 +73,6 @@ type Storer interface { // GetServiceEndpoints returns the Endpoints of a Service matching key. GetServiceEndpoints(key string) (*corev1.Endpoints, error) - // GetIngress returns the Ingress matching key. - GetIngress(key string) (*extensions.Ingress, error) - // ListIngresses returns a list of all Ingresses in the store. ListIngresses() []*ingress.Ingress @@ -132,13 +129,13 @@ type Informer struct { // Lister contains object listers (stores). type Lister struct { - Ingress IngressLister - Service ServiceLister - Endpoint EndpointLister - Secret SecretLister - ConfigMap ConfigMapLister - IngressAnnotation IngressAnnotationsLister - Pod PodLister + Ingress IngressLister + Service ServiceLister + Endpoint EndpointLister + Secret SecretLister + ConfigMap ConfigMapLister + IngressWithAnnotation IngressWithAnnotationsLister + Pod PodLister } // NotExistsError is returned when an object does not exist in a local store. @@ -261,7 +258,7 @@ func New(checkOCSP bool, // k8sStore fulfills resolver.Resolver interface store.annotations = annotations.NewAnnotationExtractor(store) - store.listers.IngressAnnotation.Store = cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc) + store.listers.IngressWithAnnotation.Store = cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc) // create informers factory, enable and assign required informers infFactory := informers.NewSharedInformerFactoryWithOptions(client, resyncPeriod, @@ -343,7 +340,7 @@ func New(checkOCSP bool, } recorder.Eventf(ing, corev1.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", ing.Namespace, ing.Name)) - store.listers.IngressAnnotation.Delete(ing) + store.listers.IngressWithAnnotation.Delete(ing) key := k8s.MetaNamespaceKey(ing) store.secretIngressMap.Delete(key) @@ -392,7 +389,7 @@ func New(checkOCSP bool, if ings := store.secretIngressMap.Reference(key); len(ings) > 0 { glog.Infof("secret %v was added and it is used in ingress annotations. Parsing...", key) for _, ingKey := range ings { - ing, err := store.GetIngress(ingKey) + ing, err := store.getIngress(ingKey) if err != nil { glog.Errorf("could not find Ingress %v in local store", ingKey) continue @@ -419,7 +416,7 @@ func New(checkOCSP bool, if ings := store.secretIngressMap.Reference(key); len(ings) > 0 { glog.Infof("secret %v was updated and it is used in ingress annotations. Parsing...", key) for _, ingKey := range ings { - ing, err := store.GetIngress(ingKey) + ing, err := store.getIngress(ingKey) if err != nil { glog.Errorf("could not find Ingress %v in local store", ingKey) continue @@ -458,7 +455,7 @@ func New(checkOCSP bool, if ings := store.secretIngressMap.Reference(key); len(ings) > 0 { glog.Infof("secret %v was deleted and it is used in ingress annotations. Parsing...", key) for _, ingKey := range ings { - ing, err := store.GetIngress(ingKey) + ing, err := store.getIngress(ingKey) if err != nil { glog.Errorf("could not find Ingress %v in local store", ingKey) continue @@ -525,10 +522,10 @@ func New(checkOCSP bool, store.setConfig(cm) } - ings := store.listers.IngressAnnotation.List() + ings := store.listers.IngressWithAnnotation.List() for _, ingKey := range ings { key := k8s.MetaNamespaceKey(ingKey) - ing, err := store.GetIngress(key) + ing, err := store.getIngress(key) if err != nil { glog.Errorf("could not find Ingress %v in local store: %v", key, err) continue @@ -597,9 +594,24 @@ func (s *k8sStore) extractAnnotations(ing *extensions.Ingress) { key := k8s.MetaNamespaceKey(ing) glog.V(3).Infof("updating annotations information for ingress %v", key) - anns := s.annotations.Extract(ing) + copyIng := ing.DeepCopy() - err := s.listers.IngressAnnotation.Update(anns) + for ri, rule := range copyIng.Spec.Rules { + if rule.HTTP == nil { + continue + } + + for pi, path := range rule.HTTP.Paths { + if path.Path == "" { + copyIng.Spec.Rules[ri].HTTP.Paths[pi].Path = "/" + } + } + } + + err := s.listers.IngressWithAnnotation.Update(&ingress.Ingress{ + Ingress: *copyIng, + ParsedAnnotations: s.annotations.Extract(copyIng), + }) if err != nil { glog.Error(err) } @@ -697,59 +709,28 @@ func (s k8sStore) GetService(key string) (*corev1.Service, error) { return s.listers.Service.ByKey(key) } -// GetIngress returns the Ingress matching key. -func (s k8sStore) GetIngress(key string) (*extensions.Ingress, error) { - return s.listers.Ingress.ByKey(key) +// getIngress returns the Ingress matching key. +func (s k8sStore) getIngress(key string) (*extensions.Ingress, error) { + ing, err := s.listers.IngressWithAnnotation.ByKey(key) + if err != nil { + return nil, err + } + + return &ing.Ingress, nil } // ListIngresses returns the list of Ingresses func (s k8sStore) ListIngresses() []*ingress.Ingress { // filter ingress rules - var ingresses []*ingress.Ingress - for _, item := range s.listers.Ingress.List() { - ing := item.(*extensions.Ingress) - if !class.IsValid(ing) { - continue - } - - ingKey := k8s.MetaNamespaceKey(ing) - anns, err := s.getIngressAnnotations(ingKey) - if err != nil { - glog.Errorf("Error getting Ingress annotations %q: %v", ingKey, err) - - } - - for ri, rule := range ing.Spec.Rules { - if rule.HTTP == nil { - continue - } - - for pi, path := range rule.HTTP.Paths { - if path.Path == "" { - ing.Spec.Rules[ri].HTTP.Paths[pi].Path = "/" - } - } - } - - ingresses = append(ingresses, &ingress.Ingress{ - Ingress: *ing, - ParsedAnnotations: anns, - }) + ingresses := make([]*ingress.Ingress, 0) + for _, item := range s.listers.IngressWithAnnotation.List() { + ing := item.(*ingress.Ingress) + ingresses = append(ingresses, ing) } return ingresses } -// getIngressAnnotations returns the parsed annotations of an Ingress matching key. -func (s k8sStore) getIngressAnnotations(key string) (*annotations.Ingress, error) { - ia, err := s.listers.IngressAnnotation.ByKey(key) - if err != nil { - return &annotations.Ingress{}, err - } - - return ia, nil -} - // GetLocalSSLCert returns the local copy of a SSLCert func (s k8sStore) GetLocalSSLCert(key string) (*ingress.SSLCert, error) { return s.sslStore.ByKey(key) diff --git a/internal/ingress/controller/store/store_test.go b/internal/ingress/controller/store/store_test.go index a14cc8121..3f7519bd0 100644 --- a/internal/ingress/controller/store/store_test.go +++ b/internal/ingress/controller/store/store_test.go @@ -38,6 +38,7 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" "k8s.io/ingress-nginx/internal/file" + "k8s.io/ingress-nginx/internal/ingress" "k8s.io/ingress-nginx/internal/ingress/annotations/parser" "k8s.io/ingress-nginx/internal/k8s" "k8s.io/ingress-nginx/test/e2e/framework" @@ -86,14 +87,6 @@ func TestStore(t *testing.T) { storer.Run(stopCh) key := fmt.Sprintf("%v/anything", ns) - ing, err := storer.GetIngress(key) - if err == nil { - t.Errorf("expected an error but none returned") - } - if ing != nil { - t.Errorf("expected an Ingres but none returned") - } - ls, err := storer.GetLocalSSLCert(key) if err == nil { t.Errorf("expected an error but none returned") @@ -753,9 +746,9 @@ func newStore(t *testing.T) *k8sStore { return &k8sStore{ listers: &Lister{ // add more listers if needed - Ingress: IngressLister{cache.NewStore(cache.MetaNamespaceKeyFunc)}, - IngressAnnotation: IngressAnnotationsLister{cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc)}, - Pod: PodLister{cache.NewStore(cache.MetaNamespaceKeyFunc)}, + Ingress: IngressLister{cache.NewStore(cache.MetaNamespaceKeyFunc)}, + IngressWithAnnotation: IngressWithAnnotationsLister{cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc)}, + Pod: PodLister{cache.NewStore(cache.MetaNamespaceKeyFunc)}, }, sslStore: NewSSLCertTracker(), filesystem: fs, @@ -833,58 +826,43 @@ func TestUpdateSecretIngressMap(t *testing.T) { func TestListIngresses(t *testing.T) { s := newStore(t) - ingEmptyClass := &extensions.Ingress{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-1", - Namespace: "testns", - }, - Spec: extensions.IngressSpec{ - Backend: &extensions.IngressBackend{ - ServiceName: "demo", - ServicePort: intstr.FromInt(80), + ingressToIgnore := &ingress.Ingress{ + Ingress: extensions.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-2", + Namespace: "testns", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "something", + }, }, - Rules: []extensions.IngressRule{ - { - Host: "foo.bar", + Spec: extensions.IngressSpec{ + Backend: &extensions.IngressBackend{ + ServiceName: "demo", + ServicePort: intstr.FromInt(80), }, }, }, } - s.listers.Ingress.Add(ingEmptyClass) + s.listers.IngressWithAnnotation.Add(ingressToIgnore) - ingressToIgnore := &extensions.Ingress{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-2", - Namespace: "testns", - Annotations: map[string]string{ - "kubernetes.io/ingress.class": "something", + ingressWithoutPath := &ingress.Ingress{ + Ingress: extensions.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-3", + Namespace: "testns", }, - }, - Spec: extensions.IngressSpec{ - Backend: &extensions.IngressBackend{ - ServiceName: "demo", - ServicePort: intstr.FromInt(80), - }, - }, - } - s.listers.Ingress.Add(ingressToIgnore) - - ingressWithoutPath := &extensions.Ingress{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-3", - Namespace: "testns", - }, - Spec: extensions.IngressSpec{ - Rules: []extensions.IngressRule{ - { - Host: "foo.bar", - IngressRuleValue: extensions.IngressRuleValue{ - HTTP: &extensions.HTTPIngressRuleValue{ - Paths: []extensions.HTTPIngressPath{ - { - Backend: extensions.IngressBackend{ - ServiceName: "demo", - ServicePort: intstr.FromInt(80), + Spec: extensions.IngressSpec{ + Rules: []extensions.IngressRule{ + { + Host: "foo.bar", + IngressRuleValue: extensions.IngressRuleValue{ + HTTP: &extensions.HTTPIngressRuleValue{ + Paths: []extensions.HTTPIngressPath{ + { + Backend: extensions.IngressBackend{ + ServiceName: "demo", + ServicePort: intstr.FromInt(80), + }, }, }, }, @@ -894,28 +872,30 @@ func TestListIngresses(t *testing.T) { }, }, } - s.listers.Ingress.Add(ingressWithoutPath) + s.listers.IngressWithAnnotation.Add(ingressWithoutPath) - ingressWithNginxClass := &extensions.Ingress{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-4", - Namespace: "testns", - Annotations: map[string]string{ - "kubernetes.io/ingress.class": "nginx", + ingressWithNginxClass := &ingress.Ingress{ + Ingress: extensions.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-4", + Namespace: "testns", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "nginx", + }, }, - }, - Spec: extensions.IngressSpec{ - Rules: []extensions.IngressRule{ - { - Host: "foo.bar", - IngressRuleValue: extensions.IngressRuleValue{ - HTTP: &extensions.HTTPIngressRuleValue{ - Paths: []extensions.HTTPIngressPath{ - { - Path: "/demo", - Backend: extensions.IngressBackend{ - ServiceName: "demo", - ServicePort: intstr.FromInt(80), + Spec: extensions.IngressSpec{ + Rules: []extensions.IngressRule{ + { + Host: "foo.bar", + IngressRuleValue: extensions.IngressRuleValue{ + HTTP: &extensions.HTTPIngressRuleValue{ + Paths: []extensions.HTTPIngressPath{ + { + Path: "/demo", + Backend: extensions.IngressBackend{ + ServiceName: "demo", + ServicePort: intstr.FromInt(80), + }, }, }, }, @@ -925,7 +905,7 @@ func TestListIngresses(t *testing.T) { }, }, } - s.listers.Ingress.Add(ingressWithNginxClass) + s.listers.IngressWithAnnotation.Add(ingressWithNginxClass) ingresses := s.ListIngresses() if s := len(ingresses); s != 3 { From f78e2e38494cd03829f17623615cea5dfb7b02e8 Mon Sep 17 00:00:00 2001 From: Manuel Alejandro de Brito Fontes Date: Fri, 30 Nov 2018 20:22:12 -0300 Subject: [PATCH 2/3] Only copy fields being used --- internal/ingress/controller/store/store.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index e77b6c097..4b0a1ef16 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -594,7 +594,9 @@ func (s *k8sStore) extractAnnotations(ing *extensions.Ingress) { key := k8s.MetaNamespaceKey(ing) glog.V(3).Infof("updating annotations information for ingress %v", key) - copyIng := ing.DeepCopy() + copyIng := &extensions.Ingress{} + ing.ObjectMeta.DeepCopyInto(©Ing.ObjectMeta) + ing.Spec.DeepCopyInto(©Ing.Spec) for ri, rule := range copyIng.Spec.Rules { if rule.HTTP == nil { @@ -610,7 +612,7 @@ func (s *k8sStore) extractAnnotations(ing *extensions.Ingress) { err := s.listers.IngressWithAnnotation.Update(&ingress.Ingress{ Ingress: *copyIng, - ParsedAnnotations: s.annotations.Extract(copyIng), + ParsedAnnotations: s.annotations.Extract(ing), }) if err != nil { glog.Error(err) 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 3/3] 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 }