Skip to content

Commit

Permalink
Fix for crash due to concurrent map access
Browse files Browse the repository at this point in the history
  • Loading branch information
@vklohiya
vklohiya committed Apr 26, 2024
1 parent 2457480 commit 18106a2
Show file tree
Hide file tree
Showing 9 changed files with 472 additions and 261 deletions.
19 changes: 11 additions & 8 deletions pkg/controller/informers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ var _ = Describe("Informers Tests", func() {
mockCtlr.resourceQueue = workqueue.NewNamedRateLimitingQueue(
workqueue.DefaultControllerRateLimiter(), "custom-resource-controller")
mockCtlr.resources = NewResourceStore()
mockCtlr.resources.ltmConfig = make(map[string]*PartitionConfig, 0)
mockCtlr.resources.ltmConfig = sync.Map{}
mockCtlr.requestQueue = &requestQueue{sync.Mutex{}, list.New()}
mockCtlr.Partition = "test"

Expand Down Expand Up @@ -101,13 +101,14 @@ var _ = Describe("Informers Tests", func() {
Partition: "dev",
})
zero := 0
mockCtlr.resources.ltmConfig[mockCtlr.Partition] = &PartitionConfig{ResourceMap: make(ResourceMap), Priority: &zero}
mockCtlr.resources.ltmConfig.Store(mockCtlr.Partition, &PartitionConfig{ResourceMap: make(ResourceMap), Priority: &zero})
mockCtlr.enqueueUpdatedVirtualServer(vs, newVS)
key, quit = mockCtlr.resourceQueue.Get()
Expect(key).ToNot(BeNil(), "Enqueue Updated VS Failed")
Expect(quit).To(BeFalse(), "Enqueue Updated VS Failed")
Expect(*mockCtlr.resources.ltmConfig[mockCtlr.Partition].Priority).To(BeEquivalentTo(1), "Priority Not Updated")
delete(mockCtlr.resources.ltmConfig, mockCtlr.Partition)
pcInt, _ := mockCtlr.resources.ltmConfig.Load(mockCtlr.Partition)
Expect(*pcInt.(*PartitionConfig).Priority).To(BeEquivalentTo(1), "Priority Not Updated")
mockCtlr.resources.ltmConfig.Delete(mockCtlr.Partition)
key, quit = mockCtlr.resourceQueue.Get()
Expect(key).ToNot(BeNil(), "Enqueue Updated VS Failed")
Expect(quit).To(BeFalse(), "Enqueue Updated VS Failed")
Expand Down Expand Up @@ -252,9 +253,10 @@ var _ = Describe("Informers Tests", func() {
tsWithPartition := newTS.DeepCopy()
tsWithPartition.Spec.Partition = "dev"
zero := 0
mockCtlr.resources.ltmConfig[mockCtlr.Partition] = &PartitionConfig{ResourceMap: make(ResourceMap), Priority: &zero}
mockCtlr.resources.ltmConfig.Store(mockCtlr.Partition, &PartitionConfig{ResourceMap: make(ResourceMap), Priority: &zero})
mockCtlr.enqueueUpdatedTransportServer(newTS, tsWithPartition)
Expect(*mockCtlr.resources.ltmConfig[mockCtlr.Partition].Priority).To(BeEquivalentTo(1), "Priority Not Updated")
pcInt, _ := mockCtlr.resources.ltmConfig.Load(mockCtlr.Partition)
Expect(*pcInt.(*PartitionConfig).Priority).To(BeEquivalentTo(1), "Priority Not Updated")

// Verify TS status update event is not queued for processing
queueLen := mockCtlr.resourceQueue.Len()
Expand Down Expand Up @@ -326,9 +328,10 @@ var _ = Describe("Informers Tests", func() {
ilWithPartition := newIL.DeepCopy()
ilWithPartition.Spec.Partition = "dev"
zero := 0
mockCtlr.resources.ltmConfig[mockCtlr.Partition] = &PartitionConfig{ResourceMap: make(ResourceMap), Priority: &zero}
mockCtlr.resources.ltmConfig.Store(mockCtlr.Partition, &PartitionConfig{ResourceMap: make(ResourceMap), Priority: &zero})
mockCtlr.enqueueUpdatedIngressLink(newIL, ilWithPartition)
Expect(*mockCtlr.resources.ltmConfig[mockCtlr.Partition].Priority).To(BeEquivalentTo(1), "Priority Not Updated")
pcInt, _ := mockCtlr.resources.ltmConfig.Load(mockCtlr.Partition)
Expect(*pcInt.(*PartitionConfig).Priority).To(BeEquivalentTo(1), "Priority Not Updated")

})

Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/nativeResourceWorker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1109,7 +1109,7 @@ func (ctlr *Controller) processRouteConfigFromGlobalCM(es extendedSpec, isDelete
_ = ctlr.processRoutes(routeGroupKey, true)
// deleting the bigip partition when partition is changes
if ctlr.resources.extdSpecMap[routeGroupKey].partition != newExtdSpecMap[routeGroupKey].partition {
if _, ok := ctlr.resources.ltmConfig[ctlr.resources.extdSpecMap[routeGroupKey].partition]; ok {
if _, ok := ctlr.resources.ltmConfig.Load(ctlr.resources.extdSpecMap[routeGroupKey].partition); ok {
ctlr.resources.updatePartitionPriority(ctlr.resources.extdSpecMap[routeGroupKey].partition, 1)
}
}
Expand Down
76 changes: 49 additions & 27 deletions pkg/controller/nativeResourceWorker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,10 @@ var _ = Describe("Routes", func() {
Partition: "test",
}
Expect(err).To(BeNil(), "Failed to process routes")
Expect(len(mockCtlr.resources.ltmConfig["test"].ResourceMap["samplevs_443"].Policies)).To(BeEquivalentTo(0), "Policy should not be created for passthrough route")
dg, ok := mockCtlr.resources.ltmConfig["test"].ResourceMap["samplevs_443"].IntDgMap[mapKey]
pcInt, _ := mockCtlr.resources.ltmConfig.Load("test")
partitionConfig := pcInt.(*PartitionConfig)
Expect(len(partitionConfig.ResourceMap["samplevs_443"].Policies)).To(BeEquivalentTo(0), "Policy should not be created for passthrough route")
dg, ok := partitionConfig.ResourceMap["samplevs_443"].IntDgMap[mapKey]
Expect(ok).To(BeTrue(), "datagroup should be created for passthrough route")
Expect(dg[ns].Records[0].Name).To(BeEquivalentTo("foo.com"), "Invalid vsHostname in datagroup")
Expect(dg[ns].Records[0].Data).To(BeEquivalentTo("foo_80_default"), "Invalid vsHostname in datagroup")
Expand Down Expand Up @@ -499,8 +501,8 @@ extendedRouteSpec:
//Process ENDS with non-matching domain
mockCtlr.addEDNS(newEDNS)
mockCtlr.processExternalDNS(newEDNS, false)
gtmConfig := mockCtlr.resources.gtmConfig[DEFAULT_GTM_PARTITION].WideIPs
DEFAULT_GTM_PARTITION = DEFAULT_GTM_PARTITION + "_gtm"
pcInt, _ := mockCtlr.resources.gtmConfig.Load(DEFAULT_GTM_PARTITION)
gtmConfig := pcInt.(GTMPartitionConfig).WideIPs
Expect(len(gtmConfig)).To(Equal(1))
Expect(len(gtmConfig["test.com"].Pools)).To(Equal(1))
// No pool member should be present
Expand All @@ -509,14 +511,16 @@ extendedRouteSpec:
//delete EDNS
mockCtlr.deleteEDNS(newEDNS)
mockCtlr.processExternalDNS(newEDNS, true)
gtmConfig = mockCtlr.resources.gtmConfig[DEFAULT_GTM_PARTITION].WideIPs
pcInt, _ = mockCtlr.resources.gtmConfig.Load(DEFAULT_GTM_PARTITION)
gtmConfig = pcInt.(GTMPartitionConfig).WideIPs
Expect(len(gtmConfig)).To(Equal(0))

// Modify EDNS with matching domain and create again
mockCtlr.addEDNS(newEDNS)
newEDNS.Spec.DomainName = "pytest-foo-1.com"
mockCtlr.processExternalDNS(newEDNS, false)
gtmConfig = mockCtlr.resources.gtmConfig[DEFAULT_GTM_PARTITION].WideIPs
pcInt, _ = mockCtlr.resources.gtmConfig.Load(DEFAULT_GTM_PARTITION)
gtmConfig = pcInt.(GTMPartitionConfig).WideIPs
Expect(len(gtmConfig)).To(Equal(1))
Expect(len(gtmConfig["pytest-foo-1.com"].Pools)).To(Equal(1))
// Pool member should be present
Expand All @@ -526,7 +530,8 @@ extendedRouteSpec:
mockCtlr.deleteRoute(route1)
mockCtlr.deleteHostPathMapEntry(route1)
mockCtlr.processRoutes(namespace1, false)
gtmConfig = mockCtlr.resources.gtmConfig[DEFAULT_GTM_PARTITION].WideIPs
pcInt, _ = mockCtlr.resources.gtmConfig.Load(DEFAULT_GTM_PARTITION)
gtmConfig = pcInt.(GTMPartitionConfig).WideIPs
Expect(len(gtmConfig)).To(Equal(1))
Expect(len(gtmConfig["pytest-foo-1.com"].Pools)).To(Equal(1))
// No pool member should be present
Expand Down Expand Up @@ -581,15 +586,17 @@ extendedRouteSpec:
//Test with 2nd route with bigIpPartition
mockCtlr.addEDNS(barEDNS)
mockCtlr.processExternalDNS(barEDNS, false)
gtmConfig = mockCtlr.resources.gtmConfig[DEFAULT_GTM_PARTITION].WideIPs
pcInt, _ = mockCtlr.resources.gtmConfig.Load(DEFAULT_GTM_PARTITION)
gtmConfig = pcInt.(GTMPartitionConfig).WideIPs
Expect(len(gtmConfig)).To(Equal(2))
Expect(len(gtmConfig["pytest-bar-1.com"].Pools)).To(Equal(1))
Expect(len(gtmConfig["pytest-bar-1.com"].Pools[0].Members)).To(Equal(1))
Expect(strings.Contains(gtmConfig["pytest-bar-1.com"].Pools[0].Members[0], "routes_10.8_3_12_dev"))

mockCtlr.deleteEDNS(barEDNS)
mockCtlr.processExternalDNS(barEDNS, true)
gtmConfig = mockCtlr.resources.gtmConfig[DEFAULT_GTM_PARTITION].WideIPs
pcInt, _ = mockCtlr.resources.gtmConfig.Load(DEFAULT_GTM_PARTITION)
gtmConfig = pcInt.(GTMPartitionConfig).WideIPs
Expect(len(gtmConfig)).To(Equal(1))

//Remove route group
Expand All @@ -605,8 +612,8 @@ extendedRouteSpec:
err, isProcessed = mockCtlr.processConfigMap(cm, false)
Expect(err).To(BeNil())
Expect(isProcessed).To(BeTrue())

gtmConfig = mockCtlr.resources.gtmConfig[DEFAULT_GTM_PARTITION].WideIPs
pcInt, _ = mockCtlr.resources.gtmConfig.Load(DEFAULT_GTM_PARTITION)
gtmConfig = pcInt.(GTMPartitionConfig).WideIPs
Expect(len(gtmConfig)).To(Equal(1))
Expect(len(gtmConfig["pytest-foo-1.com"].Pools)).To(Equal(1))
//No pool members should present
Expand All @@ -616,7 +623,8 @@ extendedRouteSpec:
barEDNS.Spec.Pools[0].Monitor.Type = "tcp"
mockCtlr.addEDNS(barEDNS)
mockCtlr.processExternalDNS(barEDNS, false)
gtmConfig = mockCtlr.resources.gtmConfig[DEFAULT_GTM_PARTITION].WideIPs
pcInt, _ = mockCtlr.resources.gtmConfig.Load(DEFAULT_GTM_PARTITION)
gtmConfig = pcInt.(GTMPartitionConfig).WideIPs
Expect(len(gtmConfig)).To(Equal(2))
Expect(len(gtmConfig["pytest-foo-1.com"].Pools)).To(Equal(1))
Expect(len(gtmConfig["pytest-bar-1.com"].Pools[0].Members)).To(Equal(1))
Expand All @@ -633,7 +641,8 @@ extendedRouteSpec:
}
mockCtlr.addEDNS(barEDNS)
mockCtlr.processExternalDNS(barEDNS, false)
gtmConfig = mockCtlr.resources.gtmConfig[DEFAULT_GTM_PARTITION].WideIPs
pcInt, _ = mockCtlr.resources.gtmConfig.Load(DEFAULT_GTM_PARTITION)
gtmConfig = pcInt.(GTMPartitionConfig).WideIPs
Expect(len(gtmConfig)).To(Equal(2))
Expect(len(gtmConfig["pytest-foo-1.com"].Pools)).To(Equal(1))
Expect(len(gtmConfig["pytest-bar-1.com"].Pools[0].Members)).To(Equal(1))
Expand Down Expand Up @@ -831,11 +840,13 @@ extendedRouteSpec:
mockCtlr.addRoute(route1)
mockCtlr.resources.invertedNamespaceLabelMap[routeGroup] = routeGroup
err := mockCtlr.processRoutes(routeGroup, false)
parition := mockCtlr.resources.extdSpecMap[routeGroup].partition
partition := mockCtlr.resources.extdSpecMap[routeGroup].partition
vsName := frameRouteVSName(mockCtlr.resources.extdSpecMap[routeGroup].global.VServerName, mockCtlr.resources.extdSpecMap[routeGroup].global.VServerAddr, portStruct{protocol: "https", port: 443})
Expect(err).To(BeNil())
Expect(len(mockCtlr.resources.ltmConfig[parition].ResourceMap[vsName].IRulesMap) == 1).To(BeTrue())
Expect(mockCtlr.resources.ltmConfig["test"].ResourceMap[vsName].Pools[0].ConnectionLimit).
pcInt, _ := mockCtlr.resources.ltmConfig.Load(partition)
partitionConfig := pcInt.(*PartitionConfig)
Expect(len(partitionConfig.ResourceMap[vsName].IRulesMap) == 1).To(BeTrue())
Expect(partitionConfig.ResourceMap[vsName].Pools[0].ConnectionLimit).
To(Equal(int32(5)), "pod concurrent connections not processed")

var alternateBackend []routeapi.RouteTargetReference
Expand All @@ -852,7 +863,8 @@ extendedRouteSpec:
mockCtlr.resources.invertedNamespaceLabelMap[routeGroup] = routeGroup
err = mockCtlr.processRoutes(routeGroup, false)
Expect(err).To(BeNil())
Expect(len(mockCtlr.resources.ltmConfig[parition].ResourceMap[vsName].IRulesMap) == 1).To(BeTrue())
pcInt, _ = mockCtlr.resources.ltmConfig.Load(partition)
Expect(len(pcInt.(*PartitionConfig).ResourceMap[vsName].IRulesMap) == 1).To(BeTrue())

spec2 := routeapi.RouteSpec{
Host: "pytest-foo-1.com",
Expand All @@ -875,8 +887,10 @@ extendedRouteSpec:
Expect(err).To(BeNil())

abPathIRule := getRSCfgResName(vsName, ABPathIRuleName)
Expect(len(mockCtlr.resources.ltmConfig["test"].ResourceMap[vsName].IRulesMap) == 2).To(BeTrue())
Expect(mockCtlr.resources.ltmConfig["test"].ResourceMap[vsName].IRulesMap[NameRef{abPathIRule, parition}].Name == abPathIRule).To(BeTrue())
pcInt, _ = mockCtlr.resources.ltmConfig.Load("test")
partitionConfig = pcInt.(*PartitionConfig)
Expect(len(partitionConfig.ResourceMap[vsName].IRulesMap) == 2).To(BeTrue())
Expect(partitionConfig.ResourceMap[vsName].IRulesMap[NameRef{abPathIRule, partition}].Name == abPathIRule).To(BeTrue())

})

Expand Down Expand Up @@ -1331,9 +1345,11 @@ extendedRouteSpec:

err := mockCtlr.processRoutes(ns, false)
Expect(err).To(BeNil(), "Failed to process routes")
Expect(len(mockCtlr.Controller.resources.ltmConfig["test"].ResourceMap["samplevs_443"].Policies)).
pcInt, _ := mockCtlr.resources.ltmConfig.Load("test")
partitionConfig := pcInt.(*PartitionConfig)
Expect(len(partitionConfig.ResourceMap["samplevs_443"].Policies)).
To(BeNumerically(">", 0), "Policy should not be empty")
createdPolicies := mockCtlr.resources.ltmConfig["test"].ResourceMap["samplevs_443"].Policies
createdPolicies := partitionConfig.ResourceMap["samplevs_443"].Policies

checkWAFRules := func(policies Policies) bool {
defaultWAFDisableRule := false
Expand Down Expand Up @@ -1760,8 +1776,10 @@ extendedRouteSpec:
err = mockCtlr.processRoutes(routeGroup, false)

Expect(err).To(BeNil())
Expect(mockCtlr.resources.ltmConfig["test"].ResourceMap["nextgenroutes_443"].Pools[0].Balance == "least-connections-node").To(BeTrue())
Expect(len(mockCtlr.resources.ltmConfig["test"].ResourceMap["nextgenroutes_443"].Policies[0].Rules) == 3).To(BeTrue())
pcInt, _ := mockCtlr.resources.ltmConfig.Load("test")
partitionConfig := pcInt.(*PartitionConfig)
Expect(partitionConfig.ResourceMap["nextgenroutes_443"].Pools[0].Balance == "least-connections-node").To(BeTrue())
Expect(len(partitionConfig.ResourceMap["nextgenroutes_443"].Policies[0].Rules) == 3).To(BeTrue())

data["extendedSpec"] = `
extendedRouteSpec:
Expand Down Expand Up @@ -1853,8 +1871,10 @@ extendedRouteSpec:
Partition: "test",
TimeUntilUp: &zero,
}
Expect(mockCtlr.resources.ltmConfig["test"].ResourceMap["nextgenroutes_443"].Pools[0].MonitorNames[0].Name).To(Equal("foo_80_default_monitor"))
Expect(mockCtlr.resources.ltmConfig["test"].ResourceMap["nextgenroutes_443"].Monitors[0]).To(Equal(expectedDefaultTCPMonitor))
pcInt, _ := mockCtlr.resources.ltmConfig.Load("test")
partitionConfig := pcInt.(*PartitionConfig)
Expect(partitionConfig.ResourceMap["nextgenroutes_443"].Pools[0].MonitorNames[0].Name).To(Equal("foo_80_default_monitor"))
Expect(partitionConfig.ResourceMap["nextgenroutes_443"].Monitors[0]).To(Equal(expectedDefaultTCPMonitor))

// Verify autoMonitorTimeout
data["extendedSpec"] = `
Expand All @@ -1880,8 +1900,10 @@ extendedRouteSpec:
Partition: "test",
TimeUntilUp: &zero,
}
Expect(mockCtlr.resources.ltmConfig["test"].ResourceMap["nextgenroutes_443"].Pools[0].MonitorNames[0].Name).To(Equal("foo_80_default_monitor"))
Expect(mockCtlr.resources.ltmConfig["test"].ResourceMap["nextgenroutes_443"].Monitors[0]).To(Equal(expectedDefaultTCPMonitor))
pcInt, _ = mockCtlr.resources.ltmConfig.Load("test")
partitionConfig = pcInt.(*PartitionConfig)
Expect(partitionConfig.ResourceMap["nextgenroutes_443"].Pools[0].MonitorNames[0].Name).To(Equal("foo_80_default_monitor"))
Expect(partitionConfig.ResourceMap["nextgenroutes_443"].Monitors[0]).To(Equal(expectedDefaultTCPMonitor))

})
})
Expand Down

0 comments on commit 18106a2

Please sign in to comment.