Skip to content

Commit 01f8216

Browse files
Aprazoraauren
authored andcommitted
fix(NRC): extend atomic.Bool to initSrcDstCheckDone and ec2IamAuthorized
bgpServerStarted was already fixed. This commit applies the same pattern to initSrcDstCheckDone and ec2IamAuthorized, which are written from Run() and the AWS src-dst-check path and read from syncInternalPeers() via bgp_peers.go — potential data race under concurrent BGP peer syncs. Also adds TestAtomicBoolFieldsNoConcurrentDataRace to network_routes_controller_test.go to exercise all three fields under the race detector.
1 parent fc89d09 commit 01f8216

File tree

4 files changed

+43
-7
lines changed

4 files changed

+43
-7
lines changed

pkg/controllers/routing/aws.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ func (nrc *NetworkRoutingController) disableSourceDestinationCheck() {
7474
var apiErr smithy.APIError
7575
if errors.As(err, &apiErr) {
7676
if apiErr.ErrorCode() == "UnauthorizedOperation" {
77-
nrc.ec2IamAuthorized = false
77+
nrc.ec2IamAuthorized.Store(false)
7878
klog.Errorf("Node does not have necessary IAM creds to modify instance attribute. So skipping "+
7979
"disabling src-dst check. %v", apiErr.ErrorMessage())
8080
return

pkg/controllers/routing/bgp_peers.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,7 @@ func (nrc *NetworkRoutingController) OnNodeUpdate(_ interface{}) {
374374

375375
// skip if first round of disableSourceDestinationCheck() is not done yet, this is to prevent
376376
// all the nodes for all the node add update trying to perform disableSourceDestinationCheck
377-
if nrc.disableSrcDstCheck && nrc.initSrcDstCheckDone && nrc.ec2IamAuthorized {
377+
if nrc.disableSrcDstCheck && nrc.initSrcDstCheckDone.Load() && nrc.ec2IamAuthorized.Load() {
378378
nrc.disableSourceDestinationCheck()
379379
}
380380
}

pkg/controllers/routing/network_routes_controller.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,8 @@ type NetworkRoutingController struct {
144144
bgpClusterID string
145145
cniConfFile string
146146
disableSrcDstCheck bool
147-
initSrcDstCheckDone bool
148-
ec2IamAuthorized bool
147+
initSrcDstCheckDone atomic.Bool
148+
ec2IamAuthorized atomic.Bool
149149
pathPrependAS string
150150
pathPrependCount uint8
151151
pathPrepend bool
@@ -190,7 +190,7 @@ func (nrc *NetworkRoutingController) Run(
190190
// In case of cluster provisioned on AWS disable source-destination check
191191
if nrc.disableSrcDstCheck {
192192
nrc.disableSourceDestinationCheck()
193-
nrc.initSrcDstCheckDone = true
193+
nrc.initSrcDstCheckDone.Store(true)
194194
}
195195

196196
// enable IP forwarding for the packets coming in/out from the pods
@@ -1273,7 +1273,7 @@ func NewNetworkRoutingController(clientset kubernetes.Interface,
12731273
nrc.bgpRRServer = false
12741274
nrc.bgpServerStarted.Store(false)
12751275
nrc.disableSrcDstCheck = kubeRouterConfig.DisableSrcDstCheck
1276-
nrc.initSrcDstCheckDone = false
1276+
nrc.initSrcDstCheckDone.Store(false)
12771277
nrc.routeSyncer = routes.NewRouteSyncer(kubeRouterConfig.InjectedRoutesSyncPeriod, kubeRouterConfig.MetricsEnabled)
12781278

12791279
nrc.bgpHoldtime = kubeRouterConfig.BGPHoldTime.Seconds()
@@ -1307,7 +1307,7 @@ func NewNetworkRoutingController(clientset kubernetes.Interface,
13071307
}
13081308

13091309
// let's start with assumption we have necessary IAM creds to access EC2 api
1310-
nrc.ec2IamAuthorized = true
1310+
nrc.ec2IamAuthorized.Store(true)
13111311

13121312
if nrc.enableCNI {
13131313
nrc.cniConfFile = os.Getenv("KUBE_ROUTER_CNI_CONF_FILE")

pkg/controllers/routing/network_routes_controller_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3032,3 +3032,39 @@ func waitForListerWithTimeout(lister cache.Indexer, timeout time.Duration, t *te
30323032
}
30333033
}
30343034
}
3035+
3036+
func TestAtomicBoolFieldsNoConcurrentDataRace(t *testing.T) {
3037+
// Verify that bgpServerStarted, initSrcDstCheckDone, and ec2IamAuthorized
3038+
// can be read and written concurrently without a data race.
3039+
// This test is only meaningful when run with -race.
3040+
nrc := &NetworkRoutingController{}
3041+
nrc.bgpServerStarted.Store(false)
3042+
nrc.initSrcDstCheckDone.Store(false)
3043+
nrc.ec2IamAuthorized.Store(true)
3044+
3045+
var wg sync.WaitGroup
3046+
3047+
// Writers
3048+
wg.Add(1)
3049+
go func() {
3050+
defer wg.Done()
3051+
for i := 0; i < 1000; i++ {
3052+
nrc.bgpServerStarted.Store(i%2 == 0)
3053+
nrc.initSrcDstCheckDone.Store(i%2 == 0)
3054+
nrc.ec2IamAuthorized.Store(i%2 != 0)
3055+
}
3056+
}()
3057+
3058+
// Readers
3059+
wg.Add(1)
3060+
go func() {
3061+
defer wg.Done()
3062+
for i := 0; i < 1000; i++ {
3063+
_ = nrc.bgpServerStarted.Load()
3064+
_ = nrc.initSrcDstCheckDone.Load()
3065+
_ = nrc.ec2IamAuthorized.Load()
3066+
}
3067+
}()
3068+
3069+
wg.Wait()
3070+
}

0 commit comments

Comments
 (0)