Skip to content

Commit d6f9f31

Browse files
tompntnmurali-reddy
tompntn
authored and
murali-reddy
committed
Fix: Send BGP Withdrawals for Service VIPs Upon Service Deletion (#756)
* Refactor: seperate fetching service VIPs from advertise/withdrawal decision * Refactor: simplify advertise/withdrawal logic * Pass svcDeleted param to getVIPsForService * Don't advertise VIPs from deleted services * Test for withdrawing VIPs from deleted service * Refactor: use explicit handleServiceDelete functions
1 parent 3aacd48 commit d6f9f31

File tree

2 files changed

+91
-22
lines changed

2 files changed

+91
-22
lines changed

‎pkg/controllers/routing/ecmp_vip.go‎

Lines changed: 55 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,13 @@ import (
66
"strconv"
77
"time"
88

9+
"strings"
10+
911
"github.com/golang/glog"
1012
"github.com/osrg/gobgp/packet/bgp"
1113
"github.com/osrg/gobgp/table"
1214
v1core "k8s.io/api/core/v1"
1315
"k8s.io/client-go/tools/cache"
14-
"strings"
1516
)
1617

1718
// bgpAdvertiseVIP advertises the service vip (cluster ip or load balancer ip or external IP) the configured peers
@@ -103,13 +104,36 @@ func (nrc *NetworkRoutingController) handleServiceUpdate(svc *v1core.Service) {
103104
nrc.withdrawVIPs(toWithdraw)
104105
}
105106

107+
func (nrc *NetworkRoutingController) handleServiceDelete(svc *v1core.Service) {
108+
109+
if !nrc.bgpServerStarted {
110+
glog.V(3).Infof("Skipping update to service: %s/%s, controller still performing bootup full-sync", svc.Namespace, svc.Name)
111+
return
112+
}
113+
114+
err := nrc.AddPolicies()
115+
if err != nil {
116+
glog.Errorf("Error adding BGP policies: %s", err.Error())
117+
}
118+
119+
nrc.withdrawVIPs(nrc.getAllVIPsForService(svc))
120+
121+
}
122+
106123
func (nrc *NetworkRoutingController) tryHandleServiceUpdate(obj interface{}, logMsgFormat string) {
107124
if svc := getServiceObject(obj); svc != nil {
108125
glog.V(1).Infof(logMsgFormat, svc.Namespace, svc.Name)
109126
nrc.handleServiceUpdate(svc)
110127
}
111128
}
112129

130+
func (nrc *NetworkRoutingController) tryHandleServiceDelete(obj interface{}, logMsgFormat string) {
131+
if svc := getServiceObject(obj); svc != nil {
132+
glog.V(1).Infof(logMsgFormat, svc.Namespace, svc.Name)
133+
nrc.handleServiceDelete(svc)
134+
}
135+
}
136+
113137
// OnServiceCreate handles new service create event from the kubernetes API server
114138
func (nrc *NetworkRoutingController) OnServiceCreate(obj interface{}) {
115139
nrc.tryHandleServiceUpdate(obj, "Received new service: %s/%s from watch API")
@@ -141,7 +165,7 @@ func getMissingPrevGen(old, new []string) (withdrawIPs []string) {
141165

142166
// OnServiceDelete handles the service delete updates from the kubernetes API server
143167
func (nrc *NetworkRoutingController) OnServiceDelete(obj interface{}) {
144-
nrc.tryHandleServiceUpdate(obj, "Received event to delete service: %s/%s from watch API")
168+
nrc.tryHandleServiceDelete(obj, "Received event to delete service: %s/%s from watch API")
145169
}
146170

147171
func (nrc *NetworkRoutingController) newEndpointsEventHandler() cache.ResourceEventHandler {
@@ -306,24 +330,38 @@ func (nrc *NetworkRoutingController) shouldAdvertiseService(svc *v1core.Service,
306330
}
307331

308332
func (nrc *NetworkRoutingController) getVIPsForService(svc *v1core.Service, onlyActiveEndpoints bool) ([]string, []string, error) {
309-
ipList := make([]string, 0)
310-
var err error
311-
312-
nodeHasEndpoints := true
313-
if onlyActiveEndpoints {
314-
_, isLocal := svc.Annotations[svcLocalAnnotation]
315-
if isLocal || svc.Spec.ExternalTrafficPolicy == v1core.ServiceExternalTrafficPolicyTypeLocal {
316-
nodeHasEndpoints, err = nrc.nodeHasEndpointsForService(svc)
317-
if err != nil {
318-
return nil, nil, err
319-
}
333+
334+
advertise := true
335+
336+
_, hasLocalAnnotation := svc.Annotations[svcLocalAnnotation]
337+
hasLocalTrafficPolicy := svc.Spec.ExternalTrafficPolicy == v1core.ServiceExternalTrafficPolicyTypeLocal
338+
isLocal := hasLocalAnnotation || hasLocalTrafficPolicy
339+
340+
if onlyActiveEndpoints && isLocal {
341+
var err error
342+
advertise, err = nrc.nodeHasEndpointsForService(svc)
343+
if err != nil {
344+
return nil, nil, err
320345
}
321346
}
322347

348+
ipList := nrc.getAllVIPsForService(svc)
349+
350+
if !advertise {
351+
return nil, ipList, nil
352+
}
353+
354+
return ipList, nil, nil
355+
}
356+
357+
func (nrc *NetworkRoutingController) getAllVIPsForService(svc *v1core.Service) []string {
358+
359+
ipList := make([]string, 0)
360+
323361
if nrc.shouldAdvertiseService(svc, svcAdvertiseClusterAnnotation, nrc.advertiseClusterIP) {
324-
clusterIp := nrc.getClusterIp(svc)
325-
if clusterIp != "" {
326-
ipList = append(ipList, clusterIp)
362+
clusterIP := nrc.getClusterIp(svc)
363+
if clusterIP != "" {
364+
ipList = append(ipList, clusterIP)
327365
}
328366
}
329367

@@ -338,11 +376,8 @@ func (nrc *NetworkRoutingController) getVIPsForService(svc *v1core.Service, only
338376
ipList = append(ipList, nrc.getLoadBalancerIps(svc)...)
339377
}
340378

341-
if !nodeHasEndpoints {
342-
return nil, ipList, nil
343-
}
379+
return ipList
344380

345-
return ipList, nil, nil
346381
}
347382

348383
func isEndpointsForLeaderElection(ep *v1core.Endpoints) bool {

‎pkg/controllers/routing/ecmp_vip_test.go‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ func Equal(a, b []string) bool {
2424
type ServiceAdvertisedIPs struct {
2525
service *v1core.Service
2626
advertisedIPs []string
27+
withdrawnIPs []string
2728
annotations map[string]string
2829
}
2930

@@ -97,21 +98,25 @@ func Test_getVIPsForService(t *testing.T) {
9798
{
9899
services["cluster"],
99100
[]string{"10.0.0.1"},
101+
[]string{},
100102
nil,
101103
},
102104
{
103105
services["external"],
104106
[]string{"10.0.0.1", "1.1.1.1"},
107+
[]string{},
105108
nil,
106109
},
107110
{
108111
services["nodeport"],
109112
[]string{"10.0.0.1", "1.1.1.1"},
113+
[]string{},
110114
nil,
111115
},
112116
{
113117
services["loadbalancer"],
114118
[]string{"10.0.0.1", "10.0.255.1", "10.0.255.2"},
119+
[]string{},
115120
nil,
116121
},
117122
},
@@ -123,21 +128,25 @@ func Test_getVIPsForService(t *testing.T) {
123128
{
124129
services["cluster"],
125130
[]string{},
131+
[]string{},
126132
nil,
127133
},
128134
{
129135
services["external"],
130136
[]string{},
137+
[]string{},
131138
nil,
132139
},
133140
{
134141
services["nodeport"],
135142
[]string{},
143+
[]string{},
136144
nil,
137145
},
138146
{
139147
services["loadbalancer"],
140148
[]string{},
149+
[]string{},
141150
nil,
142151
},
143152
},
@@ -149,21 +158,25 @@ func Test_getVIPsForService(t *testing.T) {
149158
{
150159
services["cluster"],
151160
[]string{"10.0.0.1"},
161+
[]string{},
152162
nil,
153163
},
154164
{
155165
services["external"],
156166
[]string{"10.0.0.1"},
167+
[]string{},
157168
nil,
158169
},
159170
{
160171
services["nodeport"],
161172
[]string{"10.0.0.1"},
173+
[]string{},
162174
nil,
163175
},
164176
{
165177
services["loadbalancer"],
166178
[]string{"10.0.0.1"},
179+
[]string{},
167180
nil,
168181
},
169182
},
@@ -175,21 +188,25 @@ func Test_getVIPsForService(t *testing.T) {
175188
{
176189
services["cluster"],
177190
[]string{},
191+
[]string{},
178192
nil,
179193
},
180194
{
181195
services["external"],
182196
[]string{"1.1.1.1"},
197+
[]string{},
183198
nil,
184199
},
185200
{
186201
services["nodeport"],
187202
[]string{"1.1.1.1"},
203+
[]string{},
188204
nil,
189205
},
190206
{
191207
services["loadbalancer"],
192208
[]string{},
209+
[]string{},
193210
nil,
194211
},
195212
},
@@ -201,21 +218,25 @@ func Test_getVIPsForService(t *testing.T) {
201218
{
202219
services["cluster"],
203220
[]string{},
221+
[]string{},
204222
nil,
205223
},
206224
{
207225
services["external"],
208226
[]string{},
227+
[]string{},
209228
nil,
210229
},
211230
{
212231
services["nodeport"],
213232
[]string{},
233+
[]string{},
214234
nil,
215235
},
216236
{
217237
services["loadbalancer"],
218238
[]string{"10.0.255.1", "10.0.255.2"},
239+
[]string{},
219240
nil,
220241
},
221242
},
@@ -227,6 +248,7 @@ func Test_getVIPsForService(t *testing.T) {
227248
{
228249
services["cluster"],
229250
[]string{"10.0.0.1"},
251+
[]string{},
230252
map[string]string{
231253
svcAdvertiseClusterAnnotation: "true",
232254
svcAdvertiseExternalAnnotation: "true",
@@ -236,6 +258,7 @@ func Test_getVIPsForService(t *testing.T) {
236258
{
237259
services["external"],
238260
[]string{"10.0.0.1", "1.1.1.1"},
261+
[]string{},
239262
map[string]string{
240263
svcAdvertiseClusterAnnotation: "true",
241264
svcAdvertiseExternalAnnotation: "true",
@@ -245,6 +268,7 @@ func Test_getVIPsForService(t *testing.T) {
245268
{
246269
services["nodeport"],
247270
[]string{"10.0.0.1", "1.1.1.1"},
271+
[]string{},
248272
map[string]string{
249273
svcAdvertiseClusterAnnotation: "true",
250274
svcAdvertiseExternalAnnotation: "true",
@@ -254,6 +278,7 @@ func Test_getVIPsForService(t *testing.T) {
254278
{
255279
services["loadbalancer"],
256280
[]string{"10.0.0.1", "10.0.255.1", "10.0.255.2"},
281+
[]string{},
257282
map[string]string{
258283
svcAdvertiseClusterAnnotation: "true",
259284
svcAdvertiseExternalAnnotation: "true",
@@ -264,6 +289,7 @@ func Test_getVIPsForService(t *testing.T) {
264289
// Special case to test svcAdvertiseLoadBalancerAnnotation vs legacy svcSkipLbIpsAnnotation
265290
services["loadbalancer"],
266291
[]string{"10.0.0.1"},
292+
[]string{},
267293
map[string]string{
268294
svcAdvertiseClusterAnnotation: "true",
269295
svcAdvertiseExternalAnnotation: "true",
@@ -280,6 +306,7 @@ func Test_getVIPsForService(t *testing.T) {
280306
{
281307
services["cluster"],
282308
[]string{},
309+
[]string{},
283310
map[string]string{
284311
svcAdvertiseClusterAnnotation: "false",
285312
svcAdvertiseExternalAnnotation: "false",
@@ -289,6 +316,7 @@ func Test_getVIPsForService(t *testing.T) {
289316
{
290317
services["external"],
291318
[]string{},
319+
[]string{},
292320
map[string]string{
293321
svcAdvertiseClusterAnnotation: "false",
294322
svcAdvertiseExternalAnnotation: "false",
@@ -298,6 +326,7 @@ func Test_getVIPsForService(t *testing.T) {
298326
{
299327
services["nodeport"],
300328
[]string{},
329+
[]string{},
301330
map[string]string{
302331
svcAdvertiseClusterAnnotation: "false",
303332
svcAdvertiseExternalAnnotation: "false",
@@ -307,6 +336,7 @@ func Test_getVIPsForService(t *testing.T) {
307336
{
308337
services["loadbalancer"],
309338
[]string{},
339+
[]string{},
310340
map[string]string{
311341
svcAdvertiseClusterAnnotation: "false",
312342
svcAdvertiseExternalAnnotation: "false",
@@ -331,10 +361,14 @@ func Test_getVIPsForService(t *testing.T) {
331361
serviceAdvertisedIP.service.ObjectMeta.Annotations = serviceAdvertisedIP.annotations
332362
}
333363
svc, _ := clientset.CoreV1().Services("default").Create(serviceAdvertisedIP.service)
334-
advertisedIPs, _, _ := nrc.getVIPsForService(svc, false)
364+
advertisedIPs, withdrawnIPs, _ := nrc.getVIPsForService(svc, false)
335365
t.Logf("AdvertisedIPs: %v\n", advertisedIPs)
366+
t.Logf("WithdrawnIPs: %v\n", withdrawnIPs)
336367
if !Equal(serviceAdvertisedIP.advertisedIPs, advertisedIPs) {
337-
t.Errorf("Advertised IPs are incorrect, got: %v, want: %v.", serviceAdvertisedIP.advertisedIPs, advertisedIPs)
368+
t.Errorf("Advertised IPs are incorrect, got: %v, want: %v.", advertisedIPs, serviceAdvertisedIP.advertisedIPs)
369+
}
370+
if !Equal(serviceAdvertisedIP.withdrawnIPs, withdrawnIPs) {
371+
t.Errorf("Withdrawn IPs are incorrect, got: %v, want: %v.", withdrawnIPs, serviceAdvertisedIP.withdrawnIPs)
338372
}
339373
}
340374
})

0 commit comments

Comments
 (0)