Merge pull request #44578 from MrHohn/ESIPP-GA-1.7

Automatic merge from submit-queue

Refine NeedsHealthCheck logic for ESIPP

This PR is separated from #41162 (will continue breaking it into small PRs). Fixes #42888.

It moves healthcheck util functions to the right place and refines NeedsHealthCheck logic.

/assign @thockin @freehan 

**Release note**:

```release-note
NONE
```

pkg/api/service/annotations.go

@@ -16,13 +16,6 @@ limitations under the License.
 
 package service
 
-import (
-	"strconv"
-
-	"github.com/golang/glog"
-	"k8s.io/kubernetes/pkg/api"
-)
-
 const (
 	// AnnotationLoadBalancerSourceRangesKey is the key of the annotation on a service to set allowed ingress ranges on their LoadBalancers
 	//
@@ -55,45 +48,3 @@ const (
 	// BetaAnnotationExternalTraffic is the beta version of AlphaAnnotationExternalTraffic.
 	BetaAnnotationExternalTraffic = "service.beta.kubernetes.io/external-traffic"
 )
-
-// NeedsHealthCheck Check service for health check annotations
-func NeedsHealthCheck(service *api.Service) bool {
-	// First check the alpha annotation and then the beta. This is so existing
-	// Services continue to work till the user decides to transition to beta.
-	// If they transition to beta, there's no way to go back to alpha without
-	// rolling back the cluster.
-	for _, annotation := range []string{AlphaAnnotationExternalTraffic, BetaAnnotationExternalTraffic} {
-		if l, ok := service.Annotations[annotation]; ok {
-			if l == AnnotationValueExternalTrafficLocal {
-				return true
-			} else if l == AnnotationValueExternalTrafficGlobal {
-				return false
-			} else {
-				glog.Errorf("Invalid value for annotation %v: %v", annotation, l)
-			}
-		}
-	}
-	return false
-}
-
-// GetServiceHealthCheckNodePort Return health check node port annotation for service, if one exists
-func GetServiceHealthCheckNodePort(service *api.Service) int32 {
-	if !NeedsHealthCheck(service) {
-		return 0
-	}
-	// First check the alpha annotation and then the beta. This is so existing
-	// Services continue to work till the user decides to transition to beta.
-	// If they transition to beta, there's no way to go back to alpha without
-	// rolling back the cluster.
-	for _, annotation := range []string{AlphaAnnotationHealthCheckNodePort, BetaAnnotationHealthCheckNodePort} {
-		if l, ok := service.Annotations[annotation]; ok {
-			p, err := strconv.Atoi(l)
-			if err != nil {
-				glog.Errorf("Failed to parse annotation %v: %v", annotation, err)
-				continue
-			}
-			return int32(p)
-		}
-	}
-	return 0
-}

pkg/api/service/util.go

@@ -18,10 +18,13 @@ package service
 
 import (
 	"fmt"
+	"strconv"
 	"strings"
 
 	"k8s.io/kubernetes/pkg/api"
 	netsets "k8s.io/kubernetes/pkg/util/net/sets"
+
+	"github.com/golang/glog"
 )
 
 const (
@@ -66,3 +69,58 @@ func GetLoadBalancerSourceRanges(service *api.Service) (netsets.IPNet, error) {
 	}
 	return ipnets, nil
 }
+
+// RequestsOnlyLocalTraffic checks if service requests OnlyLocal traffic.
+func RequestsOnlyLocalTraffic(service *api.Service) bool {
+	if service.Spec.Type != api.ServiceTypeLoadBalancer &&
+		service.Spec.Type != api.ServiceTypeNodePort {
+		return false
+	}
+	// First check the alpha annotation and then the beta. This is so existing
+	// Services continue to work till the user decides to transition to beta.
+	// If they transition to beta, there's no way to go back to alpha without
+	// rolling back the cluster.
+	for _, annotation := range []string{AlphaAnnotationExternalTraffic, BetaAnnotationExternalTraffic} {
+		if l, ok := service.Annotations[annotation]; ok {
+			switch l {
+			case AnnotationValueExternalTrafficLocal:
+				return true
+			case AnnotationValueExternalTrafficGlobal:
+				return false
+			default:
+				glog.Errorf("Invalid value for annotation %v: %v", annotation, l)
+			}
+		}
+	}
+	return false
+}
+
+// NeedsHealthCheck Check if service needs health check.
+func NeedsHealthCheck(service *api.Service) bool {
+	if service.Spec.Type != api.ServiceTypeLoadBalancer {
+		return false
+	}
+	return RequestsOnlyLocalTraffic(service)
+}
+
+// GetServiceHealthCheckNodePort Return health check node port annotation for service, if one exists
+func GetServiceHealthCheckNodePort(service *api.Service) int32 {
+	if !NeedsHealthCheck(service) {
+		return 0
+	}
+	// First check the alpha annotation and then the beta. This is so existing
+	// Services continue to work till the user decides to transition to beta.
+	// If they transition to beta, there's no way to go back to alpha without
+	// rolling back the cluster.
+	for _, annotation := range []string{AlphaAnnotationHealthCheckNodePort, BetaAnnotationHealthCheckNodePort} {
+		if l, ok := service.Annotations[annotation]; ok {
+			p, err := strconv.Atoi(l)
+			if err != nil {
+				glog.Errorf("Failed to parse annotation %v: %v", annotation, err)
+				continue
+			}
+			return int32(p)
+		}
+	}
+	return 0
+}

pkg/api/v1/service/annotations.go

@@ -16,13 +16,6 @@ limitations under the License.
 
 package service
 
-import (
-	"strconv"
-
-	"github.com/golang/glog"
-	"k8s.io/kubernetes/pkg/api/v1"
-)
-
 const (
 	// AnnotationLoadBalancerSourceRangesKey is the key of the annotation on a service to set allowed ingress ranges on their LoadBalancers
 	//
@@ -55,57 +48,3 @@ const (
 	// BetaAnnotationExternalTraffic is the beta version of AlphaAnnotationExternalTraffic.
 	BetaAnnotationExternalTraffic = "service.beta.kubernetes.io/external-traffic"
 )
-
-// NeedsHealthCheck Check service for health check annotations
-func NeedsHealthCheck(service *v1.Service) bool {
-	// First check the alpha annotation and then the beta. This is so existing
-	// Services continue to work till the user decides to transition to beta.
-	// If they transition to beta, there's no way to go back to alpha without
-	// rolling back the cluster.
-	for _, annotation := range []string{AlphaAnnotationExternalTraffic, BetaAnnotationExternalTraffic} {
-		if l, ok := service.Annotations[annotation]; ok {
-			if l == AnnotationValueExternalTrafficLocal {
-				return true
-			} else if l == AnnotationValueExternalTrafficGlobal {
-				return false
-			} else {
-				glog.Errorf("Invalid value for annotation %v: %v", annotation, l)
-			}
-		}
-	}
-	return false
-}
-
-// GetServiceHealthCheckNodePort Return health check node port annotation for service, if one exists
-func GetServiceHealthCheckNodePort(service *v1.Service) int32 {
-	if !NeedsHealthCheck(service) {
-		return 0
-	}
-	// First check the alpha annotation and then the beta. This is so existing
-	// Services continue to work till the user decides to transition to beta.
-	// If they transition to beta, there's no way to go back to alpha without
-	// rolling back the cluster.
-	for _, annotation := range []string{AlphaAnnotationHealthCheckNodePort, BetaAnnotationHealthCheckNodePort} {
-		if l, ok := service.Annotations[annotation]; ok {
-			p, err := strconv.Atoi(l)
-			if err != nil {
-				glog.Errorf("Failed to parse annotation %v: %v", annotation, err)
-				continue
-			}
-			return int32(p)
-		}
-	}
-	return 0
-}
-
-// GetServiceHealthCheckPathPort Return the path and nodePort programmed into the Cloud LB Health Check
-func GetServiceHealthCheckPathPort(service *v1.Service) (string, int32) {
-	if !NeedsHealthCheck(service) {
-		return "", 0
-	}
-	port := GetServiceHealthCheckNodePort(service)
-	if port == 0 {
-		return "", 0
-	}
-	return "/healthz", port
-}

pkg/api/v1/service/util.go

@@ -18,10 +18,13 @@ package service
 
 import (
 	"fmt"
+	"strconv"
 	"strings"
 
 	"k8s.io/kubernetes/pkg/api/v1"
 	netsets "k8s.io/kubernetes/pkg/util/net/sets"
+
+	"github.com/golang/glog"
 )
 
 const (
@@ -66,3 +69,70 @@ func GetLoadBalancerSourceRanges(service *v1.Service) (netsets.IPNet, error) {
 	}
 	return ipnets, nil
 }
+
+// RequestsOnlyLocalTraffic checks if service requests OnlyLocal traffic.
+func RequestsOnlyLocalTraffic(service *v1.Service) bool {
+	if service.Spec.Type != v1.ServiceTypeLoadBalancer &&
+		service.Spec.Type != v1.ServiceTypeNodePort {
+		return false
+	}
+	// First check the alpha annotation and then the beta. This is so existing
+	// Services continue to work till the user decides to transition to beta.
+	// If they transition to beta, there's no way to go back to alpha without
+	// rolling back the cluster.
+	for _, annotation := range []string{AlphaAnnotationExternalTraffic, BetaAnnotationExternalTraffic} {
+		if l, ok := service.Annotations[annotation]; ok {
+			switch l {
+			case AnnotationValueExternalTrafficLocal:
+				return true
+			case AnnotationValueExternalTrafficGlobal:
+				return false
+			default:
+				glog.Errorf("Invalid value for annotation %v: %v", annotation, l)
+			}
+		}
+	}
+	return false
+}
+
+// NeedsHealthCheck Check if service needs health check.
+func NeedsHealthCheck(service *v1.Service) bool {
+	if service.Spec.Type != v1.ServiceTypeLoadBalancer {
+		return false
+	}
+	return RequestsOnlyLocalTraffic(service)
+}
+
+// GetServiceHealthCheckNodePort Return health check node port annotation for service, if one exists
+func GetServiceHealthCheckNodePort(service *v1.Service) int32 {
+	if !NeedsHealthCheck(service) {
+		return 0
+	}
+	// First check the alpha annotation and then the beta. This is so existing
+	// Services continue to work till the user decides to transition to beta.
+	// If they transition to beta, there's no way to go back to alpha without
+	// rolling back the cluster.
+	for _, annotation := range []string{AlphaAnnotationHealthCheckNodePort, BetaAnnotationHealthCheckNodePort} {
+		if l, ok := service.Annotations[annotation]; ok {
+			p, err := strconv.Atoi(l)
+			if err != nil {
+				glog.Errorf("Failed to parse annotation %v: %v", annotation, err)
+				continue
+			}
+			return int32(p)
+		}
+	}
+	return 0
+}
+
+// GetServiceHealthCheckPathPort Return the path and nodePort programmed into the Cloud LB Health Check
+func GetServiceHealthCheckPathPort(service *v1.Service) (string, int32) {
+	if !NeedsHealthCheck(service) {
+		return "", 0
+	}
+	port := GetServiceHealthCheckNodePort(service)
+	if port == 0 {
+		return "", 0
+	}
+	return "/healthz", port
+}

pkg/proxy/iptables/proxier.go

@@ -163,8 +163,7 @@ func (e *endpointsInfo) String() string {
 func newServiceInfo(serviceName proxy.ServicePortName, port *api.ServicePort, service *api.Service) *serviceInfo {
 	onlyNodeLocalEndpoints := false
 	if utilfeature.DefaultFeatureGate.Enabled(features.ExternalTrafficLocalOnly) &&
-		(service.Spec.Type == api.ServiceTypeLoadBalancer || service.Spec.Type == api.ServiceTypeNodePort) &&
-		apiservice.NeedsHealthCheck(service) {
+		apiservice.RequestsOnlyLocalTraffic(service) {
 		onlyNodeLocalEndpoints = true
 	}
 	info := &serviceInfo{

pkg/registry/core/service/rest.go

@@ -567,12 +567,7 @@ func shouldAssignNodePorts(service *api.Service) bool {
 }
 
 func shouldCheckOrAssignHealthCheckNodePort(service *api.Service) bool {
-	if service.Spec.Type == api.ServiceTypeLoadBalancer {
-		// True if Service-type == LoadBalancer AND annotation AnnotationExternalTraffic present
-		return (utilfeature.DefaultFeatureGate.Enabled(features.ExternalTrafficLocalOnly) && apiservice.NeedsHealthCheck(service))
-	}
-	glog.V(4).Infof("Service type: %v does not need health check node port", service.Spec.Type)
-	return false
+	return (utilfeature.DefaultFeatureGate.Enabled(features.ExternalTrafficLocalOnly) && apiservice.NeedsHealthCheck(service))
 }
 
 // Loop through the service ports list, find one with the same port number and