Merge pull request #44741 from MrHohn/esipp-validation-refine

Automatic merge from submit-queue

Refine ESIPP validation logic in validation.go

Separated from #41162.

The previous ESIPP validation logic in validation.go has a huge overlap with [function healthCheckNodePortUpdate in service/rest.go](870585e8e1/pkg/registry/core/service/rest.go (L283-L373)), in which we reject any invalid modifications on ESIPP annotations.

This PR removes the overlap, and make validation.go only check if values are legal and whether user mixes different API versions (alpha & beta).

We are indeed removing the alpha annotation support, but it is kept in the codes for the ease of transiting the same logic onto beta/GA.

/assign @thockin @freehan 

**Release note**:

```release-note
NONE
```

pkg/api/validation/validation.go

@@ -24,6 +24,7 @@ import (
 	"path"
 	"reflect"
 	"regexp"
+	"strconv"
 	"strings"
 
 	"github.com/golang/glog"
@@ -2569,13 +2570,6 @@ var supportedServiceType = sets.NewString(string(api.ServiceTypeClusterIP), stri
 
 // ValidateService tests if required fields/annotations of a Service are valid.
 func ValidateService(service *api.Service) field.ErrorList {
-	allErrs := validateServiceFields(service)
-	allErrs = append(allErrs, validateServiceAnnotations(service, nil)...)
-	return allErrs
-}
-
-// validateServiceFields tests if required fields in the service are set.
-func validateServiceFields(service *api.Service) field.ErrorList {
 	allErrs := ValidateObjectMeta(&service.ObjectMeta, true, ValidateServiceName, field.NewPath("metadata"))
 
 	specPath := field.NewPath("spec")
@@ -2719,6 +2713,9 @@ func validateServiceFields(service *api.Service) field.ErrorList {
 			allErrs = append(allErrs, field.Invalid(fieldPath, val, "must be a list of IP ranges. For example, 10.240.0.0/24,10.250.0.0/24 "))
 		}
 	}
+
+	allErrs = append(allErrs, validateServiceExternalTrafficFields(service)...)
+
 	return allErrs
 }
 
@@ -2760,61 +2757,84 @@ func validateServicePort(sp *api.ServicePort, requireName, isHeadlessService boo
 	return allErrs
 }
 
-func validateServiceAnnotations(service *api.Service, oldService *api.Service) (allErrs field.ErrorList) {
+// validateServiceExternalTrafficFields validates ExternalTraffic related annotations
-	// 2 annotations went from alpha to beta in 1.5: healthcheck-nodeport and
+// have legal value.
-	// external-traffic. The user cannot mix these. All updates to the alpha
+func validateServiceExternalTrafficFields(service *api.Service) field.ErrorList {
-	// annotation are disallowed. The user must change both alpha annotations
+	allErrs := field.ErrorList{}
-	// to beta before making any modifications, even though the system continues
-	// to respect the alpha version.
-	hcAlpha, healthCheckAlphaOk := service.Annotations[apiservice.AlphaAnnotationHealthCheckNodePort]
-	onlyLocalAlpha, onlyLocalAlphaOk := service.Annotations[apiservice.AlphaAnnotationExternalTraffic]
 
-	_, healthCheckBetaOk := service.Annotations[apiservice.BetaAnnotationHealthCheckNodePort]
+	for _, annotation := range []string{apiservice.AlphaAnnotationExternalTraffic, apiservice.BetaAnnotationExternalTraffic} {
-	_, onlyLocalBetaOk := service.Annotations[apiservice.BetaAnnotationExternalTraffic]
+		if l, ok := service.Annotations[annotation]; ok {
-
+			if l != apiservice.AnnotationValueExternalTrafficLocal &&
-	var oldHealthCheckAlpha, oldOnlyLocalAlpha string
+				l != apiservice.AnnotationValueExternalTrafficGlobal {
-	var oldHealthCheckAlphaOk, oldOnlyLocalAlphaOk bool
+				allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "annotations").Key(annotation), l,
-	if oldService != nil {
+					fmt.Sprintf("ExternalTraffic must be %v or %v", apiservice.AnnotationValueExternalTrafficLocal, apiservice.AnnotationValueExternalTrafficGlobal)))
-		oldHealthCheckAlpha, oldHealthCheckAlphaOk = oldService.Annotations[apiservice.AlphaAnnotationHealthCheckNodePort]
+			}
-		oldOnlyLocalAlpha, oldOnlyLocalAlphaOk = oldService.Annotations[apiservice.AlphaAnnotationExternalTraffic]
+		}
+	}
+	for _, annotation := range []string{apiservice.AlphaAnnotationHealthCheckNodePort, apiservice.BetaAnnotationHealthCheckNodePort} {
+		if l, ok := service.Annotations[annotation]; ok {
+			p, err := strconv.Atoi(l)
+			if err != nil {
+				allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "annotations").Key(annotation), l,
+					"HealthCheckNodePort must be a valid port number"))
+			} else if p <= 0 {
+				allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "annotations").Key(annotation), l,
+					"HealthCheckNodePort must be greater than 0"))
+			}
+		}
 	}
-	hcValueChanged := oldHealthCheckAlphaOk && healthCheckAlphaOk && oldHealthCheckAlpha != hcAlpha
-	hcValueNew := !oldHealthCheckAlphaOk && healthCheckAlphaOk
-	hcValueGone := !healthCheckAlphaOk && !healthCheckBetaOk && oldHealthCheckAlphaOk
-	onlyLocalHCMismatch := onlyLocalBetaOk && healthCheckAlphaOk
 
-	// On upgrading to a 1.5 cluster, the user is locked in at the current
+	allErrs = append(allErrs, validateServiceExternalTrafficAPIVersion(service)...)
-	// alpha setting, till they modify the Service such that the pair of
-	// annotations are both beta. Basically this means we need to:
-	// Disallow updates to the alpha annotation.
-	// Disallow creating a Service with the alpha annotation.
-	// Disallow removing both alpha annotations. Removing the health-check
-	// annotation is rejected at a later stage anyway, so if we allow removing
-	// just onlyLocal we might leak the port.
-	// Disallow a single field from transitioning to beta. Mismatched annotations
-	// cause confusion.
-	// Ignore changes to the fields if they're both transitioning to beta.
-	// Allow modifications to Services in fields other than the alpha annotation.
 
-	if hcValueNew || hcValueChanged || hcValueGone || onlyLocalHCMismatch {
+	return allErrs
+}
+
+// serviceExternalTrafficStatus stores flags indicating whether ExternalTraffic
+// related beta annotations and alpha annotations are set on service.
+type serviceExternalTrafficStatus struct {
+	alphaExternalTrafficIsSet bool
+	alphaHealthCheckIsSet     bool
+	betaExternalTrafficIsSet  bool
+	betaHealthCheckIsSet      bool
+}
+
+func (s *serviceExternalTrafficStatus) useAlphaExternalTrafficWithBeta() bool {
+	return s.alphaExternalTrafficIsSet && (s.betaExternalTrafficIsSet || s.betaHealthCheckIsSet)
+}
+
+func (s *serviceExternalTrafficStatus) useAlphaHealthCheckWithBeta() bool {
+	return s.alphaHealthCheckIsSet && (s.betaExternalTrafficIsSet || s.betaHealthCheckIsSet)
+}
+
+func getServiceExternalTrafficStatus(service *api.Service) *serviceExternalTrafficStatus {
+	s := serviceExternalTrafficStatus{}
+	_, s.alphaExternalTrafficIsSet = service.Annotations[apiservice.AlphaAnnotationExternalTraffic]
+	_, s.alphaHealthCheckIsSet = service.Annotations[apiservice.AlphaAnnotationHealthCheckNodePort]
+	_, s.betaExternalTrafficIsSet = service.Annotations[apiservice.BetaAnnotationExternalTraffic]
+	_, s.betaHealthCheckIsSet = service.Annotations[apiservice.BetaAnnotationHealthCheckNodePort]
+	return &s
+}
+
+// validateServiceExternalTrafficAPIVersion checks if user mixes ExternalTraffic
+// API versions.
+func validateServiceExternalTrafficAPIVersion(service *api.Service) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	status := getServiceExternalTrafficStatus(service)
+
+	if status.useAlphaExternalTrafficWithBeta() {
+		fieldPath := field.NewPath("metadata", "annotations").Key(apiservice.AlphaAnnotationExternalTraffic)
+		msg := fmt.Sprintf("please replace the alpha annotation with beta annotation")
+		allErrs = append(allErrs, field.Invalid(fieldPath, apiservice.AlphaAnnotationExternalTraffic, msg))
+	}
+
+	if status.useAlphaHealthCheckWithBeta() {
 		fieldPath := field.NewPath("metadata", "annotations").Key(apiservice.AlphaAnnotationHealthCheckNodePort)
-		msg := fmt.Sprintf("please replace the alpha annotation with the beta version %v",
+		msg := fmt.Sprintf("please replace the alpha annotation with beta annotation")
-			apiservice.BetaAnnotationHealthCheckNodePort)
 		allErrs = append(allErrs, field.Invalid(fieldPath, apiservice.AlphaAnnotationHealthCheckNodePort, msg))
 	}
 
-	onlyLocalValueChanged := oldOnlyLocalAlphaOk && onlyLocalAlphaOk && oldOnlyLocalAlpha != onlyLocalAlpha
+	return allErrs
-	onlyLocalValueNew := !oldOnlyLocalAlphaOk && onlyLocalAlphaOk
-	onlyLocalValueGone := !onlyLocalAlphaOk && !onlyLocalBetaOk && oldOnlyLocalAlphaOk
-	hcOnlyLocalMismatch := onlyLocalAlphaOk && healthCheckBetaOk
-
-	if onlyLocalValueNew || onlyLocalValueChanged || onlyLocalValueGone || hcOnlyLocalMismatch {
-		fieldPath := field.NewPath("metadata", "annotations").Key(apiservice.AlphaAnnotationExternalTraffic)
-		msg := fmt.Sprintf("please replace the alpha annotation with the beta version %v",
-			apiservice.BetaAnnotationExternalTraffic)
-		allErrs = append(allErrs, field.Invalid(fieldPath, apiservice.AlphaAnnotationExternalTraffic, msg))
-	}
-	return
 }
 
 // ValidateServiceUpdate tests if required fields in the service are set during an update
@@ -2829,8 +2849,7 @@ func ValidateServiceUpdate(service, oldService *api.Service) field.ErrorList {
 		}
 	}
 
-	allErrs = append(allErrs, validateServiceFields(service)...)
+	allErrs = append(allErrs, ValidateService(service)...)
-	allErrs = append(allErrs, validateServiceAnnotations(service, oldService)...)
 	return allErrs
 }
 

pkg/api/validation/validation_test.go

@@ -5622,10 +5622,36 @@ func TestValidateService(t *testing.T) {
 			numErrs: 1,
 		},
 		{
-			name: "LoadBalancer disallows onlyLocal alpha annotations",
+			name: "LoadBalancer allows onlyLocal alpha annotations",
 			tweakSvc: func(s *api.Service) {
 				s.Annotations[service.AlphaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficLocal
 			},
+			numErrs: 0,
+		},
+		{
+			name: "invalid externalTraffic beta annotation",
+			tweakSvc: func(s *api.Service) {
+				s.Spec.Type = api.ServiceTypeLoadBalancer
+				s.Annotations[service.BetaAnnotationExternalTraffic] = "invalid"
+			},
+			numErrs: 1,
+		},
+		{
+			name: "nagative healthCheckNodePort beta annotation",
+			tweakSvc: func(s *api.Service) {
+				s.Spec.Type = api.ServiceTypeLoadBalancer
+				s.Annotations[service.BetaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficLocal
+				s.Annotations[service.BetaAnnotationHealthCheckNodePort] = "-1"
+			},
+			numErrs: 1,
+		},
+		{
+			name: "invalid healthCheckNodePort beta annotation",
+			tweakSvc: func(s *api.Service) {
+				s.Spec.Type = api.ServiceTypeLoadBalancer
+				s.Annotations[service.BetaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficLocal
+				s.Annotations[service.BetaAnnotationHealthCheckNodePort] = "whatisthis"
+			},
 			numErrs: 1,
 		},
 		{
@@ -7028,22 +7054,22 @@ func TestValidateServiceUpdate(t *testing.T) {
 			numErrs: 1,
 		},
 		{
-			name: "Service disallows removing one onlyLocal alpha annotation",
+			name: "Service allows removing onlyLocal alpha annotations",
 			tweakSvc: func(oldSvc, newSvc *api.Service) {
 				oldSvc.Annotations[service.AlphaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficLocal
 				oldSvc.Annotations[service.AlphaAnnotationHealthCheckNodePort] = "3001"
 			},
-			numErrs: 2,
+			numErrs: 0,
 		},
 		{
-			name: "Service disallows modifying onlyLocal alpha annotations",
+			name: "Service allows modifying onlyLocal alpha annotations",
 			tweakSvc: func(oldSvc, newSvc *api.Service) {
 				oldSvc.Annotations[service.AlphaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficLocal
 				oldSvc.Annotations[service.AlphaAnnotationHealthCheckNodePort] = "3001"
 				newSvc.Annotations[service.AlphaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficGlobal
 				newSvc.Annotations[service.AlphaAnnotationHealthCheckNodePort] = oldSvc.Annotations[service.AlphaAnnotationHealthCheckNodePort]
 			},
-			numErrs: 1,
+			numErrs: 0,
 		},
 		{
 			name: "Service disallows promoting one of the onlyLocal pair to beta",