Skip to content

Commit 7231edb

Browse files
pranavsriram8YashwantGohokar
pranavsriram8
authored and
YashwantGohokar
committed
for externalTrafficPolicy local the healthcheck port security rule should be retained
1 parent 7ae68cd commit 7231edb

File tree

2 files changed

+173
-25
lines changed

2 files changed

+173
-25
lines changed

‎pkg/cloudprovider/providers/oci/load_balancer_security_lists.go

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ import (
3030
sets "k8s.io/apimachinery/pkg/util/sets"
3131
informersv1 "k8s.io/client-go/informers/core/v1"
3232
listersv1 "k8s.io/client-go/listers/core/v1"
33-
helper "k8s.io/cloud-provider/service/helpers"
3433
)
3534

3635
const (
@@ -429,7 +428,7 @@ func getNodeIngressRules(
429428
"source", *rule.Source,
430429
"destinationPortRangeMin", *rule.TcpOptions.DestinationPortRange.Min,
431430
"destinationPortRangeMax", *rule.TcpOptions.DestinationPortRange.Max,
432-
).Debug("Deleting node ingres security rule")
431+
).Debug("Deleting node ingress security rule")
433432
}
434433

435434
if desiredBackend.Len() == 0 && desiredHealthChecker.Len() == 0 {
@@ -676,25 +675,26 @@ func portInUse(serviceLister listersv1.ServiceLister, port int32) (bool, error)
676675
}
677676

678677
func healthCheckPortInUse(serviceLister listersv1.ServiceLister, port int32) (bool, error) {
679-
if port != lbNodesHealthCheckPort {
680-
// This service is using a custom healthcheck port (enabled through setting
681-
// extenalTrafficPolicy=Local on the service). As this port is unique
682-
// per service, we know no other service will be using this port too.
683-
return false, nil
684-
}
685-
686-
// This service is using the default healthcheck port, so we must check if
687-
// any other service is also using this default healthcheck port.
688678
serviceList, err := serviceLister.List(labels.Everything())
689679
if err != nil {
690680
return false, err
691681
}
692682
for _, service := range serviceList {
693-
if service.Spec.Type == api.ServiceTypeLoadBalancer {
694-
healthCheckPath, _ := helper.GetServiceHealthCheckPathPort(service)
695-
if healthCheckPath == "" {
696-
// We have found another service using the default port.
697-
return true, nil
683+
if service.DeletionTimestamp == nil || service.Spec.Type == api.ServiceTypeLoadBalancer {
684+
if service.Spec.ExternalTrafficPolicy == api.ServiceExternalTrafficPolicyCluster {
685+
// This service is using the default healthcheck port, so we must check if
686+
// any other service is also using this default healthcheck port.
687+
if port == lbNodesHealthCheckPort {
688+
return true, nil
689+
}
690+
} else if service.Spec.ExternalTrafficPolicy == api.ServiceExternalTrafficPolicyLocal {
691+
// This service is using a custom healthcheck port (enabled through setting
692+
// externalTrafficPolicy=Local on the service). As this port is unique
693+
// per service, we know no other service will be using this port too.
694+
if port == service.Spec.HealthCheckNodePort {
695+
// Service with this healthCheckerPort is still not deleted (this would be a "delete listener" call in that case)
696+
return true, nil
697+
}
698698
}
699699
}
700700
}

‎pkg/cloudprovider/providers/oci/load_balancer_security_lists_test.go

Lines changed: 157 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2727
v1listers "k8s.io/client-go/listers/core/v1"
2828
"k8s.io/client-go/tools/cache"
29+
api "k8s.io/kubernetes/pkg/apis/core"
2930
k8sports "k8s.io/kubernetes/pkg/cluster/ports"
3031
)
3132

@@ -126,7 +127,8 @@ func TestGetNodeIngressRules(t *testing.T) {
126127
makeIngressSecurityRule("3", 80),
127128
makeIngressSecurityRule("3", k8sports.ProxyHealthzPort),
128129
},
129-
}, {
130+
},
131+
{
130132
name: "remove lb subnets",
131133
securityList: &core.SecurityList{
132134
IngressSecurityRules: []core.IngressSecurityRule{
@@ -150,7 +152,8 @@ func TestGetNodeIngressRules(t *testing.T) {
150152
makeIngressSecurityRule("existing", 9000),
151153
makeIngressSecurityRule("existing", 9001),
152154
},
153-
}, {
155+
},
156+
{
154157
name: "do not delete health check rules that are used by other services",
155158
securityList: &core.SecurityList{
156159
IngressSecurityRules: []core.IngressSecurityRule{
@@ -167,8 +170,9 @@ func TestGetNodeIngressRules(t *testing.T) {
167170
{
168171
ObjectMeta: metav1.ObjectMeta{Namespace: "namespace", Name: "using-default-health-check-port"},
169172
Spec: v1.ServiceSpec{
170-
Type: v1.ServiceTypeLoadBalancer,
171-
Ports: []v1.ServicePort{{Port: 443}},
173+
Type: v1.ServiceTypeLoadBalancer,
174+
ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicy(api.ServiceExternalTrafficPolicyCluster),
175+
Ports: []v1.ServicePort{{Port: 443}},
172176
},
173177
},
174178
},
@@ -177,7 +181,48 @@ func TestGetNodeIngressRules(t *testing.T) {
177181
expected: []core.IngressSecurityRule{
178182
makeIngressSecurityRule("0.0.0.0/0", lbNodesHealthCheckPort),
179183
},
180-
}, {
184+
},
185+
{
186+
name: "multiple services for same cluster; one uses default healthcheck and other uses HealthcheckNodeport",
187+
securityList: &core.SecurityList{
188+
IngressSecurityRules: []core.IngressSecurityRule{
189+
makeIngressSecurityRule("0.0.0.0/0", lbNodesHealthCheckPort),
190+
makeIngressSecurityRule("0.0.0.0/0", 80),
191+
makeIngressSecurityRule("1.1.1.1/1", 32000),
192+
},
193+
},
194+
lbSubnets: []*core.Subnet{},
195+
desiredPorts: portSpec{
196+
BackendPort: 80,
197+
HealthCheckerPort: k8sports.ProxyHealthzPort,
198+
},
199+
services: []*v1.Service{
200+
{
201+
ObjectMeta: metav1.ObjectMeta{Namespace: "namespace", Name: "using-default-health-check-port"},
202+
Spec: v1.ServiceSpec{
203+
Type: v1.ServiceTypeLoadBalancer,
204+
ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicy(api.ServiceExternalTrafficPolicyCluster),
205+
Ports: []v1.ServicePort{{Port: 443}},
206+
},
207+
},
208+
{
209+
ObjectMeta: metav1.ObjectMeta{Namespace: "namespace", Name: "using-NodePort-health-check-port"},
210+
Spec: v1.ServiceSpec{
211+
Type: v1.ServiceTypeLoadBalancer,
212+
ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicy(api.ServiceExternalTrafficPolicyLocal),
213+
Ports: []v1.ServicePort{{Port: 8081}},
214+
HealthCheckNodePort: 32000,
215+
},
216+
},
217+
},
218+
isPreserveSource: false,
219+
sourceCIDRs: []string{"0.0.0.0/0"},
220+
expected: []core.IngressSecurityRule{
221+
makeIngressSecurityRule("0.0.0.0/0", lbNodesHealthCheckPort),
222+
makeIngressSecurityRule("1.1.1.1/1", 32000),
223+
},
224+
},
225+
{
181226
name: "update service port",
182227
securityList: &core.SecurityList{
183228
IngressSecurityRules: []core.IngressSecurityRule{
@@ -243,6 +288,48 @@ func TestGetNodeIngressRules(t *testing.T) {
243288
makeIngressSecurityRule("10.0.50.0/24", k8sports.ProxyHealthzPort+1),
244289
makeIngressSecurityRule("10.0.51.0/24", k8sports.ProxyHealthzPort+1),
245290
},
291+
}, {
292+
name: "external traffic policy local service health check port",
293+
securityList: &core.SecurityList{
294+
IngressSecurityRules: []core.IngressSecurityRule{
295+
core.IngressSecurityRule{Source: common.String("0.0.0.0/0")},
296+
makeIngressSecurityRule("10.0.50.0/24", 8081),
297+
makeIngressSecurityRule("10.0.51.0/24", 8081),
298+
makeIngressSecurityRule("10.0.50.0/24", k8sports.ProxyHealthzPort),
299+
makeIngressSecurityRule("10.0.51.0/24", k8sports.ProxyHealthzPort),
300+
},
301+
},
302+
lbSubnets: []*core.Subnet{
303+
{CidrBlock: common.String("10.0.50.0/24")},
304+
{CidrBlock: common.String("10.0.51.0/24")},
305+
},
306+
actualPorts: &portSpec{
307+
BackendPort: 8081,
308+
HealthCheckerPort: k8sports.ProxyHealthzPort,
309+
},
310+
desiredPorts: portSpec{
311+
BackendPort: 8081,
312+
HealthCheckerPort: 30000,
313+
},
314+
services: []*v1.Service{
315+
{
316+
ObjectMeta: metav1.ObjectMeta{Namespace: "namespace", Name: "using-non-default-health-check-port"},
317+
Spec: v1.ServiceSpec{
318+
Type: v1.ServiceTypeLoadBalancer,
319+
ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicy(api.ServiceExternalTrafficPolicyLocal),
320+
Ports: []v1.ServicePort{{Port: 8081}},
321+
},
322+
},
323+
},
324+
isPreserveSource: false,
325+
sourceCIDRs: []string{"0.0.0.0/0"},
326+
expected: []core.IngressSecurityRule{
327+
core.IngressSecurityRule{Source: common.String("0.0.0.0/0")},
328+
makeIngressSecurityRule("10.0.50.0/24", 8081),
329+
makeIngressSecurityRule("10.0.51.0/24", 8081),
330+
makeIngressSecurityRule("10.0.50.0/24", 30000),
331+
makeIngressSecurityRule("10.0.51.0/24", 30000),
332+
},
246333
},
247334
}
248335

@@ -407,8 +494,9 @@ func TestGetNodeIngressRules_NLB(t *testing.T) {
407494
{
408495
ObjectMeta: metav1.ObjectMeta{Namespace: "namespace", Name: "using-default-health-check-port"},
409496
Spec: v1.ServiceSpec{
410-
Type: v1.ServiceTypeLoadBalancer,
411-
Ports: []v1.ServicePort{{Port: 443}},
497+
Type: v1.ServiceTypeLoadBalancer,
498+
ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicy(api.ServiceExternalTrafficPolicyCluster),
499+
Ports: []v1.ServicePort{{Port: 443}},
412500
},
413501
},
414502
},
@@ -907,12 +995,72 @@ func TestGetLoadBalancerEgressRules(t *testing.T) {
907995
{
908996
ObjectMeta: metav1.ObjectMeta{Namespace: "namespace", Name: "using-default-health-check-port"},
909997
Spec: v1.ServiceSpec{
910-
Type: v1.ServiceTypeLoadBalancer,
911-
Ports: []v1.ServicePort{{Port: 80}},
998+
Type: v1.ServiceTypeLoadBalancer,
999+
ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicy(api.ServiceExternalTrafficPolicyCluster),
1000+
Ports: []v1.ServicePort{{Port: 80}},
1001+
},
1002+
},
1003+
},
1004+
expected: []core.EgressSecurityRule{
1005+
makeEgressSecurityRule("0.0.0.0/0", lbNodesHealthCheckPort),
1006+
},
1007+
},
1008+
{
1009+
name: "do not delete a port rule during listener deletes",
1010+
securityList: &core.SecurityList{
1011+
EgressSecurityRules: []core.EgressSecurityRule{
1012+
makeEgressSecurityRule("0.0.0.0/0", 30000),
1013+
},
1014+
},
1015+
subnets: []*core.Subnet{},
1016+
actualPort: 30000,
1017+
desiredPort: 30000,
1018+
services: []*v1.Service{
1019+
{
1020+
ObjectMeta: metav1.ObjectMeta{Namespace: "namespace", Name: "using-default-health-check-port"},
1021+
Spec: v1.ServiceSpec{
1022+
Type: v1.ServiceTypeLoadBalancer,
1023+
ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicy(api.ServiceExternalTrafficPolicyLocal),
1024+
HealthCheckNodePort: 30000,
1025+
},
1026+
},
1027+
},
1028+
expected: []core.EgressSecurityRule{
1029+
makeEgressSecurityRule("0.0.0.0/0", 30000),
1030+
},
1031+
},
1032+
{
1033+
name: "multiple services in the same cluster; one using default healthcheck and other using healthcheck Nodeport",
1034+
securityList: &core.SecurityList{
1035+
EgressSecurityRules: []core.EgressSecurityRule{
1036+
makeEgressSecurityRule("0.0.0.0/0", 30000),
1037+
makeEgressSecurityRule("0.0.0.0/0", lbNodesHealthCheckPort),
1038+
},
1039+
},
1040+
subnets: []*core.Subnet{},
1041+
actualPort: 31000,
1042+
desiredPort: 31000,
1043+
services: []*v1.Service{
1044+
{
1045+
ObjectMeta: metav1.ObjectMeta{Namespace: "namespace", Name: "using-Nodeport-health-check-port"},
1046+
Spec: v1.ServiceSpec{
1047+
Type: v1.ServiceTypeLoadBalancer,
1048+
ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicy(api.ServiceExternalTrafficPolicyLocal),
1049+
Ports: []v1.ServicePort{{Port: 80}},
1050+
HealthCheckNodePort: 30000,
1051+
},
1052+
},
1053+
{
1054+
ObjectMeta: metav1.ObjectMeta{Namespace: "namespace", Name: "using-default-health-check-port"},
1055+
Spec: v1.ServiceSpec{
1056+
Type: v1.ServiceTypeLoadBalancer,
1057+
Ports: []v1.ServicePort{{Port: 8080}},
1058+
ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicy(api.ServiceExternalTrafficPolicyCluster),
9121059
},
9131060
},
9141061
},
9151062
expected: []core.EgressSecurityRule{
1063+
makeEgressSecurityRule("0.0.0.0/0", 30000),
9161064
makeEgressSecurityRule("0.0.0.0/0", lbNodesHealthCheckPort),
9171065
},
9181066
},

0 commit comments

Comments
 (0)