From b6d5cba6d8100fd41c64467ca5cc4c54f03917b9 Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Wed, 9 Sep 2020 08:47:11 -0300 Subject: [PATCH 01/13] add per path config reader Add a backend's internal field `pathConfig` that holds per path configurations in a way haproxy can understand and use. Per path configurations will be added to the `[]Paths` field, simplifying the public interface. `pathConfig` is populated and data is deduplicated in an abstract way. --- pkg/haproxy/types/backend.go | 89 ++++++++++++++- pkg/haproxy/types/backend_test.go | 129 ++++++++++++++++++++-- pkg/haproxy/types/types.go | 39 ++++++- rootfs/etc/templates/haproxy/haproxy.tmpl | 2 +- 4 files changed, 244 insertions(+), 15 deletions(-) diff --git a/pkg/haproxy/types/backend.go b/pkg/haproxy/types/backend.go index f175108fc..658553c78 100644 --- a/pkg/haproxy/types/backend.go +++ b/pkg/haproxy/types/backend.go @@ -18,6 +18,7 @@ package types import ( "fmt" + "reflect" "sort" "strings" ) @@ -122,7 +123,7 @@ func (b *Backend) AddBackendPath(link PathLink) *BackendPath { Link: link, } b.Paths = append(b.Paths, backendPath) - sortPaths(b.Paths, true) + sortPaths(b.Paths, false) return backendPath } @@ -232,13 +233,99 @@ func (b *Backend) HasSSLRedirectPaths(paths *BackendPaths) bool { return false } +// PathConfig ... +func (b *Backend) PathConfig(attr string) *BackendPathConfig { + b.ensurePathConfig(attr) + return b.pathConfig[attr] +} + // NeedACL ... func (b *Backend) NeedACL() bool { + b.ensurePathConfig("") + for _, path := range b.pathConfig { + if path.NeedACL() { + return true + } + } return len(b.HSTS) > 1 || len(b.MaxBodySize) > 1 || len(b.RewriteURL) > 1 || len(b.WhitelistHTTP) > 1 || len(b.Cors) > 1 || len(b.AuthHTTP) > 1 || len(b.WAF) > 1 } +func (b *Backend) ensurePathConfig(attr string) { + if b.pathConfig == nil { + b.pathConfig = b.createPathConfig() + } + if attr == "" { + return + } + if _, found := b.pathConfig[attr]; !found { + panic(fmt.Errorf("field does not exist: %s", attr)) + } +} + +func (b *Backend) createPathConfig() map[string]*BackendPathConfig { + pathconfig := make(map[string]*BackendPathConfig, len(b.Paths)) + pathType := reflect.TypeOf(BackendPath{}) + for i := 0; i < pathType.NumField(); i++ { + name := pathType.Field(i).Name + // filter out core fields + if name != "ID" && name != "Link" { + pathconfig[name] = &BackendPathConfig{} + } + } + for _, path := range b.Paths { + pathValue := reflect.ValueOf(*path) + for name, config := range pathconfig { + newconfig := pathValue.FieldByName(name).Interface() + hasconfig := false + for _, item := range config.items { + if reflect.DeepEqual(item.config, newconfig) { + item.paths = append(item.paths, path) + hasconfig = true + break + } + } + if !hasconfig { + config.items = append(config.items, &BackendPathItem{ + paths: []*BackendPath{path}, + config: newconfig, + }) + } + } + } + return pathconfig +} + +// NeedACL ... +func (b *BackendPathConfig) NeedACL() bool { + return len(b.items) > 1 +} + +// Items ... +func (b *BackendPathConfig) Items() []interface{} { + items := make([]interface{}, len(b.items)) + for i, item := range b.items { + items[i] = item.config + } + return items +} + +// Paths ... +func (b *BackendPathConfig) Paths(index int) []*BackendPath { + return b.items[index].paths +} + +// PathIDs ... +func (b *BackendPathConfig) PathIDs(index int) string { + paths := b.items[index].paths + ids := make([]string, len(paths)) + for i, path := range paths { + ids[i] = path.ID + } + return strings.Join(ids, " ") +} + // IsEmpty ... func (ep *Endpoint) IsEmpty() bool { return ep.IP == "127.0.0.1" diff --git a/pkg/haproxy/types/backend_test.go b/pkg/haproxy/types/backend_test.go index 8c0223bf1..b7d9b2928 100644 --- a/pkg/haproxy/types/backend_test.go +++ b/pkg/haproxy/types/backend_test.go @@ -18,6 +18,7 @@ package types import ( "reflect" + "strings" "testing" ) @@ -30,47 +31,47 @@ func TestAddPath(t *testing.T) { { input: []string{"/"}, expected: []*BackendPath{ - {"path01", PathLink{"d1.local", "/"}}, + {ID: "path01", Link: PathLink{"d1.local", "/"}}, }, }, // 1 { input: []string{"/app", "/app"}, expected: []*BackendPath{ - {"path01", PathLink{"d1.local", "/app"}}, + {ID: "path01", Link: PathLink{"d1.local", "/app"}}, }, }, // 2 { input: []string{"/app", "/root"}, expected: []*BackendPath{ - {"path02", PathLink{"d1.local", "/root"}}, - {"path01", PathLink{"d1.local", "/app"}}, + {ID: "path01", Link: PathLink{"d1.local", "/app"}}, + {ID: "path02", Link: PathLink{"d1.local", "/root"}}, }, }, // 3 { input: []string{"/app", "/root", "/root"}, expected: []*BackendPath{ - {"path02", PathLink{"d1.local", "/root"}}, - {"path01", PathLink{"d1.local", "/app"}}, + {ID: "path01", Link: PathLink{"d1.local", "/app"}}, + {ID: "path02", Link: PathLink{"d1.local", "/root"}}, }, }, // 4 { input: []string{"/app", "/root", "/app"}, expected: []*BackendPath{ - {"path02", PathLink{"d1.local", "/root"}}, - {"path01", PathLink{"d1.local", "/app"}}, + {ID: "path01", Link: PathLink{"d1.local", "/app"}}, + {ID: "path02", Link: PathLink{"d1.local", "/root"}}, }, }, // 5 { input: []string{"/", "/app", "/root"}, expected: []*BackendPath{ - {"path03", PathLink{"d1.local", "/root"}}, - {"path02", PathLink{"d1.local", "/app"}}, - {"path01", PathLink{"d1.local", "/"}}, + {ID: "path01", Link: PathLink{"d1.local", "/"}}, + {ID: "path02", Link: PathLink{"d1.local", "/app"}}, + {ID: "path03", Link: PathLink{"d1.local", "/root"}}, }, }, } @@ -84,3 +85,109 @@ func TestAddPath(t *testing.T) { } } } + +func TestCreatePathConfig(t *testing.T) { + type pathConfig struct { + paths string + config interface{} + } + testCases := []struct { + paths []*BackendPath + filter string + expected map[string][]pathConfig + }{ + // 0 + { + filter: "SSLRedirect", + expected: map[string][]pathConfig{ + "SSLRedirect": nil, + }, + }, + // 1 + { + paths: []*BackendPath{{ID: "path1"}}, + filter: "SSLRedirect", + expected: map[string][]pathConfig{ + "SSLRedirect": { + {paths: "path1", config: false}, + }, + }, + }, + // 2 + { + paths: []*BackendPath{ + {ID: "path1", HSTS: HSTS{Enabled: true, MaxAge: 10}}, + {ID: "path2", HSTS: HSTS{Enabled: true, MaxAge: 10}}, + {ID: "path3", HSTS: HSTS{Enabled: true, MaxAge: 20}}, + }, + filter: "HSTS", + expected: map[string][]pathConfig{ + "HSTS": { + { + paths: "path1,path2", + config: HSTS{Enabled: true, MaxAge: 10}, + }, + { + paths: "path3", + config: HSTS{Enabled: true, MaxAge: 20}, + }, + }, + }, + }, + // 3 + { + paths: []*BackendPath{ + {ID: "path1", HSTS: HSTS{Enabled: true, MaxAge: 10}, WhitelistHTTP: []string{"10.0.0.0/8"}}, + {ID: "path2", HSTS: HSTS{Enabled: true, MaxAge: 20}, WhitelistHTTP: []string{"10.0.0.0/8"}}, + {ID: "path3", HSTS: HSTS{Enabled: true, MaxAge: 20}}, + }, + filter: "HSTS,WhitelistHTTP", + expected: map[string][]pathConfig{ + "HSTS": { + { + paths: "path1", + config: HSTS{Enabled: true, MaxAge: 10}, + }, + { + paths: "path2,path3", + config: HSTS{Enabled: true, MaxAge: 20}, + }, + }, + "WhitelistHTTP": { + { + paths: "path1,path2", + config: []string{"10.0.0.0/8"}, + }, + { + paths: "path3", + }, + }, + }, + }, + } + for i, test := range testCases { + c := setup(t) + backend := Backend{Paths: test.paths} + actualConfig := map[string][]pathConfig{} + for _, attr := range strings.Split(test.filter, ",") { + config := backend.PathConfig(attr) + pathConfigs := actualConfig[attr] + for _, item := range config.items { + paths := []string{} + for _, p := range item.paths { + paths = append(paths, p.ID) + } + itemConfig := item.config + configValue := reflect.ValueOf(itemConfig) + if configValue.Kind() == reflect.Slice && configValue.Len() == 0 { + // empty slices and nil are semantically identical but DeepEquals disagrees + itemConfig = nil + } + pathConfigs = append(pathConfigs, pathConfig{paths: strings.Join(paths, ","), config: itemConfig}) + } + actualConfig[attr] = pathConfigs + } + c.compareObjects("pathconfig", i, actualConfig, test.expected) + c.teardown() + } +} diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index 1632b613d..a0a7a0cc0 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -441,8 +441,12 @@ type Backend struct { Port string Endpoints []*Endpoint EpNaming EndpointNaming - Paths []*BackendPath - PathsMap *HostsMap + // + // Paths + // + Paths []*BackendPath + PathsMap *HostsMap + pathConfig map[string]*BackendPathConfig // // per backend config // @@ -517,10 +521,35 @@ type BackendPaths struct { Items []*BackendPath } +// BackendPathConfig ... +type BackendPathConfig struct { + items []*BackendPathItem +} + +// BackendPathItem ... +type BackendPathItem struct { + paths []*BackendPath + config interface{} +} + // BackendPath ... type BackendPath struct { + // + // core fields, filter out new fields in `Backend.createPathConfig()` + // ID string Link PathLink + // + // config fields + // + AuthHTTP AuthHTTP + Cors Cors + HSTS HSTS + MaxBodySize int + RewriteURL string + SSLRedirect bool + WAF WAF + WhitelistHTTP []string } // BackendHeader ... @@ -670,6 +699,12 @@ type Cookie struct { Keywords string } +// AuthHTTP ... +type AuthHTTP struct { + UserlistName string + Realm string +} + // Cors ... type Cors struct { Enabled bool diff --git a/rootfs/etc/templates/haproxy/haproxy.tmpl b/rootfs/etc/templates/haproxy/haproxy.tmpl index 686fa4ad2..a1ac17014 100644 --- a/rootfs/etc/templates/haproxy/haproxy.tmpl +++ b/rootfs/etc/templates/haproxy/haproxy.tmpl @@ -386,7 +386,7 @@ backend {{ $backend.ID }} {{- /*------------------------------------*/}} {{- if $backend.NeedACL }} -{{- range $path := reverse $backend.Paths }} +{{- range $path := $backend.Paths }} # {{ $path.ID }} = {{ $path.Hostname }}{{ $path.Path }} {{- end }} {{- range $match := $backend.PathsMap.MatchTypes }} From 9aa50f9fec9879400e275b26ff9fb4d0ffa95ce1 Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Thu, 10 Sep 2020 17:25:32 -0300 Subject: [PATCH 02/13] add per path config getter in the ann mapper A small refactor in the annotation mapper to allow a direct reference to the per path configs. Most of the other methods should be removed after convert the annotation parsers. --- pkg/converters/ingress/annotations/mapper.go | 90 ++++++++++++++------ 1 file changed, 63 insertions(+), 27 deletions(-) diff --git a/pkg/converters/ingress/annotations/mapper.go b/pkg/converters/ingress/annotations/mapper.go index f6b6debb8..bed7849c4 100644 --- a/pkg/converters/ingress/annotations/mapper.go +++ b/pkg/converters/ingress/annotations/mapper.go @@ -35,17 +35,17 @@ type MapBuilder struct { // Mapper ... type Mapper struct { MapBuilder - maps map[string][]*Map + maps map[string][]*Map + configs map[hatypes.PathLink]*AnnConfig +} + +// AnnConfig ... +type AnnConfig struct { + mapper *Mapper + keys map[string]*ConfigValue } // Map ... -// -// TODO rename URI to Hostpath -- currently this is a little mess. -// Fix also testCase data in order to represent a hostname+path. -// Hostname is the domain name. Path is the declared starting path on ingress -// Together they populate a map_beg() converter in order to match HAProxy's -// `base` sample fetch method. -// type Map struct { Source *Source Link hatypes.PathLink @@ -84,41 +84,56 @@ func NewMapBuilder(logger types.Logger, annPrefix string, annDefaults map[string func (b *MapBuilder) NewMapper() *Mapper { return &Mapper{ MapBuilder: *b, - maps: map[string][]*Map{}, + // + maps: map[string][]*Map{}, + configs: map[hatypes.PathLink]*AnnConfig{}, } } -func (c *Mapper) addAnnotation(source *Source, link hatypes.PathLink, key, value string) (conflict bool) { - conflict = false - annMaps, found := c.maps[key] +func newAnnConfig(mapper *Mapper) *AnnConfig { + return &AnnConfig{ + mapper: mapper, + keys: map[string]*ConfigValue{}, + } +} + +// Add a new annotation to the current mapper. +// Return the conflict state: true if a conflict was found, false if the annotation was assigned or at least handled +func (c *Mapper) addAnnotation(source *Source, link hatypes.PathLink, key, value string) bool { if link.IsEmpty() { - // empty means default value + // empty means default value, cannot register as an annotation panic("path link cannot be empty") } - if found { - for _, annMap := range annMaps { - if annMap.Link == link { - return annMap.Value != value - } - } + // check overlap + config, configfound := c.configs[link] + if !configfound { + config = newAnnConfig(c) + c.configs[link] = config } - var realValue string - var ok bool - validator, found := validators[key] - if found { + if cfg, found := config.keys[key]; found { + return cfg.Value != value + } + // validate (bool; int; ...) and normalize (int "01" => "1"; ...) + realValue := value + if validator, found := validators[key]; found { + var ok bool if realValue, ok = validator(validate{logger: c.logger, source: source, key: key, value: value}); !ok { - return + return false } - } else { - realValue = value } + // update internal fields + config.keys[key] = &ConfigValue{ + Source: source, + Value: realValue, + } + annMaps, _ := c.maps[key] annMaps = append(annMaps, &Map{ Source: source, Link: link, Value: realValue, }) c.maps[key] = annMaps - return + return false } // AddAnnotations ... @@ -145,6 +160,16 @@ func (c *Mapper) GetStrMap(key string) ([]*Map, bool) { return nil, false } +// GetConfig ... +func (c *Mapper) GetConfig(link hatypes.PathLink) *AnnConfig { + if config, found := c.configs[link]; found { + return config + } + config := newAnnConfig(c) + c.configs[link] = config + return config +} + // Get ... func (c *Mapper) Get(key string) *ConfigValue { annMaps, found := c.GetStrMap(key) @@ -262,6 +287,17 @@ func findConfig(config []*BackendConfig, kv map[string]*ConfigValue) *BackendCon return nil } +// Get ... +func (c *AnnConfig) Get(key string) *ConfigValue { + if value, found := c.keys[key]; found { + return value + } + if value, found := c.mapper.annDefaults[key]; found { + return &ConfigValue{Value: value} + } + return &ConfigValue{} +} + // ConfigEquals ... func (b *BackendConfig) ConfigEquals(other map[string]*ConfigValue) bool { if len(b.Config) != len(other) { From 837a6ba32827c9e5fbee4e00aa724aeaebb9682e Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Thu, 10 Sep 2020 17:45:54 -0300 Subject: [PATCH 03/13] convert hsts to the new per path config --- pkg/converters/ingress/annotations/backend.go | 21 ++---- .../ingress/annotations/backend_test.go | 58 +++++++--------- pkg/haproxy/instance_test.go | 66 ++++++++----------- pkg/haproxy/types/backend.go | 22 ++++--- pkg/haproxy/types/types.go | 7 -- rootfs/etc/templates/haproxy/haproxy.tmpl | 14 ++-- 6 files changed, 78 insertions(+), 110 deletions(-) diff --git a/pkg/converters/ingress/annotations/backend.go b/pkg/converters/ingress/annotations/backend.go index ebae495ee..0f9ce95dc 100644 --- a/pkg/converters/ingress/annotations/backend.go +++ b/pkg/converters/ingress/annotations/backend.go @@ -530,21 +530,12 @@ func (c *updater) buildBackendHeaders(d *backData) { } func (c *updater) buildBackendHSTS(d *backData) { - rawHSTSList := d.mapper.GetBackendConfig( - d.backend, - []string{ingtypes.BackHSTS, ingtypes.BackHSTSMaxAge, ingtypes.BackHSTSPreload, ingtypes.BackHSTSIncludeSubdomains}, - nil, - ) - for _, cfg := range rawHSTSList { - d.backend.HSTS = append(d.backend.HSTS, &hatypes.BackendConfigHSTS{ - Paths: cfg.Paths, - Config: hatypes.HSTS{ - Enabled: cfg.Get(ingtypes.BackHSTS).Bool(), - MaxAge: cfg.Get(ingtypes.BackHSTSMaxAge).Int(), - Subdomains: cfg.Get(ingtypes.BackHSTSIncludeSubdomains).Bool(), - Preload: cfg.Get(ingtypes.BackHSTSPreload).Bool(), - }, - }) + for _, path := range d.backend.Paths { + config := d.mapper.GetConfig(path.Link) + path.HSTS.Enabled = config.Get(ingtypes.BackHSTS).Bool() + path.HSTS.MaxAge = config.Get(ingtypes.BackHSTSMaxAge).Int() + path.HSTS.Subdomains = config.Get(ingtypes.BackHSTSIncludeSubdomains).Bool() + path.HSTS.Preload = config.Get(ingtypes.BackHSTSPreload).Bool() } } diff --git a/pkg/converters/ingress/annotations/backend_test.go b/pkg/converters/ingress/annotations/backend_test.go index 14f227574..60914dd37 100644 --- a/pkg/converters/ingress/annotations/backend_test.go +++ b/pkg/converters/ingress/annotations/backend_test.go @@ -1007,7 +1007,7 @@ func TestHSTS(t *testing.T) { source Source annDefault map[string]string ann map[string]map[string]string - expected []*hatypes.BackendConfigHSTS + expected map[string]hatypes.HSTS logging string }{ // 0 @@ -1024,24 +1024,18 @@ func TestHSTS(t *testing.T) { ingtypes.BackHSTSPreload: "true", }, }, - expected: []*hatypes.BackendConfigHSTS{ - { - Paths: createBackendPaths("/"), - Config: hatypes.HSTS{ - Enabled: true, - MaxAge: 15768000, - Subdomains: false, - Preload: false, - }, + expected: map[string]hatypes.HSTS{ + "/": { + Enabled: true, + MaxAge: 15768000, + Subdomains: false, + Preload: false, }, - { - Paths: createBackendPaths("/url"), - Config: hatypes.HSTS{ - Enabled: true, - MaxAge: 50, - Subdomains: false, - Preload: true, - }, + "/url": { + Enabled: true, + MaxAge: 50, + Subdomains: false, + Preload: true, }, }, }, @@ -1059,15 +1053,12 @@ func TestHSTS(t *testing.T) { ingtypes.BackHSTSIncludeSubdomains: "true", }, }, - expected: []*hatypes.BackendConfigHSTS{ - { - Paths: createBackendPaths("/"), - Config: hatypes.HSTS{ - Enabled: true, - MaxAge: 50, - Subdomains: true, - Preload: false, - }, + expected: map[string]hatypes.HSTS{ + "/": { + Enabled: true, + MaxAge: 50, + Subdomains: true, + Preload: false, }, }, source: Source{Namespace: "default", Name: "ing1", Type: "ingress"}, @@ -1076,11 +1067,8 @@ func TestHSTS(t *testing.T) { // 2 { paths: []string{"/"}, - expected: []*hatypes.BackendConfigHSTS{ - { - Paths: createBackendPaths("/"), - Config: hatypes.HSTS{}, - }, + expected: map[string]hatypes.HSTS{ + "/": {}, }, }, } @@ -1089,7 +1077,11 @@ func TestHSTS(t *testing.T) { d := c.createBackendMappingData("default/app", &test.source, test.annDefault, test.ann, test.paths) u := c.createUpdater() u.buildBackendHSTS(d) - c.compareObjects("hsts", i, d.backend.HSTS, test.expected) + actual := map[string]hatypes.HSTS{} + for _, path := range d.backend.Paths { + actual[path.Path()] = path.HSTS + } + c.compareObjects("hsts", i, actual, test.expected) c.logger.CompareLogging(test.logging) c.teardown() } diff --git a/pkg/haproxy/instance_test.go b/pkg/haproxy/instance_test.go index 6716400dd..ccb2adf20 100644 --- a/pkg/haproxy/instance_test.go +++ b/pkg/haproxy/instance_test.go @@ -146,25 +146,23 @@ d1.local/ path01`, }, { doconfig: func(g *hatypes.Global, h *hatypes.Host, b *hatypes.Backend) { - b.HSTS = []*hatypes.BackendConfigHSTS{ - { - Paths: createBackendPaths(b, "d1.local/"), - Config: hatypes.HSTS{ - Enabled: true, - MaxAge: 15768000, - Preload: true, - Subdomains: true, - }, - }, - { - Paths: createBackendPaths(b, "d1.local/path", "d1.local/uri"), - Config: hatypes.HSTS{ - Enabled: true, - MaxAge: 15768000, - Preload: false, - Subdomains: false, - }, - }, + b.FindBackendPath(h.FindPath("/").Link).HSTS = hatypes.HSTS{ + Enabled: true, + MaxAge: 15768000, + Preload: true, + Subdomains: true, + } + b.FindBackendPath(h.FindPath("/path").Link).HSTS = hatypes.HSTS{ + Enabled: true, + MaxAge: 15768000, + Preload: false, + Subdomains: false, + } + b.FindBackendPath(h.FindPath("/uri").Link).HSTS = hatypes.HSTS{ + Enabled: true, + MaxAge: 15768000, + Preload: false, + Subdomains: false, } }, path: []string{"/", "/path", "/uri"}, @@ -1034,16 +1032,11 @@ func TestInstanceFrontingProxyUseProto(t *testing.T) { h = c.config.Hosts().AcquireHost(test.domain) h.AddPath(b, "/", hatypes.MatchBegin) b.Endpoints = []*hatypes.Endpoint{endpointS1} - b.HSTS = []*hatypes.BackendConfigHSTS{ - { - Paths: createBackendPaths(b, test.domain+"/"), - Config: hatypes.HSTS{ - Enabled: true, - MaxAge: 15768000, - Subdomains: true, - Preload: true, - }, - }, + b.FindBackendPath(h.FindPath("/").Link).HSTS = hatypes.HSTS{ + Enabled: true, + MaxAge: 15768000, + Subdomains: true, + Preload: true, } h.TLS.CAHash = "1" h.TLS.CAFilename = "/var/haproxy/ssl/ca.pem" @@ -1196,16 +1189,11 @@ func TestInstanceFrontingProxyIgnoreProto(t *testing.T) { h = c.config.Hosts().AcquireHost(test.domain) h.AddPath(b, "/", hatypes.MatchBegin) b.Endpoints = []*hatypes.Endpoint{endpointS1} - b.HSTS = []*hatypes.BackendConfigHSTS{ - { - Paths: createBackendPaths(b, test.domain+"/"), - Config: hatypes.HSTS{ - Enabled: true, - MaxAge: 15768000, - Subdomains: true, - Preload: true, - }, - }, + b.FindBackendPath(h.FindPath("/").Link).HSTS = hatypes.HSTS{ + Enabled: true, + MaxAge: 15768000, + Subdomains: true, + Preload: true, } h.TLS.CAHash = "1" h.TLS.CAFilename = "/var/haproxy/ssl/ca.pem" diff --git a/pkg/haproxy/types/backend.go b/pkg/haproxy/types/backend.go index 658553c78..886f764a8 100644 --- a/pkg/haproxy/types/backend.go +++ b/pkg/haproxy/types/backend.go @@ -191,6 +191,16 @@ func (b *Backend) HasCorsEnabled() bool { return false } +// HasHSTS ... +func (b *Backend) HasHSTS() bool { + for _, path := range b.Paths { + if path.HSTS.Enabled { + return true + } + } + return false +} + // HasModsec is a method to verify if a Backend has ModSecurity Enabled func (b *Backend) HasModsec() bool { for _, waf := range b.WAF { @@ -224,8 +234,8 @@ func (b *Backend) LinkHasSSLRedirect(link PathLink) bool { } // HasSSLRedirectPaths ... -func (b *Backend) HasSSLRedirectPaths(paths *BackendPaths) bool { - for _, path := range paths.Items { +func (b *Backend) HasSSLRedirectPaths(paths []*BackendPath) bool { + for _, path := range paths { if b.LinkHasSSLRedirect(path.Link) { return true } @@ -247,8 +257,7 @@ func (b *Backend) NeedACL() bool { return true } } - return len(b.HSTS) > 1 || - len(b.MaxBodySize) > 1 || len(b.RewriteURL) > 1 || len(b.WhitelistHTTP) > 1 || + return len(b.MaxBodySize) > 1 || len(b.RewriteURL) > 1 || len(b.WhitelistHTTP) > 1 || len(b.Cors) > 1 || len(b.AuthHTTP) > 1 || len(b.WAF) > 1 } @@ -406,11 +415,6 @@ func (b *BackendConfigCors) String() string { return fmt.Sprintf("%+v", *b) } -// String ... -func (b *BackendConfigHSTS) String() string { - return fmt.Sprintf("%+v", *b) -} - // String ... func (b *BackendConfigWhitelist) String() string { return fmt.Sprintf("%+v", *b) diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index a0a7a0cc0..dca0fb5cb 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -490,7 +490,6 @@ type Backend struct { // AuthHTTP []*BackendConfigAuth Cors []*BackendConfigCors - HSTS []*BackendConfigHSTS MaxBodySize []*BackendConfigInt RewriteURL []*BackendConfigStr SSLRedirect []*BackendConfigBool @@ -595,12 +594,6 @@ type BackendConfigWAF struct { Config WAF } -// BackendConfigHSTS ... -type BackendConfigHSTS struct { - Paths BackendPaths - Config HSTS -} - // BackendConfigWhitelist ... type BackendConfigWhitelist struct { Paths BackendPaths diff --git a/rootfs/etc/templates/haproxy/haproxy.tmpl b/rootfs/etc/templates/haproxy/haproxy.tmpl index a1ac17014..c08788bd8 100644 --- a/rootfs/etc/templates/haproxy/haproxy.tmpl +++ b/rootfs/etc/templates/haproxy/haproxy.tmpl @@ -349,7 +349,7 @@ backend {{ $backend.ID }} {{- $hasFrontingProxy := $global.Bind.HasFrontingProxy }} {{- $frontingUseProto := and $hasFrontingProxy $global.Bind.FrontingUseProto }} {{- $frontingIgnoreProto := and $hasFrontingProxy (not $global.Bind.FrontingUseProto) }} -{{- if and $backend.HSTS (not $frontingIgnoreProto) }} +{{- if and $backend.HasHSTS (not $frontingIgnoreProto) }} acl https-request ssl_fc {{- if $frontingUseProto }} acl https-request var(txn.proto) https @@ -360,7 +360,7 @@ backend {{ $backend.ID }} {{- end }} {{- /*------------------------------------*/}} -{{- if and $frontingUseProto $backend.HSTS }} +{{- if and $frontingUseProto $backend.HasHSTS }} http-request set-var(txn.proto) hdr(X-Forwarded-Proto) {{- end }} @@ -536,18 +536,18 @@ backend {{ $backend.ID }} {{- end }} {{- /*------------------------------------*/}} -{{- $needACL := gt (len $backend.HSTS) 1 }} -{{- range $hstsCfg := $backend.HSTS }} -{{- $hsts := $hstsCfg.Config }} +{{- $hstsCfg := $backend.PathConfig "HSTS" }} +{{- $needACL := $hstsCfg.NeedACL }} +{{- range $i, $hsts := $hstsCfg.Items }} {{- if $hsts.Enabled }} -{{- $paths := $hstsCfg.Paths }} +{{- $paths := $hstsCfg.Paths $i }} {{- $needSSLACL := and (not $frontingIgnoreProto) (not ($backend.HasSSLRedirectPaths $paths)) }} http-response set-header Strict-Transport-Security "max-age={{ $hsts.MaxAge }} {{- if $hsts.Subdomains }}; includeSubDomains{{ end }} {{- if $hsts.Preload }}; preload{{ end }}" {{- if or $needSSLACL $needACL }} if {{- if $needSSLACL }} https-request{{ end }} - {{- if $needACL }} { var(txn.pathID) {{ $paths.IDList }} }{{ end }} + {{- if $needACL }} { var(txn.pathID) {{ $hstsCfg.PathIDs $i }} }{{ end }} {{- end }} {{- end }} {{- end }} From 9920a5d58bc9922e32b80720192641219fd8d74d Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Thu, 10 Sep 2020 19:31:27 -0300 Subject: [PATCH 04/13] convert auth http to the new per path config --- pkg/converters/ingress/annotations/backend.go | 120 ++++++++---------- .../ingress/annotations/backend_test.go | 17 +-- pkg/haproxy/instance_test.go | 12 +- pkg/haproxy/types/backend.go | 7 +- pkg/haproxy/types/types.go | 8 -- rootfs/etc/templates/haproxy/haproxy.tmpl | 13 +- 6 files changed, 72 insertions(+), 105 deletions(-) diff --git a/pkg/converters/ingress/annotations/backend.go b/pkg/converters/ingress/annotations/backend.go index 0f9ce95dc..3f58257fc 100644 --- a/pkg/converters/ingress/annotations/backend.go +++ b/pkg/converters/ingress/annotations/backend.go @@ -65,77 +65,61 @@ func (c *updater) buildBackendAffinity(d *backData) { } func (c *updater) buildBackendAuthHTTP(d *backData) { - config := d.mapper.GetBackendConfig( - d.backend, - []string{ - ingtypes.BackAuthType, - ingtypes.BackAuthSecret, - ingtypes.BackAuthRealm, - }, - func(path *hatypes.BackendPath, values map[string]*ConfigValue) map[string]*ConfigValue { - authType := values[ingtypes.BackAuthType] - if authType == nil || authType.Source == nil { - return nil - } - if authType.Value != "basic" { - c.logger.Error("unsupported authentication type on %v: %s", authType.Source, authType.Value) - return nil - } - authSecret := values[ingtypes.BackAuthSecret] - if authSecret == nil || authSecret.Source == nil { - c.logger.Error("missing secret name on basic authentication on %v", authType.Source) - return nil - } - secretName := authSecret.Value - if strings.Index(secretName, "/") < 0 { - secretName = authSecret.Source.Namespace + "/" + secretName - } - listName := strings.Replace(secretName, "/", "_", 1) - userlist := c.haproxy.Userlists().Find(listName) - if userlist == nil { - userb, err := c.cache.GetSecretContent( - authSecret.Source.Namespace, - authSecret.Value, "auth", - convtypes.TrackingTarget{ - Backend: d.backend.BackendID(), - Userlist: listName, - }, - ) - if err != nil { - c.logger.Error("error reading basic authentication on %v: %v", authSecret.Source, err) - return nil - } - userstr := string(userb) - users, errs := extractUserlist(authSecret.Source.Name, secretName, userstr) - for _, err := range errs { - c.logger.Warn("ignoring malformed usr/passwd on secret '%s', declared on %v: %v", secretName, authSecret.Source, err) - } - userlist = c.haproxy.Userlists().Replace(listName, users) - if len(users) == 0 { - c.logger.Warn("userlist on %v for basic authentication is empty", authSecret.Source) - } + for _, path := range d.backend.Paths { + config := d.mapper.GetConfig(path.Link) + authType := config.Get(ingtypes.BackAuthType) + if authType == nil || authType.Source == nil { + continue + } + if authType.Value != "basic" { + c.logger.Error("unsupported authentication type on %v: %s", authType.Source, authType.Value) + continue + } + authSecret := config.Get(ingtypes.BackAuthSecret) + if authSecret == nil || authSecret.Source == nil { + c.logger.Error("missing secret name on basic authentication on %v", authType.Source) + continue + } + secretName := authSecret.Value + if strings.Index(secretName, "/") < 0 { + secretName = authSecret.Source.Namespace + "/" + secretName + } + listName := strings.Replace(secretName, "/", "_", 1) + userlist := c.haproxy.Userlists().Find(listName) + if userlist == nil { + userb, err := c.cache.GetSecretContent( + authSecret.Source.Namespace, + authSecret.Value, "auth", + convtypes.TrackingTarget{ + Backend: d.backend.BackendID(), + Userlist: listName, + }, + ) + if err != nil { + c.logger.Error("error reading basic authentication on %v: %v", authSecret.Source, err) + continue } - realm := "localhost" // HAProxy's backend name would be used if missing - authRealm := values[ingtypes.BackAuthRealm] - if authRealm == nil || authRealm.Source == nil { - // leave default - } else if strings.Index(authRealm.Value, `"`) >= 0 { - c.logger.Warn("ignoring auth-realm with quotes on %v", authRealm.Source) - } else if authRealm.Value != "" { - realm = authRealm.Value + userstr := string(userb) + users, errs := extractUserlist(authSecret.Source.Name, secretName, userstr) + for _, err := range errs { + c.logger.Warn("ignoring malformed usr/passwd on secret '%s', declared on %v: %v", secretName, authSecret.Source, err) } - return map[string]*ConfigValue{ - "username": {Value: userlist.Name}, - "realm": {Value: realm}, + userlist = c.haproxy.Userlists().Replace(listName, users) + if len(users) == 0 { + c.logger.Warn("userlist on %v for basic authentication is empty", authSecret.Source) } - }, - ) - for _, cfg := range config { - d.backend.AuthHTTP = append(d.backend.AuthHTTP, &hatypes.BackendConfigAuth{ - Paths: cfg.Paths, - UserlistName: cfg.Get("username").Value, - Realm: cfg.Get("realm").Value, - }) + } + realm := "localhost" // HAProxy's backend name would be used if missing + authRealm := config.Get(ingtypes.BackAuthRealm) + if authRealm == nil || authRealm.Source == nil { + // leave default + } else if strings.Index(authRealm.Value, `"`) >= 0 { + c.logger.Warn("ignoring auth-realm with quotes on %v", authRealm.Source) + } else if authRealm.Value != "" { + realm = authRealm.Value + } + path.AuthHTTP.UserlistName = userlist.Name + path.AuthHTTP.Realm = realm } } diff --git a/pkg/converters/ingress/annotations/backend_test.go b/pkg/converters/ingress/annotations/backend_test.go index 60914dd37..317e9fafc 100644 --- a/pkg/converters/ingress/annotations/backend_test.go +++ b/pkg/converters/ingress/annotations/backend_test.go @@ -147,7 +147,7 @@ func TestAuthHTTP(t *testing.T) { ann map[string]map[string]string secrets conv_helper.SecretContent expUserlists []*hatypes.Userlist - expConfig []*hatypes.BackendConfigAuth + expConfig map[string]hatypes.AuthHTTP expLogging string }{ // 0 @@ -288,12 +288,9 @@ usr2::clearpwd2`)}}, {Name: "usr1", Passwd: "encpwd1", Encrypted: true}, {Name: "usr2", Passwd: "clearpwd2", Encrypted: false}, }}}, - expConfig: []*hatypes.BackendConfigAuth{ - { - Paths: createBackendPaths("/"), - }, - { - Paths: createBackendPaths("/admin"), + expConfig: map[string]hatypes.AuthHTTP{ + "/": {}, + "/admin": { UserlistName: "default_basicpwd", Realm: "localhost", }, @@ -318,7 +315,11 @@ usr2::clearpwd2`)}}, userlists := u.haproxy.Userlists().BuildSortedItems() c.compareObjects("userlists", i, userlists, test.expUserlists) if test.expConfig != nil { - c.compareObjects("auth http", i, d.backend.AuthHTTP, test.expConfig) + actual := map[string]hatypes.AuthHTTP{} + for _, path := range d.backend.Paths { + actual[path.Path()] = path.AuthHTTP + } + c.compareObjects("auth http", i, actual, test.expConfig) } c.logger.CompareLogging(test.expLogging) c.teardown() diff --git a/pkg/haproxy/instance_test.go b/pkg/haproxy/instance_test.go index ccb2adf20..0f2c85bcd 100644 --- a/pkg/haproxy/instance_test.go +++ b/pkg/haproxy/instance_test.go @@ -2475,15 +2475,9 @@ userlist default_auth2 for _, list := range test.lists { c.config.Userlists().Replace(list.name, list.users) } - b.AuthHTTP = []*hatypes.BackendConfigAuth{ - { - Paths: createBackendPaths(b, "d1.local/"), - }, - { - Paths: createBackendPaths(b, "d1.local/admin"), - UserlistName: test.listname, - Realm: test.realm, - }, + b.FindBackendPath(h.FindPath("/admin").Link).AuthHTTP = hatypes.AuthHTTP{ + UserlistName: test.listname, + Realm: test.realm, } var realm string diff --git a/pkg/haproxy/types/backend.go b/pkg/haproxy/types/backend.go index 886f764a8..ccc27dd11 100644 --- a/pkg/haproxy/types/backend.go +++ b/pkg/haproxy/types/backend.go @@ -258,7 +258,7 @@ func (b *Backend) NeedACL() bool { } } return len(b.MaxBodySize) > 1 || len(b.RewriteURL) > 1 || len(b.WhitelistHTTP) > 1 || - len(b.Cors) > 1 || len(b.AuthHTTP) > 1 || len(b.WAF) > 1 + len(b.Cors) > 1 || len(b.WAF) > 1 } func (b *Backend) ensurePathConfig(attr string) { @@ -390,11 +390,6 @@ func (h *BackendHeader) String() string { return fmt.Sprintf("%+v", *h) } -// String ... -func (b *BackendConfigAuth) String() string { - return fmt.Sprintf("%+v", *b) -} - // String ... func (b *BackendConfigBool) String() string { return fmt.Sprintf("%+v", *b) diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index dca0fb5cb..01882a61b 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -488,7 +488,6 @@ type Backend struct { // Template uses this func in order to know if a config // has two or more paths, and so need to be configured with ACL. // - AuthHTTP []*BackendConfigAuth Cors []*BackendConfigCors MaxBodySize []*BackendConfigInt RewriteURL []*BackendConfigStr @@ -575,13 +574,6 @@ type BackendConfigStr struct { Config string } -// BackendConfigAuth ... -type BackendConfigAuth struct { - Paths BackendPaths - UserlistName string - Realm string -} - // BackendConfigCors ... type BackendConfigCors struct { Paths BackendPaths diff --git a/rootfs/etc/templates/haproxy/haproxy.tmpl b/rootfs/etc/templates/haproxy/haproxy.tmpl index c08788bd8..4805ba2ad 100644 --- a/rootfs/etc/templates/haproxy/haproxy.tmpl +++ b/rootfs/etc/templates/haproxy/haproxy.tmpl @@ -411,14 +411,15 @@ backend {{ $backend.ID }} {{- end }} {{- /*------------------------------------*/}} -{{- $needACL := gt (len $backend.AuthHTTP) 1 }} -{{- range $authCfg := $backend.AuthHTTP }} -{{- if $authCfg.UserlistName }} +{{- $authHTTPCfg := $backend.PathConfig "AuthHTTP" }} +{{- $needACL := $authHTTPCfg.NeedACL }} +{{- range $i, $authHTTP := $authHTTPCfg.Items }} +{{- if $authHTTP.UserlistName }} http-request auth - {{- if $authCfg.Realm }} realm "{{ $authCfg.Realm }}"{{ end }} + {{- if $authHTTP.Realm }} realm "{{ $authHTTP.Realm }}"{{ end }} {{- "" }} if{{ if and $backend.HasCorsEnabled }} !METH_OPTIONS{{ end }} - {{- if $needACL }} { var(txn.pathID) {{ $authCfg.Paths.IDList }} }{{ end }} - {{- "" }} !{ http_auth({{ $authCfg.UserlistName }}) } + {{- if $needACL }} { var(txn.pathID) {{ $authHTTPCfg.PathIDs $i }} }{{ end }} + {{- "" }} !{ http_auth({{ $authHTTP.UserlistName }}) } {{- end }} {{- end }} From 7c5005d929638fcbb3c9a1a5c3a9f4bf0a9369e6 Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Thu, 10 Sep 2020 20:28:52 -0300 Subject: [PATCH 05/13] convert cors to the new per path config --- pkg/converters/ingress/annotations/backend.go | 50 ++++----------- .../ingress/annotations/backend_test.go | 62 ++++++++----------- pkg/haproxy/instance_test.go | 19 +----- pkg/haproxy/types/backend.go | 11 +--- pkg/haproxy/types/types.go | 7 --- rootfs/etc/templates/haproxy/haproxy.tmpl | 29 +++++---- 6 files changed, 60 insertions(+), 118 deletions(-) diff --git a/pkg/converters/ingress/annotations/backend.go b/pkg/converters/ingress/annotations/backend.go index 3f58257fc..ba23f7060 100644 --- a/pkg/converters/ingress/annotations/backend.go +++ b/pkg/converters/ingress/annotations/backend.go @@ -397,44 +397,20 @@ func (c *updater) buildBackendBodySize(d *backData) { } func (c *updater) buildBackendCors(d *backData) { - config := d.mapper.GetBackendConfig(d.backend, - []string{ - ingtypes.BackCorsEnable, - ingtypes.BackCorsAllowCredentials, - ingtypes.BackCorsAllowHeaders, - ingtypes.BackCorsAllowMethods, - ingtypes.BackCorsAllowOrigin, - ingtypes.BackCorsExposeHeaders, - ingtypes.BackCorsMaxAge, - }, - func(path *hatypes.BackendPath, values map[string]*ConfigValue) map[string]*ConfigValue { - enabled, found := values[ingtypes.BackCorsEnable] - if !found || !enabled.Bool() { - return nil - } - return values - }, - ) - for _, cfg := range config { - enabled := cfg.Get(ingtypes.BackCorsEnable).Bool() - allowCredentials := cfg.Get(ingtypes.BackCorsAllowCredentials).Bool() - allowHeaders := cfg.Get(ingtypes.BackCorsAllowHeaders).Value - allowMethods := cfg.Get(ingtypes.BackCorsAllowMethods).Value - allowOrigin := cfg.Get(ingtypes.BackCorsAllowOrigin).Value - exposeHeaders := cfg.Get(ingtypes.BackCorsExposeHeaders).Value - maxAge := cfg.Get(ingtypes.BackCorsMaxAge).Int() - d.backend.Cors = append(d.backend.Cors, &hatypes.BackendConfigCors{ - Paths: cfg.Paths, - Config: hatypes.Cors{ + for _, path := range d.backend.Paths { + config := d.mapper.GetConfig(path.Link) + enabled := config.Get(ingtypes.BackCorsEnable).Bool() + if enabled { + path.Cors = hatypes.Cors{ Enabled: enabled, - AllowCredentials: allowCredentials, - AllowHeaders: allowHeaders, - AllowMethods: allowMethods, - AllowOrigin: allowOrigin, - ExposeHeaders: exposeHeaders, - MaxAge: maxAge, - }, - }) + AllowCredentials: config.Get(ingtypes.BackCorsAllowCredentials).Bool(), + AllowHeaders: config.Get(ingtypes.BackCorsAllowHeaders).Value, + AllowMethods: config.Get(ingtypes.BackCorsAllowMethods).Value, + AllowOrigin: config.Get(ingtypes.BackCorsAllowOrigin).Value, + ExposeHeaders: config.Get(ingtypes.BackCorsExposeHeaders).Value, + MaxAge: config.Get(ingtypes.BackCorsMaxAge).Int(), + } + } } } diff --git a/pkg/converters/ingress/annotations/backend_test.go b/pkg/converters/ingress/annotations/backend_test.go index 317e9fafc..1d3a516d7 100644 --- a/pkg/converters/ingress/annotations/backend_test.go +++ b/pkg/converters/ingress/annotations/backend_test.go @@ -850,17 +850,14 @@ func TestCors(t *testing.T) { testCases := []struct { paths []string ann map[string]map[string]string - expected []*hatypes.BackendConfigCors + expected map[string]hatypes.Cors logging string }{ // 0 { paths: []string{"/"}, - expected: []*hatypes.BackendConfigCors{ - { - Paths: createBackendPaths("/"), - Config: hatypes.Cors{}, - }, + expected: map[string]hatypes.Cors{ + "/": {}, }, }, // 1 @@ -870,18 +867,15 @@ func TestCors(t *testing.T) { ingtypes.BackCorsEnable: "true", }, }, - expected: []*hatypes.BackendConfigCors{ - { - Paths: createBackendPaths("/"), - Config: hatypes.Cors{ - Enabled: true, - AllowCredentials: false, - AllowHeaders: corsDefaultHeaders, - AllowMethods: corsDefaultMethods, - AllowOrigin: corsDefaultOrigin, - ExposeHeaders: "", - MaxAge: corsDefaultMaxAge, - }, + expected: map[string]hatypes.Cors{ + "/": { + Enabled: true, + AllowCredentials: false, + AllowHeaders: corsDefaultHeaders, + AllowMethods: corsDefaultMethods, + AllowOrigin: corsDefaultOrigin, + ExposeHeaders: "", + MaxAge: corsDefaultMaxAge, }, }, }, @@ -895,22 +889,16 @@ func TestCors(t *testing.T) { ingtypes.BackCorsEnable: "true", }, }, - expected: []*hatypes.BackendConfigCors{ - { - Paths: createBackendPaths("/"), - Config: hatypes.Cors{}, - }, - { - Paths: createBackendPaths("/sub"), - Config: hatypes.Cors{ - Enabled: true, - AllowCredentials: false, - AllowHeaders: corsDefaultHeaders, - AllowMethods: corsDefaultMethods, - AllowOrigin: corsDefaultOrigin, - ExposeHeaders: "", - MaxAge: corsDefaultMaxAge, - }, + expected: map[string]hatypes.Cors{ + "/": {}, + "/sub": { + Enabled: true, + AllowCredentials: false, + AllowHeaders: corsDefaultHeaders, + AllowMethods: corsDefaultMethods, + AllowOrigin: corsDefaultOrigin, + ExposeHeaders: "", + MaxAge: corsDefaultMaxAge, }, }, }, @@ -925,7 +913,11 @@ func TestCors(t *testing.T) { c := setup(t) d := c.createBackendMappingData("default/app", &Source{}, annDefault, test.ann, test.paths) c.createUpdater().buildBackendCors(d) - c.compareObjects("cors", i, d.backend.Cors, test.expected) + actual := map[string]hatypes.Cors{} + for _, path := range d.backend.Paths { + actual[path.Path()] = path.Cors + } + c.compareObjects("cors", i, actual, test.expected) c.logger.CompareLogging(test.logging) c.teardown() } diff --git a/pkg/haproxy/instance_test.go b/pkg/haproxy/instance_test.go index 0f2c85bcd..ac8c067b0 100644 --- a/pkg/haproxy/instance_test.go +++ b/pkg/haproxy/instance_test.go @@ -91,12 +91,8 @@ func TestBackends(t *testing.T) { AllowMethods: "GET, PUT, POST, DELETE, PATCH, OPTIONS", MaxAge: 86400, } - b.Cors = []*hatypes.BackendConfigCors{ - { - Paths: createBackendPaths(b, "d1.local/", "d1.local/sub"), - Config: config, - }, - } + b.FindBackendPath(h.FindPath("/").Link).Cors = config + b.FindBackendPath(h.FindPath("/sub").Link).Cors = config }, path: []string{"/", "/sub"}, expected: ` @@ -116,16 +112,7 @@ func TestBackends(t *testing.T) { MaxAge: 86400, AllowCredentials: true, } - b.Cors = []*hatypes.BackendConfigCors{ - { - Paths: createBackendPaths(b, "d1.local/"), - Config: config, - }, - { - Paths: createBackendPaths(b, "d1.local/sub"), - Config: hatypes.Cors{}, - }, - } + b.FindBackendPath(h.FindPath("/").Link).Cors = config }, path: []string{"/", "/sub"}, expected: ` diff --git a/pkg/haproxy/types/backend.go b/pkg/haproxy/types/backend.go index ccc27dd11..209068db7 100644 --- a/pkg/haproxy/types/backend.go +++ b/pkg/haproxy/types/backend.go @@ -183,8 +183,8 @@ func (b *Backend) CreateConfigInt(value int64) []*BackendConfigInt { // HasCorsEnabled ... func (b *Backend) HasCorsEnabled() bool { - for _, cors := range b.Cors { - if cors.Config.Enabled { + for _, path := range b.Paths { + if path.Cors.Enabled { return true } } @@ -258,7 +258,7 @@ func (b *Backend) NeedACL() bool { } } return len(b.MaxBodySize) > 1 || len(b.RewriteURL) > 1 || len(b.WhitelistHTTP) > 1 || - len(b.Cors) > 1 || len(b.WAF) > 1 + len(b.WAF) > 1 } func (b *Backend) ensurePathConfig(attr string) { @@ -405,11 +405,6 @@ func (b *BackendConfigStr) String() string { return fmt.Sprintf("%+v", *b) } -// String ... -func (b *BackendConfigCors) String() string { - return fmt.Sprintf("%+v", *b) -} - // String ... func (b *BackendConfigWhitelist) String() string { return fmt.Sprintf("%+v", *b) diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index 01882a61b..59dd1c26c 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -488,7 +488,6 @@ type Backend struct { // Template uses this func in order to know if a config // has two or more paths, and so need to be configured with ACL. // - Cors []*BackendConfigCors MaxBodySize []*BackendConfigInt RewriteURL []*BackendConfigStr SSLRedirect []*BackendConfigBool @@ -574,12 +573,6 @@ type BackendConfigStr struct { Config string } -// BackendConfigCors ... -type BackendConfigCors struct { - Paths BackendPaths - Config Cors -} - // BackendConfigWAF defines Web Application Firewall Configurations type BackendConfigWAF struct { Paths BackendPaths diff --git a/rootfs/etc/templates/haproxy/haproxy.tmpl b/rootfs/etc/templates/haproxy/haproxy.tmpl index 4805ba2ad..6630c03d2 100644 --- a/rootfs/etc/templates/haproxy/haproxy.tmpl +++ b/rootfs/etc/templates/haproxy/haproxy.tmpl @@ -464,15 +464,15 @@ backend {{ $backend.ID }} {{- end }} {{- /*------------------------------------*/}} -{{- $needACL := gt (len $backend.Cors) 1 }} -{{- range $corsCfg := $backend.Cors }} -{{- $cors := $corsCfg.Config }} +{{- $corsCfg := $backend.PathConfig "Cors" }} +{{- $needACL := $corsCfg.NeedACL }} +{{- range $i, $cors := $corsCfg.Items }} {{- if $cors.Enabled }} -{{- $paths := $corsCfg.Paths }} +{{- $pathIDs := $corsCfg.PathIDs $i }} http-request set-var(txn.cors_max_age) str({{ $cors.MaxAge }}) if METH_OPTIONS - {{- if $needACL }} { var(txn.pathID) {{ $paths.IDList }} }{{ end }} + {{- if $needACL }} { var(txn.pathID) {{ $pathIDs }} }{{ end }} http-request use-service lua.send-cors-preflight if METH_OPTIONS - {{- if $needACL }} { var(txn.pathID) {{ $paths.IDList }} }{{ end }} + {{- if $needACL }} { var(txn.pathID) {{ $pathIDs }} }{{ end }} {{- end }} {{- end }} @@ -554,24 +554,23 @@ backend {{ $backend.ID }} {{- end }} {{- /*------------------------------------*/}} -{{- $needACL := gt (len $backend.Cors) 1 }} -{{- range $corsCfg := $backend.Cors }} -{{- $cors := $corsCfg.Config }} +{{- $needACL := $corsCfg.NeedACL }} +{{- range $i, $cors := $corsCfg.Items }} {{- if $cors.Enabled }} -{{- $paths := $corsCfg.Paths }} +{{- $pathIDs := $corsCfg.PathIDs $i }} http-response set-header Access-Control-Allow-Origin "{{ $cors.AllowOrigin }}" - {{- if $needACL }} if { var(txn.pathID) {{ $paths.IDList }} }{{ end }} + {{- if $needACL }} if { var(txn.pathID) {{ $pathIDs }} }{{ end }} http-response set-header Access-Control-Allow-Methods "{{ $cors.AllowMethods }}" - {{- if $needACL }} if { var(txn.pathID) {{ $paths.IDList }} }{{ end }} + {{- if $needACL }} if { var(txn.pathID) {{ $pathIDs }} }{{ end }} http-response set-header Access-Control-Allow-Headers "{{ $cors.AllowHeaders }}" - {{- if $needACL }} if { var(txn.pathID) {{ $paths.IDList }} }{{ end }} + {{- if $needACL }} if { var(txn.pathID) {{ $pathIDs }} }{{ end }} {{- if $cors.AllowCredentials }} http-response set-header Access-Control-Allow-Credentials "{{ $cors.AllowCredentials }}" - {{- if $needACL }} if { var(txn.pathID) {{ $paths.IDList }} }{{ end }} + {{- if $needACL }} if { var(txn.pathID) {{ $pathIDs }} }{{ end }} {{- end }} {{- if $cors.ExposeHeaders }} http-response set-header Access-Control-Expose-Headers "{{ $cors.ExposeHeaders }}" - {{- if $needACL }} if { var(txn.pathID) {{ $paths.IDList }} }{{ end }} + {{- if $needACL }} if { var(txn.pathID) {{ $pathIDs }} }{{ end }} {{- end }} {{- end }} {{- end }} From f366f59b2f682ea0293ed5fb2c6370cb5327c4fc Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Thu, 10 Sep 2020 20:58:04 -0300 Subject: [PATCH 06/13] convert maxbodysize to the new per path config --- pkg/converters/ingress/annotations/backend.go | 34 ++++------- .../ingress/annotations/backend_test.go | 58 ++++++------------- pkg/haproxy/instance_test.go | 14 +---- pkg/haproxy/types/backend.go | 17 +----- pkg/haproxy/types/types.go | 9 +-- rootfs/etc/templates/haproxy/haproxy.tmpl | 11 ++-- 6 files changed, 42 insertions(+), 101 deletions(-) diff --git a/pkg/converters/ingress/annotations/backend.go b/pkg/converters/ingress/annotations/backend.go index ba23f7060..248571526 100644 --- a/pkg/converters/ingress/annotations/backend.go +++ b/pkg/converters/ingress/annotations/backend.go @@ -371,28 +371,18 @@ func (c *updater) buildBackendBlueGreenSelector(d *backData) { } func (c *updater) buildBackendBodySize(d *backData) { - config := d.mapper.GetBackendConfig( - d.backend, - []string{ingtypes.BackProxyBodySize}, - func(path *hatypes.BackendPath, values map[string]*ConfigValue) map[string]*ConfigValue { - bodysize := values[ingtypes.BackProxyBodySize] - if bodysize == nil || bodysize.Value == "unlimited" { - return nil - } - value, err := utils.SizeSuffixToInt64(bodysize.Value) - if err != nil { - c.logger.Warn("ignoring invalid body size on %v: %s", bodysize.Source, bodysize.Value) - return nil - } - bodysize.Value = strconv.FormatInt(value, 10) - return values - }, - ) - for _, cfg := range config { - d.backend.MaxBodySize = append(d.backend.MaxBodySize, &hatypes.BackendConfigInt{ - Paths: cfg.Paths, - Config: cfg.Get(ingtypes.BackProxyBodySize).Int64(), - }) + for _, path := range d.backend.Paths { + config := d.mapper.GetConfig(path.Link) + bodysize := config.Get(ingtypes.BackProxyBodySize) + if bodysize == nil || bodysize.Value == "" || bodysize.Value == "unlimited" { + continue + } + value, err := utils.SizeSuffixToInt64(bodysize.Value) + if err != nil { + c.logger.Warn("ignoring invalid body size on %v: %s", bodysize.Source, bodysize.Value) + continue + } + path.MaxBodySize = value } } diff --git a/pkg/converters/ingress/annotations/backend_test.go b/pkg/converters/ingress/annotations/backend_test.go index 1d3a516d7..f65492746 100644 --- a/pkg/converters/ingress/annotations/backend_test.go +++ b/pkg/converters/ingress/annotations/backend_test.go @@ -733,7 +733,7 @@ func TestBodySize(t *testing.T) { annDefault map[string]string ann map[string]map[string]string paths []string - expected []*hatypes.BackendConfigInt + expected map[string]int64 logging string }{ // 0 @@ -743,11 +743,8 @@ func TestBodySize(t *testing.T) { ingtypes.BackProxyBodySize: "10", }, }, - expected: []*hatypes.BackendConfigInt{ - { - Paths: createBackendPaths("/"), - Config: 10, - }, + expected: map[string]int64{ + "/": 10, }, }, // 1 @@ -763,19 +760,10 @@ func TestBodySize(t *testing.T) { ingtypes.BackProxyBodySize: "10g", }, }, - expected: []*hatypes.BackendConfigInt{ - { - Paths: createBackendPaths("/"), - Config: 10240, - }, - { - Paths: createBackendPaths("/app"), - Config: 10485760, - }, - { - Paths: createBackendPaths("/sub"), - Config: 10737418240, - }, + expected: map[string]int64{ + "/": 10240, + "/app": 10485760, + "/sub": 10737418240, }, }, // 2 @@ -785,11 +773,8 @@ func TestBodySize(t *testing.T) { ingtypes.BackProxyBodySize: "unlimited", }, }, - expected: []*hatypes.BackendConfigInt{ - { - Paths: createBackendPaths("/"), - Config: 0, - }, + expected: map[string]int64{ + "/": 0, }, }, // 3 @@ -799,11 +784,8 @@ func TestBodySize(t *testing.T) { ingtypes.BackProxyBodySize: "10e", }, }, - expected: []*hatypes.BackendConfigInt{ - { - Paths: createBackendPaths("/"), - Config: 0, - }, + expected: map[string]int64{ + "/": 0, }, source: Source{Namespace: "default", Name: "ing1", Type: "ingress"}, logging: `WARN ignoring invalid body size on ingress 'default/ing1': 10e`, @@ -815,15 +797,9 @@ func TestBodySize(t *testing.T) { ingtypes.BackProxyBodySize: "1m", }, }, - expected: []*hatypes.BackendConfigInt{ - { - Paths: createBackendPaths("/"), - Config: 0, - }, - { - Paths: createBackendPaths("/app"), - Config: 1048576, - }, + expected: map[string]int64{ + "/": 0, + "/app": 1048576, }, paths: []string{"/"}, source: Source{Namespace: "default", Name: "ing1", Type: "ingress"}, @@ -833,7 +809,11 @@ func TestBodySize(t *testing.T) { c := setup(t) d := c.createBackendMappingData("default/app", &test.source, test.annDefault, test.ann, test.paths) c.createUpdater().buildBackendBodySize(d) - c.compareObjects("proxy body size", i, d.backend.MaxBodySize, test.expected) + actual := map[string]int64{} + for _, path := range d.backend.Paths { + actual[path.Path()] = path.MaxBodySize + } + c.compareObjects("proxy body size", i, actual, test.expected) c.logger.CompareLogging(test.logging) c.teardown() } diff --git a/pkg/haproxy/instance_test.go b/pkg/haproxy/instance_test.go index ac8c067b0..cc83684c2 100644 --- a/pkg/haproxy/instance_test.go +++ b/pkg/haproxy/instance_test.go @@ -364,7 +364,8 @@ d1.local/api path02`, }, { doconfig: func(g *hatypes.Global, h *hatypes.Host, b *hatypes.Backend) { - b.MaxBodySize = b.CreateConfigInt(1024) + b.FindBackendPath(h.FindPath("/").Link).MaxBodySize = 1024 + b.FindBackendPath(h.FindPath("/app").Link).MaxBodySize = 1024 }, path: []string{"/", "/app"}, expected: ` @@ -372,16 +373,7 @@ d1.local/api path02`, }, { doconfig: func(g *hatypes.Global, h *hatypes.Host, b *hatypes.Backend) { - b.MaxBodySize = []*hatypes.BackendConfigInt{ - { - Paths: createBackendPaths(b, "d1.local/app"), - Config: 2048, - }, - { - Paths: createBackendPaths(b, "d1.local/"), - Config: 0, - }, - } + b.FindBackendPath(h.FindPath("/app").Link).MaxBodySize = 2048 }, path: []string{"/", "/app"}, expected: ` diff --git a/pkg/haproxy/types/backend.go b/pkg/haproxy/types/backend.go index 209068db7..d85c7ae33 100644 --- a/pkg/haproxy/types/backend.go +++ b/pkg/haproxy/types/backend.go @@ -171,16 +171,6 @@ func (b *Backend) CreateConfigBool(value bool) []*BackendConfigBool { } } -// CreateConfigInt ... -func (b *Backend) CreateConfigInt(value int64) []*BackendConfigInt { - return []*BackendConfigInt{ - { - Paths: NewBackendPaths(b.Paths...), - Config: value, - }, - } -} - // HasCorsEnabled ... func (b *Backend) HasCorsEnabled() bool { for _, path := range b.Paths { @@ -257,7 +247,7 @@ func (b *Backend) NeedACL() bool { return true } } - return len(b.MaxBodySize) > 1 || len(b.RewriteURL) > 1 || len(b.WhitelistHTTP) > 1 || + return len(b.RewriteURL) > 1 || len(b.WhitelistHTTP) > 1 || len(b.WAF) > 1 } @@ -395,11 +385,6 @@ func (b *BackendConfigBool) String() string { return fmt.Sprintf("%+v", *b) } -// String ... -func (b *BackendConfigInt) String() string { - return fmt.Sprintf("%+v", *b) -} - // String ... func (b *BackendConfigStr) String() string { return fmt.Sprintf("%+v", *b) diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index 59dd1c26c..8b60fb168 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -488,7 +488,6 @@ type Backend struct { // Template uses this func in order to know if a config // has two or more paths, and so need to be configured with ACL. // - MaxBodySize []*BackendConfigInt RewriteURL []*BackendConfigStr SSLRedirect []*BackendConfigBool WAF []*BackendConfigWAF @@ -542,7 +541,7 @@ type BackendPath struct { AuthHTTP AuthHTTP Cors Cors HSTS HSTS - MaxBodySize int + MaxBodySize int64 RewriteURL string SSLRedirect bool WAF WAF @@ -561,12 +560,6 @@ type BackendConfigBool struct { Config bool } -// BackendConfigInt ... -type BackendConfigInt struct { - Paths BackendPaths - Config int64 -} - // BackendConfigStr ... type BackendConfigStr struct { Paths BackendPaths diff --git a/rootfs/etc/templates/haproxy/haproxy.tmpl b/rootfs/etc/templates/haproxy/haproxy.tmpl index 6630c03d2..9dfb3df49 100644 --- a/rootfs/etc/templates/haproxy/haproxy.tmpl +++ b/rootfs/etc/templates/haproxy/haproxy.tmpl @@ -424,12 +424,13 @@ backend {{ $backend.ID }} {{- end }} {{- /*------------------------------------*/}} -{{- $needACL := gt (len $backend.MaxBodySize) 1 }} -{{- range $maxbody := $backend.MaxBodySize }} -{{- if $maxbody.Config }} +{{- $maxbodyCfg := $backend.PathConfig "MaxBodySize" }} +{{- $needACL := $maxbodyCfg.NeedACL }} +{{- range $i, $maxbody := $maxbodyCfg.Items }} +{{- if $maxbody }} http-request use-service lua.send-413 if - {{- if $needACL }} { var(txn.pathID) {{ $maxbody.Paths.IDList }} }{{ end }} - {{- "" }} { req.body_size,sub({{ $maxbody.Config }}) gt 0 } + {{- if $needACL }} { var(txn.pathID) {{ $maxbodyCfg.PathIDs $i }} }{{ end }} + {{- "" }} { req.body_size,sub({{ $maxbody }}) gt 0 } {{- end }} {{- end }} From c2015c749c3b7d1244dad3c9e577ae56565201af Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Thu, 10 Sep 2020 21:18:34 -0300 Subject: [PATCH 07/13] convert rewriteurl to the new per path config --- pkg/converters/ingress/annotations/backend.go | 35 +++++++------------ .../ingress/annotations/backend_test.go | 9 ++--- pkg/haproxy/instance_test.go | 35 ++++--------------- pkg/haproxy/types/backend.go | 7 +--- pkg/haproxy/types/types.go | 7 ---- rootfs/etc/templates/haproxy/haproxy.tmpl | 8 ++--- 6 files changed, 27 insertions(+), 74 deletions(-) diff --git a/pkg/converters/ingress/annotations/backend.go b/pkg/converters/ingress/annotations/backend.go index 248571526..bead6ef21 100644 --- a/pkg/converters/ingress/annotations/backend.go +++ b/pkg/converters/ingress/annotations/backend.go @@ -638,28 +638,19 @@ var ( ) func (c *updater) buildBackendRewriteURL(d *backData) { - config := d.mapper.GetBackendConfig( - d.backend, - []string{ingtypes.BackRewriteTarget}, - func(path *hatypes.BackendPath, values map[string]*ConfigValue) map[string]*ConfigValue { - rewrite, found := values[ingtypes.BackRewriteTarget] - if !found { - return nil - } - if !rewriteURLRegex.MatchString(rewrite.Value) { - c.logger.Warn( - "rewrite-target does not allow white spaces or single/double quotes on %v: '%s'", - rewrite.Source, rewrite.Value) - return nil - } - return values - }, - ) - for _, cfg := range config { - d.backend.RewriteURL = append(d.backend.RewriteURL, &hatypes.BackendConfigStr{ - Paths: cfg.Paths, - Config: cfg.Get(ingtypes.BackRewriteTarget).Value, - }) + for _, path := range d.backend.Paths { + config := d.mapper.GetConfig(path.Link) + rewrite := config.Get(ingtypes.BackRewriteTarget) + if rewrite == nil || rewrite.Value == "" { + continue + } + if !rewriteURLRegex.MatchString(rewrite.Value) { + c.logger.Warn( + "rewrite-target does not allow white spaces or single/double quotes on %v: '%s'", + rewrite.Source, rewrite.Value) + continue + } + path.RewriteURL = rewrite.Value } } diff --git a/pkg/converters/ingress/annotations/backend_test.go b/pkg/converters/ingress/annotations/backend_test.go index f65492746..8eb466df3 100644 --- a/pkg/converters/ingress/annotations/backend_test.go +++ b/pkg/converters/ingress/annotations/backend_test.go @@ -1252,13 +1252,8 @@ func TestRewriteURL(t *testing.T) { d.backend.AddBackendPath(hatypes.CreatePathLink("d1.local", "/")) d.mapper.AddAnnotations(&test.source, hatypes.CreatePathLink("d1.local", "/"), ann) c.createUpdater().buildBackendRewriteURL(d) - expected := []*hatypes.BackendConfigStr{ - { - Paths: hatypes.NewBackendPaths(d.backend.Paths...), - Config: test.expected, - }, - } - c.compareObjects("rewrite", i, d.backend.RewriteURL, expected) + actual := d.backend.Paths[0].RewriteURL + c.compareObjects("rewrite", i, actual, test.expected) c.logger.CompareLogging(test.logging) c.teardown() } diff --git a/pkg/haproxy/instance_test.go b/pkg/haproxy/instance_test.go index cc83684c2..83dfdd6f1 100644 --- a/pkg/haproxy/instance_test.go +++ b/pkg/haproxy/instance_test.go @@ -179,12 +179,7 @@ d1.local/ path01`, }, { doconfig: func(g *hatypes.Global, h *hatypes.Host, b *hatypes.Backend) { - b.RewriteURL = []*hatypes.BackendConfigStr{ - { - Paths: createBackendPaths(b, "d1.local/app"), - Config: "/", - }, - } + b.FindBackendPath(h.FindPath("/app").Link).RewriteURL = "/" }, path: []string{"/app"}, expected: ` @@ -192,12 +187,7 @@ d1.local/ path01`, }, { doconfig: func(g *hatypes.Global, h *hatypes.Host, b *hatypes.Backend) { - b.RewriteURL = []*hatypes.BackendConfigStr{ - { - Paths: createBackendPaths(b, "d1.local/app"), - Config: "/other", - }, - } + b.FindBackendPath(h.FindPath("/app").Link).RewriteURL = "/other" }, path: []string{"/app"}, expected: ` @@ -205,12 +195,8 @@ d1.local/ path01`, }, { doconfig: func(g *hatypes.Global, h *hatypes.Host, b *hatypes.Backend) { - b.RewriteURL = []*hatypes.BackendConfigStr{ - { - Paths: createBackendPaths(b, "d1.local/app", "d1.local/app/sub"), - Config: "/other/", - }, - } + b.FindBackendPath(h.FindPath("/app").Link).RewriteURL = "/other/" + b.FindBackendPath(h.FindPath("/app/sub").Link).RewriteURL = "/other/" }, path: []string{"/app", "/app/sub"}, expected: ` @@ -219,16 +205,9 @@ d1.local/ path01`, }, { doconfig: func(g *hatypes.Global, h *hatypes.Host, b *hatypes.Backend) { - b.RewriteURL = []*hatypes.BackendConfigStr{ - { - Paths: createBackendPaths(b, "d1.local/path1"), - Config: "/sub1", - }, - { - Paths: createBackendPaths(b, "d1.local/path2", "d1.local/path3"), - Config: "/sub2", - }, - } + b.FindBackendPath(h.FindPath("/path1").Link).RewriteURL = "/sub1" + b.FindBackendPath(h.FindPath("/path2").Link).RewriteURL = "/sub2" + b.FindBackendPath(h.FindPath("/path3").Link).RewriteURL = "/sub2" }, path: []string{"/path1", "/path2", "/path3"}, expected: ` diff --git a/pkg/haproxy/types/backend.go b/pkg/haproxy/types/backend.go index d85c7ae33..2e41a0e01 100644 --- a/pkg/haproxy/types/backend.go +++ b/pkg/haproxy/types/backend.go @@ -247,7 +247,7 @@ func (b *Backend) NeedACL() bool { return true } } - return len(b.RewriteURL) > 1 || len(b.WhitelistHTTP) > 1 || + return len(b.WhitelistHTTP) > 1 || len(b.WAF) > 1 } @@ -385,11 +385,6 @@ func (b *BackendConfigBool) String() string { return fmt.Sprintf("%+v", *b) } -// String ... -func (b *BackendConfigStr) String() string { - return fmt.Sprintf("%+v", *b) -} - // String ... func (b *BackendConfigWhitelist) String() string { return fmt.Sprintf("%+v", *b) diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index 8b60fb168..3cc5cf914 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -488,7 +488,6 @@ type Backend struct { // Template uses this func in order to know if a config // has two or more paths, and so need to be configured with ACL. // - RewriteURL []*BackendConfigStr SSLRedirect []*BackendConfigBool WAF []*BackendConfigWAF WhitelistHTTP []*BackendConfigWhitelist @@ -560,12 +559,6 @@ type BackendConfigBool struct { Config bool } -// BackendConfigStr ... -type BackendConfigStr struct { - Paths BackendPaths - Config string -} - // BackendConfigWAF defines Web Application Firewall Configurations type BackendConfigWAF struct { Paths BackendPaths diff --git a/rootfs/etc/templates/haproxy/haproxy.tmpl b/rootfs/etc/templates/haproxy/haproxy.tmpl index 9dfb3df49..f5dd68e94 100644 --- a/rootfs/etc/templates/haproxy/haproxy.tmpl +++ b/rootfs/etc/templates/haproxy/haproxy.tmpl @@ -521,11 +521,11 @@ backend {{ $backend.ID }} {{- end }} {{- /*------------------------------------*/}} -{{- $needACL := gt (len $backend.RewriteURL) 1 }} -{{- range $rewriteCfg := $backend.RewriteURL }} -{{- $rewrite := $rewriteCfg.Config }} +{{- $rewriteCfg := $backend.PathConfig "RewriteURL" }} +{{- $needACL := $rewriteCfg.NeedACL }} +{{- range $i, $rewrite := $rewriteCfg.Items }} {{- if $rewrite }} -{{- range $path := $rewriteCfg.Paths.Items }} +{{- range $path := ($rewriteCfg.Paths $i) }} {{- if eq $rewrite "/" }} http-request replace-uri ^{{ $path.Path }}/?(.*)$ {{ $rewrite }}\1 {{- if $needACL }} if { var(txn.pathID) {{ $path.ID }} }{{ end }} From 19e2646026ded6b7e0fdc9f781e3ef58c15b1841 Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Thu, 10 Sep 2020 21:49:08 -0300 Subject: [PATCH 08/13] convert waf to the new per path config --- pkg/converters/ingress/annotations/backend.go | 52 ++++------- .../ingress/annotations/backend_test.go | 87 +++++++++---------- pkg/haproxy/instance_test.go | 14 +-- pkg/haproxy/types/backend.go | 12 +-- pkg/haproxy/types/types.go | 7 -- rootfs/etc/templates/haproxy/haproxy.tmpl | 9 +- 6 files changed, 69 insertions(+), 112 deletions(-) diff --git a/pkg/converters/ingress/annotations/backend.go b/pkg/converters/ingress/annotations/backend.go index bead6ef21..50b181132 100644 --- a/pkg/converters/ingress/annotations/backend.go +++ b/pkg/converters/ingress/annotations/backend.go @@ -723,39 +723,25 @@ func (c *updater) buildBackendTimeout(d *backData) { } func (c *updater) buildBackendWAF(d *backData) { - config := d.mapper.GetBackendConfig( - d.backend, - []string{ingtypes.BackWAF, ingtypes.BackWAFMode}, - func(path *hatypes.BackendPath, values map[string]*ConfigValue) map[string]*ConfigValue { - waf, foundWAF := values[ingtypes.BackWAF] - if !foundWAF { - return nil - } - if waf.Value != "modsecurity" { - c.logger.Warn("ignoring invalid WAF module on %s: %s", waf.Source, waf.Value) - return nil - } - wafMode, foundWAFMode := values[ingtypes.BackWAFMode] - if !foundWAFMode { - return values - } - if wafMode.Value != "deny" && wafMode.Value != "detect" { - c.logger.Warn("ignoring invalid WAF mode '%s' on %s, using 'deny' instead", wafMode.Value, wafMode.Source) - wafMode.Value = "deny" - } - return values - }, - ) - for _, cfg := range config { - wafModule := cfg.Get(ingtypes.BackWAF).Value - wafMode := cfg.Get(ingtypes.BackWAFMode).Value - d.backend.WAF = append(d.backend.WAF, &hatypes.BackendConfigWAF{ - Paths: cfg.Paths, - Config: hatypes.WAF{ - Module: wafModule, - Mode: wafMode, - }, - }) + for _, path := range d.backend.Paths { + config := d.mapper.GetConfig(path.Link) + waf := config.Get(ingtypes.BackWAF) + module := waf.Value + if module == "" { + continue + } + if module != "modsecurity" { + c.logger.Warn("ignoring invalid WAF module on %s: %s", waf.Source, module) + continue + } + wafMode := config.Get(ingtypes.BackWAFMode) + mode := wafMode.Value + if mode != "" && mode != "deny" && mode != "detect" { + c.logger.Warn("ignoring invalid WAF mode '%s' on %s, using 'deny' instead", mode, wafMode.Source) + mode = "deny" + } + path.WAF.Module = module + path.WAF.Mode = mode } } diff --git a/pkg/converters/ingress/annotations/backend_test.go b/pkg/converters/ingress/annotations/backend_test.go index 8eb466df3..5c5799721 100644 --- a/pkg/converters/ingress/annotations/backend_test.go +++ b/pkg/converters/ingress/annotations/backend_test.go @@ -1672,59 +1672,59 @@ func TestTimeout(t *testing.T) { func TestWAF(t *testing.T) { testCase := []struct { - waf string - wafmode string - expected string - expectedmode string - logging string + waf string + wafmode string + expected hatypes.WAF + logging string }{ // 0 - { - waf: "", - wafmode: "", - expected: "", - expectedmode: "", - logging: "", - }, + {}, // 1 { - waf: "none", - wafmode: "deny", - expected: "", - expectedmode: "", - logging: "WARN ignoring invalid WAF module on ingress 'default/ing1': none", + waf: "none", + wafmode: "deny", + expected: hatypes.WAF{}, + logging: "WARN ignoring invalid WAF module on ingress 'default/ing1': none", }, // 2 { - waf: "modsecurity", - wafmode: "XXXXXX", - expected: "modsecurity", - expectedmode: "deny", - logging: "WARN ignoring invalid WAF mode 'XXXXXX' on ingress 'default/ing1', using 'deny' instead", + waf: "modsecurity", + wafmode: "XXXXXX", + expected: hatypes.WAF{ + Module: "modsecurity", + Mode: "deny", + }, + logging: "WARN ignoring invalid WAF mode 'XXXXXX' on ingress 'default/ing1', using 'deny' instead", }, // 3 { - waf: "modsecurity", - wafmode: "detect", - expected: "modsecurity", - expectedmode: "detect", - logging: "", + waf: "modsecurity", + wafmode: "detect", + expected: hatypes.WAF{ + Module: "modsecurity", + Mode: "detect", + }, + logging: "", }, // 4 { - waf: "modsecurity", - wafmode: "deny", - expected: "modsecurity", - expectedmode: "deny", - logging: "", + waf: "modsecurity", + wafmode: "deny", + expected: hatypes.WAF{ + Module: "modsecurity", + Mode: "deny", + }, + logging: "", }, // 5 { - waf: "modsecurity", - wafmode: "", - expected: "modsecurity", - expectedmode: "deny", - logging: "", + waf: "modsecurity", + wafmode: "", + expected: hatypes.WAF{ + Module: "modsecurity", + Mode: "deny", + }, + logging: "", }, } source := &Source{ @@ -1735,7 +1735,6 @@ func TestWAF(t *testing.T) { for i, test := range testCase { c := setup(t) var ann map[string]map[string]string - var expected []*hatypes.BackendConfigWAF ann = map[string]map[string]string{ "/": {}, } @@ -1745,18 +1744,10 @@ func TestWAF(t *testing.T) { if test.wafmode != "" { ann["/"][ingtypes.BackWAFMode] = test.wafmode } - expected = []*hatypes.BackendConfigWAF{ - { - Paths: createBackendPaths("/"), - Config: hatypes.WAF{ - Module: test.expected, - Mode: test.expectedmode, - }, - }, - } d := c.createBackendMappingData("default/app", source, map[string]string{ingtypes.BackWAFMode: "deny"}, ann, []string{}) c.createUpdater().buildBackendWAF(d) - c.compareObjects("WAF", i, d.backend.WAF, expected) + actual := d.backend.Paths[0].WAF + c.compareObjects("WAF", i, actual, test.expected) c.logger.CompareLogging(test.logging) c.teardown() } diff --git a/pkg/haproxy/instance_test.go b/pkg/haproxy/instance_test.go index 83dfdd6f1..381d37db4 100644 --- a/pkg/haproxy/instance_test.go +++ b/pkg/haproxy/instance_test.go @@ -2733,20 +2733,12 @@ func TestModSecurity(t *testing.T) { test.path = "/" } h.AddPath(b, test.path, hatypes.MatchBegin) - b.WAF = []*hatypes.BackendConfigWAF{ - { - Paths: createBackendPaths(b, "d1.local"+test.path), - Config: hatypes.WAF{ - Module: test.waf, - Mode: test.wafmode, - }, - }, + b.FindBackendPath(h.FindPath(test.path).Link).WAF = hatypes.WAF{ + Module: test.waf, + Mode: test.wafmode, } if test.path != "/" { h.AddPath(b, "/", hatypes.MatchBegin) - b.WAF = append(b.WAF, &hatypes.BackendConfigWAF{ - Paths: createBackendPaths(b, "d1.local/"), - }) } globalModsec := &c.config.Global().ModSecurity diff --git a/pkg/haproxy/types/backend.go b/pkg/haproxy/types/backend.go index 2e41a0e01..04d46fae8 100644 --- a/pkg/haproxy/types/backend.go +++ b/pkg/haproxy/types/backend.go @@ -193,8 +193,8 @@ func (b *Backend) HasHSTS() bool { // HasModsec is a method to verify if a Backend has ModSecurity Enabled func (b *Backend) HasModsec() bool { - for _, waf := range b.WAF { - if waf.Config.Module == "modsecurity" { + for _, path := range b.Paths { + if path.WAF.Module == "modsecurity" { return true } } @@ -247,8 +247,7 @@ func (b *Backend) NeedACL() bool { return true } } - return len(b.WhitelistHTTP) > 1 || - len(b.WAF) > 1 + return len(b.WhitelistHTTP) > 1 } func (b *Backend) ensurePathConfig(attr string) { @@ -389,8 +388,3 @@ func (b *BackendConfigBool) String() string { func (b *BackendConfigWhitelist) String() string { return fmt.Sprintf("%+v", *b) } - -// String ... -func (b *BackendConfigWAF) String() string { - return fmt.Sprintf("%+v", *b) -} diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index 3cc5cf914..2a23121c3 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -489,7 +489,6 @@ type Backend struct { // has two or more paths, and so need to be configured with ACL. // SSLRedirect []*BackendConfigBool - WAF []*BackendConfigWAF WhitelistHTTP []*BackendConfigWhitelist } @@ -559,12 +558,6 @@ type BackendConfigBool struct { Config bool } -// BackendConfigWAF defines Web Application Firewall Configurations -type BackendConfigWAF struct { - Paths BackendPaths - Config WAF -} - // BackendConfigWhitelist ... type BackendConfigWhitelist struct { Paths BackendPaths diff --git a/rootfs/etc/templates/haproxy/haproxy.tmpl b/rootfs/etc/templates/haproxy/haproxy.tmpl index f5dd68e94..2dfd9c7e9 100644 --- a/rootfs/etc/templates/haproxy/haproxy.tmpl +++ b/rootfs/etc/templates/haproxy/haproxy.tmpl @@ -437,11 +437,12 @@ backend {{ $backend.ID }} {{- /*------------------------------------*/}} {{- if and $global.ModSecurity.Endpoints $backend.HasModsec }} filter spoe engine modsecurity config /etc/haproxy/spoe-modsecurity.conf -{{- $needACL := gt (len $backend.WAF) 1 }} -{{- range $waf := $backend.WAF }} -{{- if eq $waf.Config.Mode "deny" }} +{{- $wafCfg := $backend.PathConfig "WAF" }} +{{- $needACL := $wafCfg.NeedACL }} +{{- range $i, $waf := $wafCfg.Items }} +{{- if eq $waf.Mode "deny" }} http-request deny if { var(txn.modsec.code) -m int gt 0 } - {{- if $needACL }} { var(txn.pathID) {{ $waf.Paths.IDList }} }{{ end }} + {{- if $needACL }} { var(txn.pathID) {{ $wafCfg.PathIDs $i }} }{{ end }} {{- end }} {{- end }} {{- end }} From 1cb0a1f8658b1605f3c1174d16b771fc15e11618 Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Fri, 11 Sep 2020 20:47:22 -0300 Subject: [PATCH 09/13] convert whitelist http to the new per path config --- pkg/converters/ingress/annotations/backend.go | 13 ++- .../ingress/annotations/backend_test.go | 87 +++++++++++++------ pkg/haproxy/instance_test.go | 53 ++++------- pkg/haproxy/types/backend.go | 7 +- pkg/haproxy/types/types.go | 9 +- rootfs/etc/templates/haproxy/haproxy.tmpl | 7 +- 6 files changed, 87 insertions(+), 89 deletions(-) diff --git a/pkg/converters/ingress/annotations/backend.go b/pkg/converters/ingress/annotations/backend.go index 50b181132..e4850d569 100644 --- a/pkg/converters/ingress/annotations/backend.go +++ b/pkg/converters/ingress/annotations/backend.go @@ -746,14 +746,11 @@ func (c *updater) buildBackendWAF(d *backData) { } func (c *updater) buildBackendWhitelistHTTP(d *backData) { - if d.backend.ModeTCP { - return - } - for _, cfg := range d.mapper.GetBackendConfig(d.backend, []string{ingtypes.BackWhitelistSourceRange}, nil) { - d.backend.WhitelistHTTP = append(d.backend.WhitelistHTTP, &hatypes.BackendConfigWhitelist{ - Paths: cfg.Paths, - Config: c.splitCIDR(cfg.Get(ingtypes.BackWhitelistSourceRange)), - }) + if !d.backend.ModeTCP { + for _, path := range d.backend.Paths { + config := d.mapper.GetConfig(path.Link) + path.WhitelistHTTP = c.splitCIDR(config.Get(ingtypes.BackWhitelistSourceRange)) + } } } diff --git a/pkg/converters/ingress/annotations/backend_test.go b/pkg/converters/ingress/annotations/backend_test.go index 5c5799721..808cd1142 100644 --- a/pkg/converters/ingress/annotations/backend_test.go +++ b/pkg/converters/ingress/annotations/backend_test.go @@ -1757,7 +1757,7 @@ func TestWhitelistHTTP(t *testing.T) { testCases := []struct { paths []string cidrlist map[string]map[string]string - expected []*hatypes.BackendConfigWhitelist + expected map[string][]string logging string }{ // 0 @@ -1768,11 +1768,8 @@ func TestWhitelistHTTP(t *testing.T) { ingtypes.BackWhitelistSourceRange: "10.0.0.0/8,192.168.0.0/16", }, }, - expected: []*hatypes.BackendConfigWhitelist{ - { - Paths: createBackendPaths("/"), - Config: []string{"10.0.0.0/8", "192.168.0.0/16"}, - }, + expected: map[string][]string{ + "/": {"10.0.0.0/8", "192.168.0.0/16"}, }, }, // 1 @@ -1783,53 +1780,89 @@ func TestWhitelistHTTP(t *testing.T) { ingtypes.BackWhitelistSourceRange: "10.0.0.0/8,192.168.0.0/16", }, "/path": { - ingtypes.BackWhitelistSourceRange: "10.0.0.0/8,192.168.0.0/16", + ingtypes.BackWhitelistSourceRange: "10.0.0.0/48,192.168.0.0/48", }, "/url": { ingtypes.BackWhitelistSourceRange: "10.0.0.0/8,192.168.0.101", }, }, - expected: []*hatypes.BackendConfigWhitelist{ - { - Paths: createBackendPaths("/", "/path"), - Config: []string{"10.0.0.0/8", "192.168.0.0/16"}, - }, - { - Paths: createBackendPaths("/url"), - Config: []string{"10.0.0.0/8", "192.168.0.101"}, - }, + expected: map[string][]string{ + "/": {"10.0.0.0/8", "192.168.0.0/16"}, + "/url": {"10.0.0.0/8", "192.168.0.101"}, }, + logging: ` +WARN skipping invalid IP or cidr on ingress 'default/ing1': 10.0.0.0/48 +WARN skipping invalid IP or cidr on ingress 'default/ing1': 192.168.0.0/48`, }, // 2 + { + paths: []string{"/"}, + expected: map[string][]string{}, + }, + // 3 { paths: []string{"/"}, - expected: []*hatypes.BackendConfigWhitelist{ - { - Paths: createBackendPaths("/"), + cidrlist: map[string]map[string]string{ + "/": { + ingtypes.BackWhitelistSourceRange: "", }, }, + expected: map[string][]string{}, }, - // 3 + // 4 { paths: []string{"/"}, cidrlist: map[string]map[string]string{ "/": { - ingtypes.BackWhitelistSourceRange: "", + ingtypes.BackWhitelistSourceRange: "10.0.0.0/8,fa00::1:1,fa00::/64", }, }, - expected: []*hatypes.BackendConfigWhitelist{ - { - Paths: createBackendPaths("/"), - Config: nil, + expected: map[string][]string{ + "/": {"10.0.0.0/8", "fa00::1:1", "fa00::/64"}, + }, + }, + // 5 + { + paths: []string{"/"}, + cidrlist: map[string]map[string]string{ + "/": { + ingtypes.BackWhitelistSourceRange: "10.0.0.0/8,fa00::/129", + }, + }, + expected: map[string][]string{ + "/": {"10.0.0.0/8"}, + }, + logging: ` +WARN skipping invalid IP or cidr on ingress 'default/ing1': fa00::/129`, + }, + // 6 + { + paths: []string{"/"}, + cidrlist: map[string]map[string]string{ + "/": { + ingtypes.BackWhitelistSourceRange: "10.0.0.0/48,192.168.0.0/24", }, }, + expected: map[string][]string{ + "/": {"192.168.0.0/24"}, + }, + logging: ` +WARN skipping invalid IP or cidr on ingress 'default/ing1': 10.0.0.0/48`, }, } + source := &Source{Namespace: "default", Name: "ing1", Type: "ingress"} for i, test := range testCases { c := setup(t) - d := c.createBackendMappingData("default/app", &Source{}, map[string]string{}, test.cidrlist, test.paths) + d := c.createBackendMappingData("default/app", source, map[string]string{}, test.cidrlist, test.paths) c.createUpdater().buildBackendWhitelistHTTP(d) - c.compareObjects("whitelist http", i, d.backend.WhitelistHTTP, test.expected) + actual := map[string][]string{} + for _, path := range d.backend.Paths { + if len(path.WhitelistHTTP) > 0 { + actual[path.Path()] = path.WhitelistHTTP + } + } + c.compareObjects("whitelist http", i, actual, test.expected) + c.logger.CompareLogging(test.logging) c.teardown() } } diff --git a/pkg/haproxy/instance_test.go b/pkg/haproxy/instance_test.go index 381d37db4..a43c74a53 100644 --- a/pkg/haproxy/instance_test.go +++ b/pkg/haproxy/instance_test.go @@ -227,16 +227,9 @@ d1.local/path1 path01`, }, { doconfig: func(g *hatypes.Global, h *hatypes.Host, b *hatypes.Backend) { - b.WhitelistHTTP = []*hatypes.BackendConfigWhitelist{ - { - Paths: createBackendPaths(b, "d1.local/app", "d1.local/api"), - Config: []string{"10.0.0.0/8", "192.168.0.0/16"}, - }, - { - Paths: createBackendPaths(b, "d1.local/path"), - Config: []string{"192.168.95.0/24"}, - }, - } + b.FindBackendPath(h.FindPath("/app").Link).WhitelistHTTP = []string{"10.0.0.0/8", "192.168.0.0/16"} + b.FindBackendPath(h.FindPath("/api").Link).WhitelistHTTP = []string{"10.0.0.0/8", "192.168.0.0/16"} + b.FindBackendPath(h.FindPath("/path").Link).WhitelistHTTP = []string{"192.168.95.0/24"} }, path: []string{"/app", "/api", "/path"}, expected: ` @@ -257,19 +250,14 @@ d1.local/api path02`, }, { doconfig: func(g *hatypes.Global, h *hatypes.Host, b *hatypes.Backend) { + b.FindBackendPath(h.FindPath("/app").Link).WhitelistHTTP = []string{"10.0.0.0/8", "192.168.0.0/16"} + b.FindBackendPath(h.FindPath("/api").Link).WhitelistHTTP = []string{"10.0.0.0/8", "192.168.0.0/16"} + b.FindBackendPath(h.FindPath("/path").Link).WhitelistHTTP = []string{"10.0.0.0/8", "192.168.0.0/16"} + b.FindBackendPath(h.FindPath("/api/v[0-9]+/").Link).WhitelistHTTP = []string{"10.0.0.0/8", "192.168.0.0/16"} + b.FindBackendPath(h.FindPath("/").Link).WhitelistHTTP = []string{"172.17.0.0/16"} h.FindPath("/app").Match = hatypes.MatchExact h.FindPath("/path").Match = hatypes.MatchPrefix h.FindPath("/api/v[0-9]+/").Match = hatypes.MatchRegex - b.WhitelistHTTP = []*hatypes.BackendConfigWhitelist{ - { - Paths: createBackendPaths(b, "d1.local/app", "d1.local/api", "d1.local/path", "d1.local/api/v[0-9]+/"), - Config: []string{"10.0.0.0/8", "192.168.0.0/16"}, - }, - { - Paths: createBackendPaths(b, "d1.local/"), - Config: []string{"172.17.0.0/16"}, - }, - } }, path: []string{"/", "/app", "/api", "/path", "/api/v[0-9]+/"}, expected: ` @@ -282,10 +270,10 @@ d1.local/api path02`, http-request set-var(txn.pathID) var(req.base),map_dir(/etc/haproxy/maps/_back_d1_app_8080_idpath__prefix.map) if !{ var(txn.pathID) -m found } http-request set-var(txn.pathID) var(req.base),lower,map_beg(/etc/haproxy/maps/_back_d1_app_8080_idpath__begin.map) if !{ var(txn.pathID) -m found } http-request set-var(txn.pathID) var(req.base),map_reg(/etc/haproxy/maps/_back_d1_app_8080_idpath__regex.map) if !{ var(txn.pathID) -m found } - acl wlist_src0 src 10.0.0.0/8 192.168.0.0/16 - http-request deny if { var(txn.pathID) path03 path05 path02 path04 } !wlist_src0 - acl wlist_src1 src 172.17.0.0/16 - http-request deny if { var(txn.pathID) path01 } !wlist_src1`, + acl wlist_src0 src 172.17.0.0/16 + http-request deny if { var(txn.pathID) path01 } !wlist_src0 + acl wlist_src1 src 10.0.0.0/8 192.168.0.0/16 + http-request deny if { var(txn.pathID) path03 path05 path02 path04 } !wlist_src1`, expFronts: "<>", expCheck: map[string]string{ "_back_d1_app_8080_idpath__exact.map": ` @@ -301,19 +289,10 @@ d1.local/ path01`, }, { doconfig: func(g *hatypes.Global, h *hatypes.Host, b *hatypes.Backend) { - b.WhitelistHTTP = []*hatypes.BackendConfigWhitelist{ - { - Paths: createBackendPaths(b, "d1.local/app", "d1.local/api"), - Config: []string{}, - }, - { - Paths: createBackendPaths(b, "d1.local/path"), - Config: []string{ - "1.1.1.1", "1.1.1.2", "1.1.1.3", "1.1.1.4", "1.1.1.5", - "1.1.1.6", "1.1.1.7", "1.1.1.8", "1.1.1.9", "1.1.1.10", - "1.1.1.11", - }, - }, + b.FindBackendPath(h.FindPath("/path").Link).WhitelistHTTP = []string{ + "1.1.1.1", "1.1.1.2", "1.1.1.3", "1.1.1.4", "1.1.1.5", + "1.1.1.6", "1.1.1.7", "1.1.1.8", "1.1.1.9", "1.1.1.10", + "1.1.1.11", } }, path: []string{"/app", "/api", "/path"}, diff --git a/pkg/haproxy/types/backend.go b/pkg/haproxy/types/backend.go index 04d46fae8..73b9b6432 100644 --- a/pkg/haproxy/types/backend.go +++ b/pkg/haproxy/types/backend.go @@ -247,7 +247,7 @@ func (b *Backend) NeedACL() bool { return true } } - return len(b.WhitelistHTTP) > 1 + return false } func (b *Backend) ensurePathConfig(attr string) { @@ -383,8 +383,3 @@ func (h *BackendHeader) String() string { func (b *BackendConfigBool) String() string { return fmt.Sprintf("%+v", *b) } - -// String ... -func (b *BackendConfigWhitelist) String() string { - return fmt.Sprintf("%+v", *b) -} diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index 2a23121c3..a5a5517f0 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -488,8 +488,7 @@ type Backend struct { // Template uses this func in order to know if a config // has two or more paths, and so need to be configured with ACL. // - SSLRedirect []*BackendConfigBool - WhitelistHTTP []*BackendConfigWhitelist + SSLRedirect []*BackendConfigBool } // Endpoint ... @@ -558,12 +557,6 @@ type BackendConfigBool struct { Config bool } -// BackendConfigWhitelist ... -type BackendConfigWhitelist struct { - Paths BackendPaths - Config []string -} - // AgentCheck ... type AgentCheck struct { Addr string diff --git a/rootfs/etc/templates/haproxy/haproxy.tmpl b/rootfs/etc/templates/haproxy/haproxy.tmpl index 2dfd9c7e9..6e2c48301 100644 --- a/rootfs/etc/templates/haproxy/haproxy.tmpl +++ b/rootfs/etc/templates/haproxy/haproxy.tmpl @@ -398,14 +398,15 @@ backend {{ $backend.ID }} {{- end }} {{- /*------------------------------------*/}} -{{- range $i, $wlistCfg := $backend.WhitelistHTTP }} -{{- $wlist := $wlistCfg.Config }} +{{- $wlistCfg := $backend.PathConfig "WhitelistHTTP" }} +{{- $needACL := $wlistCfg.NeedACL }} +{{- range $i, $wlist := $wlistCfg.Items }} {{- if $wlist }} {{- range $w1 := short 10 $wlist }} acl wlist_src{{ $i }} src{{ range $w := $w1 }} {{ $w }}{{ end }} {{- end }} http-request deny if - {{- if gt (len $backend.WhitelistHTTP) 1 }} { var(txn.pathID) {{ $wlistCfg.Paths.IDList }} }{{ end }} + {{- if $needACL }} { var(txn.pathID) {{ $wlistCfg.PathIDs $i }} }{{ end }} {{- "" }} !wlist_src{{ $i }} {{- end }} {{- end }} From cdb14bb79d600963f1778ff5d01f86f5572de6f0 Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Fri, 11 Sep 2020 21:23:42 -0300 Subject: [PATCH 10/13] convert ssl redirect to the new per path config --- pkg/converters/ingress/annotations/backend.go | 24 +++----- .../ingress/annotations/backend_test.go | 59 ++++++++----------- pkg/haproxy/instance_test.go | 32 +++++----- pkg/haproxy/types/backend.go | 27 ++------- pkg/haproxy/types/types.go | 7 --- 5 files changed, 56 insertions(+), 93 deletions(-) diff --git a/pkg/converters/ingress/annotations/backend.go b/pkg/converters/ingress/annotations/backend.go index e4850d569..0ced3ecfa 100644 --- a/pkg/converters/ingress/annotations/backend.go +++ b/pkg/converters/ingress/annotations/backend.go @@ -678,23 +678,17 @@ func (c *updater) buildBackendSSL(d *backData) { func (c *updater) buildBackendSSLRedirect(d *backData) { noTLSRedir := utils.Split(d.mapper.Get(ingtypes.GlobalNoTLSRedirectLocations).Value, ",") - for _, redir := range d.mapper.GetBackendConfig( - d.backend, - []string{ingtypes.BackSSLRedirect}, - func(path *hatypes.BackendPath, values map[string]*ConfigValue) map[string]*ConfigValue { - for _, redir := range noTLSRedir { - if strings.HasPrefix(path.Path(), redir) { - values[ingtypes.BackSSLRedirect].Value = "false" - return values + for _, path := range d.backend.Paths { + config := d.mapper.GetConfig(path.Link) + redir := config.Get(ingtypes.BackSSLRedirect).Bool() + if redir { + for _, noredir := range noTLSRedir { + if strings.HasPrefix(path.Path(), noredir) { + redir = false } } - return values - }, - ) { - d.backend.SSLRedirect = append(d.backend.SSLRedirect, &hatypes.BackendConfigBool{ - Paths: redir.Paths, - Config: redir.Get(ingtypes.BackSSLRedirect).Bool(), - }) + } + path.SSLRedirect = redir } } diff --git a/pkg/converters/ingress/annotations/backend_test.go b/pkg/converters/ingress/annotations/backend_test.go index 808cd1142..d093c1b2e 100644 --- a/pkg/converters/ingress/annotations/backend_test.go +++ b/pkg/converters/ingress/annotations/backend_test.go @@ -1505,18 +1505,15 @@ func TestSSLRedirect(t *testing.T) { annDefault map[string]string ann map[string]map[string]string addPaths []string - expected []*hatypes.BackendConfigBool + expected map[bool][]string source Source logging string }{ // 0 { addPaths: []string{"/"}, - expected: []*hatypes.BackendConfigBool{ - { - Paths: createBackendPaths("/"), - Config: false, - }, + expected: map[bool][]string{ + false: {"/"}, }, }, // 1 @@ -1526,11 +1523,8 @@ func TestSSLRedirect(t *testing.T) { ingtypes.BackSSLRedirect: "true", }, }, - expected: []*hatypes.BackendConfigBool{ - { - Paths: createBackendPaths("/"), - Config: true, - }, + expected: map[bool][]string{ + true: {"/"}, }, }, // 2 @@ -1540,11 +1534,8 @@ func TestSSLRedirect(t *testing.T) { ingtypes.BackSSLRedirect: "invalid", }, }, - expected: []*hatypes.BackendConfigBool{ - { - Paths: createBackendPaths("/"), - Config: false, - }, + expected: map[bool][]string{ + false: {"/"}, }, source: Source{Namespace: "default", Name: "ing1", Type: "ingress"}, logging: `WARN ignoring invalid bool expression on ingress 'default/ing1' key 'ssl-redirect': invalid`, @@ -1566,15 +1557,9 @@ func TestSSLRedirect(t *testing.T) { ingtypes.BackSSLRedirect: "no-bool", }, }, - expected: []*hatypes.BackendConfigBool{ - { - Paths: createBackendPaths("/"), - Config: true, - }, - { - Paths: createBackendPaths("/other", "/path", "/url"), - Config: false, - }, + expected: map[bool][]string{ + false: {"/other", "/path", "/url"}, + true: {"/"}, }, source: Source{Namespace: "system1", Name: "app", Type: "service"}, logging: `WARN ignoring invalid bool expression on service 'system1/app' key 'ssl-redirect': no-bool`, @@ -1598,15 +1583,9 @@ func TestSSLRedirect(t *testing.T) { ingtypes.BackSSLRedirect: "true", }, }, - expected: []*hatypes.BackendConfigBool{ - { - Paths: createBackendPaths("/", "/.hidden/api", "/app"), - Config: false, - }, - { - Paths: createBackendPaths("/api"), - Config: true, - }, + expected: map[bool][]string{ + false: {"/", "/.hidden/api", "/app"}, + true: {"/api"}, }, }, } @@ -1614,7 +1593,17 @@ func TestSSLRedirect(t *testing.T) { c := setup(t) d := c.createBackendMappingData("default/app", &test.source, test.annDefault, test.ann, test.addPaths) c.createUpdater().buildBackendSSLRedirect(d) - c.compareObjects("sslredirect", i, d.backend.SSLRedirect, test.expected) + actual := map[bool][]string{} + for _, path := range d.backend.Paths { + actual[path.SSLRedirect] = append(actual[path.SSLRedirect], path.Path()) + } + if len(actual[false]) == 0 { + delete(actual, false) + } + if len(actual[true]) == 0 { + delete(actual, true) + } + c.compareObjects("sslredirect", i, actual, test.expected) c.logger.CompareLogging(test.logging) c.teardown() } diff --git a/pkg/haproxy/instance_test.go b/pkg/haproxy/instance_test.go index a43c74a53..dd6a6b8a5 100644 --- a/pkg/haproxy/instance_test.go +++ b/pkg/haproxy/instance_test.go @@ -1319,7 +1319,7 @@ func TestInstanceDefaultHost(t *testing.T) { h.AddPath(b, "/", hatypes.MatchBegin) h.TLS.TLSFilename = "/var/haproxy/ssl/certs/default.pem" h.TLS.TLSHash = "0" - b.SSLRedirect = b.CreateConfigBool(true) + b.FindBackendPath(h.FindPath("/").Link).SSLRedirect = true b.Endpoints = []*hatypes.Endpoint{endpointS1} h.VarNamespace = true @@ -1328,7 +1328,7 @@ func TestInstanceDefaultHost(t *testing.T) { h.AddPath(b, "/app", hatypes.MatchBegin) h.TLS.TLSFilename = "/var/haproxy/ssl/certs/default.pem" h.TLS.TLSHash = "0" - b.SSLRedirect = b.CreateConfigBool(true) + b.FindBackendPath(h.FindPath("/app").Link).SSLRedirect = true b.Endpoints = []*hatypes.Endpoint{endpointS1} h.VarNamespace = true @@ -1491,7 +1491,7 @@ func TestInstanceFrontend(t *testing.T) { b = c.config.Backends().AcquireBackend("d1", "app", "8080") h = c.config.Hosts().AcquireHost("d1.local") h.AddPath(b, "/", hatypes.MatchBegin) - b.SSLRedirect = b.CreateConfigBool(true) + b.FindBackendPath(h.FindPath("/").Link).SSLRedirect = true b.Endpoints = []*hatypes.Endpoint{endpointS1} h.VarNamespace = true h.TLS.TLSFilename = "/var/haproxy/ssl/certs/d1.pem" @@ -1500,7 +1500,7 @@ func TestInstanceFrontend(t *testing.T) { b = c.config.Backends().AcquireBackend("d2", "app", "8080") h = c.config.Hosts().AcquireHost("d2.local") h.AddPath(b, "/app", hatypes.MatchPrefix) - b.SSLRedirect = b.CreateConfigBool(true) + b.FindBackendPath(h.FindPath("/app").Link).SSLRedirect = true b.Endpoints = []*hatypes.Endpoint{endpointS1} h.TLS.TLSFilename = "/var/haproxy/ssl/certs/d2.pem" h.TLS.TLSHash = "2" @@ -1628,7 +1628,9 @@ func TestInstanceFrontendCA(t *testing.T) { h.TLS.TLSHash = "0" h.TLS.Options = "ssl-min-ver TLSv1.0 ssl-max-ver TLSv1.2" - b.SSLRedirect = b.CreateConfigBool(true) + for _, path := range b.Paths { + path.SSLRedirect = true + } b.TLS.AddCertHeader = true b.TLS.FingerprintLower = true b.Endpoints = []*hatypes.Endpoint{endpointS1} @@ -1763,12 +1765,12 @@ func TestInstanceSomePaths(t *testing.T) { h.TLS.TLSFilename = "/var/haproxy/ssl/certs/default.pem" h.TLS.TLSHash = "0" h.AddPath(b, "/", hatypes.MatchBegin) - b.SSLRedirect = b.CreateConfigBool(true) + b.FindBackendPath(h.FindPath("/").Link).SSLRedirect = true b.Endpoints = []*hatypes.Endpoint{endpointS1} b = c.config.Backends().AcquireBackend("d", "app1", "8080") h.AddPath(b, "/app", hatypes.MatchBegin) - b.SSLRedirect = b.CreateConfigBool(true) + b.FindBackendPath(h.FindPath("/app").Link).SSLRedirect = true b.Endpoints = []*hatypes.Endpoint{endpointS1} b = c.config.Backends().AcquireBackend("d", "app2", "8080") @@ -1894,7 +1896,7 @@ func TestInstanceSSLRedirect(t *testing.T) { h.TLS.TLSFilename = "/var/haproxy/ssl/certs/default.pem" h.TLS.TLSHash = "0" h.AddPath(b, "/", hatypes.MatchBegin) - b.SSLRedirect = b.CreateConfigBool(true) + b.FindBackendPath(h.FindPath("/").Link).SSLRedirect = true b = c.config.Backends().AcquireBackend("d2", "app-front", "8080") b.Endpoints = []*hatypes.Endpoint{endpointS21} @@ -1902,7 +1904,7 @@ func TestInstanceSSLRedirect(t *testing.T) { h.TLS.TLSFilename = "" h.TLS.TLSHash = "" h.AddPath(b, "/", hatypes.MatchBegin) - b.SSLRedirect = b.CreateConfigBool(true) + b.FindBackendPath(h.FindPath("/").Link).SSLRedirect = true c.config.Global().SSL.RedirectCode = 301 @@ -1950,7 +1952,7 @@ func TestInstanceSSLPassthrough(t *testing.T) { b = c.config.Backends().AcquireBackend("d2", "app", "8080") h = c.config.Hosts().AcquireHost("d2.local") h.AddPath(b, "/", hatypes.MatchBegin) - b.SSLRedirect = b.CreateConfigBool(true) + b.FindBackendPath(h.FindPath("/").Link).SSLRedirect = true b.Endpoints = []*hatypes.Endpoint{endpointS31} h.SetSSLPassthrough(true) @@ -2029,7 +2031,6 @@ func TestInstanceRootRedirect(t *testing.T) { h.TLS.TLSHash = "0" h.AddPath(b, "/", hatypes.MatchBegin) h.RootRedirect = "/app" - b.SSLRedirect = b.CreateConfigBool(false) b.Endpoints = []*hatypes.Endpoint{endpointS1} b = c.config.Backends().AcquireBackend("d2", "app", "8080") @@ -2039,7 +2040,9 @@ func TestInstanceRootRedirect(t *testing.T) { h.AddPath(b, "/app1", hatypes.MatchBegin) h.AddPath(b, "/app2", hatypes.MatchBegin) h.RootRedirect = "/app1" - b.SSLRedirect = b.CreateConfigBool(true) + for _, path := range b.Paths { + path.SSLRedirect = true + } b.Endpoints = []*hatypes.Endpoint{endpointS21} c.Update() @@ -2772,14 +2775,15 @@ func TestInstanceWildcardHostname(t *testing.T) { h.TLS.CAHash = "1" h.TLS.CAVerifyOptional = true h.TLS.CAErrorPage = "http://sub.d1.local/error.html" - b.SSLRedirect = b.CreateConfigBool(true) + for _, path := range b.Paths { + path.SSLRedirect = true + } b.Endpoints = []*hatypes.Endpoint{endpointS1} b = c.config.Backends().AcquireBackend("d2", "app", "8080") h = c.config.Hosts().AcquireHost("*.d2.local") h.AddPath(b, "/", hatypes.MatchBegin) h.RootRedirect = "/app" - b.SSLRedirect = b.CreateConfigBool(false) b.Endpoints = []*hatypes.Endpoint{endpointS21} c.Update() diff --git a/pkg/haproxy/types/backend.go b/pkg/haproxy/types/backend.go index 73b9b6432..6ead398c6 100644 --- a/pkg/haproxy/types/backend.go +++ b/pkg/haproxy/types/backend.go @@ -161,16 +161,6 @@ func sortPaths(paths []*BackendPath, pathReverse bool) { }) } -// CreateConfigBool ... -func (b *Backend) CreateConfigBool(value bool) []*BackendConfigBool { - return []*BackendConfigBool{ - { - Paths: NewBackendPaths(b.Paths...), - Config: value, - }, - } -} - // HasCorsEnabled ... func (b *Backend) HasCorsEnabled() bool { for _, path := range b.Paths { @@ -203,8 +193,8 @@ func (b *Backend) HasModsec() bool { // HasSSLRedirect ... func (b *Backend) HasSSLRedirect() bool { - for _, sslredirect := range b.SSLRedirect { - if sslredirect.Config { + for _, path := range b.Paths { + if path.SSLRedirect { return true } } @@ -213,11 +203,9 @@ func (b *Backend) HasSSLRedirect() bool { // LinkHasSSLRedirect ... func (b *Backend) LinkHasSSLRedirect(link PathLink) bool { - for _, sslredirect := range b.SSLRedirect { - for _, p := range sslredirect.Paths.Items { - if p.Link == link { - return sslredirect.Config - } + for _, path := range b.Paths { + if path.Link == link { + return path.SSLRedirect } } return false @@ -378,8 +366,3 @@ func (p *BackendPath) String() string { func (h *BackendHeader) String() string { return fmt.Sprintf("%+v", *h) } - -// String ... -func (b *BackendConfigBool) String() string { - return fmt.Sprintf("%+v", *b) -} diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index a5a5517f0..9e47fed63 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -488,7 +488,6 @@ type Backend struct { // Template uses this func in order to know if a config // has two or more paths, and so need to be configured with ACL. // - SSLRedirect []*BackendConfigBool } // Endpoint ... @@ -551,12 +550,6 @@ type BackendHeader struct { Value string } -// BackendConfigBool ... -type BackendConfigBool struct { - Paths BackendPaths - Config bool -} - // AgentCheck ... type AgentCheck struct { Addr string From cb625584ec95fdebba5ac9202e011f849d7592a9 Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Sat, 12 Sep 2020 15:17:05 -0300 Subject: [PATCH 11/13] cleanup of unused code remove legacy code used to deduplicate and redistribute per-path config in the annotation parser. --- .../ingress/annotations/backend_test.go | 13 -- pkg/converters/ingress/annotations/mapper.go | 126 ----------- .../ingress/annotations/mapper_test.go | 206 ------------------ pkg/haproxy/instance_test.go | 11 - pkg/haproxy/types/backend.go | 27 --- pkg/haproxy/types/types.go | 27 --- 6 files changed, 410 deletions(-) diff --git a/pkg/converters/ingress/annotations/backend_test.go b/pkg/converters/ingress/annotations/backend_test.go index d093c1b2e..75c97196e 100644 --- a/pkg/converters/ingress/annotations/backend_test.go +++ b/pkg/converters/ingress/annotations/backend_test.go @@ -1904,16 +1904,3 @@ WARN skipping invalid IP or cidr on ingress 'default/ing1': 192.168.0/16`, c.teardown() } } - -func createBackendPaths(paths ...string) hatypes.BackendPaths { - backendPaths := make([]*hatypes.BackendPath, 0, len(paths)) - for _, path := range paths { - backendPaths = append(backendPaths, &hatypes.BackendPath{ - // ignoring ID which isn't the focus of the test - // removing on createBackendMappingData() as well - ID: "", - Link: hatypes.CreatePathLink(testingHostname, path), - }) - } - return hatypes.NewBackendPaths(backendPaths...) -} diff --git a/pkg/converters/ingress/annotations/mapper.go b/pkg/converters/ingress/annotations/mapper.go index bed7849c4..53ae6d871 100644 --- a/pkg/converters/ingress/annotations/mapper.go +++ b/pkg/converters/ingress/annotations/mapper.go @@ -18,7 +18,6 @@ package annotations import ( "fmt" - "sort" "strconv" hatypes "github.com/jcmoraisjr/haproxy-ingress/pkg/haproxy/types" @@ -59,12 +58,6 @@ type Source struct { Type string } -// BackendConfig ... -type BackendConfig struct { - Paths hatypes.BackendPaths - Config map[string]*ConfigValue -} - // ConfigValue ... type ConfigValue struct { Source *Source @@ -196,97 +189,6 @@ func (c *Mapper) Get(key string) *ConfigValue { return value } -// ConfigOverwrite ... -type ConfigOverwrite func(path *hatypes.BackendPath, values map[string]*ConfigValue) map[string]*ConfigValue - -// GetBackendConfig builds a generic BackendConfig using -// annotation maps registered previously as its data source -// -// An annotation map is a `map[]` collected on -// ingress/service parsing phase. A HAProxy backend need a group -// of annotation keys - ie a group of maps - grouped by URI in -// order to create and apply ACLs. -// -// The rule of thumb on the final BackendConfig array is: -// -// 1. Every backend path must be declared, so a HAProxy method can -// just `if len(BackendConfig) > 1 then need-acl`; -// 2. Added annotation means declared annotation (ingress, service -// or default) so the config reader `GetFromMap()`` can -// distinguish between `undeclared` and `declared empty`. -// -func (c *Mapper) GetBackendConfig(backend *hatypes.Backend, keys []string, overwrite ConfigOverwrite) []*BackendConfig { - // all backend paths need to be declared, filling up previously with default values - rawConfig := make(map[hatypes.PathLink]map[string]*ConfigValue, len(backend.Paths)) - for _, path := range backend.Paths { - kv := make(map[string]*ConfigValue, len(keys)) - for _, key := range keys { - if value, found := c.annDefaults[key]; found { - kv[key] = &ConfigValue{ - Value: value, - } - } - } - rawConfig[path.Link] = kv - } - // populate rawConfig with declared annotations, grouping annotation maps by URI - for _, key := range keys { - if maps, found := c.GetStrMap(key); found { - for _, m := range maps { - // skip default value - if !m.Link.IsEmpty() { - if cfg, found := rawConfig[m.Link]; found { - cfg[key] = &ConfigValue{ - Source: m.Source, - Value: m.Value, - } - } else { - panic(fmt.Sprintf("backend '%s/%s' is missing hostname/path '%+v'", backend.Namespace, backend.Name, m.Link)) - } - } - } - } - } - // iterate the URIs and create the BackendConfig array - // most configs should have just one item with default kv - config := make([]*BackendConfig, 0, 1) - for link, kv := range rawConfig { - path := backend.FindBackendPath(link) - realKV := kv - if overwrite != nil { - realKV = overwrite(path, kv) - if realKV == nil { - realKV = map[string]*ConfigValue{} - } - } - if cfg := findConfig(config, realKV); cfg != nil { - cfg.Paths.Add(path) - } else { - config = append(config, &BackendConfig{ - Paths: hatypes.NewBackendPaths(path), - Config: realKV, - }) - } - } - // rawConfig is a map which by definition does not have explicit order. - // sort in order to the same input generates the same output - sort.SliceStable(config, func(i, j int) bool { - l1 := config[i].Paths.Items[0].Link - l2 := config[j].Paths.Items[0].Link - return l1.Less(l2, false) - }) - return config -} - -func findConfig(config []*BackendConfig, kv map[string]*ConfigValue) *BackendConfig { - for _, cfg := range config { - if cfg.ConfigEquals(kv) { - return cfg - } - } - return nil -} - // Get ... func (c *AnnConfig) Get(key string) *ConfigValue { if value, found := c.keys[key]; found { @@ -298,34 +200,6 @@ func (c *AnnConfig) Get(key string) *ConfigValue { return &ConfigValue{} } -// ConfigEquals ... -func (b *BackendConfig) ConfigEquals(other map[string]*ConfigValue) bool { - if len(b.Config) != len(other) { - return false - } - for key, value := range b.Config { - if otherValue, found := other[key]; !found { - return false - } else if value.Value != otherValue.Value { - return false - } - } - return true -} - -// Get ... -func (b *BackendConfig) Get(key string) *ConfigValue { - if configValue, found := b.Config[key]; found && configValue != nil { - return configValue - } - return &ConfigValue{} -} - -// String ... -func (b *BackendConfig) String() string { - return fmt.Sprintf("%+v", *b) -} - // String ... func (cv *ConfigValue) String() string { return cv.Value diff --git a/pkg/converters/ingress/annotations/mapper_test.go b/pkg/converters/ingress/annotations/mapper_test.go index 767e72204..6d1549761 100644 --- a/pkg/converters/ingress/annotations/mapper_test.go +++ b/pkg/converters/ingress/annotations/mapper_test.go @@ -301,209 +301,3 @@ func TestGetDefault(t *testing.T) { c.teardown() } } - -func TestGetBackendConfig(t *testing.T) { - testCases := []struct { - paths []string - source Source - annDefault map[string]string - keyValues map[string]map[string]string - getKeys []string - overwrite ConfigOverwrite - expected []*BackendConfig - logging string - }{ - // 0 - { - keyValues: map[string]map[string]string{ - "/": { - "ann-1": "10", - }, - }, - getKeys: []string{"ann-1"}, - expected: []*BackendConfig{ - { - Paths: createBackendPaths("/"), - Config: map[string]*ConfigValue{ - "ann-1": {Value: "10"}, - }, - }, - }, - }, - // 1 - { - keyValues: map[string]map[string]string{ - "/": { - "ann-1": "10", - "ann-2": "10", - }, - }, - getKeys: []string{"ann-1", "ann-2"}, - expected: []*BackendConfig{ - { - Paths: createBackendPaths("/"), - Config: map[string]*ConfigValue{ - "ann-1": {Value: "10"}, - "ann-2": {Value: "10"}, - }, - }, - }, - }, - // 2 - { - annDefault: map[string]string{ - "ann-1": "0", - }, - getKeys: []string{"ann-1", "ann-2"}, - keyValues: map[string]map[string]string{ - "/": { - "ann-1": "10", - "ann-2": "20", - }, - "/url": { - "ann-1": "10", - "ann-2": "20", - }, - "/path": { - "ann-2": "20", - }, - }, - expected: []*BackendConfig{ - { - Paths: createBackendPaths("/", "/url"), - Config: map[string]*ConfigValue{ - "ann-1": {Value: "10"}, - "ann-2": {Value: "20"}, - }, - }, - { - Paths: createBackendPaths("/path"), - Config: map[string]*ConfigValue{ - "ann-1": {Value: "0"}, - "ann-2": {Value: "20"}, - }, - }, - }, - }, - // 3 - { - annDefault: map[string]string{ - "ann-1": "5", - }, - keyValues: map[string]map[string]string{ - "/url": { - "ann-1": "10", - }, - }, - getKeys: []string{"ann-1"}, - expected: []*BackendConfig{ - { - Paths: createBackendPaths("/url"), - Config: map[string]*ConfigValue{ - "ann-1": {Value: "10"}, - }, - }, - }, - }, - // 4 - { - annDefault: map[string]string{ - "ann-1": "5", - "ann-2": "0", - "ann-3": "0", - }, - keyValues: map[string]map[string]string{ - "/": { - "ann-1": "10", - }, - "/url": { - "ann-2": "20", - }, - }, - getKeys: []string{"ann-1", "ann-2", "ann-3"}, - expected: []*BackendConfig{ - { - Paths: createBackendPaths("/"), - Config: map[string]*ConfigValue{ - "ann-1": {Value: "10"}, - "ann-2": {Value: "0"}, - "ann-3": {Value: "0"}, - }, - }, - { - Paths: createBackendPaths("/url"), - Config: map[string]*ConfigValue{ - "ann-1": {Value: "5"}, - "ann-2": {Value: "20"}, - "ann-3": {Value: "0"}, - }, - }, - }, - }, - // 5 - { - annDefault: map[string]string{ - "ann-1": "0", - }, - keyValues: map[string]map[string]string{ - "/": { - "ann-1": "err", - }, - "/url": { - "ann-1": "0", - }, - }, - getKeys: []string{"ann-1"}, - expected: []*BackendConfig{ - { - Paths: createBackendPaths("/", "/url"), - Config: map[string]*ConfigValue{ - "ann-1": {Value: "0"}, - }, - }, - }, - source: Source{Namespace: "default", Name: "ing1", Type: "service"}, - logging: `WARN ignoring invalid int expression on service 'default/ing1' key 'ann-1': err`, - }, - // 6 - { - keyValues: map[string]map[string]string{ - "/": { - "ann-1": "10", - }, - "/sub": { - "ann-1": "20", - }, - }, - getKeys: []string{"ann-1"}, - expected: []*BackendConfig{ - { - Paths: createBackendPaths("/", "/sub"), - Config: map[string]*ConfigValue{}, - }, - }, - overwrite: func(path *hatypes.BackendPath, values map[string]*ConfigValue) map[string]*ConfigValue { - return nil - }, - }, - } - validators["ann-1"] = validateInt - defer delete(validators, "ann-1") - for i, test := range testCases { - c := setup(t) - d := c.createBackendMappingData("default/app", &test.source, test.annDefault, test.keyValues, test.paths) - config := d.mapper.GetBackendConfig(d.backend, test.getKeys, test.overwrite) - for _, cfg := range config { - for _, value := range cfg.Config { - // Source is inconsistent and irrelevant here - value.Source = nil - } - for _, value := range cfg.Config { - value.Source = nil - } - } - c.compareObjects("backend config", i, config, test.expected) - c.logger.CompareLogging(test.logging) - c.teardown() - } -} diff --git a/pkg/haproxy/instance_test.go b/pkg/haproxy/instance_test.go index dd6a6b8a5..3f21fd8b5 100644 --- a/pkg/haproxy/instance_test.go +++ b/pkg/haproxy/instance_test.go @@ -3284,14 +3284,3 @@ func (c *testConfig) compareText(name, actual, expected string) { c.t.Error("\ndiff of " + name + ":" + diff.Diff(txtExpected, txtActual)) } } - -func createBackendPaths(backend *hatypes.Backend, uris ...string) hatypes.BackendPaths { - paths := make([]*hatypes.BackendPath, len(uris)) - for i, uri := range uris { - j := strings.IndexAny(uri, "^/") - hostname := uri[:j] - path := uri[j:] - paths[i] = backend.FindBackendPath(hatypes.CreatePathLink(hostname, path)) - } - return hatypes.NewBackendPaths(paths...) -} diff --git a/pkg/haproxy/types/backend.go b/pkg/haproxy/types/backend.go index 6ead398c6..f72579aa5 100644 --- a/pkg/haproxy/types/backend.go +++ b/pkg/haproxy/types/backend.go @@ -23,13 +23,6 @@ import ( "strings" ) -// NewBackendPaths ... -func NewBackendPaths(paths ...*BackendPath) BackendPaths { - b := BackendPaths{} - b.Add(paths...) - return b -} - // BackendID ... func (b *Backend) BackendID() BackendID { // IMPLEMENT as pointer @@ -317,26 +310,6 @@ func (ep *Endpoint) IsEmpty() bool { return ep.IP == "127.0.0.1" } -// IDList ... -func (p *BackendPaths) IDList() string { - ids := make([]string, len(p.Items)) - for i, item := range p.Items { - ids[i] = item.ID - } - return strings.Join(ids, " ") -} - -// Add ... -func (p *BackendPaths) Add(paths ...*BackendPath) { - for _, path := range paths { - if path == nil { - panic("path cannot be nil") - } - p.Items = append(p.Items, path) - } - sortPaths(p.Items, false) -} - // Hostname ... func (p *BackendPath) Hostname() string { return p.Link.hostname diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index 9e47fed63..33b4e20f6 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -466,28 +466,6 @@ type Backend struct { Timeout BackendTimeoutConfig TLS BackendTLSConfig WhitelistTCP []string - // - // per path config - // - // TODO refactor - // - // The current implementation is tricky. A small refactor is welcome - // but can wait a little more. Multipath unit tests need to do a - // better job as well. - // - // Following some tips in order to multipath work properly: - // - // 1. On backend annotation parsing, do not filter - // mapper.GetBackendConfig/Str() slice, instead populate - // haproxy type even with empty data. Backend.NeedACL() need - // to know all paths in order to work properly. Filter out - // empty/disabled items in the template. - // - // 2. Every config array added here, need also to be added - // in Backend.NeedACL() - haproxy/types/backend.go. - // Template uses this func in order to know if a config - // has two or more paths, and so need to be configured with ACL. - // } // Endpoint ... @@ -508,11 +486,6 @@ type BlueGreenConfig struct { HeaderName string } -// BackendPaths ... -type BackendPaths struct { - Items []*BackendPath -} - // BackendPathConfig ... type BackendPathConfig struct { items []*BackendPathItem From 0341265b54992a1f034ab0980c7aa444c8125886 Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Sat, 12 Sep 2020 17:42:42 -0300 Subject: [PATCH 12/13] rename of mapper internals Change the name of the hash maps and support types to something one can understand its purpose. Annotation mapper has now two maps and the old naming was a bit confusing. --- pkg/converters/ingress/annotations/mapper.go | 89 ++++++++++--------- .../ingress/annotations/mapper_test.go | 28 +++--- 2 files changed, 59 insertions(+), 58 deletions(-) diff --git a/pkg/converters/ingress/annotations/mapper.go b/pkg/converters/ingress/annotations/mapper.go index 53ae6d871..e2e375505 100644 --- a/pkg/converters/ingress/annotations/mapper.go +++ b/pkg/converters/ingress/annotations/mapper.go @@ -34,20 +34,20 @@ type MapBuilder struct { // Mapper ... type Mapper struct { MapBuilder - maps map[string][]*Map - configs map[hatypes.PathLink]*AnnConfig + configByKey map[string][]*PathConfig + configByPath map[hatypes.PathLink]*KeyConfig } -// AnnConfig ... -type AnnConfig struct { +// KeyConfig ... +type KeyConfig struct { mapper *Mapper keys map[string]*ConfigValue } -// Map ... -type Map struct { +// PathConfig ... +type PathConfig struct { Source *Source - Link hatypes.PathLink + path hatypes.PathLink Value string } @@ -78,13 +78,13 @@ func (b *MapBuilder) NewMapper() *Mapper { return &Mapper{ MapBuilder: *b, // - maps: map[string][]*Map{}, - configs: map[hatypes.PathLink]*AnnConfig{}, + configByKey: map[string][]*PathConfig{}, + configByPath: map[hatypes.PathLink]*KeyConfig{}, } } -func newAnnConfig(mapper *Mapper) *AnnConfig { - return &AnnConfig{ +func newKeyConfig(mapper *Mapper) *KeyConfig { + return &KeyConfig{ mapper: mapper, keys: map[string]*ConfigValue{}, } @@ -92,19 +92,20 @@ func newAnnConfig(mapper *Mapper) *AnnConfig { // Add a new annotation to the current mapper. // Return the conflict state: true if a conflict was found, false if the annotation was assigned or at least handled -func (c *Mapper) addAnnotation(source *Source, link hatypes.PathLink, key, value string) bool { - if link.IsEmpty() { +func (c *Mapper) addAnnotation(source *Source, path hatypes.PathLink, key, value string) bool { + if path.IsEmpty() { // empty means default value, cannot register as an annotation panic("path link cannot be empty") } // check overlap - config, configfound := c.configs[link] + config, configfound := c.configByPath[path] if !configfound { - config = newAnnConfig(c) - c.configs[link] = config + config = newKeyConfig(c) + c.configByPath[path] = config } - if cfg, found := config.keys[key]; found { - return cfg.Value != value + if cv, found := config.keys[key]; found { + // there is a conflict only if values differ + return cv.Value != value } // validate (bool; int; ...) and normalize (int "01" => "1"; ...) realValue := value @@ -119,21 +120,21 @@ func (c *Mapper) addAnnotation(source *Source, link hatypes.PathLink, key, value Source: source, Value: realValue, } - annMaps, _ := c.maps[key] - annMaps = append(annMaps, &Map{ + pathConfigs, _ := c.configByKey[key] + pathConfigs = append(pathConfigs, &PathConfig{ Source: source, - Link: link, + path: path, Value: realValue, }) - c.maps[key] = annMaps + c.configByKey[key] = pathConfigs return false } // AddAnnotations ... -func (c *Mapper) AddAnnotations(source *Source, link hatypes.PathLink, ann map[string]string) (conflicts []string) { +func (c *Mapper) AddAnnotations(source *Source, path hatypes.PathLink, ann map[string]string) (conflicts []string) { conflicts = make([]string, 0, len(ann)) for key, value := range ann { - if conflict := c.addAnnotation(source, link, key, value); conflict { + if conflict := c.addAnnotation(source, path, key, value); conflict { conflicts = append(conflicts, key) } } @@ -141,43 +142,43 @@ func (c *Mapper) AddAnnotations(source *Source, link hatypes.PathLink, ann map[s } // GetStrMap ... -func (c *Mapper) GetStrMap(key string) ([]*Map, bool) { - annMaps, found := c.maps[key] - if found && len(annMaps) > 0 { - return annMaps, true +func (c *Mapper) GetStrMap(key string) ([]*PathConfig, bool) { + configs, found := c.configByKey[key] + if found && len(configs) > 0 { + return configs, true } value, found := c.annDefaults[key] if found { - return []*Map{{Value: value}}, true + return []*PathConfig{{Value: value}}, true } return nil, false } // GetConfig ... -func (c *Mapper) GetConfig(link hatypes.PathLink) *AnnConfig { - if config, found := c.configs[link]; found { +func (c *Mapper) GetConfig(path hatypes.PathLink) *KeyConfig { + if config, found := c.configByPath[path]; found { return config } - config := newAnnConfig(c) - c.configs[link] = config + config := newKeyConfig(c) + c.configByPath[path] = config return config } // Get ... func (c *Mapper) Get(key string) *ConfigValue { - annMaps, found := c.GetStrMap(key) + configs, found := c.GetStrMap(key) if !found { return &ConfigValue{} } value := &ConfigValue{ - Source: annMaps[0].Source, - Value: annMaps[0].Value, - } - if len(annMaps) > 1 { - sources := make([]*Source, 0, len(annMaps)) - for _, annMap := range annMaps { - if value.Value != annMap.Value { - sources = append(sources, annMap.Source) + Source: configs[0].Source, + Value: configs[0].Value, + } + if len(configs) > 1 { + sources := make([]*Source, 0, len(configs)) + for _, config := range configs { + if value.Value != config.Value { + sources = append(sources, config.Source) } } if len(sources) > 0 { @@ -190,7 +191,7 @@ func (c *Mapper) Get(key string) *ConfigValue { } // Get ... -func (c *AnnConfig) Get(key string) *ConfigValue { +func (c *KeyConfig) Get(key string) *ConfigValue { if value, found := c.keys[key]; found { return value } @@ -229,7 +230,7 @@ func (s *Source) FullName() string { } // String ... -func (m *Map) String() string { +func (m *PathConfig) String() string { return fmt.Sprintf("%+v", *m) } diff --git a/pkg/converters/ingress/annotations/mapper_test.go b/pkg/converters/ingress/annotations/mapper_test.go index 6d1549761..daecd195e 100644 --- a/pkg/converters/ingress/annotations/mapper_test.go +++ b/pkg/converters/ingress/annotations/mapper_test.go @@ -25,7 +25,7 @@ import ( type ann struct { src *Source - link hatypes.PathLink + path hatypes.PathLink key string val string expConflict bool @@ -133,7 +133,7 @@ func TestAddAnnotation(t *testing.T) { c := setup(t) mapper := NewMapBuilder(c.logger, test.annPrefix, map[string]string{}).NewMapper() for j, ann := range test.ann { - if conflict := mapper.addAnnotation(ann.src, ann.link, ann.key, ann.val); conflict != ann.expConflict { + if conflict := mapper.addAnnotation(ann.src, ann.path, ann.key, ann.val); conflict != ann.expConflict { t.Errorf("expect conflict '%t' on '// %d (%d)', but was '%t'", ann.expConflict, i, j, conflict) } } @@ -161,7 +161,7 @@ func TestGetAnnotation(t *testing.T) { annPrefix string getKey string expMiss bool - expAnnMap []*Map + expConfig []*PathConfig }{ // 0 { @@ -170,9 +170,9 @@ func TestGetAnnotation(t *testing.T) { {srcing2, pathURL, "auth-basic", "default/basic2", false}, }, getKey: "auth-basic", - expAnnMap: []*Map{ - {Source: srcing1, Link: pathRoot, Value: "default/basic1"}, - {Source: srcing2, Link: pathURL, Value: "default/basic2"}, + expConfig: []*PathConfig{ + {Source: srcing1, path: pathRoot, Value: "default/basic1"}, + {Source: srcing2, path: pathURL, Value: "default/basic2"}, }, }, // 1 @@ -183,8 +183,8 @@ func TestGetAnnotation(t *testing.T) { {srcing2, pathRoot, "auth-basic", "default/basic2", true}, }, getKey: "auth-basic", - expAnnMap: []*Map{ - {Source: srcing1, Link: pathRoot, Value: "default/basic1"}, + expConfig: []*PathConfig{ + {Source: srcing1, path: pathRoot, Value: "default/basic1"}, }, }, // 2 @@ -195,8 +195,8 @@ func TestGetAnnotation(t *testing.T) { {srcing2, pathRoot, "auth-basic", "default/basic2", true}, }, getKey: "auth-type", - expAnnMap: []*Map{ - {Source: srcing1, Link: pathRoot, Value: "basic"}, + expConfig: []*PathConfig{ + {Source: srcing1, path: pathRoot, Value: "basic"}, }, }, } @@ -204,17 +204,17 @@ func TestGetAnnotation(t *testing.T) { c := setup(t) mapper := NewMapBuilder(c.logger, test.annPrefix, map[string]string{}).NewMapper() for j, ann := range test.ann { - if conflict := mapper.addAnnotation(ann.src, ann.link, ann.key, ann.val); conflict != ann.expConflict { + if conflict := mapper.addAnnotation(ann.src, ann.path, ann.key, ann.val); conflict != ann.expConflict { t.Errorf("expect conflict '%t' on '// %d (%d)', but was '%t'", ann.expConflict, i, j, conflict) } } - annMap, found := mapper.GetStrMap(test.getKey) + pathConfig, found := mapper.GetStrMap(test.getKey) if !found { if !test.expMiss { t.Errorf("expect to find '%s' key on '%d', but was not found", test.getKey, i) } - } else if !reflect.DeepEqual(annMap, test.expAnnMap) { - t.Errorf("expected and actual differ on '%d' - expected: %+v - actual: %+v", i, test.expAnnMap, annMap) + } else if !reflect.DeepEqual(pathConfig, test.expConfig) { + t.Errorf("expected and actual differ on '%d' - expected: %+v - actual: %+v", i, test.expConfig, pathConfig) } c.teardown() } From ad2c8323f75ce72a0fdc281c59c10820071ba416 Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Sat, 12 Sep 2020 17:46:02 -0300 Subject: [PATCH 13/13] reuse ConfigValue struct A small refactor to reuse the final `ConfigValue` data in both annotation mapper maps. --- pkg/converters/ingress/annotations/mapper.go | 29 ++++++++----------- .../ingress/annotations/mapper_test.go | 10 +++---- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/pkg/converters/ingress/annotations/mapper.go b/pkg/converters/ingress/annotations/mapper.go index e2e375505..d04774b12 100644 --- a/pkg/converters/ingress/annotations/mapper.go +++ b/pkg/converters/ingress/annotations/mapper.go @@ -46,9 +46,8 @@ type KeyConfig struct { // PathConfig ... type PathConfig struct { - Source *Source - path hatypes.PathLink - Value string + path hatypes.PathLink + value *ConfigValue } // Source ... @@ -116,15 +115,15 @@ func (c *Mapper) addAnnotation(source *Source, path hatypes.PathLink, key, value } } // update internal fields - config.keys[key] = &ConfigValue{ + configValue := &ConfigValue{ Source: source, Value: realValue, } + config.keys[key] = configValue pathConfigs, _ := c.configByKey[key] pathConfigs = append(pathConfigs, &PathConfig{ - Source: source, - path: path, - Value: realValue, + path: path, + value: configValue, }) c.configByKey[key] = pathConfigs return false @@ -141,15 +140,14 @@ func (c *Mapper) AddAnnotations(source *Source, path hatypes.PathLink, ann map[s return conflicts } -// GetStrMap ... -func (c *Mapper) GetStrMap(key string) ([]*PathConfig, bool) { +func (c *Mapper) findPathConfig(key string) ([]*PathConfig, bool) { configs, found := c.configByKey[key] if found && len(configs) > 0 { return configs, true } value, found := c.annDefaults[key] if found { - return []*PathConfig{{Value: value}}, true + return []*PathConfig{{value: &ConfigValue{Value: value}}}, true } return nil, false } @@ -166,19 +164,16 @@ func (c *Mapper) GetConfig(path hatypes.PathLink) *KeyConfig { // Get ... func (c *Mapper) Get(key string) *ConfigValue { - configs, found := c.GetStrMap(key) + configs, found := c.findPathConfig(key) if !found { return &ConfigValue{} } - value := &ConfigValue{ - Source: configs[0].Source, - Value: configs[0].Value, - } + value := configs[0].value if len(configs) > 1 { sources := make([]*Source, 0, len(configs)) for _, config := range configs { - if value.Value != config.Value { - sources = append(sources, config.Source) + if value.Value != config.value.Value { + sources = append(sources, config.value.Source) } } if len(sources) > 0 { diff --git a/pkg/converters/ingress/annotations/mapper_test.go b/pkg/converters/ingress/annotations/mapper_test.go index daecd195e..c4aec920f 100644 --- a/pkg/converters/ingress/annotations/mapper_test.go +++ b/pkg/converters/ingress/annotations/mapper_test.go @@ -171,8 +171,8 @@ func TestGetAnnotation(t *testing.T) { }, getKey: "auth-basic", expConfig: []*PathConfig{ - {Source: srcing1, path: pathRoot, Value: "default/basic1"}, - {Source: srcing2, path: pathURL, Value: "default/basic2"}, + {path: pathRoot, value: &ConfigValue{Source: srcing1, Value: "default/basic1"}}, + {path: pathURL, value: &ConfigValue{Source: srcing2, Value: "default/basic2"}}, }, }, // 1 @@ -184,7 +184,7 @@ func TestGetAnnotation(t *testing.T) { }, getKey: "auth-basic", expConfig: []*PathConfig{ - {Source: srcing1, path: pathRoot, Value: "default/basic1"}, + {path: pathRoot, value: &ConfigValue{Source: srcing1, Value: "default/basic1"}}, }, }, // 2 @@ -196,7 +196,7 @@ func TestGetAnnotation(t *testing.T) { }, getKey: "auth-type", expConfig: []*PathConfig{ - {Source: srcing1, path: pathRoot, Value: "basic"}, + {path: pathRoot, value: &ConfigValue{Source: srcing1, Value: "basic"}}, }, }, } @@ -208,7 +208,7 @@ func TestGetAnnotation(t *testing.T) { t.Errorf("expect conflict '%t' on '// %d (%d)', but was '%t'", ann.expConflict, i, j, conflict) } } - pathConfig, found := mapper.GetStrMap(test.getKey) + pathConfig, found := mapper.findPathConfig(test.getKey) if !found { if !test.expMiss { t.Errorf("expect to find '%s' key on '%d', but was not found", test.getKey, i)