Skip to content

Commit dae9d01

Browse files
pranavsriram8YashwantGohokar
pranavsriram8
authored and
YashwantGohokar
committed
Provide an option to skip private IP in LB Status for public NLBs
add max pods per node for e2e nodepools to preserve private ip's
1 parent 23c4cce commit dae9d01

File tree

6 files changed

+270
-11
lines changed

6 files changed

+270
-11
lines changed

‎hack/run_e2e_test.sh

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,8 @@ function run_e2e_tests_existing_cluster() {
6868
--static-snapshot-compartment-id=${STATIC_SNAPSHOT_COMPARTMENT_ID} \
6969
--enable-parallel-run=${ENABLE_PARALLEL_RUN} \
7070
--run-uhp-e2e=${RUN_UHP_E2E} \
71-
--add-oke-system-tags="false"
71+
--add-oke-system-tags="false" \
72+
--maxpodspernode=${MAX_PODS_PER_NODE}
7273
else
7374
ginkgo -v -progress --trace -nodes=${E2E_NODE_COUNT} "${FOCUS_OPT}" "${FOCUS_SKIP_OPT}" "${FOCUS_FP_OPT}" \
7475
ginkgo -v -p -progress --trace "${FOCUS_OPT}" "${FOCUS_FP_OPT}" \
@@ -91,7 +92,8 @@ function run_e2e_tests_existing_cluster() {
9192
--static-snapshot-compartment-id=${STATIC_SNAPSHOT_COMPARTMENT_ID} \
9293
--enable-parallel-run=${ENABLE_PARALLEL_RUN} \
9394
--run-uhp-e2e=${RUN_UHP_E2E} \
94-
--add-oke-system-tags="false"
95+
--add-oke-system-tags="false" \
96+
--maxpodspernode=${MAX_PODS_PER_NODE}
9597
fi
9698
retval=$?
9799
return $retval

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

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -212,8 +212,11 @@ func (cp *CloudProvider) GetLoadBalancer(ctx context.Context, clusterName string
212212

213213
return nil, false, err
214214
}
215-
216-
lbStatus, err := loadBalancerToStatus(lb, nil)
215+
skipPrivateIP, err := isSkipPrivateIP(service)
216+
if err != nil {
217+
return nil, false, err
218+
}
219+
lbStatus, err := loadBalancerToStatus(lb, nil, skipPrivateIP)
217220
return lbStatus, err == nil, err
218221
}
219222

@@ -464,7 +467,12 @@ func (clb *CloudLoadBalancerProvider) createLoadBalancer(ctx context.Context, sp
464467
}
465468

466469
logger.With("loadBalancerID", *lb.Id).Info("Load balancer created")
467-
status, err := loadBalancerToStatus(lb, spec.ingressIpMode)
470+
471+
skipPrivateIP, err := isSkipPrivateIP(spec.service)
472+
if err != nil {
473+
return nil, "", err
474+
}
475+
status, err := loadBalancerToStatus(lb, spec.ingressIpMode, skipPrivateIP)
468476

469477
if status != nil && len(status.Ingress) > 0 {
470478
// If the LB is successfully provisioned then open lb/node subnet seclists egress/ingress.
@@ -903,7 +911,11 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
903911
dimensionsMap[metrics.BackendSetsCountDimension] = strconv.Itoa(len(lb.BackendSets))
904912
metrics.SendMetricData(cp.metricPusher, getMetric(loadBalancerType, Update), syncTime, dimensionsMap)
905913

906-
return loadBalancerToStatus(lb, spec.ingressIpMode)
914+
skipPrivateIP, err := isSkipPrivateIP(service)
915+
if err != nil {
916+
return nil, err
917+
}
918+
return loadBalancerToStatus(lb, spec.ingressIpMode, skipPrivateIP)
907919
}
908920

909921
func getDefaultLBSubnets(subnet1, subnet2 string) []string {
@@ -1954,7 +1966,7 @@ func (clb *CloudLoadBalancerProvider) updateLoadBalancerIpVersion(ctx context.Co
19541966
}
19551967

19561968
// Given an OCI load balancer, return a LoadBalancerStatus
1957-
func loadBalancerToStatus(lb *client.GenericLoadBalancer, ipMode *v1.LoadBalancerIPMode) (*v1.LoadBalancerStatus, error) {
1969+
func loadBalancerToStatus(lb *client.GenericLoadBalancer, ipMode *v1.LoadBalancerIPMode, skipPrivateIp bool) (*v1.LoadBalancerStatus, error) {
19581970
if len(lb.IpAddresses) == 0 {
19591971
return nil, errors.Errorf("no ip addresses found for load balancer %q", *lb.DisplayName)
19601972
}
@@ -1964,6 +1976,12 @@ func loadBalancerToStatus(lb *client.GenericLoadBalancer, ipMode *v1.LoadBalance
19641976
if ip.IpAddress == nil {
19651977
continue // should never happen but appears to when EnsureLoadBalancer is called with 0 nodes.
19661978
}
1979+
1980+
if skipPrivateIp {
1981+
if !pointer.BoolDeref(ip.IsPublic, false) {
1982+
continue
1983+
}
1984+
}
19671985
ingress = append(ingress, v1.LoadBalancerIngress{IP: *ip.IpAddress, IPMode: ipMode})
19681986
}
19691987

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

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,9 @@ const (
244244

245245
// ServiceAnnotationNetworkLoadBalancerIsPpv2Enabled is a service annotation to enable/disable PPv2 feature for the listeners of this NLB.
246246
ServiceAnnotationNetworkLoadBalancerIsPpv2Enabled = "oci-network-load-balancer.oraclecloud.com/is-ppv2-enabled"
247+
248+
// ServiceAnnotationNetworkLoadBalancerExternalIpOnly is a service a boolean annotation to skip private ip when assigning to ingress resource for NLB service
249+
ServiceAnnotationNetworkLoadBalancerExternalIpOnly = "oci-network-load-balancer.oraclecloud.com/external-ip-only"
247250
)
248251

249252
const (
@@ -1583,7 +1586,7 @@ func isServiceDualStack(svc *v1.Service) bool {
15831586
return false
15841587
}
15851588

1586-
// patchIngressIpMode reads ingress ipMode specified in the service annotation if exists
1589+
// getIngressIpMode reads ingress ipMode specified in the service annotation if exists
15871590
func getIngressIpMode(service *v1.Service) (*v1.LoadBalancerIPMode, error) {
15881591
var ipMode, exists = "", false
15891592

@@ -1602,3 +1605,33 @@ func getIngressIpMode(service *v1.Service) (*v1.LoadBalancerIPMode, error) {
16021605
return nil, errors.New("IpMode can only be set as Proxy or VIP")
16031606
}
16041607
}
1608+
1609+
// isSkipPrivateIP determines if skipPrivateIP annotation is set or not
1610+
func isSkipPrivateIP(svc *v1.Service) (bool, error) {
1611+
lbType := getLoadBalancerType(svc)
1612+
annotationValue := ""
1613+
annotationExists := false
1614+
annotationString := ""
1615+
annotationValue, annotationExists = svc.Annotations[ServiceAnnotationNetworkLoadBalancerExternalIpOnly]
1616+
if !annotationExists {
1617+
return false, nil
1618+
}
1619+
1620+
if lbType != NLB {
1621+
return false, nil
1622+
}
1623+
1624+
internal, err := isInternalLB(svc)
1625+
if err != nil {
1626+
return false, err
1627+
}
1628+
if internal {
1629+
return false, nil
1630+
}
1631+
1632+
skipPrivateIp, err := strconv.ParseBool(annotationValue)
1633+
if err != nil {
1634+
return false, errors.Wrap(err, fmt.Sprintf("invalid value: %s provided for annotation: %s", annotationValue, annotationString))
1635+
}
1636+
return skipPrivateIp, nil
1637+
}

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

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import (
1818
"context"
1919
"encoding/json"
2020
"fmt"
21-
"k8s.io/utils/pointer"
2221
"net/http"
2322
"reflect"
2423
"testing"
@@ -28,6 +27,7 @@ import (
2827
v1 "k8s.io/api/core/v1"
2928
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3029
"k8s.io/apimachinery/pkg/util/sets"
30+
"k8s.io/utils/pointer"
3131

3232
providercfg "github.com/oracle/oci-cloud-controller-manager/pkg/cloudprovider/providers/oci/config"
3333
"github.com/oracle/oci-cloud-controller-manager/pkg/oci/client"
@@ -11858,3 +11858,74 @@ func Test_getLoadBalancerSourceRanges(t *testing.T) {
1185811858
})
1185911859
}
1186011860
}
11861+
11862+
func TestIsSkipPrivateIP_NLB(t *testing.T) {
11863+
tests := []struct {
11864+
name string
11865+
svcAnnotations map[string]string
11866+
expected bool
11867+
wantErr bool
11868+
}{
11869+
{
11870+
name: "skip-private-ip-enabled",
11871+
svcAnnotations: map[string]string{
11872+
ServiceAnnotationLoadBalancerType: NLB,
11873+
ServiceAnnotationNetworkLoadBalancerExternalIpOnly: "true",
11874+
},
11875+
expected: true,
11876+
wantErr: false,
11877+
},
11878+
{
11879+
name: "skip-private-ip-disabled",
11880+
svcAnnotations: map[string]string{
11881+
ServiceAnnotationLoadBalancerType: NLB,
11882+
ServiceAnnotationNetworkLoadBalancerExternalIpOnly: "false",
11883+
},
11884+
expected: false,
11885+
wantErr: false,
11886+
},
11887+
{
11888+
name: "skip-private-ip-invalid-value",
11889+
svcAnnotations: map[string]string{
11890+
ServiceAnnotationLoadBalancerType: NLB,
11891+
ServiceAnnotationNetworkLoadBalancerExternalIpOnly: "invalid",
11892+
},
11893+
expected: false,
11894+
wantErr: true,
11895+
},
11896+
{
11897+
name: "skip-private-ip with internal loadbalancer",
11898+
svcAnnotations: map[string]string{
11899+
ServiceAnnotationLoadBalancerType: NLB,
11900+
ServiceAnnotationNetworkLoadBalancerInternal: "true",
11901+
ServiceAnnotationNetworkLoadBalancerExternalIpOnly: "true",
11902+
},
11903+
expected: false,
11904+
wantErr: false,
11905+
},
11906+
{
11907+
name: "no-skip-private-ip-annotation",
11908+
svcAnnotations: map[string]string{},
11909+
expected: false,
11910+
wantErr: false,
11911+
},
11912+
}
11913+
11914+
for _, tt := range tests {
11915+
t.Run(tt.name, func(t *testing.T) {
11916+
svc := &v1.Service{
11917+
ObjectMeta: metav1.ObjectMeta{
11918+
Annotations: tt.svcAnnotations,
11919+
},
11920+
}
11921+
got, err := isSkipPrivateIP(svc)
11922+
if (err != nil) != tt.wantErr {
11923+
t.Errorf("isSkipPrivateIP() error = %v, wantErr %v", err, tt.wantErr)
11924+
return
11925+
}
11926+
if got != tt.expected {
11927+
t.Errorf("isSkipPrivateIP() = %v, expected %v", got, tt.expected)
11928+
}
11929+
})
11930+
}
11931+
}

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

Lines changed: 130 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1359,7 +1359,7 @@ func Test_getGoadBalancerStatus(t *testing.T) {
13591359
}
13601360
for name, test := range tests {
13611361
t.Run(name, func(t *testing.T) {
1362-
actual, err := loadBalancerToStatus(test.lb, test.setIpMode)
1362+
actual, err := loadBalancerToStatus(test.lb, test.setIpMode, false)
13631363
if !assertError(err, test.wantErr) {
13641364
t.Errorf("Expected error = %v, but got %v", test.wantErr, err)
13651365
return
@@ -2249,3 +2249,132 @@ func Test_getOciIpVersions(t *testing.T) {
22492249
})
22502250
}
22512251
}
2252+
2253+
func TestLoadBalancerToStatus(t *testing.T) {
2254+
type testCase struct {
2255+
name string
2256+
lb *client.GenericLoadBalancer
2257+
ipMode v1.LoadBalancerIPMode
2258+
skipPrivateIp bool
2259+
expectedOutput *v1.LoadBalancerStatus
2260+
expectedError error
2261+
}
2262+
2263+
testCases := []testCase{
2264+
{
2265+
name: "No IP Addresses",
2266+
lb: &client.GenericLoadBalancer{
2267+
DisplayName: common.String("test-lb"),
2268+
IpAddresses: []client.GenericIpAddress{},
2269+
},
2270+
ipMode: v1.LoadBalancerIPModeVIP,
2271+
skipPrivateIp: false,
2272+
expectedOutput: nil,
2273+
expectedError: errors1.Errorf("no ip addresses found for load balancer \"test-lb\""),
2274+
},
2275+
{
2276+
name: "Single Public IP Address",
2277+
lb: &client.GenericLoadBalancer{
2278+
DisplayName: common.String("test-lb"),
2279+
IpAddresses: []client.GenericIpAddress{
2280+
{
2281+
IpAddress: common.String("192.168.1.100"),
2282+
IsPublic: common.Bool(true),
2283+
},
2284+
},
2285+
},
2286+
ipMode: v1.LoadBalancerIPModeVIP,
2287+
skipPrivateIp: false,
2288+
expectedOutput: &v1.LoadBalancerStatus{
2289+
Ingress: []v1.LoadBalancerIngress{
2290+
{
2291+
IP: "192.168.1.100",
2292+
},
2293+
},
2294+
},
2295+
expectedError: nil,
2296+
},
2297+
{
2298+
name: "Multiple IP Addresses",
2299+
lb: &client.GenericLoadBalancer{
2300+
DisplayName: common.String("test-lb"),
2301+
IpAddresses: []client.GenericIpAddress{
2302+
{
2303+
IpAddress: common.String("192.168.1.100"),
2304+
IsPublic: common.Bool(true),
2305+
},
2306+
{
2307+
IpAddress: common.String("10.0.0.100"),
2308+
IsPublic: common.Bool(false),
2309+
},
2310+
},
2311+
},
2312+
ipMode: v1.LoadBalancerIPModeVIP,
2313+
skipPrivateIp: false,
2314+
expectedOutput: &v1.LoadBalancerStatus{
2315+
Ingress: []v1.LoadBalancerIngress{
2316+
{
2317+
IP: "192.168.1.100",
2318+
},
2319+
{
2320+
IP: "10.0.0.100",
2321+
},
2322+
},
2323+
},
2324+
expectedError: nil,
2325+
},
2326+
{
2327+
name: "Skip Private IP",
2328+
lb: &client.GenericLoadBalancer{
2329+
DisplayName: common.String("test-lb"),
2330+
IpAddresses: []client.GenericIpAddress{
2331+
{
2332+
IpAddress: common.String("192.168.1.100"),
2333+
IsPublic: common.Bool(true),
2334+
},
2335+
{
2336+
IpAddress: common.String("10.0.0.100"),
2337+
IsPublic: common.Bool(false),
2338+
},
2339+
},
2340+
},
2341+
ipMode: v1.LoadBalancerIPModeVIP,
2342+
skipPrivateIp: true,
2343+
expectedOutput: &v1.LoadBalancerStatus{
2344+
Ingress: []v1.LoadBalancerIngress{
2345+
{
2346+
IP: "192.168.1.100",
2347+
},
2348+
},
2349+
},
2350+
expectedError: nil,
2351+
},
2352+
{
2353+
name: "Nil IpAddress",
2354+
lb: &client.GenericLoadBalancer{
2355+
DisplayName: common.String("test-lb"),
2356+
IpAddresses: []client.GenericIpAddress{
2357+
{
2358+
IpAddress: nil,
2359+
},
2360+
},
2361+
},
2362+
ipMode: v1.LoadBalancerIPModeVIP,
2363+
skipPrivateIp: false,
2364+
expectedOutput: &v1.LoadBalancerStatus{
2365+
Ingress: []v1.LoadBalancerIngress{},
2366+
},
2367+
expectedError: nil,
2368+
},
2369+
}
2370+
2371+
for _, tc := range testCases {
2372+
t.Run(tc.name, func(t *testing.T) {
2373+
actualOutput, actualError := loadBalancerToStatus(tc.lb, &tc.ipMode, tc.skipPrivateIp)
2374+
reflect.DeepEqual(tc.expectedOutput, actualOutput)
2375+
if tc.expectedError != nil {
2376+
reflect.DeepEqual(tc.expectedError.Error(), actualError.Error())
2377+
}
2378+
})
2379+
}
2380+
}

0 commit comments

Comments
 (0)