Merge pull request #33957 from bprashanth/esipp-beta

Automatic merge from submit-queue Loadbalanced client src ip preservation enters beta Sounds like we're going to try out the proposal (https://github.com/kubernetes/kubernetes/issues/30819#issuecomment-249877334) for annotations -> fields on just one feature in 1.5 (scheduler). Or do we want to just convert to fields right now?
2016-10-20 06:53:07 -07:00
parent 10ebd65913 5029bb06e9
commit e6b5b076b8
9 changed files with 610 additions and 131 deletions
--- a/pkg/api/validation/validation.go
+++ b/pkg/api/validation/validation.go
@@ -2385,8 +2385,15 @@ var supportedSessionAffinityType = sets.NewString(string(api.ServiceAffinityClie
 var supportedServiceType = sets.NewString(string(api.ServiceTypeClusterIP), string(api.ServiceTypeNodePort),
 	string(api.ServiceTypeLoadBalancer), string(api.ServiceTypeExternalName))

-// ValidateService tests if required fields in the service are set.
+// 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")
@@ -2567,6 +2574,63 @@ func validateServicePort(sp *api.ServicePort, requireName, isHeadlessService boo
 	return allErrs
 }

+func validateServiceAnnotations(service *api.Service, oldService *api.Service) (allErrs field.ErrorList) {
+	// 2 annotations went from alpha to beta in 1.5: healthcheck-nodeport and
+	// external-traffic. The user cannot mix these. All updates to the alpha
+	// annotation are disallowed. The user must change both alpha annotations
+	// 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]
+	_, onlyLocalBetaOk := service.Annotations[apiservice.BetaAnnotationExternalTraffic]
+
+	var oldHealthCheckAlpha, oldOnlyLocalAlpha string
+	var oldHealthCheckAlphaOk, oldOnlyLocalAlphaOk bool
+	if oldService != nil {
+		oldHealthCheckAlpha, oldHealthCheckAlphaOk = oldService.Annotations[apiservice.AlphaAnnotationHealthCheckNodePort]
+		oldOnlyLocalAlpha, oldOnlyLocalAlphaOk = oldService.Annotations[apiservice.AlphaAnnotationExternalTraffic]
+	}
+	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
+	// 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 {
+		fieldPath := field.NewPath("metadata", "annotations").Key(apiservice.AlphaAnnotationHealthCheckNodePort)
+		msg := fmt.Sprintf("please replace the alpha annotation with the beta version %v",
+			apiservice.BetaAnnotationHealthCheckNodePort)
+		allErrs = append(allErrs, field.Invalid(fieldPath, apiservice.AlphaAnnotationHealthCheckNodePort, msg))
+	}
+
+	onlyLocalValueChanged := oldOnlyLocalAlphaOk && onlyLocalAlphaOk && oldOnlyLocalAlpha != onlyLocalAlpha
+	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
 func ValidateServiceUpdate(service, oldService *api.Service) field.ErrorList {
 	allErrs := ValidateObjectMetaUpdate(&service.ObjectMeta, &oldService.ObjectMeta, field.NewPath("metadata"))
@@ -2578,7 +2642,8 @@ func ValidateServiceUpdate(service, oldService *api.Service) field.ErrorList {
 	// TODO(freehan): allow user to update loadbalancerSourceRanges
 	allErrs = append(allErrs, ValidateImmutableField(service.Spec.LoadBalancerSourceRanges, oldService.Spec.LoadBalancerSourceRanges, field.NewPath("spec", "loadBalancerSourceRanges"))...)

-	allErrs = append(allErrs, ValidateService(service)...)
+	allErrs = append(allErrs, validateServiceFields(service)...)
+	allErrs = append(allErrs, validateServiceAnnotations(service, oldService)...)
 	return allErrs
 }

--- a/pkg/api/validation/validation_test.go
+++ b/pkg/api/validation/validation_test.go
@@ -5207,6 +5207,13 @@ func TestValidateService(t *testing.T) {
 			},
 			numErrs: 1,
 		},
+		{
+			name: "LoadBalancer disallows onlyLocal alpha annotations",
+			tweakSvc: func(s *api.Service) {
+				s.Annotations[service.AlphaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficLocal
+			},
+			numErrs: 1,
+		},
 	}

 	for _, tc := range testCases {
@@ -6474,6 +6481,44 @@ func TestValidateServiceUpdate(t *testing.T) {
 			},
 			numErrs: 1,
 		},
+		{
+			name: "Service disallows removing one onlyLocal alpha annotation",
+			tweakSvc: func(oldSvc, newSvc *api.Service) {
+				oldSvc.Annotations[service.AlphaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficLocal
+				oldSvc.Annotations[service.AlphaAnnotationHealthCheckNodePort] = "3001"
+			},
+			numErrs: 2,
+		},
+		{
+			name: "Service disallows 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,
+		},
+		{
+			name: "Service disallows promoting one of the onlyLocal pair to beta",
+			tweakSvc: func(oldSvc, newSvc *api.Service) {
+				oldSvc.Annotations[service.AlphaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficLocal
+				oldSvc.Annotations[service.AlphaAnnotationHealthCheckNodePort] = "3001"
+				newSvc.Annotations[service.BetaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficGlobal
+				newSvc.Annotations[service.AlphaAnnotationHealthCheckNodePort] = oldSvc.Annotations[service.AlphaAnnotationHealthCheckNodePort]
+			},
+			numErrs: 1,
+		},
+		{
+			name: "Service allows changing both onlyLocal annotations from alpha to beta",
+			tweakSvc: func(oldSvc, newSvc *api.Service) {
+				oldSvc.Annotations[service.AlphaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficLocal
+				oldSvc.Annotations[service.AlphaAnnotationHealthCheckNodePort] = "3001"
+				newSvc.Annotations[service.BetaAnnotationExternalTraffic] = service.AnnotationValueExternalTrafficLocal
+				newSvc.Annotations[service.BetaAnnotationHealthCheckNodePort] = oldSvc.Annotations[service.AlphaAnnotationHealthCheckNodePort]
+			},
+			numErrs: 0,
+		},
 	}

 	for _, tc := range testCases {