diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 396f3b4017f..aa9ef469ae7 100644 --- 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 } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 9d11bea90fa..d9461071670 100644 --- 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 { diff --git a/pkg/registry/core/service/rest.go b/pkg/registry/core/service/rest.go index 10ef74d8f62..bcb14fa5584 100644 --- a/pkg/registry/core/service/rest.go +++ b/pkg/registry/core/service/rest.go @@ -352,14 +352,11 @@ func (rs *REST) healthCheckNodePortUpdate(oldService, service *api.Service) (boo } case oldServiceHasHealthCheckNodePort && assignHealthCheckNodePort: - for _, annotation := range []string{ - apiservice.AlphaAnnotationHealthCheckNodePort, apiservice.BetaAnnotationHealthCheckNodePort} { - if _, ok := service.Annotations[annotation]; !ok { - glog.Warningf("Attempt to delete health check node port annotation DENIED") - el := field.ErrorList{field.Invalid(field.NewPath("metadata", "annotations"), - annotation, "Cannot delete healthcheck nodePort annotation")} - return false, errors.NewInvalid(api.Kind("Service"), service.Name, el) - } + if _, ok := service.Annotations[apiservice.BetaAnnotationHealthCheckNodePort]; !ok { + glog.Warningf("Attempt to delete health check node port annotation DENIED") + el := field.ErrorList{field.Invalid(field.NewPath("metadata", "annotations"), + apiservice.BetaAnnotationHealthCheckNodePort, "Cannot delete healthcheck nodePort annotation")} + return false, errors.NewInvalid(api.Kind("Service"), service.Name, el) } if oldHealthCheckNodePort != requestedHealthCheckNodePort { glog.Warningf("Attempt to change value of health check node port annotation DENIED") diff --git a/test/e2e/service.go b/test/e2e/service.go index a94d0c7d156..218061cbf96 100644 --- a/test/e2e/service.go +++ b/test/e2e/service.go @@ -1095,7 +1095,7 @@ var _ = framework.KubeDescribe("ESIPP [Slow][Feature:ExternalTrafficLocalOnly]", c = f.Client cs = f.ClientSet - if nodes := framework.GetReadySchedulableNodesOrDie(c); len(nodes.Items) > largeClusterMinNodesNumber { + if nodes := framework.GetReadySchedulableNodesOrDie(cs); len(nodes.Items) > largeClusterMinNodesNumber { loadBalancerCreateTimeout = loadBalancerCreateTimeoutLarge } }) @@ -2104,7 +2104,7 @@ func (j *ServiceTestJig) createOnlyLocalNodePortService(namespace, serviceName s svc := j.CreateTCPServiceOrFail(namespace, func(svc *api.Service) { svc.Spec.Type = api.ServiceTypeNodePort svc.ObjectMeta.Annotations = map[string]string{ - service.AlphaAnnotationExternalTraffic: service.AnnotationValueExternalTrafficLocal} + service.BetaAnnotationExternalTraffic: service.AnnotationValueExternalTrafficLocal} svc.Spec.Ports = []api.ServicePort{{Protocol: "TCP", Port: 80}} }) @@ -2126,7 +2126,7 @@ func (j *ServiceTestJig) createOnlyLocalLoadBalancerService(namespace, serviceNa // We need to turn affinity off for our LB distribution tests svc.Spec.SessionAffinity = api.ServiceAffinityNone svc.ObjectMeta.Annotations = map[string]string{ - service.AlphaAnnotationExternalTraffic: service.AnnotationValueExternalTrafficLocal} + service.BetaAnnotationExternalTraffic: service.AnnotationValueExternalTrafficLocal} svc.Spec.Ports = []api.ServicePort{{Protocol: "TCP", Port: 80}} }) @@ -2171,7 +2171,7 @@ func (j *ServiceTestJig) getEndpointNodes(svc *api.Service) map[string][]string // getNodes returns the first maxNodesForTest nodes. Useful in large clusters // where we don't eg: want to create an endpoint per node. func (j *ServiceTestJig) getNodes(maxNodesForTest int) (nodes *api.NodeList) { - nodes = framework.GetReadySchedulableNodesOrDie(j.Client) + nodes = framework.GetReadySchedulableNodesOrDie(j.ClientSet) if len(nodes.Items) <= maxNodesForTest { maxNodesForTest = len(nodes.Items) }