Repository: kubernetes-sigs/cloud-provider-azure (go)

Issue:
Add more parameters support to annotations in order to customize probe behavior. 
#### What would you like to be added:

Add  numOfProbe and probeInterval annotation to supported annotation list in order to customize probe behavior. 

#### Why is this needed:
Customer may want to customize health probe behavior rather than using default configuration when they are running mission critical workloads.

Pull request:
Add numOfProbe and probeInterval in health probe configuration
Signed-off-by: Shangxiang Fan <shafan@microsoft.com>



#### What type of PR is this?

/kind feature


#### What this PR does / why we need it:

#### Which issue(s) this PR fixes:

Fixes #948 

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?

```release-note
Two new annotations are added in order to customize health probe behavior

service.beta.kubernetes.io/azure-load-balancer-health-probe-interval: "15"
service.beta.kubernetes.io/azure-load-balancer-health-probe-num-of-probe: "2"
```

#### Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

```docs

```


Patch:
diff --git a/pkg/consts/consts.go b/pkg/consts/consts.go
index 6edd9f1a2..bcb415587 100644
--- a/pkg/consts/consts.go
+++ b/pkg/consts/consts.go
@@ -259,6 +259,14 @@ const (
 	// If not set, the local service would use the HTTP and the cluster service would use the TCP by default.
 	ServiceAnnotationLoadBalancerHealthProbeProtocol = "service.beta.kubernetes.io/azure-load-balancer-health-probe-protocol"
 
+	// ServiceAnnotationLoadBalancerHealthProbeInterval determines the probe interval of the load balancer health probe.
+	// The minimum probe interval is 5 seconds and the default value is 15. The total duration of all intervals cannot exceed 120 seconds.
+	ServiceAnnotationLoadBalancerHealthProbeInterval = "service.beta.kubernetes.io/azure-load-balancer-health-probe-interval"
+
+	// ServiceAnnotationLoadBalancerHealthProbeNumOfProbe determines the minimum number of unhealthy responses which load balancer cannot tolerate.
+	// The minimum number of probe is 1. The total duration of all intervals cannot exceed 120 seconds.
+	ServiceAnnotationLoadBalancerHealthProbeNumOfProbe = "service.beta.kubernetes.io/azure-load-balancer-health-probe-num-of-probe"
+
 	// ServiceAnnotationLoadBalancerHealthProbeRequestPath determines the request path of the load balancer health probe.
 	// This is only useful for the HTTP and HTTPS, and would be ignored when using TCP. If not set,
 	// `/healthz` would be configured by default.
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
index db9b0a3bf..72180a3c6 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -1296,6 +1296,87 @@ func getIdleTimeout(s *v1.Service) (*int32, error) {
 	return &to32, nil
 }
 
+// getProbeIntervalInSecondsAndNumOfProbe parse probeInterval and numberOfProbes from the annotations of service object.
+func getProbeIntervalInSecondsAndNumOfProbe(s *v1.Service) (*int32, *int32, error) {
+	// get number of probes
+	numberOfProbes, err := getInt32FromAnnotations(s.Annotations, consts.ServiceAnnotationLoadBalancerHealthProbeNumOfProbe, func(val *int32) error {
+		//minimum number of unhealthy responses is 2. ref: https://docs.microsoft.com/en-us/rest/api/load-balancer/load-balancers/create-or-update#probe
+		const (
+			MinimumNumOfProbe = 2
+		)
+		if *val < MinimumNumOfProbe {
+			return fmt.Errorf("the minimum value of %s is %d", consts.ServiceAnnotationLoadBalancerHealthProbeNumOfProbe, MinimumNumOfProbe)
+		}
+		return nil
+	})
+	if err != nil {
+		return nil, nil, err
+	}
+	// if numberOfProbes is not set, set it to default instead ref: https://docs.microsoft.com/en-us/rest/api/load-balancer/load-balancers/create-or-update#probe
+	if numberOfProbes == nil {
+		numberOfProbes = to.Int32Ptr(2)
+	}
+
+	probeInterval, err := getInt32FromAnnotations(s.Annotations, consts.ServiceAnnotationLoadBalancerHealthProbeInterval, func(val *int32) error {
+		//minimum probe interval in seconds is 5. ref: https://docs.microsoft.com/en-us/rest/api/load-balancer/load-balancers/create-or-update#probe
+		const (
+			MinimumProbeIntervalInSecond = 5
+		)
+		if *val < 5 {
+			return fmt.Errorf("the minimum value of %s is %d", consts.ServiceAnnotationLoadBalancerHealthProbeInterval, MinimumProbeIntervalInSecond)
+		}
+		return nil
+	})
+	if err != nil {
+		return nil, nil, err
+	}
+	// if probeInterval is not set, set it to default instead ref: https://docs.microsoft.com/en-us/rest/api/load-balancer/load-balancers/create-or-update#probe
+	if probeInterval == nil {
+		probeInterval = to.Int32Ptr(5)
+	}
+
+	// total probe should be less than 120 seconds ref: https://docs.microsoft.com/en-us/rest/api/load-balancer/load-balancers/create-or-update#probe
+	if (*probeInterval)*(*numberOfProbes) >= 120 {
+		return nil, nil, fmt.Errorf("total probe should be less than 120, please adjust interval and number of probe accordingly")
+	}
+
+	return probeInterval, numberOfProbes, nil
+}
+
+// Int32BusinessValidator is validator function which is invoked after values are parsed in order to make sure input value meets the businees need.
+type Int32BusinessValidator func(*int32) error
+
+// getInt32FromAnnotations parse integer value from annotation and return an reference to int32 object
+func getInt32FromAnnotations(annotations map[string]string, key string, businessValidator ...Int32BusinessValidator) (*int32, error) {
+	if len(key) <= 0 {
+		return nil, fmt.Errorf("annotation key should not be empty")
+	}
+	if annotations == nil {
+		// Return a nil here as this will set the value to the azure default
+		return nil, nil
+	}
+	val, ok := annotations[key]
+	if !ok {
+		// Return a nil here as this will set the value to the azure default
+		return nil, nil
+	}
+	errKey := fmt.Errorf("%s value must be a whole number", key)
+	toInt, err := strconv.ParseInt(val, 10, 32)
+	if err != nil {
+		return nil, fmt.Errorf("error parsing %s value: %w: %v", key, err, errKey)
+	}
+	parsedInt := int32(toInt)
+	for _, validator := range businessValidator {
+		if validator != nil {
+			err := validator(&parsedInt)
+			if err != nil {
+				return nil, fmt.Errorf("error parsing %s value: %w", key, err)
+			}
+		}
+	}
+	return &parsedInt, nil
+}
+
 func (az *Cloud) isFrontendIPChanged(clusterName string, config network.FrontendIPConfiguration, service *v1.Service, lbFrontendIPConfigName string) (bool, error) {
 	isServiceOwnsFrontendIP, isPrimaryService, err := az.serviceOwnsFrontendIP(config, service)
 	if err != nil {
@@ -2011,6 +2092,10 @@ func (az *Cloud) getExpectedLBRules(
 		}
 
 		probeProtocol, requestPath := parseHealthProbeProtocolAndPath(service)
+		probeInterval, numberOfProbe, err := getProbeIntervalInSecondsAndNumOfProbe(service)
+		if err != nil {
+			return expectedProbes, expectedRules, err
+		}
 		if servicehelpers.NeedsHealthCheck(service) {
 			podPresencePath, podPresencePort := servicehelpers.GetServiceHealthCheckPathPort(service)
 			if probeProtocol == "" {
@@ -2028,8 +2113,8 @@ func (az *Cloud) getExpectedLBRules(
 					RequestPath:       to.StringPtr(requestPath),
 					Protocol:          network.ProbeProtocol(probeProtocol),
 					Port:              to.Int32Ptr(podPresencePort),
-					IntervalInSeconds: to.Int32Ptr(5),
-					NumberOfProbes:    to.Int32Ptr(2),
+					IntervalInSeconds: probeInterval,
+					NumberOfProbes:    numberOfProbe,
 				},
 			})
 		} else if port.Protocol != v1.ProtocolUDP && port.Protocol != v1.ProtocolSCTP {
@@ -2051,8 +2136,8 @@ func (az *Cloud) getExpectedLBRules(
 					Protocol:          network.ProbeProtocol(probeProtocol),
 					RequestPath:       actualPath,
 					Port:              to.Int32Ptr(port.NodePort),
-					IntervalInSeconds: to.Int32Ptr(5),
-					NumberOfProbes:    to.Int32Ptr(2),
+					IntervalInSeconds: probeInterval,
+					NumberOfProbes:    numberOfProbe,
 				},
 			})
 		}


Test patch:
diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
index 3b49d776b..2d490541f 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -34,10 +34,8 @@ import (
 	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
-
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/loadbalancerclient/mockloadbalancerclient"
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/publicipclient/mockpublicipclient"
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/securitygroupclient/mocksecuritygroupclient"
@@ -4855,3 +4853,133 @@ func TestGetClusterFromPIPClusterTags(t *testing.T) {
 		assert.Equal(t, actual, c.expected, "TestCase[%d]: %s", i, c.desc)
 	}
 }
+
+func Test_getInt32FromAnnotations(t *testing.T) {
+	type args struct {
+		annotations       map[string]string
+		key               string
+		businessValidator []Int32BusinessValidator
+	}
+	key := "testkey"
+	tests := []struct {
+		name    string
+		args    args
+		want    *int32
+		wantErr bool
+	}{
+		{name: "no key specified", args: args{}, want: nil, wantErr: true},
+		{name: "no key specified even though annotation set is not empty", args: args{annotations: map[string]string{key: ""}}, want: nil, wantErr: true},
+		{name: "no annotation", args: args{key: key}, want: nil, wantErr: false},
+		{name: "annotation empty value", args: args{annotations: map[string]string{key: ""}, key: key}, wantErr: true},
+		{name: "annotation not a number", args: args{annotations: map[string]string{key: "cookies"}, key: key}, wantErr: true},
+		{name: "annotation zero value", args: args{annotations: map[string]string{key: "0"}, key: key}, wantErr: false, want: to.Int32Ptr(0)},
+		{name: "annotation float value", args: args{annotations: map[string]string{key: "0.1"}, key: key}, wantErr: true},
+		{name: "annotation positive value", args: args{annotations: map[string]string{key: "24"}, key: key}, want: to.Int32Ptr(24), wantErr: false},
+		{name: "annotation negative value", args: args{annotations: map[string]string{key: "-6"}, key: key}, want: to.Int32Ptr(-6), wantErr: false},
+		{name: "validator is nil", args: args{annotations: map[string]string{key: "-6"}, key: key, businessValidator: []Int32BusinessValidator{
+			nil,
+		}}, want: to.Int32Ptr(-6), wantErr: false},
+		{name: "validator failed", args: args{annotations: map[string]string{key: "-6"}, key: key, businessValidator: []Int32BusinessValidator{
+			func(i *int32) error {
+				return fmt.Errorf("validator failed")
+			},
+		}}, wantErr: true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := getInt32FromAnnotations(tt.args.annotations, tt.args.key, tt.args.businessValidator...)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("getInt32FromAnnotations() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("getInt32FromAnnotations() = %v, want %v", *got, *tt.want)
+			}
+		})
+	}
+}
+
+func Test_getProbeIntervalInSecondsAndNumOfProbe(t *testing.T) {
+	type args struct {
+		annotations map[string]string
+	}
+	tests := []struct {
+		name              string
+		args              args
+		wantProveInterval *int32
+		wantNumOfProbe    *int32
+		wantErr           bool
+	}{
+		{
+			name: "numOfProbe * probeInterval is not less than 120",
+			args: args{
+				annotations: map[string]string{
+					consts.ServiceAnnotationLoadBalancerHealthProbeInterval:   "60",
+					consts.ServiceAnnotationLoadBalancerHealthProbeNumOfProbe: "2",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "numOfProbe and probeInterval should be type of integer",
+			args: args{
+				annotations: map[string]string{
+					consts.ServiceAnnotationLoadBalancerHealthProbeInterval:   "48.0",
+					consts.ServiceAnnotationLoadBalancerHealthProbeNumOfProbe: "2",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid numOfProbe",
+			args: args{
+				annotations: map[string]string{
+					consts.ServiceAnnotationLoadBalancerHealthProbeInterval:   "48",
+					consts.ServiceAnnotationLoadBalancerHealthProbeNumOfProbe: "1",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid probeInterval",
+			args: args{
+				annotations: map[string]string{
+					consts.ServiceAnnotationLoadBalancerHealthProbeInterval:   "3",
+					consts.ServiceAnnotationLoadBalancerHealthProbeNumOfProbe: "5",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "valid numOfProbe and probeInterval",
+			args: args{
+				annotations: map[string]string{
+					consts.ServiceAnnotationLoadBalancerHealthProbeInterval:   "48",
+					consts.ServiceAnnotationLoadBalancerHealthProbeNumOfProbe: "2",
+				},
+			},
+			wantProveInterval: to.Int32Ptr(48),
+			wantNumOfProbe:    to.Int32Ptr(2),
+			wantErr:           false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotPodInterval, gotNumOfProbe, err := getProbeIntervalInSecondsAndNumOfProbe(&v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: tt.args.annotations,
+				},
+			})
+			if (err != nil) != tt.wantErr {
+				t.Errorf("getProbeIntervalInSecondsAndNumOfProbe() gotPodInterval = %v, gotNumOfProbe = %v, error = %v, wantErr %v", *gotPodInterval, *gotNumOfProbe, err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(gotPodInterval, tt.wantProveInterval) {
+				t.Errorf("getProbeIntervalInSecondsAndNumOfProbe() got = %v, want %v", *gotPodInterval, *tt.wantProveInterval)
+			}
+			if !reflect.DeepEqual(gotNumOfProbe, tt.wantNumOfProbe) {
+				t.Errorf("getProbeIntervalInSecondsAndNumOfProbe() got1 = %v, want %v", *gotNumOfProbe, *tt.wantProveInterval)
+			}
+		})
+	}
+}