Skip to content

Commit 847530a

Browse files
prydieprydie
committed
Fix unit tests associated bugs
1 parent 0eea28b commit 847530a

5 files changed

+253
-251
lines changed

‎pkg/oci/load_balancer_security_lists_test.go

+28-37
Original file line numberDiff line numberDiff line change
@@ -19,29 +19,20 @@ import (
1919
"testing"
2020

2121
baremetal "github.com/oracle/bmcs-go-sdk"
22+
"github.com/oracle/oci-go-sdk/common"
23+
"github.com/oracle/oci-go-sdk/core"
2224

2325
"k8s.io/api/core/v1"
2426
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2527
v1listers "k8s.io/client-go/listers/core/v1"
2628
"k8s.io/client-go/tools/cache"
2729
)
2830

29-
func TestGetBackendPort(t *testing.T) {
30-
backends := []baremetal.Backend{
31-
{Port: 80},
32-
}
33-
34-
port := getBackendPort(backends)
35-
if port != 80 {
36-
t.Errorf("expected port 80 but got %d", port)
37-
}
38-
}
39-
4031
func TestGetNodeIngressRules(t *testing.T) {
4132
testCases := []struct {
4233
name string
4334
securityList *baremetal.SecurityList
44-
lbSubnets []*baremetal.Subnet
35+
lbSubnets []*core.Subnet
4536
port uint64
4637
services []*v1.Service
4738
expected []baremetal.IngressSecurityRule
@@ -53,9 +44,9 @@ func TestGetNodeIngressRules(t *testing.T) {
5344
makeIngressSecurityRule("existing", 9000),
5445
},
5546
},
56-
lbSubnets: []*baremetal.Subnet{
57-
{CIDRBlock: "1"},
58-
{CIDRBlock: "2"},
47+
lbSubnets: []*core.Subnet{
48+
{CidrBlock: common.String("1")},
49+
{CidrBlock: common.String("2")},
5950
},
6051
port: 80,
6152
services: []*v1.Service{},
@@ -73,9 +64,9 @@ func TestGetNodeIngressRules(t *testing.T) {
7364
makeIngressSecurityRule("2", 80),
7465
},
7566
},
76-
lbSubnets: []*baremetal.Subnet{
77-
{CIDRBlock: "1"},
78-
{CIDRBlock: "2"},
67+
lbSubnets: []*core.Subnet{
68+
{CidrBlock: common.String("1")},
69+
{CidrBlock: common.String("2")},
7970
},
8071
port: 80,
8172
services: []*v1.Service{},
@@ -94,9 +85,9 @@ func TestGetNodeIngressRules(t *testing.T) {
9485
makeIngressSecurityRule("existing", 9001),
9586
},
9687
},
97-
lbSubnets: []*baremetal.Subnet{
98-
{CIDRBlock: "1"},
99-
{CIDRBlock: "3"},
88+
lbSubnets: []*core.Subnet{
89+
{CidrBlock: common.String("1")},
90+
{CidrBlock: common.String("3")},
10091
},
10192
port: 80,
10293
services: []*v1.Service{},
@@ -116,7 +107,7 @@ func TestGetNodeIngressRules(t *testing.T) {
116107
makeIngressSecurityRule("existing", 9001),
117108
},
118109
},
119-
lbSubnets: []*baremetal.Subnet{},
110+
lbSubnets: []*core.Subnet{},
120111
port: 80,
121112
services: []*v1.Service{},
122113
expected: []baremetal.IngressSecurityRule{
@@ -130,7 +121,7 @@ func TestGetNodeIngressRules(t *testing.T) {
130121
makeIngressSecurityRule("0.0.0.0/0", lbNodesHealthCheckPort),
131122
},
132123
},
133-
lbSubnets: []*baremetal.Subnet{},
124+
lbSubnets: []*core.Subnet{},
134125
port: lbNodesHealthCheckPort,
135126
services: []*v1.Service{
136127
{
@@ -295,7 +286,7 @@ func TestGetLoadBalancerEgressRules(t *testing.T) {
295286
testCases := []struct {
296287
name string
297288
securityList *baremetal.SecurityList
298-
subnets []*baremetal.Subnet
289+
subnets []*core.Subnet
299290
port uint64
300291
services []*v1.Service
301292
expected []baremetal.EgressSecurityRule
@@ -307,9 +298,9 @@ func TestGetLoadBalancerEgressRules(t *testing.T) {
307298
makeEgressSecurityRule("existing", 9000),
308299
},
309300
},
310-
subnets: []*baremetal.Subnet{
311-
{CIDRBlock: "1"},
312-
{CIDRBlock: "2"},
301+
subnets: []*core.Subnet{
302+
{CidrBlock: common.String("1")},
303+
{CidrBlock: common.String("2")},
313304
},
314305
port: 80,
315306
services: []*v1.Service{},
@@ -327,9 +318,9 @@ func TestGetLoadBalancerEgressRules(t *testing.T) {
327318
makeEgressSecurityRule("2", 80),
328319
},
329320
},
330-
subnets: []*baremetal.Subnet{
331-
{CIDRBlock: "1"},
332-
{CIDRBlock: "2"},
321+
subnets: []*core.Subnet{
322+
{CidrBlock: common.String("1")},
323+
{CidrBlock: common.String("2")},
333324
},
334325
port: 80,
335326
services: []*v1.Service{},
@@ -348,9 +339,9 @@ func TestGetLoadBalancerEgressRules(t *testing.T) {
348339
makeEgressSecurityRule("existing", 9001),
349340
},
350341
},
351-
subnets: []*baremetal.Subnet{
352-
{CIDRBlock: "1"},
353-
{CIDRBlock: "3"},
342+
subnets: []*core.Subnet{
343+
{CidrBlock: common.String("1")},
344+
{CidrBlock: common.String("3")},
354345
},
355346
port: 80,
356347
services: []*v1.Service{},
@@ -370,7 +361,7 @@ func TestGetLoadBalancerEgressRules(t *testing.T) {
370361
makeEgressSecurityRule("existing", 9001),
371362
},
372363
},
373-
subnets: []*baremetal.Subnet{},
364+
subnets: []*core.Subnet{},
374365
port: 80,
375366
services: []*v1.Service{},
376367
expected: []baremetal.EgressSecurityRule{
@@ -384,7 +375,7 @@ func TestGetLoadBalancerEgressRules(t *testing.T) {
384375
makeEgressSecurityRule("0.0.0.0/0", lbNodesHealthCheckPort),
385376
},
386377
},
387-
subnets: []*baremetal.Subnet{},
378+
subnets: []*core.Subnet{},
388379
port: lbNodesHealthCheckPort,
389380
services: []*v1.Service{
390381
{
@@ -538,11 +529,11 @@ func TestDelete(t *testing.T) {
538529
// lbSubnets := []*baremetal.Subnet{
539530
// {
540531
// ID: "lb-subnet-1",
541-
// CIDRBlock: "lb-subnet-1",
532+
// CidrBlock: "lb-subnet-1",
542533
// },
543534
// {
544535
// ID: "lb-subnet-2",
545-
// CIDRBlock: "lb-subnet-2",
536+
// CidrBlock: "lb-subnet-2",
546537
// },
547538
// }
548539
// lbSecurityLists := []*baremetal.SecurityList{

‎pkg/oci/load_balancer_spec.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ func NewLBSpec(service *api.Service, nodes []*api.Node, defaultSubnets []string,
108108
if internal {
109109
// Only public load balancers need two subnets. Internal load
110110
// balancers will always use the first subnet.
111-
subnets = subnets[:0]
111+
subnets = subnets[:1]
112112
}
113113

114114
return LBSpec{

‎pkg/oci/load_balancer_spec_test.go

+5-20
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import (
1818
"reflect"
1919
"testing"
2020

21-
"github.com/oracle/oci-cloud-controller-manager/pkg/oci/client"
2221
api "k8s.io/api/core/v1"
2322
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2423
)
@@ -139,16 +138,9 @@ func TestNewLBSpecSuccess(t *testing.T) {
139138
for name, tc := range testCases {
140139
t.Run(name, func(t *testing.T) {
141140
// we expect the service to be unchanged
142-
tc.expected.Service = tc.service
143-
cp := &CloudProvider{
144-
config: &client.Config{
145-
LoadBalancer: client.LoadBalancerConfig{
146-
Subnet1: tc.defaultSubnetOne,
147-
Subnet2: tc.defaultSubnetTwo,
148-
},
149-
},
150-
}
151-
result, err := NewLBSpec(cp, tc.service, tc.nodes)
141+
tc.expected.service = tc.service
142+
subnets := []string{tc.defaultSubnetOne, tc.defaultSubnetTwo}
143+
result, err := NewLBSpec(tc.service, tc.nodes, subnets, nil)
152144
if err != nil {
153145
t.Error(err)
154146
}
@@ -204,15 +196,8 @@ func TestNewLBSpecFailure(t *testing.T) {
204196

205197
for name, tc := range testCases {
206198
t.Run(name, func(t *testing.T) {
207-
cp := &CloudProvider{
208-
config: &client.Config{
209-
LoadBalancer: client.LoadBalancerConfig{
210-
Subnet1: tc.defaultSubnetOne,
211-
Subnet2: tc.defaultSubnetTwo,
212-
},
213-
},
214-
}
215-
_, err := NewLBSpec(cp, tc.service, tc.nodes)
199+
subnets := []string{tc.defaultSubnetOne, tc.defaultSubnetTwo}
200+
_, err := NewLBSpec(tc.service, tc.nodes, subnets, nil)
216201
if err == nil || err.Error() != tc.expectedErrMsg {
217202
t.Errorf("Expected error with message %q but got `%v`", tc.expectedErrMsg, err)
218203
}

‎pkg/oci/load_balancer_util.go

+5-7
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,10 @@ func toInt(i *int) int {
124124
}
125125

126126
func hasHealthCheckerChanged(actual *loadbalancer.HealthChecker, desired *loadbalancer.HealthCheckerDetails) bool {
127+
if actual == nil {
128+
return !(desired == nil)
129+
}
130+
127131
if toInt(actual.Port) != toInt(desired.Port) {
128132
return false
129133
}
@@ -398,7 +402,7 @@ func validateProtocols(servicePorts []api.ServicePort) error {
398402
func getSSLEnabledPorts(svc *api.Service) ([]int, error) {
399403
ports := []int{}
400404
annotation, ok := svc.Annotations[ServiceAnnotationLoadBalancerSSLPorts]
401-
if !ok {
405+
if !ok || annotation == "" {
402406
return ports, nil
403407
}
404408

@@ -460,9 +464,3 @@ func sortAndCombineActions(backendSetActions []Action, listenerActions []Action)
460464
})
461465
return actions
462466
}
463-
464-
func getBackendPort(backends []loadbalancer.Backend) uint64 {
465-
// TODO: what happens if this is 0? e.g. we scale the pods to 0 for a
466-
// deployment.
467-
return uint64(*backends[0].Port)
468-
}

0 commit comments

Comments
 (0)