Skip to content

Commit 2427888

Browse files
jaer-tsunjaer-tsun
authored
Lock update operations to prevent race in between updates. (#536)
* Lock update operations to prevent race in between updates. * fixing tests * fixing nwpolicy files
1 parent 79d8758 commit 2427888

File tree

7 files changed

+67
-41
lines changed

7 files changed

+67
-41
lines changed

‎npm/namespace.go‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func (ns *namespace) policyExists(npObj *networkingv1.NetworkPolicy) bool {
5555
// InitAllNsList syncs all-namespace ipset list.
5656
func (npMgr *NetworkPolicyManager) InitAllNsList() error {
5757
allNs := npMgr.nsMap[util.KubeAllNamespacesFlag]
58-
for ns:= range npMgr.nsMap {
58+
for ns := range npMgr.nsMap {
5959
if ns == util.KubeAllNamespacesFlag {
6060
continue
6161
}
@@ -88,12 +88,9 @@ func (npMgr *NetworkPolicyManager) UninitAllNsList() error {
8888

8989
// AddNamespace handles adding namespace to ipset.
9090
func (npMgr *NetworkPolicyManager) AddNamespace(nsObj *corev1.Namespace) error {
91-
npMgr.Lock()
92-
defer npMgr.Unlock()
93-
9491
var err error
9592

96-
nsName, nsLabel := "ns-" + nsObj.ObjectMeta.Name, nsObj.ObjectMeta.Labels
93+
nsName, nsLabel := "ns-"+nsObj.ObjectMeta.Name, nsObj.ObjectMeta.Labels
9794
log.Printf("NAMESPACE CREATING: [%s/%v]", nsName, nsLabel)
9895

9996
ipsMgr := npMgr.nsMap[util.KubeAllNamespacesFlag].ipsMgr
@@ -139,8 +136,8 @@ func (npMgr *NetworkPolicyManager) AddNamespace(nsObj *corev1.Namespace) error {
139136
func (npMgr *NetworkPolicyManager) UpdateNamespace(oldNsObj *corev1.Namespace, newNsObj *corev1.Namespace) error {
140137
var err error
141138

142-
oldNsNs, oldNsLabel := "ns-" + oldNsObj.ObjectMeta.Name, oldNsObj.ObjectMeta.Labels
143-
newNsNs, newNsLabel := "ns-" + newNsObj.ObjectMeta.Name, newNsObj.ObjectMeta.Labels
139+
oldNsNs, oldNsLabel := "ns-"+oldNsObj.ObjectMeta.Name, oldNsObj.ObjectMeta.Labels
140+
newNsNs, newNsLabel := "ns-"+newNsObj.ObjectMeta.Name, newNsObj.ObjectMeta.Labels
144141
log.Printf(
145142
"NAMESPACE UPDATING:\n old namespace: [%s/%v]\n new namespace: [%s/%v]",
146143
oldNsNs, oldNsLabel, newNsNs, newNsLabel,
@@ -161,12 +158,9 @@ func (npMgr *NetworkPolicyManager) UpdateNamespace(oldNsObj *corev1.Namespace, n
161158

162159
// DeleteNamespace handles deleting namespace from ipset.
163160
func (npMgr *NetworkPolicyManager) DeleteNamespace(nsObj *corev1.Namespace) error {
164-
npMgr.Lock()
165-
defer npMgr.Unlock()
166-
167161
var err error
168162

169-
nsName, nsLabel := "ns-" + nsObj.ObjectMeta.Name, nsObj.ObjectMeta.Labels
163+
nsName, nsLabel := "ns-"+nsObj.ObjectMeta.Name, nsObj.ObjectMeta.Labels
170164
log.Printf("NAMESPACE DELETING: [%s/%v]", nsName, nsLabel)
171165

172166
_, exists := npMgr.nsMap[nsName]

‎npm/namespace_test.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
package npm
44

55
import (
6-
"testing"
76
"os"
7+
"testing"
88

99
"github.com/Azure/azure-container-networking/npm/iptm"
1010

@@ -75,9 +75,11 @@ func TestAddNamespace(t *testing.T) {
7575
},
7676
}
7777

78+
npMgr.Lock()
7879
if err := npMgr.AddNamespace(nsObj); err != nil {
7980
t.Errorf("TestAddNamespace @ npMgr.AddNamespace")
8081
}
82+
npMgr.Unlock()
8183
}
8284

8385
func TestUpdateNamespace(t *testing.T) {
@@ -121,13 +123,15 @@ func TestUpdateNamespace(t *testing.T) {
121123
},
122124
}
123125

126+
npMgr.Lock()
124127
if err := npMgr.AddNamespace(oldNsObj); err != nil {
125128
t.Errorf("TestUpdateNamespace failed @ npMgr.AddNamespace")
126129
}
127130

128131
if err := npMgr.UpdateNamespace(oldNsObj, newNsObj); err != nil {
129132
t.Errorf("TestUpdateNamespace failed @ npMgr.UpdateNamespace")
130133
}
134+
npMgr.Unlock()
131135
}
132136

133137
func TestDeleteNamespace(t *testing.T) {
@@ -162,13 +166,15 @@ func TestDeleteNamespace(t *testing.T) {
162166
},
163167
}
164168

169+
npMgr.Lock()
165170
if err := npMgr.AddNamespace(nsObj); err != nil {
166171
t.Errorf("TestDeleteNamespace @ npMgr.AddNamespace")
167172
}
168173

169174
if err := npMgr.DeleteNamespace(nsObj); err != nil {
170175
t.Errorf("TestDeleteNamespace @ npMgr.DeleteNamespace")
171176
}
177+
npMgr.Unlock()
172178
}
173179

174180
func TestMain(m *testing.M) {

‎npm/npm.go‎

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ type NetworkPolicyManager struct {
4949
isAzureNpmChainCreated bool
5050
isSafeToCleanUpAzureNpmChain bool
5151

52-
clusterState telemetry.ClusterState
53-
version string
52+
clusterState telemetry.ClusterState
53+
version string
5454

5555
serverVersion *version.Info
5656
TelemetryEnabled bool
@@ -93,26 +93,26 @@ func (npMgr *NetworkPolicyManager) SendAiMetrics() {
9393
GetEnvRetryCount: 5,
9494
GetEnvRetryWaitTimeInSecs: 3,
9595
}
96-
97-
th, err = aitelemetry.NewAITelemetry("", aiMetadata, aiConfig)
98-
heartbeat = time.NewTicker(time.Minute * heartbeatIntervalInMinutes).C
96+
97+
th, err = aitelemetry.NewAITelemetry("", aiMetadata, aiConfig)
98+
heartbeat = time.NewTicker(time.Minute * heartbeatIntervalInMinutes).C
9999
customDimensions = map[string]string{"ClusterID": util.GetClusterID(npMgr.nodeName),
100-
"APIServer": npMgr.serverVersion.String()}
100+
"APIServer": npMgr.serverVersion.String()}
101101
podCount = aitelemetry.Metric{
102-
Name: "PodCount",
102+
Name: "PodCount",
103103
CustomDimensions: customDimensions,
104104
}
105105
nsCount = aitelemetry.Metric{
106-
Name: "NsCount",
106+
Name: "NsCount",
107107
CustomDimensions: customDimensions,
108108
}
109109
nwPolicyCount = aitelemetry.Metric{
110-
Name: "NwPolicyCount",
110+
Name: "NwPolicyCount",
111111
CustomDimensions: customDimensions,
112112
}
113113
)
114114

115-
for i := 0; err != nil && i < 5; i++{
115+
for i := 0; err != nil && i < 5; i++ {
116116
log.Logf("Failed to init AppInsights with err: %+v", err)
117117
time.Sleep(time.Minute * 5)
118118
th, err = aitelemetry.NewAITelemetry("", aiMetadata, aiConfig)
@@ -123,7 +123,7 @@ func (npMgr *NetworkPolicyManager) SendAiMetrics() {
123123

124124
defer th.Close(10)
125125

126-
for {
126+
for {
127127
<-heartbeat
128128
clusterState := npMgr.GetClusterState()
129129
podCount.Value = float64(clusterState.PodCount)
@@ -206,7 +206,7 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in
206206
err error
207207
)
208208

209-
for ticker, start := time.NewTicker(1 * time.Second).C, time.Now(); time.Since(start) < time.Minute * 1; {
209+
for ticker, start := time.NewTicker(1*time.Second).C, time.Now(); time.Since(start) < time.Minute*1; {
210210
<-ticker
211211
serverVersion, err = clientset.ServerVersion()
212212
if err == nil {
@@ -239,7 +239,7 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in
239239
NsCount: 0,
240240
NwPolicyCount: 0,
241241
},
242-
version: npmVersion,
242+
version: npmVersion,
243243
serverVersion: serverVersion,
244244
TelemetryEnabled: true,
245245
}
@@ -257,13 +257,19 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in
257257
// Pod event handlers
258258
cache.ResourceEventHandlerFuncs{
259259
AddFunc: func(obj interface{}) {
260+
npMgr.Lock()
260261
npMgr.AddPod(obj.(*corev1.Pod))
262+
npMgr.Unlock()
261263
},
262264
UpdateFunc: func(old, new interface{}) {
265+
npMgr.Lock()
263266
npMgr.UpdatePod(old.(*corev1.Pod), new.(*corev1.Pod))
267+
npMgr.Unlock()
264268
},
265269
DeleteFunc: func(obj interface{}) {
270+
npMgr.Lock()
266271
npMgr.DeletePod(obj.(*corev1.Pod))
272+
npMgr.Unlock()
267273
},
268274
},
269275
)
@@ -272,13 +278,19 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in
272278
// Namespace event handlers
273279
cache.ResourceEventHandlerFuncs{
274280
AddFunc: func(obj interface{}) {
281+
npMgr.Lock()
275282
npMgr.AddNamespace(obj.(*corev1.Namespace))
283+
npMgr.Unlock()
276284
},
277285
UpdateFunc: func(old, new interface{}) {
286+
npMgr.Lock()
278287
npMgr.UpdateNamespace(old.(*corev1.Namespace), new.(*corev1.Namespace))
288+
npMgr.Unlock()
279289
},
280290
DeleteFunc: func(obj interface{}) {
291+
npMgr.Lock()
281292
npMgr.DeleteNamespace(obj.(*corev1.Namespace))
293+
npMgr.Unlock()
282294
},
283295
},
284296
)
@@ -287,13 +299,19 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in
287299
// Network policy event handlers
288300
cache.ResourceEventHandlerFuncs{
289301
AddFunc: func(obj interface{}) {
302+
npMgr.Lock()
290303
npMgr.AddNetworkPolicy(obj.(*networkingv1.NetworkPolicy))
304+
npMgr.Unlock()
291305
},
292306
UpdateFunc: func(old, new interface{}) {
307+
npMgr.Lock()
293308
npMgr.UpdateNetworkPolicy(old.(*networkingv1.NetworkPolicy), new.(*networkingv1.NetworkPolicy))
309+
npMgr.Unlock()
294310
},
295311
DeleteFunc: func(obj interface{}) {
312+
npMgr.Lock()
296313
npMgr.DeleteNetworkPolicy(obj.(*networkingv1.NetworkPolicy))
314+
npMgr.Unlock()
297315
},
298316
},
299317
)

‎npm/nwpolicy.go‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,6 @@ func (npMgr *NetworkPolicyManager) canCleanUpNpmChains() bool {
2525

2626
// AddNetworkPolicy handles adding network policy to iptables.
2727
func (npMgr *NetworkPolicyManager) AddNetworkPolicy(npObj *networkingv1.NetworkPolicy) error {
28-
npMgr.Lock()
29-
defer npMgr.Unlock()
30-
3128
var (
3229
err error
3330
ns *namespace
@@ -75,9 +72,7 @@ func (npMgr *NetworkPolicyManager) AddNetworkPolicy(npObj *networkingv1.NetworkP
7572
log.Printf("Error adding policy %s to %s", npName, oldPolicy.ObjectMeta.Name)
7673
}
7774
npMgr.isSafeToCleanUpAzureNpmChain = false
78-
npMgr.Unlock()
7975
npMgr.DeleteNetworkPolicy(oldPolicy)
80-
npMgr.Lock()
8176
npMgr.isSafeToCleanUpAzureNpmChain = true
8277
} else {
8378
ns.processedNpMap[hashedSelector] = npObj
@@ -140,9 +135,6 @@ func (npMgr *NetworkPolicyManager) UpdateNetworkPolicy(oldNpObj *networkingv1.Ne
140135

141136
// DeleteNetworkPolicy handles deleting network policy from iptables.
142137
func (npMgr *NetworkPolicyManager) DeleteNetworkPolicy(npObj *networkingv1.NetworkPolicy) error {
143-
npMgr.Lock()
144-
defer npMgr.Unlock()
145-
146138
var (
147139
err error
148140
ns *namespace

‎npm/nwpolicy_test.go‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,11 @@ func TestAddNetworkPolicy(t *testing.T) {
6161
},
6262
}
6363

64+
npMgr.Lock()
6465
if err := npMgr.AddNamespace(nsObj); err != nil {
6566
t.Errorf("TestAddNetworkPolicy @ npMgr.AddNamespace")
6667
}
68+
npMgr.Unlock()
6769

6870
tcp := corev1.ProtocolTCP
6971
port8000 := intstr.FromInt(8000)
@@ -82,17 +84,19 @@ func TestAddNetworkPolicy(t *testing.T) {
8284
}},
8385
Ports: []networkingv1.NetworkPolicyPort{{
8486
Protocol: &tcp,
85-
Port: &port8000,
87+
Port: &port8000,
8688
}},
8789
},
8890
},
8991
},
9092
}
9193

94+
npMgr.Lock()
9295
if err := npMgr.AddNetworkPolicy(allowIngress); err != nil {
9396
t.Errorf("TestAddNetworkPolicy failed @ allowIngress AddNetworkPolicy")
9497
t.Errorf("Error: %v", err)
9598
}
99+
npMgr.Unlock()
96100

97101
allowEgress := &networkingv1.NetworkPolicy{
98102
ObjectMeta: metav1.ObjectMeta{
@@ -109,17 +113,19 @@ func TestAddNetworkPolicy(t *testing.T) {
109113
}},
110114
Ports: []networkingv1.NetworkPolicyPort{{
111115
Protocol: &tcp,
112-
Port: &port8000,
116+
Port: &port8000,
113117
}},
114118
},
115119
},
116120
},
117121
}
118122

123+
npMgr.Lock()
119124
if err := npMgr.AddNetworkPolicy(allowEgress); err != nil {
120125
t.Errorf("TestAddNetworkPolicy failed @ allowEgress AddNetworkPolicy")
121126
t.Errorf("Error: %v", err)
122127
}
128+
npMgr.Unlock()
123129
}
124130

125131
func TestUpdateNetworkPolicy(t *testing.T) {
@@ -168,9 +174,11 @@ func TestUpdateNetworkPolicy(t *testing.T) {
168174
},
169175
}
170176

177+
npMgr.Lock()
171178
if err := npMgr.AddNamespace(nsObj); err != nil {
172179
t.Errorf("TestUpdateNetworkPolicy @ npMgr.AddNamespace")
173180
}
181+
npMgr.Unlock()
174182

175183
tcp, udp := corev1.ProtocolTCP, corev1.ProtocolUDP
176184
allowIngress := &networkingv1.NetworkPolicy{
@@ -221,13 +229,15 @@ func TestUpdateNetworkPolicy(t *testing.T) {
221229
},
222230
}
223231

232+
npMgr.Lock()
224233
if err := npMgr.AddNetworkPolicy(allowIngress); err != nil {
225234
t.Errorf("TestUpdateNetworkPolicy failed @ AddNetworkPolicy")
226235
}
227236

228237
if err := npMgr.UpdateNetworkPolicy(allowIngress, allowEgress); err != nil {
229238
t.Errorf("TestUpdateNetworkPolicy failed @ UpdateNetworkPolicy")
230239
}
240+
npMgr.Unlock()
231241
}
232242

233243
func TestDeleteNetworkPolicy(t *testing.T) {
@@ -276,9 +286,11 @@ func TestDeleteNetworkPolicy(t *testing.T) {
276286
},
277287
}
278288

289+
npMgr.Lock()
279290
if err := npMgr.AddNamespace(nsObj); err != nil {
280291
t.Errorf("TestDeleteNetworkPolicy @ npMgr.AddNamespace")
281292
}
293+
npMgr.Unlock()
282294

283295
tcp := corev1.ProtocolTCP
284296
allow := &networkingv1.NetworkPolicy{
@@ -305,11 +317,13 @@ func TestDeleteNetworkPolicy(t *testing.T) {
305317
},
306318
}
307319

320+
npMgr.Lock()
308321
if err := npMgr.AddNetworkPolicy(allow); err != nil {
309322
t.Errorf("TestAddNetworkPolicy failed @ AddNetworkPolicy")
310323
}
311324

312325
if err := npMgr.DeleteNetworkPolicy(allow); err != nil {
313326
t.Errorf("TestDeleteNetworkPolicy failed @ DeleteNetworkPolicy")
314327
}
328+
npMgr.Unlock()
315329
}

‎npm/pod.go‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,6 @@ func isSystemPod(podObj *corev1.Pod) bool {
1919

2020
// AddPod handles adding pod ip to its label's ipset.
2121
func (npMgr *NetworkPolicyManager) AddPod(podObj *corev1.Pod) error {
22-
npMgr.Lock()
23-
defer npMgr.Unlock()
24-
2522
if !isValidPod(podObj) {
2623
return nil
2724
}
@@ -113,9 +110,6 @@ func (npMgr *NetworkPolicyManager) UpdatePod(oldPodObj, newPodObj *corev1.Pod) e
113110

114111
// DeletePod handles deleting pod from its label's ipset.
115112
func (npMgr *NetworkPolicyManager) DeletePod(podObj *corev1.Pod) error {
116-
npMgr.Lock()
117-
defer npMgr.Unlock()
118-
119113
if !isValidPod(podObj) {
120114
return nil
121115
}

0 commit comments

Comments
 (0)