Merge pull request #1396 from MartinForReal/release-1.0/backport_1384

[release-1.0]fix: fallback to tcp if appprotocol is not supported
kubernetes-sigs · Apr 2, 2022 · 0ab0c96 · 0ab0c96
2 parents 2f3c6a4 + 4b22c81
commit 0ab0c96
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 35 deletions.
diff --git a/pkg/consts/consts.go b/pkg/consts/consts.go
@@ -386,7 +386,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
@@ -2066,52 +2066,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/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
@@ -1778,11 +1778,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)",
@@ -1853,13 +1862,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",
@@ -1953,7 +1973,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),
 		},
 		{