Skip to content

Commit 9d95af7

Browse files
akshaysnguptaakshaysngupta
authored
fix(prohibited target): incorrect merge when rules being merged reference 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
1 parent 0646582 commit 9d95af7

File tree

6 files changed

+130
-5
lines changed

6 files changed

+130
-5
lines changed

‎pkg/brownfield/brownfield_suite_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,19 @@
88
package brownfield
99

1010
import (
11+
"flag"
1112
"testing"
1213

1314
. "github.com/onsi/ginkgo"
1415
. "github.com/onsi/gomega"
16+
"k8s.io/klog/v2"
1517
)
1618

1719
func TestApplicationGatewayKubernetesIngress(t *testing.T) {
20+
klog.InitFlags(nil)
21+
_ = flag.Set("v", "5")
22+
_ = flag.Lookup("logtostderr").Value.Set("true")
23+
1824
RegisterFailHandler(Fail)
1925
RunSpecs(t, "ApplicationGatewayKubernetesIngress Suite")
2026
}

‎pkg/brownfield/pathmaps.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,12 +109,17 @@ func mergePathMapsWithBasicRule(pathMap *n.ApplicationGatewayURLPathMap, rule *n
109109
func mergePathRules(pathRulesBucket ...*[]n.ApplicationGatewayPathRule) *[]n.ApplicationGatewayPathRule {
110110
uniq := make(pathRulesByName)
111111
for _, bucket := range pathRulesBucket {
112+
if bucket == nil {
113+
continue
114+
}
115+
112116
for _, pathRule := range *bucket {
113117
uniq[pathRuleName(*pathRule.Name)] = pathRule
114118
}
115119
}
116120
var merged []n.ApplicationGatewayPathRule
117121
for _, pathRule := range uniq {
122+
klog.V(5).Infof("[brownfield] Appending %s with paths %v", *pathRule.Name, pathRule.Paths)
118123
merged = append(merged, pathRule)
119124
}
120125
return &merged

‎pkg/brownfield/routing_rules.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,12 @@ func mergeRoutingRules(appGw *n.ApplicationGateway, firstRoutingRule *n.Applicat
116116
return firstRoutingRule
117117
}
118118

119+
if *firstRoutingRule.URLPathMap.ID == *secondRoutingRule.URLPathMap.ID {
120+
// No merge needed as both routing rule point to the same URL Path map
121+
klog.V(5).Infof("[brownfield] Rule %s and rule %s share the same path map %s; Skipping merge", *firstRoutingRule.Name, *secondRoutingRule.Name, *firstPathMap.Name)
122+
return firstRoutingRule
123+
}
124+
119125
// Get the url path map for the second rule
120126
secondPathMap := lookupPathMap(appGw.URLPathMaps, secondRoutingRule.URLPathMap.ID)
121127

‎pkg/brownfield/routing_rules_test.go‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
ptv1 "github.com/Azure/application-gateway-kubernetes-ingress/pkg/apis/azureingressprohibitedtarget/v1"
1313
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/tests"
1414
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/tests/fixtures"
15+
n "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-03-01/network"
1516
)
1617

1718
var _ = Describe("Test blacklist request routing rules", func() {
@@ -101,4 +102,107 @@ var _ = Describe("Test blacklist request routing rules", func() {
101102
Expect(blacklisted).To(ContainElement(rulePathBased3))
102103
})
103104
})
105+
106+
Context("Test MergeRules", func() {
107+
It("should merge correctly when same set of routing rules need to be merged", func() {
108+
Expect(len(*appGw.RequestRoutingRules)).To(Equal(5))
109+
Expect(len(*appGw.URLPathMaps)).To(Equal(4))
110+
111+
requestRoutingRules := MergeRules(&appGw, *appGw.RequestRoutingRules, *appGw.RequestRoutingRules)
112+
113+
// No change
114+
Expect(len(requestRoutingRules)).To(Equal(5))
115+
Expect(len(*appGw.URLPathMaps)).To(Equal(4))
116+
117+
pathBasedRuleCount := 0
118+
basicRuleCount := 0
119+
for _, rule := range *appGw.RequestRoutingRules {
120+
if rule.RuleType == n.ApplicationGatewayRequestRoutingRuleTypePathBasedRouting {
121+
pathBasedRuleCount++
122+
Expect(lookupPathMap(appGw.URLPathMaps, rule.URLPathMap.ID)).ToNot(BeNil())
123+
}
124+
125+
if rule.RuleType == n.ApplicationGatewayRequestRoutingRuleTypeBasic {
126+
basicRuleCount++
127+
}
128+
}
129+
130+
Expect(pathBasedRuleCount).To(Equal(3))
131+
Expect(basicRuleCount).To(Equal(2))
132+
})
133+
134+
It("should merge correctly when different set of routing rules need to be merged", func() {
135+
requestRoutingRules := MergeRules(
136+
&appGw,
137+
[]n.ApplicationGatewayRequestRoutingRule{
138+
ruleBasic,
139+
ruleDefault,
140+
rulePathBased1,
141+
},
142+
[]n.ApplicationGatewayRequestRoutingRule{
143+
rulePathBased2,
144+
rulePathBased3,
145+
})
146+
147+
Expect(len(requestRoutingRules)).To(Equal(5))
148+
Expect(len(*appGw.URLPathMaps)).To(Equal(4))
149+
150+
pathBasedRuleCount := 0
151+
basicRuleCount := 0
152+
for _, rule := range *appGw.RequestRoutingRules {
153+
if rule.RuleType == n.ApplicationGatewayRequestRoutingRuleTypePathBasedRouting {
154+
pathBasedRuleCount++
155+
Expect(lookupPathMap(appGw.URLPathMaps, rule.URLPathMap.ID)).ToNot(BeNil())
156+
}
157+
158+
if rule.RuleType == n.ApplicationGatewayRequestRoutingRuleTypeBasic {
159+
basicRuleCount++
160+
}
161+
}
162+
163+
Expect(pathBasedRuleCount).To(Equal(3))
164+
Expect(basicRuleCount).To(Equal(2))
165+
Expect(len(*appGw.URLPathMaps)).To(Equal(4))
166+
})
167+
168+
It("should merge correctly when 2 routing rule use the same http listener but different url paths", func() {
169+
// When routing rule uses same listener but different url path maps, they need to be merged together
170+
// as AppGw doesn't allow 2 rules using same listener
171+
172+
// Setup 2 path maps
173+
pathMap1 := (*appGw.URLPathMaps)[1]
174+
pathMap2 := (*appGw.URLPathMaps)[2]
175+
urlPathMap := &[]n.ApplicationGatewayURLPathMap{
176+
pathMap1,
177+
pathMap2,
178+
}
179+
appGw.URLPathMaps = urlPathMap
180+
Expect(len(*pathMap1.PathRules)).To(Equal(2))
181+
Expect(len(*pathMap2.PathRules)).To(Equal(1))
182+
183+
// Setup first rule to use first path map
184+
rulePathBased1.URLPathMap.ID = pathMap1.ID
185+
186+
// Setup second rule to use the same listener but second url path map
187+
rulePathBased2.HTTPListener.ID = rulePathBased1.HTTPListener.ID
188+
rulePathBased2.URLPathMap.ID = pathMap2.ID
189+
190+
// Merge the two rule sets
191+
requestRoutingRules := MergeRules(
192+
&appGw,
193+
[]n.ApplicationGatewayRequestRoutingRule{
194+
rulePathBased1,
195+
},
196+
[]n.ApplicationGatewayRequestRoutingRule{
197+
rulePathBased2,
198+
})
199+
200+
// Since both rules are using same listener, they should have been merged to 1 rule and 1 path map
201+
Expect(len(requestRoutingRules)).To(Equal(1))
202+
Expect(len(*appGw.URLPathMaps)).To(Equal(1))
203+
204+
pathRules := *(*appGw.URLPathMaps)[0].PathRules
205+
Expect(len(pathRules)).To(Equal(3))
206+
})
207+
})
104208
})

‎pkg/tests/fixtures/paths.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ const (
3434
func GetURLPathMap1() *n.ApplicationGatewayURLPathMap {
3535
return &n.ApplicationGatewayURLPathMap{
3636
Name: to.StringPtr(URLPathMapName1),
37+
ID: to.StringPtr("x/y/z/" + URLPathMapName1),
3738
ApplicationGatewayURLPathMapPropertiesFormat: &n.ApplicationGatewayURLPathMapPropertiesFormat{
3839
// DefaultBackendAddressPool - Default backend address pool resource of URL path map.
3940
DefaultBackendAddressPool: &n.SubResource{
@@ -67,7 +68,7 @@ func GetURLPathMap1() *n.ApplicationGatewayURLPathMap {
6768
// GetPathRulePathBased1 creates a new struct for use in unit tests.
6869
func GetPathRulePathBased1() *n.ApplicationGatewayPathRule {
6970
return &n.ApplicationGatewayPathRule{
70-
Name: to.StringPtr(PathRuleName),
71+
Name: to.StringPtr(PathRuleName + URLPathMapName1),
7172
ApplicationGatewayPathRulePropertiesFormat: &n.ApplicationGatewayPathRulePropertiesFormat{
7273
// Paths - Path rules of URL path map.
7374
Paths: &[]string{
@@ -135,6 +136,7 @@ func GetDefaultURLPathMap() *n.ApplicationGatewayURLPathMap {
135136
return &n.ApplicationGatewayURLPathMap{
136137
Etag: to.StringPtr("*"),
137138
Name: to.StringPtr(DefaultPathMapName),
139+
ID: to.StringPtr("x/y/z/" + DefaultPathMapName),
138140
ApplicationGatewayURLPathMapPropertiesFormat: &n.ApplicationGatewayURLPathMapPropertiesFormat{
139141
DefaultBackendAddressPool: &n.SubResource{ID: to.StringPtr("/" + DefaultBackendPoolName)},
140142
DefaultBackendHTTPSettings: &n.SubResource{ID: to.StringPtr("/" + DefaultBackendHTTPSettingsName)},
@@ -146,6 +148,7 @@ func GetDefaultURLPathMap() *n.ApplicationGatewayURLPathMap {
146148
func GetURLPathMap2() *n.ApplicationGatewayURLPathMap {
147149
return &n.ApplicationGatewayURLPathMap{
148150
Name: to.StringPtr(URLPathMapName2),
151+
ID: to.StringPtr("x/y/z/" + URLPathMapName2),
149152
ApplicationGatewayURLPathMapPropertiesFormat: &n.ApplicationGatewayURLPathMapPropertiesFormat{
150153
// DefaultBackendAddressPool - Default backend address pool resource of URL path map.
151154
DefaultBackendAddressPool: &n.SubResource{
@@ -178,7 +181,7 @@ func GetURLPathMap2() *n.ApplicationGatewayURLPathMap {
178181
// GetPathRulePathBased2 creates a new struct for use in unit tests.
179182
func GetPathRulePathBased2() *n.ApplicationGatewayPathRule {
180183
return &n.ApplicationGatewayPathRule{
181-
Name: to.StringPtr(PathRuleName),
184+
Name: to.StringPtr(PathRuleName + URLPathMapName2),
182185
ApplicationGatewayPathRulePropertiesFormat: &n.ApplicationGatewayPathRulePropertiesFormat{
183186
// Paths - Path rules of URL path map.
184187
Paths: &[]string{
@@ -212,6 +215,7 @@ func GetPathRulePathBased2() *n.ApplicationGatewayPathRule {
212215
func GetURLPathMap3() *n.ApplicationGatewayURLPathMap {
213216
return &n.ApplicationGatewayURLPathMap{
214217
Name: to.StringPtr(URLPathMapName3),
218+
ID: to.StringPtr("x/y/z/" + URLPathMapName3),
215219
ApplicationGatewayURLPathMapPropertiesFormat: &n.ApplicationGatewayURLPathMapPropertiesFormat{
216220
// DefaultBackendAddressPool - Default backend address pool resource of URL path map.
217221
DefaultBackendAddressPool: &n.SubResource{
@@ -244,7 +248,7 @@ func GetURLPathMap3() *n.ApplicationGatewayURLPathMap {
244248
// GetPathRulePathBased3 creates a new struct for use in unit tests.
245249
func GetPathRulePathBased3() *n.ApplicationGatewayPathRule {
246250
return &n.ApplicationGatewayPathRule{
247-
Name: to.StringPtr(PathRuleName),
251+
Name: to.StringPtr(PathRuleName + URLPathMapName3),
248252
ApplicationGatewayPathRulePropertiesFormat: &n.ApplicationGatewayPathRulePropertiesFormat{
249253
// Paths - Path rules of URL path map.
250254
Paths: &[]string{

‎pkg/tests/fixtures/paths_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ var _ = Describe("Test Fixtures", func() {
2121
Context("Testing GetPathRulePathBased1", func() {
2222
It("should work as expected", func() {
2323
actual := GetPathRulePathBased1()
24-
Expect(*actual.Name).To(Equal("PathRule-1"))
24+
Expect(*actual.Name).To(Equal("PathRule-1URLPathMap-1"))
2525
})
2626
})
2727

@@ -49,7 +49,7 @@ var _ = Describe("Test Fixtures", func() {
4949
Context("Testing GetPathRulePathBased2", func() {
5050
It("should work as expected", func() {
5151
actual := GetPathRulePathBased2()
52-
Expect(*actual.Name).To(Equal("PathRule-1"))
52+
Expect(*actual.Name).To(Equal("PathRule-1URLPathMap-2"))
5353
})
5454
})
5555
})

0 commit comments

Comments
 (0)