Skip to content

Commit c5816bf

Browse files
kristenjacobsowainlewis
kristenjacobs
authored and
owainlewis
committed
Add security rules for a services health check port (#125)
* Add security rules for a services healthcheck port This commit adds the following security list rules for a services healthcheck port: 1. Egress from the load balancers sec-lists on the required default/custom port. 2. Ingress to the workers sec-lists on the required default/custom port. * More healthcheck port security rule updates * Optimisied the construction of the ingress/egress rules w.r.t healthcheck ports
1 parent 0e48873 commit c5816bf

File tree

3 files changed

+199
-63
lines changed

3 files changed

+199
-63
lines changed

‎pkg/oci/load_balancer.go‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,8 @@ func (cp *CloudProvider) updateLoadBalancer(
266266
}
267267

268268
backendPort := uint64(getBackendPort(backendSet.Backends))
269-
err := cp.updateListener(lbOCID, a, backendPort, lbSubnets, nodeSubnets, sslConfigMap, sourceCIDRs)
269+
healthCheckPort := uint64(backendSet.HealthChecker.Port)
270+
err := cp.updateListener(lbOCID, a, backendPort, healthCheckPort, lbSubnets, nodeSubnets, sslConfigMap, sourceCIDRs)
270271
if err != nil {
271272
return fmt.Errorf("error updating Listener: %v", err)
272273
}
@@ -286,10 +287,11 @@ func (cp *CloudProvider) updateBackendSet(lbOCID string, action *BackendSetActio
286287
glog.V(2).Infof("Applying %q action on backend set `%s` for lb `%s`", action.Type(), be.Name, lbOCID)
287288

288289
backendPort := uint64(getBackendPort(be.Backends))
290+
healthCheckPort := uint64(be.HealthChecker.Port)
289291

290292
switch action.Type() {
291293
case Create:
292-
err = cp.securityListManager.Update(lbSubnets, nodeSubnets, sourceCIDRs, listenerPort, backendPort)
294+
err = cp.securityListManager.Update(lbSubnets, nodeSubnets, sourceCIDRs, listenerPort, backendPort, healthCheckPort)
293295
if err != nil {
294296
return err
295297
}
@@ -305,7 +307,7 @@ func (cp *CloudProvider) updateBackendSet(lbOCID string, action *BackendSetActio
305307
nil, // create opts
306308
)
307309
case Update:
308-
err = cp.securityListManager.Update(lbSubnets, nodeSubnets, sourceCIDRs, listenerPort, backendPort)
310+
err = cp.securityListManager.Update(lbSubnets, nodeSubnets, sourceCIDRs, listenerPort, backendPort, healthCheckPort)
309311
if err != nil {
310312
return err
311313
}
@@ -316,7 +318,7 @@ func (cp *CloudProvider) updateBackendSet(lbOCID string, action *BackendSetActio
316318
Backends: be.Backends,
317319
})
318320
case Delete:
319-
err = cp.securityListManager.Delete(lbSubnets, nodeSubnets, listenerPort, backendPort)
321+
err = cp.securityListManager.Delete(lbSubnets, nodeSubnets, listenerPort, backendPort, healthCheckPort)
320322
if err != nil {
321323
return err
322324
}
@@ -339,6 +341,7 @@ func (cp *CloudProvider) updateBackendSet(lbOCID string, action *BackendSetActio
339341
func (cp *CloudProvider) updateListener(lbOCID string,
340342
action *ListenerAction,
341343
backendPort uint64,
344+
healthCheckPort uint64,
342345
lbSubnets []*baremetal.Subnet,
343346
nodeSubnets []*baremetal.Subnet,
344347
sslConfigMap map[int]*baremetal.SSLConfiguration,
@@ -353,7 +356,7 @@ func (cp *CloudProvider) updateListener(lbOCID string,
353356

354357
switch action.Type() {
355358
case Create:
356-
err = cp.securityListManager.Update(lbSubnets, nodeSubnets, sourceCIDRs, listenerPort, backendPort)
359+
err = cp.securityListManager.Update(lbSubnets, nodeSubnets, sourceCIDRs, listenerPort, backendPort, healthCheckPort)
357360
if err != nil {
358361
return err
359362
}
@@ -368,7 +371,7 @@ func (cp *CloudProvider) updateListener(lbOCID string,
368371
nil, // create opts
369372
)
370373
case Update:
371-
err = cp.securityListManager.Update(lbSubnets, nodeSubnets, sourceCIDRs, listenerPort, backendPort)
374+
err = cp.securityListManager.Update(lbSubnets, nodeSubnets, sourceCIDRs, listenerPort, backendPort, healthCheckPort)
372375
if err != nil {
373376
return err
374377
}
@@ -380,7 +383,7 @@ func (cp *CloudProvider) updateListener(lbOCID string,
380383
SSLConfig: l.SSLConfig,
381384
})
382385
case Delete:
383-
err = cp.securityListManager.Delete(lbSubnets, nodeSubnets, listenerPort, backendPort)
386+
err = cp.securityListManager.Delete(lbSubnets, nodeSubnets, listenerPort, backendPort, healthCheckPort)
384387
if err != nil {
385388
return err
386389
}
@@ -486,10 +489,12 @@ func (cp *CloudProvider) EnsureLoadBalancerDeleted(clusterName string, service *
486489
for _, listener := range spec.GetListeners(sslConfigMap) {
487490
glog.V(4).Infof("Deleting security rules for listener `%s` for load balancer `%s`", listener.Name, lb.ID)
488491

489-
backends := spec.GetBackendSets()[listener.DefaultBackendSetName].Backends
492+
backendSet := spec.GetBackendSets()[listener.DefaultBackendSetName]
493+
backends := backendSet.Backends
490494
backendPort := uint64(getBackendPort(backends))
495+
healthCheckPort := uint64(backendSet.HealthChecker.Port)
491496

492-
err := cp.securityListManager.Delete(lbSubnets, nodeSubnets, uint64(listener.Port), backendPort)
497+
err := cp.securityListManager.Delete(lbSubnets, nodeSubnets, uint64(listener.Port), backendPort, healthCheckPort)
493498
if err != nil {
494499
return fmt.Errorf("delete security rules for listener %s: %v", listener.Name, err)
495500
}

‎pkg/oci/load_balancer_security_lists.go‎

Lines changed: 108 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
"k8s.io/apimachinery/pkg/labels"
2727
"k8s.io/apimachinery/pkg/util/sets"
2828
listersv1 "k8s.io/client-go/listers/core/v1"
29+
apiservice "k8s.io/kubernetes/pkg/api/v1/service"
2930
)
3031

3132
const (
@@ -45,13 +46,13 @@ type securityListManager interface {
4546
// from LB subnets to backend subnets on the backend port
4647
// Egress rules added:
4748
// from LB subnets to backend subnets on the backend port
48-
Update(lbSubnets []*baremetal.Subnet, backendSubnets []*baremetal.Subnet, sourceCIDRs []string, listenerPort uint64, backendPort uint64) error
49+
Update(lbSubnets []*baremetal.Subnet, backendSubnets []*baremetal.Subnet, sourceCIDRs []string, listenerPort uint64, backendPort uint64, healthCheckPort uint64) error
4950
// Delete the security list rules associated with the listener & backends.
5051
//
5152
// If the listener is nil, then only the egress rules from the LB's to the backends and the
5253
// ingress rules from the LB's to the backends will be cleaned up.
5354
// If the listener is not nil, then the ingress rules to the LB's will be cleaned up.
54-
Delete(lbSubnets []*baremetal.Subnet, backendSubnets []*baremetal.Subnet, listenerPort uint64, backendPort uint64) error
55+
Delete(lbSubnets []*baremetal.Subnet, backendSubnets []*baremetal.Subnet, listenerPort uint64, backendPort uint64, healthCheckPort uint64) error
5556
}
5657

5758
type securityListManagerImpl struct {
@@ -71,42 +72,45 @@ func (s *securityListManagerImpl) Update(
7172
backendSubnets []*baremetal.Subnet,
7273
sourceCIDRs []string,
7374
listenerPort uint64,
74-
backendPort uint64) error {
75+
backendPort uint64,
76+
healthCheckPort uint64) error {
7577

76-
err := s.updateLoadBalancerRules(lbSubnets, backendSubnets, sourceCIDRs, listenerPort, backendPort)
78+
err := s.updateLoadBalancerRules(lbSubnets, backendSubnets, sourceCIDRs, listenerPort, backendPort, healthCheckPort)
7779
if err != nil {
7880
return err
7981
}
8082

81-
return s.updateBackendRules(lbSubnets, backendSubnets, backendPort)
83+
return s.updateBackendRules(lbSubnets, backendSubnets, backendPort, healthCheckPort)
8284
}
8385

8486
func (s *securityListManagerImpl) Delete(
8587
lbSubnets []*baremetal.Subnet,
8688
backendSubnets []*baremetal.Subnet,
8789
listenerPort uint64,
8890
backendPort uint64,
89-
) error {
91+
healthCheckPort uint64) error {
92+
9093
noSubnets := []*baremetal.Subnet{}
9194
noSourceCIDRs := []string{}
9295

93-
err := s.updateLoadBalancerRules(lbSubnets, noSubnets, noSourceCIDRs, listenerPort, backendPort)
96+
err := s.updateLoadBalancerRules(lbSubnets, noSubnets, noSourceCIDRs, listenerPort, backendPort, healthCheckPort)
9497
if err != nil {
9598
return err
9699
}
97100

98-
return s.updateBackendRules(noSubnets, backendSubnets, backendPort)
101+
return s.updateBackendRules(noSubnets, backendSubnets, backendPort, healthCheckPort)
99102
}
100103

101104
// updateBackendRules handles adding ingress rules to the backend subnets from the load balancer subnets.
102-
func (s *securityListManagerImpl) updateBackendRules(lbSubnets []*baremetal.Subnet, nodeSubnets []*baremetal.Subnet, backendPort uint64) error {
105+
func (s *securityListManagerImpl) updateBackendRules(lbSubnets []*baremetal.Subnet, nodeSubnets []*baremetal.Subnet, backendPort uint64, healthCheckPort uint64) error {
103106
for _, subnet := range nodeSubnets {
104107
secList, err := s.client.GetDefaultSecurityList(subnet)
105108
if err != nil {
106109
return fmt.Errorf("get security list for subnet `%s`: %v", subnet.ID, err)
107110
}
108111

109-
ingressRules := getNodeIngressRules(secList, lbSubnets, backendPort)
112+
ingressRules := getNodeIngressRules(secList.IngressSecurityRules, lbSubnets, backendPort, s.serviceLister)
113+
ingressRules = getNodeIngressRules(ingressRules, lbSubnets, healthCheckPort, s.serviceLister)
110114

111115
if !securityListRulesChanged(secList, ingressRules, secList.EgressSecurityRules) {
112116
glog.V(4).Infof("No changes for node subnet security list `%s`", secList.ID)
@@ -124,18 +128,19 @@ func (s *securityListManagerImpl) updateBackendRules(lbSubnets []*baremetal.Subn
124128

125129
// updateLoadBalancerRules handles updating the ingress and egress rules for the load balance subnets.
126130
// If the listener is nil, then only egress rules from the load balancer to the backend subnets will be checked.
127-
func (s *securityListManagerImpl) updateLoadBalancerRules(lbSubnets []*baremetal.Subnet, nodeSubnets []*baremetal.Subnet, sourceCIDRs []string, listenerPort uint64, backendPort uint64) error {
131+
func (s *securityListManagerImpl) updateLoadBalancerRules(lbSubnets []*baremetal.Subnet, nodeSubnets []*baremetal.Subnet, sourceCIDRs []string, listenerPort uint64, backendPort uint64, healthCheckPort uint64) error {
128132
for _, lbSubnet := range lbSubnets {
129133
lbSecurityList, err := s.client.GetDefaultSecurityList(lbSubnet)
130134
if err != nil {
131135
return fmt.Errorf("get lb security list for subnet `%s`: %v", lbSubnet.ID, err)
132136
}
133137

134-
lbEgressRules := getLoadBalancerEgressRules(lbSecurityList, nodeSubnets, backendPort)
138+
lbEgressRules := getLoadBalancerEgressRules(lbSecurityList.EgressSecurityRules, nodeSubnets, backendPort, s.serviceLister)
139+
lbEgressRules = getLoadBalancerEgressRules(lbEgressRules, nodeSubnets, healthCheckPort, s.serviceLister)
135140

136141
lbIngressRules := lbSecurityList.IngressSecurityRules
137142
if listenerPort != 0 {
138-
lbIngressRules = getLoadBalancerIngressRules(lbSecurityList, sourceCIDRs, listenerPort, s.serviceLister)
143+
lbIngressRules = getLoadBalancerIngressRules(lbIngressRules, sourceCIDRs, listenerPort, s.serviceLister)
139144
}
140145

141146
if !securityListRulesChanged(lbSecurityList, lbIngressRules, lbEgressRules) {
@@ -205,18 +210,18 @@ func (s *securityListManagerImpl) updateSecurityListRules(securityListID string,
205210
return err
206211
}
207212

208-
func getNodeIngressRules(securityList *baremetal.SecurityList, lbSubnets []*baremetal.Subnet, port uint64) []baremetal.IngressSecurityRule {
213+
func getNodeIngressRules(rules []baremetal.IngressSecurityRule, lbSubnets []*baremetal.Subnet, port uint64, serviceLister listersv1.ServiceLister) []baremetal.IngressSecurityRule {
209214
desired := sets.NewString()
210215
for _, lbSubnet := range lbSubnets {
211216
desired.Insert(lbSubnet.CIDRBlock)
212217
}
213218

214219
ingressRules := []baremetal.IngressSecurityRule{}
215220

216-
for _, rule := range securityList.IngressSecurityRules {
221+
for _, rule := range rules {
217222
if rule.TCPOptions == nil || rule.TCPOptions.SourcePortRange != nil || rule.TCPOptions.DestinationPortRange == nil ||
218-
(rule.TCPOptions.DestinationPortRange.Min != port &&
219-
rule.TCPOptions.DestinationPortRange.Max != port) {
223+
rule.TCPOptions.DestinationPortRange.Min != port ||
224+
rule.TCPOptions.DestinationPortRange.Max != port {
220225
// this rule doesn't apply to this service so nothing to do but keep it
221226
ingressRules = append(ingressRules, rule)
222227
continue
@@ -228,6 +233,24 @@ func getNodeIngressRules(securityList *baremetal.SecurityList, lbSubnets []*bare
228233
desired.Delete(rule.Source)
229234
continue
230235
}
236+
237+
inUse, err := healthCheckPortInUse(serviceLister, int32(port))
238+
if err != nil {
239+
// Unable to determine if this port is in use by another service, so I guess
240+
// we better err on the safe side and keep the rule.
241+
glog.Errorf("failed to determine if port: %d is still in use: %v", port, err)
242+
ingressRules = append(ingressRules, rule)
243+
continue
244+
}
245+
246+
if inUse {
247+
// This rule is no longer needed for this service, but is still used
248+
// by another service, so we must still keep it.
249+
glog.V(4).Infof("Port %d still in use by another service.", port)
250+
ingressRules = append(ingressRules, rule)
251+
continue
252+
}
253+
231254
// else the actual cidr no longer exists so we don't need to do
232255
// anything but ignore / delete it.
233256
}
@@ -246,14 +269,14 @@ func getNodeIngressRules(securityList *baremetal.SecurityList, lbSubnets []*bare
246269
return ingressRules
247270
}
248271

249-
func getLoadBalancerIngressRules(lbSecurityList *baremetal.SecurityList, sourceCIDRs []string, port uint64, serviceLister listersv1.ServiceLister) []baremetal.IngressSecurityRule {
272+
func getLoadBalancerIngressRules(rules []baremetal.IngressSecurityRule, sourceCIDRs []string, port uint64, serviceLister listersv1.ServiceLister) []baremetal.IngressSecurityRule {
250273
desired := sets.NewString(sourceCIDRs...)
251274

252275
ingressRules := []baremetal.IngressSecurityRule{}
253-
for _, rule := range lbSecurityList.IngressSecurityRules {
276+
for _, rule := range rules {
254277
if rule.TCPOptions == nil || rule.TCPOptions.SourcePortRange != nil || rule.TCPOptions.DestinationPortRange == nil ||
255-
(rule.TCPOptions.DestinationPortRange.Min != port &&
256-
rule.TCPOptions.DestinationPortRange.Max != port) {
278+
rule.TCPOptions.DestinationPortRange.Min != port ||
279+
rule.TCPOptions.DestinationPortRange.Max != port {
257280
// this rule doesn't apply to this service so nothing to do but keep it
258281
ingressRules = append(ingressRules, rule)
259282
continue
@@ -266,23 +289,21 @@ func getLoadBalancerIngressRules(lbSecurityList *baremetal.SecurityList, sourceC
266289
continue
267290
}
268291

269-
if rule.TCPOptions.DestinationPortRange.Min == port && rule.TCPOptions.DestinationPortRange.Max == port {
270-
inUse, err := portInUse(serviceLister, int32(port))
271-
if err != nil {
272-
// Unable to determine if this port is in use by another service, so I guess
273-
// we better err on the safe side and keep the rule.
274-
glog.Errorf("failed to determine if port: %d is still in use: %v", port, err)
275-
ingressRules = append(ingressRules, rule)
276-
continue
277-
}
292+
inUse, err := portInUse(serviceLister, int32(port))
293+
if err != nil {
294+
// Unable to determine if this port is in use by another service, so I guess
295+
// we better err on the safe side and keep the rule.
296+
glog.Errorf("failed to determine if port: %d is still in use: %v", port, err)
297+
ingressRules = append(ingressRules, rule)
298+
continue
299+
}
278300

279-
if inUse {
280-
// This rule is no longer needed for this service, but is still used
281-
// by another service, so we must still keep it.
282-
glog.V(4).Infof("Port %d still in use by another service.", port)
283-
ingressRules = append(ingressRules, rule)
284-
continue
285-
}
301+
if inUse {
302+
// This rule is no longer needed for this service, but is still used
303+
// by another service, so we must still keep it.
304+
glog.V(4).Infof("Port %d still in use by another service.", port)
305+
ingressRules = append(ingressRules, rule)
306+
continue
286307
}
287308

288309
// else the actual cidr no longer exists so we don't need to do
@@ -303,17 +324,17 @@ func getLoadBalancerIngressRules(lbSecurityList *baremetal.SecurityList, sourceC
303324
return ingressRules
304325
}
305326

306-
func getLoadBalancerEgressRules(lbSecurityList *baremetal.SecurityList, nodeSubnets []*baremetal.Subnet, port uint64) []baremetal.EgressSecurityRule {
327+
func getLoadBalancerEgressRules(rules []baremetal.EgressSecurityRule, nodeSubnets []*baremetal.Subnet, port uint64, serviceLister listersv1.ServiceLister) []baremetal.EgressSecurityRule {
307328
nodeCIDRs := sets.NewString()
308329
for _, subnet := range nodeSubnets {
309330
nodeCIDRs.Insert(subnet.CIDRBlock)
310331
}
311332

312333
egressRules := []baremetal.EgressSecurityRule{}
313-
for _, rule := range lbSecurityList.EgressSecurityRules {
334+
for _, rule := range rules {
314335
if rule.TCPOptions == nil || rule.TCPOptions.SourcePortRange != nil || rule.TCPOptions.DestinationPortRange == nil ||
315-
(rule.TCPOptions.DestinationPortRange.Min != port &&
316-
rule.TCPOptions.DestinationPortRange.Max != port) {
336+
rule.TCPOptions.DestinationPortRange.Min != port ||
337+
rule.TCPOptions.DestinationPortRange.Max != port {
317338
// this rule doesn't apply to this service so nothing to do but keep it
318339
egressRules = append(egressRules, rule)
319340
continue
@@ -325,6 +346,24 @@ func getLoadBalancerEgressRules(lbSecurityList *baremetal.SecurityList, nodeSubn
325346
nodeCIDRs.Delete(rule.Destination)
326347
continue
327348
}
349+
350+
inUse, err := healthCheckPortInUse(serviceLister, int32(port))
351+
if err != nil {
352+
// Unable to determine if this port is in use by another service, so I guess
353+
// we better err on the safe side and keep the rule.
354+
glog.Errorf("failed to determine if port: %d is still in use: %v", port, err)
355+
egressRules = append(egressRules, rule)
356+
continue
357+
}
358+
359+
if inUse {
360+
// This rule is no longer needed for this service, but is still used
361+
// by another service, so we must still keep it.
362+
glog.V(4).Infof("Port %d still in use by another service.", port)
363+
egressRules = append(egressRules, rule)
364+
continue
365+
}
366+
328367
// else the actual cidr no longer exists so we don't need to do
329368
// anything but ignore / delete it.
330369
}
@@ -390,16 +429,42 @@ func portInUse(serviceLister listersv1.ServiceLister, port int32) (bool, error)
390429
return false, nil
391430
}
392431

432+
func healthCheckPortInUse(serviceLister listersv1.ServiceLister, port int32) (bool, error) {
433+
if port != lbNodesHealthCheckPort {
434+
// This service is using a custom healthcheck port (enabled through setting
435+
// extenalTrafficPolicy=Local on the service). As this port is unique
436+
// per service, we know no other service will be using this port too.
437+
return false, nil
438+
}
439+
440+
// This service is using the default healthcheck port, so we must check if
441+
// any other service is also using this default healthcheck port.
442+
serviceList, err := serviceLister.List(labels.Everything())
443+
if err != nil {
444+
return false, err
445+
}
446+
for _, service := range serviceList {
447+
if service.Spec.Type == api.ServiceTypeLoadBalancer {
448+
healthCheckPath, _ := apiservice.GetServiceHealthCheckPathPort(service)
449+
if healthCheckPath == "" {
450+
// We have found another service using the default port.
451+
return true, nil
452+
}
453+
}
454+
}
455+
return false, nil
456+
}
457+
393458
// securityListManagerNOOP implements the securityListManager interface but does
394459
// no logic, so that it can be used to not handle security lists if the user doesn't wish
395460
// to use that feature.
396461
type securityListManagerNOOP struct{}
397462

398-
func (s *securityListManagerNOOP) Update(lbSubnets []*baremetal.Subnet, backendSubnets []*baremetal.Subnet, sourceCIDRs []string, listenerPort uint64, backendPort uint64) error {
463+
func (s *securityListManagerNOOP) Update(lbSubnets []*baremetal.Subnet, backendSubnets []*baremetal.Subnet, sourceCIDRs []string, listenerPort uint64, backendPort uint64, healthCheckPort uint64) error {
399464
return nil
400465
}
401466

402-
func (s *securityListManagerNOOP) Delete(lbSubnets []*baremetal.Subnet, backendSubnets []*baremetal.Subnet, listenerPort uint64, backendPort uint64) error {
467+
func (s *securityListManagerNOOP) Delete(lbSubnets []*baremetal.Subnet, backendSubnets []*baremetal.Subnet, listenerPort uint64, backendPort uint64, healthCheckPort uint64) error {
403468
return nil
404469
}
405470

0 commit comments

Comments
 (0)