Skip to content

Commit 91c6f7b

Browse files
iamjaeholeeiamjaeholee
committed
refactor: pass cidrMap as parameter to satisfy protectedby linter
Private helper functions that access cidrMap now receive it as a parameter instead of accessing r.cidrMap directly. Lock-holding callers (AllocateOrOccupyCIDR, ReleaseCIDR, reconcileCreate, reconcileBootstrap, reconcileDelete, NewMultiCIDRRangeAllocator) pass r.cidrMap within their lock scope, satisfying the protectedby linter without introducing deadlocks.
1 parent e161a13 commit 91c6f7b

2 files changed

Lines changed: 39 additions & 37 deletions

File tree

‎pkg/controller/ipam/multi_cidr_range_allocator.go‎

Lines changed: 37 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,9 @@ func NewMultiCIDRRangeAllocator(
199199

200200
// testCIDRMap is only set for testing purposes.
201201
if len(testCIDRMap) > 0 {
202+
ra.lock.Lock()
202203
ra.cidrMap = testCIDRMap
204+
ra.lock.Unlock()
203205
logger.Info("TestCIDRMap should only be set for testing purposes, if this is seen in production logs, it might be a misconfiguration or a bug")
204206
}
205207

@@ -258,13 +260,13 @@ func NewMultiCIDRRangeAllocator(
258260

259261
ra.lock.Lock()
260262
if allocatorParams.ServiceCIDR != nil {
261-
ra.filterOutServiceRange(logger, allocatorParams.ServiceCIDR)
263+
ra.filterOutServiceRange(logger, allocatorParams.ServiceCIDR, ra.cidrMap)
262264
} else {
263265
logger.Info("No Service CIDR provided. Skipping filtering out service addresses")
264266
}
265267

266268
if allocatorParams.SecondaryServiceCIDR != nil {
267-
ra.filterOutServiceRange(logger, allocatorParams.SecondaryServiceCIDR)
269+
ra.filterOutServiceRange(logger, allocatorParams.SecondaryServiceCIDR, ra.cidrMap)
268270
} else {
269271
logger.Info("No Secondary Service CIDR provided. Skipping filtering out secondary service addresses")
270272
}
@@ -278,7 +280,7 @@ func NewMultiCIDRRangeAllocator(
278280
continue
279281
}
280282
logger.Info("Node has CIDR, occupying it in CIDR map", "node", klog.KObj(&node), "podCIDRs", node.Spec.PodCIDRs)
281-
if err := ra.occupyCIDRs(logger, &node); err != nil {
283+
if err := ra.occupyCIDRs(logger, &node, ra.cidrMap); err != nil {
282284
// This will happen if:
283285
// 1. We find garbage in the podCIDRs field. Retrying is useless.
284286
// 2. CIDR out of range: This means ClusterCIDR is not yet created
@@ -514,11 +516,11 @@ func (r *multiCIDRRangeAllocator) syncClusterCIDR(ctx context.Context, key strin
514516
}
515517

516518
// occupyCIDRs marks node.PodCIDRs[...] as used in allocator's tracked cidrSet.
517-
func (r *multiCIDRRangeAllocator) occupyCIDRs(logger klog.Logger, node *corev1.Node) error {
519+
func (r *multiCIDRRangeAllocator) occupyCIDRs(logger klog.Logger, node *corev1.Node, cidrMap map[string][]*cidrset.ClusterCIDR) error {
518520
if len(node.Spec.PodCIDRs) == 0 {
519521
return nil
520522
}
521-
clusterCIDRList, err := r.orderedMatchingClusterCIDRs(node, true)
523+
clusterCIDRList, err := r.orderedMatchingClusterCIDRs(node, true, cidrMap)
522524
if err != nil {
523525
return err
524526
}
@@ -621,10 +623,10 @@ func (r *multiCIDRRangeAllocator) AllocateOrOccupyCIDR(logger klog.Logger, node
621623
}
622624

623625
if len(node.Spec.PodCIDRs) > 0 {
624-
return r.occupyCIDRs(logger, node)
626+
return r.occupyCIDRs(logger, node, r.cidrMap)
625627
}
626628

627-
cidrs, clusterCIDR, err := r.prioritizedCIDRs(logger, node)
629+
cidrs, clusterCIDR, err := r.prioritizedCIDRs(logger, node, r.cidrMap)
628630
if err != nil {
629631
controllerutil.RecordNodeStatusChange(logger, r.recorder, node, "CIDRNotAvailable")
630632
return fmt.Errorf("failed to get cidrs for node %s: %w", node.Name, err)
@@ -656,7 +658,7 @@ func (r *multiCIDRRangeAllocator) ReleaseCIDR(logger klog.Logger, node *corev1.N
656658
return nil
657659
}
658660

659-
clusterCIDR, err := r.allocatedClusterCIDR(node)
661+
clusterCIDR, err := r.allocatedClusterCIDR(node, r.cidrMap)
660662
if err != nil {
661663
return err
662664
}
@@ -681,13 +683,13 @@ func (r *multiCIDRRangeAllocator) ReleaseCIDR(logger klog.Logger, node *corev1.N
681683

682684
// Marks all CIDRs with subNetMaskSize that belongs to serviceCIDR as used across all cidrs
683685
// so that they won't be assignable.
684-
func (r *multiCIDRRangeAllocator) filterOutServiceRange(logger klog.Logger, serviceCIDR *net.IPNet) {
686+
func (r *multiCIDRRangeAllocator) filterOutServiceRange(logger klog.Logger, serviceCIDR *net.IPNet, cidrMap map[string][]*cidrset.ClusterCIDR) {
685687
// Checks if service CIDR has a nonempty intersection with cluster
686688
// CIDR. It is the case if either clusterCIDR contains serviceCIDR with
687689
// clusterCIDR's Mask applied (this means that clusterCIDR contains
688690
// serviceCIDR) or vice versa (which means that serviceCIDR contains
689691
// clusterCIDR).
690-
for _, clusterCIDRList := range r.cidrMap {
692+
for _, clusterCIDRList := range cidrMap {
691693
for _, clusterCIDR := range clusterCIDRList {
692694
if err := r.occupyServiceCIDR(clusterCIDR, serviceCIDR); err != nil {
693695
logger.Error(err, "Unable to occupy service CIDR")
@@ -803,17 +805,17 @@ func defaultNodeSelector() *corev1.NodeSelector {
803805
// Returns 1 CIDR if single stack.
804806
// Returns 2 CIDRs , 1 from each ip family if dual stack.
805807
func (r *multiCIDRRangeAllocator) prioritizedCIDRs(
806-
logger klog.Logger, node *corev1.Node,
808+
logger klog.Logger, node *corev1.Node, cidrMap map[string][]*cidrset.ClusterCIDR,
807809
) ([]*net.IPNet, *cidrset.ClusterCIDR, error) {
808-
clusterCIDRList, err := r.orderedMatchingClusterCIDRs(node, true)
810+
clusterCIDRList, err := r.orderedMatchingClusterCIDRs(node, true, cidrMap)
809811
if err != nil {
810812
return nil, nil, fmt.Errorf("unable to get a clusterCIDR for node %s: %w", node.Name, err)
811813
}
812814

813815
for _, clusterCIDR := range clusterCIDRList {
814816
cidrs := make([]*net.IPNet, 0)
815817
if clusterCIDR.IPv4CIDRSet != nil {
816-
cidr, err := r.allocateCIDR(logger, clusterCIDR, clusterCIDR.IPv4CIDRSet)
818+
cidr, err := r.allocateCIDR(logger, clusterCIDR, clusterCIDR.IPv4CIDRSet, cidrMap)
817819
if err != nil {
818820
logger.V(3).Info("Unable to allocate IPv4 CIDR, trying next range", "err", err)
819821
continue
@@ -822,7 +824,7 @@ func (r *multiCIDRRangeAllocator) prioritizedCIDRs(
822824
}
823825

824826
if clusterCIDR.IPv6CIDRSet != nil {
825-
cidr, err := r.allocateCIDR(logger, clusterCIDR, clusterCIDR.IPv6CIDRSet)
827+
cidr, err := r.allocateCIDR(logger, clusterCIDR, clusterCIDR.IPv6CIDRSet, cidrMap)
826828
if err != nil {
827829
logger.V(3).Info("Unable to allocate IPv6 CIDR, trying next range", "err", err)
828830
continue
@@ -836,7 +838,7 @@ func (r *multiCIDRRangeAllocator) prioritizedCIDRs(
836838
}
837839

838840
func (r *multiCIDRRangeAllocator) allocateCIDR(
839-
logger klog.Logger, clusterCIDR *cidrset.ClusterCIDR, cidrSet *cidrset.MultiCIDRSet,
841+
logger klog.Logger, clusterCIDR *cidrset.ClusterCIDR, cidrSet *cidrset.MultiCIDRSet, cidrMap map[string][]*cidrset.ClusterCIDR,
840842
) (*net.IPNet, error) {
841843
for evaluated := 0; evaluated < cidrSet.MaxCIDRs; evaluated++ {
842844
candidate, lastEvaluated, err := cidrSet.NextCandidate()
@@ -846,12 +848,12 @@ func (r *multiCIDRRangeAllocator) allocateCIDR(
846848

847849
evaluated += lastEvaluated
848850

849-
if r.cidrInAllocatedList(logger, candidate) {
851+
if r.cidrInAllocatedList(logger, candidate, cidrMap) {
850852
continue
851853
}
852854

853855
// Deep Check.
854-
if r.cidrOverlapWithAllocatedList(logger, candidate) {
856+
if r.cidrOverlapWithAllocatedList(logger, candidate, cidrMap) {
855857
continue
856858
}
857859

@@ -868,8 +870,8 @@ func (r *multiCIDRRangeAllocator) allocateCIDR(
868870
}
869871
}
870872

871-
func (r *multiCIDRRangeAllocator) cidrInAllocatedList(logger klog.Logger, cidr *net.IPNet) bool {
872-
for _, clusterCIDRList := range r.cidrMap {
873+
func (r *multiCIDRRangeAllocator) cidrInAllocatedList(logger klog.Logger, cidr *net.IPNet, cidrMap map[string][]*cidrset.ClusterCIDR) bool {
874+
for _, clusterCIDRList := range cidrMap {
873875
for _, clusterCIDR := range clusterCIDRList {
874876
cidrSet, err := r.associatedCIDRSet(clusterCIDR, cidr)
875877
if err != nil {
@@ -886,8 +888,8 @@ func (r *multiCIDRRangeAllocator) cidrInAllocatedList(logger klog.Logger, cidr *
886888
return false
887889
}
888890

889-
func (r *multiCIDRRangeAllocator) cidrOverlapWithAllocatedList(logger klog.Logger, cidr *net.IPNet) bool {
890-
for _, clusterCIDRList := range r.cidrMap {
891+
func (r *multiCIDRRangeAllocator) cidrOverlapWithAllocatedList(logger klog.Logger, cidr *net.IPNet, cidrMap map[string][]*cidrset.ClusterCIDR) bool {
892+
for _, clusterCIDRList := range cidrMap {
891893
for _, clusterCIDR := range clusterCIDRList {
892894
cidrSet, err := r.associatedCIDRSet(clusterCIDR, cidr)
893895
if err != nil {
@@ -905,8 +907,8 @@ func (r *multiCIDRRangeAllocator) cidrOverlapWithAllocatedList(logger klog.Logge
905907
}
906908

907909
// allocatedClusterCIDR returns the ClusterCIDR from which the node CIDRs were allocated.
908-
func (r *multiCIDRRangeAllocator) allocatedClusterCIDR(node *corev1.Node) (*cidrset.ClusterCIDR, error) {
909-
clusterCIDRList, err := r.orderedMatchingClusterCIDRs(node, false)
910+
func (r *multiCIDRRangeAllocator) allocatedClusterCIDR(node *corev1.Node, cidrMap map[string][]*cidrset.ClusterCIDR) (*cidrset.ClusterCIDR, error) {
911+
clusterCIDRList, err := r.orderedMatchingClusterCIDRs(node, false, cidrMap)
910912
if err != nil {
911913
return nil, fmt.Errorf("unable to get a clusterCIDR for node %s: %w", node.Name, err)
912914
}
@@ -930,11 +932,11 @@ func (r *multiCIDRRangeAllocator) allocatedClusterCIDR(node *corev1.Node) (*cidr
930932
// orderedMatchingClusterCIDRs takes `occupy` as an argument, it determines whether the function
931933
// is called during an occupy or a release operation. For a release operation, a ClusterCIDR must
932934
// be added to the matching ClusterCIDRs list, irrespective of whether the ClusterCIDR is terminating.
933-
func (r *multiCIDRRangeAllocator) orderedMatchingClusterCIDRs(node *corev1.Node, occupy bool) ([]*cidrset.ClusterCIDR, error) {
935+
func (r *multiCIDRRangeAllocator) orderedMatchingClusterCIDRs(node *corev1.Node, occupy bool, cidrMap map[string][]*cidrset.ClusterCIDR) ([]*cidrset.ClusterCIDR, error) {
934936
matchingCIDRs := make([]*cidrset.ClusterCIDR, 0)
935937
pq := make(PriorityQueue, 0)
936938

937-
for label, clusterCIDRList := range r.cidrMap {
939+
for label, clusterCIDRList := range cidrMap {
938940
labelsMatch, matchCnt, err := r.matchCIDRLabels(node, label)
939941
if err != nil {
940942
return nil, err
@@ -972,7 +974,7 @@ func (r *multiCIDRRangeAllocator) orderedMatchingClusterCIDRs(node *corev1.Node,
972974
if err != nil {
973975
return nil, err
974976
}
975-
if clusterCIDRList, ok := r.cidrMap[defaultSelector.String()]; ok {
977+
if clusterCIDRList, ok := cidrMap[defaultSelector.String()]; ok {
976978
matchingCIDRs = append(matchingCIDRs, clusterCIDRList...)
977979
}
978980
return matchingCIDRs, nil
@@ -1085,7 +1087,7 @@ func (r *multiCIDRRangeAllocator) reconcileCreate(ctx context.Context, clusterCI
10851087

10861088
logger := klog.FromContext(ctx)
10871089
logger.V(3).Info("Reconciling ClusterCIDR", "clusterCIDR", clusterCIDR.Name)
1088-
if err := r.createClusterCIDR(ctx, clusterCIDR, false); err != nil {
1090+
if err := r.createClusterCIDR(ctx, clusterCIDR, false, r.cidrMap); err != nil {
10891091
logger.Error(err, "failed to reconcile ClusterCIDR", "clusterCIDR", clusterCIDR.Name)
10901092
return err
10911093
}
@@ -1108,7 +1110,7 @@ func (r *multiCIDRRangeAllocator) reconcileBootstrap(ctx context.Context, cluste
11081110
}
11091111

11101112
logger.V(2).Info("Creating ClusterCIDR during bootstrap", "clusterCIDR", clusterCIDR.Name)
1111-
if err := r.createClusterCIDR(ctx, clusterCIDR, terminating); err != nil {
1113+
if err := r.createClusterCIDR(ctx, clusterCIDR, terminating, r.cidrMap); err != nil {
11121114
logger.Error(err, "Unable to create ClusterCIDR", "clusterCIDR", clusterCIDR.Name)
11131115
return err
11141116
}
@@ -1117,7 +1119,7 @@ func (r *multiCIDRRangeAllocator) reconcileBootstrap(ctx context.Context, cluste
11171119
}
11181120

11191121
// createClusterCIDR creates and maps the cidrSets in the cidrMap.
1120-
func (r *multiCIDRRangeAllocator) createClusterCIDR(ctx context.Context, clusterCIDR *v1.ClusterCIDR, terminating bool) error {
1122+
func (r *multiCIDRRangeAllocator) createClusterCIDR(ctx context.Context, clusterCIDR *v1.ClusterCIDR, terminating bool, cidrMap map[string][]*cidrset.ClusterCIDR) error {
11211123
nodeSelector, err := r.nodeSelectorKey(clusterCIDR)
11221124
if err != nil {
11231125
return fmt.Errorf("unable to get labelSelector key: %w", err)
@@ -1132,7 +1134,7 @@ func (r *multiCIDRRangeAllocator) createClusterCIDR(ctx context.Context, cluster
11321134
return errors.New("invalid ClusterCIDR: must provide IPv4 and/or IPv6 config")
11331135
}
11341136

1135-
if err := r.mapClusterCIDRSet(r.cidrMap, nodeSelector, clusterCIDRSet); err != nil {
1137+
if err := r.mapClusterCIDRSet(cidrMap, nodeSelector, clusterCIDRSet); err != nil {
11361138
return fmt.Errorf("unable to map clusterCIDRSet: %w", err)
11371139
}
11381140

@@ -1226,7 +1228,7 @@ func (r *multiCIDRRangeAllocator) reconcileDelete(ctx context.Context, clusterCI
12261228
logger := klog.FromContext(ctx)
12271229
if slices.Contains(clusterCIDR.GetFinalizers(), clusterCIDRFinalizer) {
12281230
logger.V(2).Info("Releasing ClusterCIDR", "clusterCIDR", clusterCIDR.Name)
1229-
if err := r.deleteClusterCIDR(logger, clusterCIDR); err != nil {
1231+
if err := r.deleteClusterCIDR(logger, clusterCIDR, r.cidrMap); err != nil {
12301232
logger.V(2).Info("Error while deleting ClusterCIDR", "err", err)
12311233
return err
12321234
}
@@ -1246,13 +1248,13 @@ func (r *multiCIDRRangeAllocator) reconcileDelete(ctx context.Context, clusterCI
12461248
}
12471249

12481250
// deleteClusterCIDR Deletes and unmaps the ClusterCIDRs from the cidrMap.
1249-
func (r *multiCIDRRangeAllocator) deleteClusterCIDR(logger klog.Logger, clusterCIDR *v1.ClusterCIDR) error {
1251+
func (r *multiCIDRRangeAllocator) deleteClusterCIDR(logger klog.Logger, clusterCIDR *v1.ClusterCIDR, cidrMap map[string][]*cidrset.ClusterCIDR) error {
12501252
labelSelector, err := r.nodeSelectorKey(clusterCIDR)
12511253
if err != nil {
12521254
return fmt.Errorf("unable to delete cidr: %w", err)
12531255
}
12541256

1255-
clusterCIDRSetList, ok := r.cidrMap[labelSelector]
1257+
clusterCIDRSetList, ok := cidrMap[labelSelector]
12561258
if !ok {
12571259
logger.Info("Label not found in CIDRMap, proceeding with delete", "labelSelector", labelSelector)
12581260
return nil
@@ -1274,12 +1276,12 @@ func (r *multiCIDRRangeAllocator) deleteClusterCIDR(logger klog.Logger, clusterC
12741276
// Remove the label from the map if this was the only clusterCIDR associated
12751277
// with it.
12761278
if len(clusterCIDRSetList) == 1 {
1277-
delete(r.cidrMap, labelSelector)
1279+
delete(cidrMap, labelSelector)
12781280
return nil
12791281
}
12801282

12811283
clusterCIDRSetList = append(clusterCIDRSetList[:i], clusterCIDRSetList[i+1:]...)
1282-
r.cidrMap[labelSelector] = clusterCIDRSetList
1284+
cidrMap[labelSelector] = clusterCIDRSetList
12831285
return nil
12841286
}
12851287
logger.V(2).Info("clusterCIDR not found, proceeding with delete", "clusterCIDR", clusterCIDR.Name, "label", labelSelector)

‎pkg/controller/ipam/multi_cidr_range_allocator_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1877,13 +1877,13 @@ func TestMultiCIDRSetDataRace(t *testing.T) {
18771877
go func() {
18781878
defer wg.Done()
18791879
ra.lock.Lock()
1880-
ra.cidrInAllocatedList(logger, lookupCIDR)
1880+
ra.cidrInAllocatedList(logger, lookupCIDR, ra.cidrMap)
18811881
ra.lock.Unlock()
18821882
}()
18831883
go func() {
18841884
defer wg.Done()
18851885
ra.lock.Lock()
1886-
ra.cidrOverlapWithAllocatedList(logger, lookupCIDR)
1886+
ra.cidrOverlapWithAllocatedList(logger, lookupCIDR, ra.cidrMap)
18871887
ra.lock.Unlock()
18881888
}()
18891889
wg.Wait()

0 commit comments

Comments
 (0)