Repository: kubernetes-sigs/cloud-provider-azure
Pull request #1627 (2022-04-30 05:55:11)

fix-1.0: ensure TCP probe is used by default for backward compatibility
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide/first-contribution.md#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/sig-release/release.md#issuepr-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/sig-testing/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

#### What type of PR is this?

<!--
Add one of the following kinds:
/kind bug
/kind cleanup
/kind documentation
/kind feature
/kind design

Optionally add one or more of the following kinds if applicable:
/kind api-change
/kind deprecation
/kind failing-test
/kind flake
/kind regression
-->

/kind bug

#### What this PR does / why we need it:

fix: ensure TCP probe is used by default for backward compatibility. 

When appProtocol is set to HTTP/HTTPS, continue to use TCP as probe protocol by default. The probe protocol would be changed same to appProtocol in 1.24 release version.

#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".

For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md
-->
```release-note
fix: ensure TCP probe is used by default for backward compatibility. When appProtocol is set to HTTP/HTTPS, continue to use TCP as probe protocol by default. The probe protocol would be changed same to appProtocol in 1.24 release version.
```

#### Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

<!--
This section can be blank if this pull request does not require a release note.

When adding links which point to resources within git repositories, like
KEPs or supporting documentation, please reference a specific commit and avoid
linking directly to the master branch. This ensures that links reference a
specific point in time, rather than a document that may change over time.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [KEP]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```



Patch:
diff --git a/pkg/consts/consts.go b/pkg/consts/consts.go
index 4ef8f0b2d..6c3bc9f31 100644
--- a/pkg/consts/consts.go
+++ b/pkg/consts/consts.go
@@ -387,7 +387,7 @@ const (
 
 	// HealthProbeParamsRequestPath 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.
+	// `/` would be configured by default.
 	HealthProbeParamsRequestPath  HealthProbeParams = "request-path"
 	HealthProbeDefaultRequestPath string            = "/"
 )
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
index 0d4497846..04a055194 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -2069,7 +2069,7 @@ func lbRuleConflictsWithPort(rule network.LoadBalancingRule, frontendIPConfigID
 		*rule.FrontendPort == port.Port
 }
 
-// buildHealthProbeRulesForPort
+// buildHealthProbeRulesForPort builds the LoadBalancer probe rules for the given port.
 // 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) {
@@ -2078,8 +2078,22 @@ func (az *Cloud) buildHealthProbeRulesForPort(annotations map[string]string, por
 	}
 	// protocol should be tcp, because sctp is handled in outer loop
 
+	// get request path for http/https probe
+	requestPath, 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 requestPath == nil {
+		if requestPath, err = consts.GetAttributeValueInSvcAnnotation(annotations, consts.ServiceAnnotationLoadBalancerHealthProbeRequestPath); err != nil {
+			return nil, fmt.Errorf("failed to parse annotation %s: %w", consts.ServiceAnnotationLoadBalancerHealthProbeRequestPath, err)
+		}
+	}
+	if requestPath == nil {
+		// TCP would be used as default probe protocol for backward compatibility when probe requestPath is not set.
+		port.AppProtocol = to.StringPtr(string(network.ProtocolTCP))
+	}
+
 	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)
@@ -2093,8 +2107,8 @@ func (az *Cloud) buildHealthProbeRulesForPort(annotations map[string]string, por
 	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
+		// HTTPS probe is only supported in standard loadbalancer
+		// For backward compatibility, fall back to TCP protocol for basic LoadBalancer.
 		if !az.useStandardLoadBalancer() {
 			properties.Protocol = network.ProbeProtocolTCP
 		} else {
@@ -2103,25 +2117,16 @@ func (az *Cloud) buildHealthProbeRulesForPort(annotations map[string]string, por
 	case strings.EqualFold(protocol, string(network.ProtocolHTTP)):
 		properties.Protocol = network.ProbeProtocolHTTP
 	default:
-		//For backward compatibility,when unsupported protocol is used, fall back to tcp protocol in basic lb mode instead
+		// For backward compatibility, fall back to TCP protocol when unsupported protocol is used.
 		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)
+		if requestPath == nil {
+			// "/" is the default request path for HTTP/HTTPS probe
+			requestPath = to.StringPtr(consts.HealthProbeDefaultRequestPath)
 		}
-		properties.RequestPath = path
+		properties.RequestPath = requestPath
 	}
 	// get number of probes
 	var numOfProbeValidator = func(val *int32) error {


Test patch:
diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
index 4a730572d..98ecd6837 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -1954,7 +1954,7 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 			expectedErr:     true,
 		},
 		{
-			desc: "getExpectedLBRules should return correct rule when health probe annotations are added",
+			desc: "getExpectedLBRules should return correct rule when health probe annotations are added with probe path",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
 				"service.beta.kubernetes.io/port_80_health-probe_interval":     "20",
 				"service.beta.kubernetes.io/port_80_health-probe_num-of-probe": "5",
@@ -1966,14 +1966,14 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 			expectedRules:   getDefaultTestRules(true),
 		},
 		{
-			desc: "getExpectedLBRules should return correct rule when health probe annotations are added,default path should be /healthy",
+			desc: "getExpectedLBRules should return correct rule when health probe annotations are added with default TCP protocol",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
 				"service.beta.kubernetes.io/port_80_health-probe_interval":     "20",
 				"service.beta.kubernetes.io/port_80_health-probe_num-of-probe": "5",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Http",
-			expectedProbes:  getTestProbes("Http", "/", to.Int32Ptr(20), to.Int32Ptr(10080), to.Int32Ptr(5)),
+			expectedProbes:  getTestProbes("Tcp", "", to.Int32Ptr(20), to.Int32Ptr(10080), to.Int32Ptr(5)),
 			expectedRules:   getDefaultTestRules(true),
 		},
 		{