diff --git a/pkg/brownfield/brownfield_suite_test.go b/pkg/brownfield/brownfield_suite_test.go index f1a5bf274..7aaa986fe 100644 --- a/pkg/brownfield/brownfield_suite_test.go +++ b/pkg/brownfield/brownfield_suite_test.go @@ -8,13 +8,19 @@ package brownfield import ( + "flag" "testing" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "k8s.io/klog/v2" ) func TestApplicationGatewayKubernetesIngress(t *testing.T) { + klog.InitFlags(nil) + _ = flag.Set("v", "5") + _ = flag.Lookup("logtostderr").Value.Set("true") + RegisterFailHandler(Fail) RunSpecs(t, "ApplicationGatewayKubernetesIngress Suite") } diff --git a/pkg/brownfield/pathmaps.go b/pkg/brownfield/pathmaps.go index b67a1de78..05a0633e9 100644 --- a/pkg/brownfield/pathmaps.go +++ b/pkg/brownfield/pathmaps.go @@ -109,12 +109,17 @@ func mergePathMapsWithBasicRule(pathMap *n.ApplicationGatewayURLPathMap, rule *n func mergePathRules(pathRulesBucket ...*[]n.ApplicationGatewayPathRule) *[]n.ApplicationGatewayPathRule { uniq := make(pathRulesByName) for _, bucket := range pathRulesBucket { + if bucket == nil { + continue + } + for _, pathRule := range *bucket { uniq[pathRuleName(*pathRule.Name)] = pathRule } } var merged []n.ApplicationGatewayPathRule for _, pathRule := range uniq { + klog.V(5).Infof("[brownfield] Appending %s with paths %v", *pathRule.Name, pathRule.Paths) merged = append(merged, pathRule) } return &merged diff --git a/pkg/brownfield/routing_rules.go b/pkg/brownfield/routing_rules.go index 188d843d5..6aae09406 100644 --- a/pkg/brownfield/routing_rules.go +++ b/pkg/brownfield/routing_rules.go @@ -116,6 +116,12 @@ func mergeRoutingRules(appGw *n.ApplicationGateway, firstRoutingRule *n.Applicat return firstRoutingRule } + if *firstRoutingRule.URLPathMap.ID == *secondRoutingRule.URLPathMap.ID { + // No merge needed as both routing rule point to the same URL Path map + klog.V(5).Infof("[brownfield] Rule %s and rule %s share the same path map %s; Skipping merge", *firstRoutingRule.Name, *secondRoutingRule.Name, *firstPathMap.Name) + return firstRoutingRule + } + // Get the url path map for the second rule secondPathMap := lookupPathMap(appGw.URLPathMaps, secondRoutingRule.URLPathMap.ID) diff --git a/pkg/brownfield/routing_rules_test.go b/pkg/brownfield/routing_rules_test.go index 2475946c7..3396e2db4 100644 --- a/pkg/brownfield/routing_rules_test.go +++ b/pkg/brownfield/routing_rules_test.go @@ -12,6 +12,7 @@ import ( ptv1 "github.com/Azure/application-gateway-kubernetes-ingress/pkg/apis/azureingressprohibitedtarget/v1" "github.com/Azure/application-gateway-kubernetes-ingress/pkg/tests" "github.com/Azure/application-gateway-kubernetes-ingress/pkg/tests/fixtures" + n "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-03-01/network" ) var _ = Describe("Test blacklist request routing rules", func() { @@ -101,4 +102,107 @@ var _ = Describe("Test blacklist request routing rules", func() { Expect(blacklisted).To(ContainElement(rulePathBased3)) }) }) + + Context("Test MergeRules", func() { + It("should merge correctly when same set of routing rules need to be merged", func() { + Expect(len(*appGw.RequestRoutingRules)).To(Equal(5)) + Expect(len(*appGw.URLPathMaps)).To(Equal(4)) + + requestRoutingRules := MergeRules(&appGw, *appGw.RequestRoutingRules, *appGw.RequestRoutingRules) + + // No change + Expect(len(requestRoutingRules)).To(Equal(5)) + Expect(len(*appGw.URLPathMaps)).To(Equal(4)) + + pathBasedRuleCount := 0 + basicRuleCount := 0 + for _, rule := range *appGw.RequestRoutingRules { + if rule.RuleType == n.ApplicationGatewayRequestRoutingRuleTypePathBasedRouting { + pathBasedRuleCount++ + Expect(lookupPathMap(appGw.URLPathMaps, rule.URLPathMap.ID)).ToNot(BeNil()) + } + + if rule.RuleType == n.ApplicationGatewayRequestRoutingRuleTypeBasic { + basicRuleCount++ + } + } + + Expect(pathBasedRuleCount).To(Equal(3)) + Expect(basicRuleCount).To(Equal(2)) + }) + + It("should merge correctly when different set of routing rules need to be merged", func() { + requestRoutingRules := MergeRules( + &appGw, + []n.ApplicationGatewayRequestRoutingRule{ + ruleBasic, + ruleDefault, + rulePathBased1, + }, + []n.ApplicationGatewayRequestRoutingRule{ + rulePathBased2, + rulePathBased3, + }) + + Expect(len(requestRoutingRules)).To(Equal(5)) + Expect(len(*appGw.URLPathMaps)).To(Equal(4)) + + pathBasedRuleCount := 0 + basicRuleCount := 0 + for _, rule := range *appGw.RequestRoutingRules { + if rule.RuleType == n.ApplicationGatewayRequestRoutingRuleTypePathBasedRouting { + pathBasedRuleCount++ + Expect(lookupPathMap(appGw.URLPathMaps, rule.URLPathMap.ID)).ToNot(BeNil()) + } + + if rule.RuleType == n.ApplicationGatewayRequestRoutingRuleTypeBasic { + basicRuleCount++ + } + } + + Expect(pathBasedRuleCount).To(Equal(3)) + Expect(basicRuleCount).To(Equal(2)) + Expect(len(*appGw.URLPathMaps)).To(Equal(4)) + }) + + It("should merge correctly when 2 routing rule use the same http listener but different url paths", func() { + // When routing rule uses same listener but different url path maps, they need to be merged together + // as AppGw doesn't allow 2 rules using same listener + + // Setup 2 path maps + pathMap1 := (*appGw.URLPathMaps)[1] + pathMap2 := (*appGw.URLPathMaps)[2] + urlPathMap := &[]n.ApplicationGatewayURLPathMap{ + pathMap1, + pathMap2, + } + appGw.URLPathMaps = urlPathMap + Expect(len(*pathMap1.PathRules)).To(Equal(2)) + Expect(len(*pathMap2.PathRules)).To(Equal(1)) + + // Setup first rule to use first path map + rulePathBased1.URLPathMap.ID = pathMap1.ID + + // Setup second rule to use the same listener but second url path map + rulePathBased2.HTTPListener.ID = rulePathBased1.HTTPListener.ID + rulePathBased2.URLPathMap.ID = pathMap2.ID + + // Merge the two rule sets + requestRoutingRules := MergeRules( + &appGw, + []n.ApplicationGatewayRequestRoutingRule{ + rulePathBased1, + }, + []n.ApplicationGatewayRequestRoutingRule{ + rulePathBased2, + }) + + // Since both rules are using same listener, they should have been merged to 1 rule and 1 path map + Expect(len(requestRoutingRules)).To(Equal(1)) + Expect(len(*appGw.URLPathMaps)).To(Equal(1)) + + pathRules := *(*appGw.URLPathMaps)[0].PathRules + Expect(len(pathRules)).To(Equal(3)) + }) + }) }) diff --git a/pkg/tests/fixtures/paths.go b/pkg/tests/fixtures/paths.go index 199ee54e9..378e21a79 100644 --- a/pkg/tests/fixtures/paths.go +++ b/pkg/tests/fixtures/paths.go @@ -34,6 +34,7 @@ const ( func GetURLPathMap1() *n.ApplicationGatewayURLPathMap { return &n.ApplicationGatewayURLPathMap{ Name: to.StringPtr(URLPathMapName1), + ID: to.StringPtr("x/y/z/" + URLPathMapName1), ApplicationGatewayURLPathMapPropertiesFormat: &n.ApplicationGatewayURLPathMapPropertiesFormat{ // DefaultBackendAddressPool - Default backend address pool resource of URL path map. DefaultBackendAddressPool: &n.SubResource{ @@ -67,7 +68,7 @@ func GetURLPathMap1() *n.ApplicationGatewayURLPathMap { // GetPathRulePathBased1 creates a new struct for use in unit tests. func GetPathRulePathBased1() *n.ApplicationGatewayPathRule { return &n.ApplicationGatewayPathRule{ - Name: to.StringPtr(PathRuleName), + Name: to.StringPtr(PathRuleName + URLPathMapName1), ApplicationGatewayPathRulePropertiesFormat: &n.ApplicationGatewayPathRulePropertiesFormat{ // Paths - Path rules of URL path map. Paths: &[]string{ @@ -135,6 +136,7 @@ func GetDefaultURLPathMap() *n.ApplicationGatewayURLPathMap { return &n.ApplicationGatewayURLPathMap{ Etag: to.StringPtr("*"), Name: to.StringPtr(DefaultPathMapName), + ID: to.StringPtr("x/y/z/" + DefaultPathMapName), ApplicationGatewayURLPathMapPropertiesFormat: &n.ApplicationGatewayURLPathMapPropertiesFormat{ DefaultBackendAddressPool: &n.SubResource{ID: to.StringPtr("/" + DefaultBackendPoolName)}, DefaultBackendHTTPSettings: &n.SubResource{ID: to.StringPtr("/" + DefaultBackendHTTPSettingsName)}, @@ -146,6 +148,7 @@ func GetDefaultURLPathMap() *n.ApplicationGatewayURLPathMap { func GetURLPathMap2() *n.ApplicationGatewayURLPathMap { return &n.ApplicationGatewayURLPathMap{ Name: to.StringPtr(URLPathMapName2), + ID: to.StringPtr("x/y/z/" + URLPathMapName2), ApplicationGatewayURLPathMapPropertiesFormat: &n.ApplicationGatewayURLPathMapPropertiesFormat{ // DefaultBackendAddressPool - Default backend address pool resource of URL path map. DefaultBackendAddressPool: &n.SubResource{ @@ -178,7 +181,7 @@ func GetURLPathMap2() *n.ApplicationGatewayURLPathMap { // GetPathRulePathBased2 creates a new struct for use in unit tests. func GetPathRulePathBased2() *n.ApplicationGatewayPathRule { return &n.ApplicationGatewayPathRule{ - Name: to.StringPtr(PathRuleName), + Name: to.StringPtr(PathRuleName + URLPathMapName2), ApplicationGatewayPathRulePropertiesFormat: &n.ApplicationGatewayPathRulePropertiesFormat{ // Paths - Path rules of URL path map. Paths: &[]string{ @@ -212,6 +215,7 @@ func GetPathRulePathBased2() *n.ApplicationGatewayPathRule { func GetURLPathMap3() *n.ApplicationGatewayURLPathMap { return &n.ApplicationGatewayURLPathMap{ Name: to.StringPtr(URLPathMapName3), + ID: to.StringPtr("x/y/z/" + URLPathMapName3), ApplicationGatewayURLPathMapPropertiesFormat: &n.ApplicationGatewayURLPathMapPropertiesFormat{ // DefaultBackendAddressPool - Default backend address pool resource of URL path map. DefaultBackendAddressPool: &n.SubResource{ @@ -244,7 +248,7 @@ func GetURLPathMap3() *n.ApplicationGatewayURLPathMap { // GetPathRulePathBased3 creates a new struct for use in unit tests. func GetPathRulePathBased3() *n.ApplicationGatewayPathRule { return &n.ApplicationGatewayPathRule{ - Name: to.StringPtr(PathRuleName), + Name: to.StringPtr(PathRuleName + URLPathMapName3), ApplicationGatewayPathRulePropertiesFormat: &n.ApplicationGatewayPathRulePropertiesFormat{ // Paths - Path rules of URL path map. Paths: &[]string{ diff --git a/pkg/tests/fixtures/paths_test.go b/pkg/tests/fixtures/paths_test.go index 025161acd..562655cbe 100644 --- a/pkg/tests/fixtures/paths_test.go +++ b/pkg/tests/fixtures/paths_test.go @@ -21,7 +21,7 @@ var _ = Describe("Test Fixtures", func() { Context("Testing GetPathRulePathBased1", func() { It("should work as expected", func() { actual := GetPathRulePathBased1() - Expect(*actual.Name).To(Equal("PathRule-1")) + Expect(*actual.Name).To(Equal("PathRule-1URLPathMap-1")) }) }) @@ -49,7 +49,7 @@ var _ = Describe("Test Fixtures", func() { Context("Testing GetPathRulePathBased2", func() { It("should work as expected", func() { actual := GetPathRulePathBased2() - Expect(*actual.Name).To(Equal("PathRule-1")) + Expect(*actual.Name).To(Equal("PathRule-1URLPathMap-2")) }) }) })