fix(prohibited target): incorrect merge when rules being merged refer…

…ence the same path map (#1278)

* fix: incorrect merge when rules use the same path map

* add a log statement when not merging

* improve test

* fix fixture test

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")
 }

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

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)
 

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))
+		})
+	})
 })

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{

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"))
 		})
 	})
 })