Repository: kubernetes-sigs/cloud-provider-azure
Pull request #1384 (2022-03-31 09:08:05)

Fix: fallback to tcp if appprotocol is not supported

#### What type of PR is this?
/kind bug
/kind regression


#### What this PR does / why we need it:

Fall back to tcp if user assigned probe protocol is not supported.

#### Which issue(s) this PR fixes:

Fixes #

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?
```release-note
The cloud provider will create TCP probe rule if the probe protocol is not supported. 
```

#### Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

```docs

```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/cloud-provider-azure/pull/1384#" title="Author self-approved">MartinForReal</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcloud-provider-azure).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/cloud-provider-azure/blob/master/OWNERS)~~ [MartinForReal]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
k8s-ci-robot: @MartinForReal: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-cloud-provider-azure-unit | b51af3e61a5c0993571a81f639d67837ac99582d | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cloud-provider-azure/1384/pull-cloud-provider-azure-unit/1509457497046388736) | true | `/test pull-cloud-provider-azure-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cloud-provider-azure&pr=1384). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3AMartinForReal). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cloud-provider-azure/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/pkg/consts/consts.go b/pkg/consts/consts.go
index 2e0f9c292..47fc7e70d 100644
--- a/pkg/consts/consts.go
+++ b/pkg/consts/consts.go
@@ -401,7 +401,7 @@ const (
 	// 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.
 	HealthProbeParamsRequestPath  HealthProbeParams = "request-path"
-	HealthProbeDefaultRequestPath string            = "/healthz"
+	HealthProbeDefaultRequestPath string            = "/"
 )
 
 type HealthProbeParams string
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
index 7cce0859c..b94c582d8 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -1921,52 +1921,56 @@ func lbRuleConflictsWithPort(rule network.LoadBalancingRule, frontendIPConfigID
 // for following sku: basic loadbalancer vs standard load balancer
 // for following protocols: TCP HTTP HTTPS(SLB only)
 func (az *Cloud) buildHealthProbeRulesForPort(annotations map[string]string, port v1.ServicePort, lbrule string) (*network.Probe, error) {
-	properties := &network.ProbePropertiesFormat{}
-	// get request path ,only used with http/https probe
-	path, err := consts.GetHealthProbeConfigOfPortFromK8sSvcAnnotation(annotations, port.Port, consts.HealthProbeParamsRequestPath)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse annotation %s: %w", consts.BuildHealthProbeAnnotationKeyForPort(port.Port, consts.HealthProbeParamsRequestPath), err)
-	}
-	if path == nil {
-		if path, err = consts.GetAttributeValueInSvcAnnotation(annotations, consts.ServiceAnnotationLoadBalancerHealthProbeRequestPath); err != nil {
-			return nil, fmt.Errorf("failed to parse annotation %s: %w", consts.ServiceAnnotationLoadBalancerHealthProbeRequestPath, err)
-		}
-	}
-	if path == nil {
-		path = to.StringPtr(consts.HealthProbeDefaultRequestPath)
+	if port.Protocol == v1.ProtocolUDP || port.Protocol == v1.ProtocolSCTP {
+		return nil, nil
 	}
+	// protocol should be tcp, because sctp is handled in outer loop
+
+	properties := &network.ProbePropertiesFormat{}
+	var err error
 	if port.AppProtocol == nil {
 		if port.AppProtocol, err = consts.GetAttributeValueInSvcAnnotation(annotations, consts.ServiceAnnotationLoadBalancerHealthProbeProtocol); err != nil {
 			return nil, fmt.Errorf("failed to parse annotation %s: %w", consts.ServiceAnnotationLoadBalancerHealthProbeProtocol, err)
 		}
 		if port.AppProtocol == nil {
-			if port.Protocol == v1.ProtocolTCP {
-				port.AppProtocol = to.StringPtr(string(network.ProtocolTCP))
-			}
-		}
-		if port.AppProtocol == nil {
-			// health probe not set, return
-			return nil, nil
+			port.AppProtocol = to.StringPtr(string(network.ProtocolTCP))
 		}
 	}
-	switch protocol := strings.TrimSpace(*port.AppProtocol); {
+	protocol := strings.TrimSpace(*port.AppProtocol)
+	switch {
+	case strings.EqualFold(protocol, string(network.ProtocolTCP)):
+		properties.Protocol = network.ProbeProtocolTCP
 	case strings.EqualFold(protocol, string(network.ProtocolHTTPS)):
 		//HTTPS probe is only supported in standard loadbalancer
+		//For backward compatibility,when unsupported protocol is used, fall back to tcp protocol in basic lb mode instead
 		if !az.useStandardLoadBalancer() {
-			return nil, fmt.Errorf("HTTPS protocol is not supported in health probe when basic lb is used")
+			properties.Protocol = network.ProbeProtocolTCP
+		} else {
+			properties.Protocol = network.ProbeProtocolHTTPS
 		}
-		//HTTP and HTTPS share the same configuration
-		properties.Protocol = network.ProbeProtocolHTTPS
-		properties.RequestPath = path
 	case strings.EqualFold(protocol, string(network.ProtocolHTTP)):
 		properties.Protocol = network.ProbeProtocolHTTP
-		properties.RequestPath = path
-	case strings.EqualFold(protocol, string(network.ProtocolTCP)):
-		properties.Protocol = network.ProbeProtocolTCP
 	default:
-		return nil, fmt.Errorf("unsupported protocol %s", protocol)
+		//For backward compatibility,when unsupported protocol is used, fall back to tcp protocol in basic lb mode instead
+		properties.Protocol = network.ProbeProtocolTCP
 	}
 
+	if strings.EqualFold(string(properties.Protocol), string(network.ProtocolHTTPS)) || strings.EqualFold(string(properties.Protocol), string(network.ProtocolHTTP)) {
+		// get request path ,only used with http/https probe
+		path, err := consts.GetHealthProbeConfigOfPortFromK8sSvcAnnotation(annotations, port.Port, consts.HealthProbeParamsRequestPath)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse annotation %s: %w", consts.BuildHealthProbeAnnotationKeyForPort(port.Port, consts.HealthProbeParamsRequestPath), err)
+		}
+		if path == nil {
+			if path, err = consts.GetAttributeValueInSvcAnnotation(annotations, consts.ServiceAnnotationLoadBalancerHealthProbeRequestPath); err != nil {
+				return nil, fmt.Errorf("failed to parse annotation %s: %w", consts.ServiceAnnotationLoadBalancerHealthProbeRequestPath, err)
+			}
+		}
+		if path == nil {
+			path = to.StringPtr(consts.HealthProbeDefaultRequestPath)
+		}
+		properties.RequestPath = path
+	}
 	// get number of probes
 	var numOfProbeValidator = 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
diff --git a/site/content/en/topics/loadbalancer.md b/site/content/en/topics/loadbalancer.md
index 8b55ec3bc..606bfe2b9 100644
--- a/site/content/en/topics/loadbalancer.md
+++ b/site/content/en/topics/loadbalancer.md
@@ -156,22 +156,27 @@ As documented [here](https://docs.microsoft.com/en-us/azure/load-balancer/load-b
 
 Currently, the default protocol of the health probe varies among services with different transport protocols, app protocols, annotations and external traffic policies.
 
-1. for local services, HTTP and /healthz would be used.
+1. for local services, HTTP and /healthz would be used. The health probe will query NodeHealthPort rather than actual backend service
 1. for cluster TCP services, TCP would be used.
 1. for cluster UDP services, no health probes.
 
 Since v1.20, two service annotations `service.beta.kubernetes.io/azure-load-balancer-health-probe-request-path` are introduced, which determine the new health probe behavior. If the spec.ports.appProtocol is set, both local and cluster TCP services would use the specified health probe protocol. If the `service.beta.kubernetes.io/azure-load-balancer-health-probe-request-path` is set, the specified request path would be used instead of `/healthz`. Note that the request path would be ignored when using TCP or the spec.ports.appProtocol is empty. More specifically:
 
-| `externalTrafficPolicy` | spec.ports.AppProtocol | `service.beta.kubernetes.io/azure-load-balancer-health-probe-request-path` | protocol | request path |
-| ------------------------------------------------------------ | ---------------------------- | ------------------------------------------------------------ |------| ----- |
-| local |  | (ignored) | http | `/healthz` |
-| local | tcp | (ignored) | tcp | null |
-| local | http/https | | http/https | `/healthz` |
-| local | http/https | `/custom-path` | http/https | `/custom-path` |
-| cluster |  | (ignored) | tcp | null |
-| cluster | tcp | (ignored) | tcp | null |
-| cluster | http/https | | http/https | `/healthz` |
-| cluster | http/https | `/custom-path` | http/https | `/custom-path` |
+|loadbalancer sku| `externalTrafficPolicy` | spec.ports.Protocol |spec.ports.AppProtocol| `service.beta.kubernetes.io/azure-load-balancer-health-probe-request-path` | protocol | request path |
+|---| ------------------------------------------------------------ | ---------------------------- | ----------------------------------------------------|-------- |------| ----- |
+| standard| local |any| any | any | http | `/healthz` |
+| standard| cluster |udp| any | any | null | null |
+| standard| cluster |tcp|  | (ignored) | tcp | null |
+| standard| cluster |tcp| tcp | (ignored) | tcp | null |
+| standard| cluster |tcp| http/https | | http/https | `/` |
+| standard| cluster |tcp| http/https | `/custom-path` | http/https | `/custom-path` |
+| standard| cluster |tcp| unsupported protocol | `/custom-path` | tcp | null (For backward compatibility) |
+| basic| local |any| any | any | http | `/healthz` |
+| basic| cluster |tcp|  | (ignored) | tcp | null |
+| basic| cluster |tcp| tcp | (ignored) | tcp | null |
+| basic| cluster |tcp| http | | http | `/` |
+| basic| cluster |tcp| http | `/custom-path` | http | `/custom-path` |
+| basic| cluster |tcp| unsupported protocol | `/custom-path` | tcp | null (For backward compatibility)|
 
 Since v1.21, two service annotations `service.beta.kubernetes.io/azure-load-balancer-health-probe-interval` and `load-balancer-health-probe-num-of-probe` are introduced, which customize the configuration of health probe. If `service.beta.kubernetes.io/azure-load-balancer-health-probe-interval` is not set, Default value of 5 is applied. If `load-balancer-health-probe-num-of-probe` is not set, Default value of 2 is applied. And total probe should be less than 120 seconds.
 


Test patch:
diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
index fab6f259d..49e7ac8dd 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -1779,11 +1779,20 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 			expectedRules:   getDefaultTestRules(false),
 		},
 		{
-			desc:            "getExpectedLBRules shall return error on non supported protocols",
+			desc:            "getExpectedLBRules shall return tcp probe on non supported protocols when basic slb sku is used",
+			service:         getTestService("test1", v1.ProtocolTCP, map[string]string{}, false, 80),
+			loadBalancerSku: "basic",
+			probeProtocol:   "Mongodb",
+			expectedRules:   getDefaultTestRules(false),
+			expectedProbes:  getDefaultTestProbes("Tcp", ""),
+		},
+		{
+			desc:            "getExpectedLBRules shall return tcp probe on https protocols when basic slb sku is used",
 			service:         getTestService("test1", v1.ProtocolTCP, map[string]string{}, false, 80),
 			loadBalancerSku: "basic",
 			probeProtocol:   "Https",
-			expectedErr:     true,
+			expectedRules:   getDefaultTestRules(false),
+			expectedProbes:  getDefaultTestProbes("Tcp", ""),
 		},
 		{
 			desc:            "getExpectedLBRules shall return error (slb with external mode and SCTP)",
@@ -1854,13 +1863,24 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 			expectedRules:   getDefaultTestRules(true),
 		},
 		{
-			desc: "getExpectedLBRules should return error when invalid protocol is defined",
+			desc: "getExpectedLBRules should return tcp probe when invalid protocol is defined",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
 				"service.beta.kubernetes.io/port_80_health-probe_request-path": "/healthy1",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "TCP1",
-			expectedErr:     true,
+			expectedProbes:  getDefaultTestProbes("Tcp", ""),
+			expectedRules:   getDefaultTestRules(true),
+		},
+		{
+			desc: "getExpectedLBRules should return tcp probe when invalid protocol is defined",
+			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
+				"service.beta.kubernetes.io/port_80_health-probe_request-path": "/healthy1",
+			}, false, 80),
+			loadBalancerSku: "basic",
+			probeProtocol:   "TCP1",
+			expectedProbes:  getDefaultTestProbes("Tcp", ""),
+			expectedRules:   getDefaultTestRules(false),
 		},
 		{
 			desc: "getExpectedLBRules should return correct rule when deprecated annotations are added",
@@ -1954,7 +1974,7 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Http",
-			expectedProbes:  getTestProbes("Http", "/healthz", to.Int32Ptr(20), to.Int32Ptr(10080), to.Int32Ptr(5)),
+			expectedProbes:  getTestProbes("Http", "/", to.Int32Ptr(20), to.Int32Ptr(10080), to.Int32Ptr(5)),
 			expectedRules:   getDefaultTestRules(true),
 		},
 		{