From 43e496764d380967c94c785c64d2718d896966e3 Mon Sep 17 00:00:00 2001 From: Vijay Bhargav Eshappa Date: Fri, 24 Jun 2022 14:18:42 +0530 Subject: [PATCH] Make lb flex shape value formats consistent for all lb shape related annotations --- .../providers/oci/load_balancer_spec.go | 10 +- .../providers/oci/load_balancer_spec_test.go | 23 ----- .../providers/oci/load_balancer_util.go | 18 ++++ .../providers/oci/load_balancer_util_test.go | 96 +++++++++++++++++++ 4 files changed, 118 insertions(+), 29 deletions(-) diff --git a/pkg/cloudprovider/providers/oci/load_balancer_spec.go b/pkg/cloudprovider/providers/oci/load_balancer_spec.go index 5ae76c6a2e..b7d3a0a5b2 100644 --- a/pkg/cloudprovider/providers/oci/load_balancer_spec.go +++ b/pkg/cloudprovider/providers/oci/load_balancer_spec.go @@ -1055,15 +1055,13 @@ func getLBShape(svc *v1.Service) (string, *int, *int, error) { ) } - flexShapeMinMbps, err := strconv.Atoi(flexMinS) + flexShapeMinMbps, err := parseFlexibleShapeBandwidth(flexMinS, ServiceAnnotationLoadBalancerShapeFlexMin) if err != nil { - return "", nil, nil, errors.Wrap(err, - fmt.Sprintf("The annotation %s should contain only integer value", ServiceAnnotationLoadBalancerShapeFlexMin)) + return "", nil, nil, err } - flexShapeMaxMbps, err = strconv.Atoi(flexMaxS) + flexShapeMaxMbps, err = parseFlexibleShapeBandwidth(flexMaxS, ServiceAnnotationLoadBalancerShapeFlexMax) if err != nil { - return "", nil, nil, errors.Wrap(err, - fmt.Sprintf("The annotation %s should contain only integer value", ServiceAnnotationLoadBalancerShapeFlexMax)) + return "", nil, nil, err } if flexShapeMinMbps < 10 { flexShapeMinMbps = 10 diff --git a/pkg/cloudprovider/providers/oci/load_balancer_spec_test.go b/pkg/cloudprovider/providers/oci/load_balancer_spec_test.go index 6465be169d..354afdc4ef 100644 --- a/pkg/cloudprovider/providers/oci/load_balancer_spec_test.go +++ b/pkg/cloudprovider/providers/oci/load_balancer_spec_test.go @@ -2200,29 +2200,6 @@ func TestNewLBSpecFailure(t *testing.T) { }, expectedErrMsg: "error parsing service annotation: service.beta.kubernetes.io/oci-load-balancer-shape=flexible requires service.beta.kubernetes.io/oci-load-balancer-shape-flex-min and service.beta.kubernetes.io/oci-load-balancer-shape-flex-max to be set", }, - "invalid flex shape non int min/max": { - defaultSubnetOne: "one", - defaultSubnetTwo: "two", - service: &v1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "kube-system", - Name: "testservice", - UID: "test-uid", - Annotations: map[string]string{ - ServiceAnnotationLoadBalancerShape: "flexible", - ServiceAnnotationLoadBalancerShapeFlexMin: "10Mbps", - ServiceAnnotationLoadBalancerShapeFlexMax: "100Mbps", - }, - }, - Spec: v1.ServiceSpec{ - SessionAffinity: v1.ServiceAffinityNone, - Ports: []v1.ServicePort{ - {Protocol: v1.ProtocolTCP}, - }, - }, - }, - expectedErrMsg: `The annotation service.beta.kubernetes.io/oci-load-balancer-shape-flex-min should contain only integer value: strconv.Atoi: parsing "10Mbps": invalid syntax`, - }, "invalid loadbalancer policy": { defaultSubnetOne: "one", defaultSubnetTwo: "two", diff --git a/pkg/cloudprovider/providers/oci/load_balancer_util.go b/pkg/cloudprovider/providers/oci/load_balancer_util.go index 38367d65f5..3ec3673dd8 100644 --- a/pkg/cloudprovider/providers/oci/load_balancer_util.go +++ b/pkg/cloudprovider/providers/oci/load_balancer_util.go @@ -18,6 +18,7 @@ import ( "context" "fmt" "os" + "regexp" "sort" "strconv" "strings" @@ -62,6 +63,8 @@ const ( Delete = "delete" ) +const nonAlphanumericRegexExpression = "[^a-zA-Z0-9]+" + // Action that should take place on the resource. type Action interface { Type() ActionType @@ -691,3 +694,18 @@ func getMetric(lbtype string, metricType string) string { } return "" } + +func parseFlexibleShapeBandwidth(shape, annotation string) (int, error) { + reg, _ := regexp.Compile(nonAlphanumericRegexExpression) + processedString := reg.ReplaceAllString(shape, "") + if strings.HasSuffix(processedString, "Mbps") { + processedString = strings.TrimSuffix(processedString, "Mbps") + } else if strings.HasSuffix(processedString, "mbps") { + processedString = strings.TrimSuffix(processedString, "mbps") + } + parsedIntFlexibleShape, err := strconv.Atoi(processedString) + if err != nil { + return 0, fmt.Errorf("invalid format for %s annotation : %v", annotation, shape) + } + return parsedIntFlexibleShape, nil +} diff --git a/pkg/cloudprovider/providers/oci/load_balancer_util_test.go b/pkg/cloudprovider/providers/oci/load_balancer_util_test.go index c8d163aac1..d9bc83e1fe 100644 --- a/pkg/cloudprovider/providers/oci/load_balancer_util_test.go +++ b/pkg/cloudprovider/providers/oci/load_balancer_util_test.go @@ -1898,3 +1898,99 @@ func TestHasLoadBalancerNetworkSecurityGroupsChanged(t *testing.T) { }) } } + +func Test_parseFlexibleShapeBandwidth(t *testing.T) { + var testCases = []struct { + name string + annotation string + inputShape string + expectedOutputShape int + errMessage string + wantErr bool + }{ + { + name: "Valid Flexible Shape Min - Only integer", + annotation: ServiceAnnotationLoadBalancerShapeFlexMin, + inputShape: "100", + expectedOutputShape: 100, + errMessage: "", + wantErr: false, + }, + { + name: "Valid Flexible Shape Max - Only integer", + annotation: ServiceAnnotationLoadBalancerShapeFlexMax, + inputShape: "8000", + expectedOutputShape: 8000, + errMessage: "", + wantErr: false, + }, + { + name: "Valid Flexible Shape Min - With Unit (Uppercase M)", + annotation: ServiceAnnotationLoadBalancerShapeFlexMin, + inputShape: "100Mbps", + expectedOutputShape: 100, + errMessage: "", + wantErr: false, + }, + { + name: "Valid Flexible Shape Max - With Unit (Uppercase M)", + annotation: ServiceAnnotationLoadBalancerShapeFlexMax, + inputShape: "8000Mbps", + expectedOutputShape: 8000, + errMessage: "", + wantErr: false, + }, + { + name: "Valid Flexible Shape Min - With Unit (Lowercase m)", + annotation: ServiceAnnotationLoadBalancerShapeFlexMin, + inputShape: "100mbps", + expectedOutputShape: 100, + errMessage: "", + wantErr: false, + }, + { + name: "Valid Flexible Shape Max - With Unit (Lowercase m)", + annotation: ServiceAnnotationLoadBalancerShapeFlexMax, + inputShape: "8000mbps", + expectedOutputShape: 8000, + errMessage: "", + wantErr: false, + }, + { + name: "Invalid Flexible Shape Min", + annotation: ServiceAnnotationLoadBalancerShapeFlexMin, + inputShape: "100kbps", + expectedOutputShape: 0, + errMessage: "invalid format for service.beta.kubernetes.io/oci-load-balancer-shape-flex-min annotation : 100kbps", + wantErr: true, + }, + { + name: "Invalid Flexible Shape Max", + annotation: ServiceAnnotationLoadBalancerShapeFlexMax, + inputShape: "8000kbps", + expectedOutputShape: 0, + errMessage: "invalid format for service.beta.kubernetes.io/oci-load-balancer-shape-flex-max annotation : 8000kbps", + wantErr: true, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + parsedShape, err := parseFlexibleShapeBandwidth(tt.inputShape, tt.annotation) + if err != nil { + if !tt.wantErr { + t.Errorf("parseFlexibleShapeBandwidth() error = %v, wantErr = %v", err, tt.wantErr) + } + if !reflect.DeepEqual(err.Error(), tt.errMessage) { + t.Errorf("parseFlexibleShapeBandwidth() got errMessage = %v, want errMessage = %v", err.Error(), tt.errMessage) + } + } + if err == nil && tt.wantErr { + t.Errorf("parseFlexibleShapeBandwidth() error = %v, wantErr = %v", err, tt.wantErr) + } + if err == nil && parsedShape != tt.expectedOutputShape { + t.Errorf("parseFlexibleShapeBandwidth() got = %v, want = %v", parsedShape, tt.expectedOutputShape) + } + }) + } +}