diff --git a/pkg/api/service/util.go b/pkg/api/service/util.go index ddf2f68d6a1..8a4b544586f 100644 --- a/pkg/api/service/util.go +++ b/pkg/api/service/util.go @@ -76,23 +76,22 @@ func RequestsOnlyLocalTraffic(service *api.Service) bool { 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) - } + + // First check the beta annotation and then the first class field. This is so that + // existing Services continue to work till the user decides to transition to the + // first class field. + if l, ok := service.Annotations[BetaAnnotationExternalTraffic]; ok { + switch l { + case AnnotationValueExternalTrafficLocal: + return true + case AnnotationValueExternalTrafficGlobal: + return false + default: + glog.Errorf("Invalid value for annotation %v: %v", BetaAnnotationExternalTraffic, l) + return false } } - return false + return service.Spec.ExternalTrafficPolicy == api.ServiceExternalTrafficPolicyTypeLocal } // NeedsHealthCheck Check if service needs health check. @@ -103,21 +102,61 @@ func NeedsHealthCheck(service *api.Service) bool { return RequestsOnlyLocalTraffic(service) } -// GetServiceHealthCheckNodePort Return health check node port annotation for service, if one exists +// GetServiceHealthCheckNodePort Return health check node port for service, if one exists func GetServiceHealthCheckNodePort(service *api.Service) int32 { - // 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) + // First check the beta annotation and then the first class field. This is so that + // existing Services continue to work till the user decides to transition to the + // first class field. + if l, ok := service.Annotations[BetaAnnotationHealthCheckNodePort]; ok { + p, err := strconv.Atoi(l) + if err != nil { + glog.Errorf("Failed to parse annotation %v: %v", BetaAnnotationHealthCheckNodePort, err) + return 0 } + return int32(p) } - return 0 + return service.Spec.HealthCheckNodePort +} + +// SetDefaultExternalTrafficPolicyIfNeeded defaults the ExternalTrafficPolicy field +// for NodePort / LoadBalancer service to Global for consistency. +// TODO: Move this default logic to default.go once beta annotation is deprecated. +func SetDefaultExternalTrafficPolicyIfNeeded(service *api.Service) { + if _, ok := service.Annotations[BetaAnnotationExternalTraffic]; ok { + // Don't default this field if beta annotation exists. + return + } else if (service.Spec.Type == api.ServiceTypeNodePort || + service.Spec.Type == api.ServiceTypeLoadBalancer) && + service.Spec.ExternalTrafficPolicy == "" { + service.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeGlobal + } +} + +// ClearExternalTrafficPolicy resets the ExternalTrafficPolicy field. +func ClearExternalTrafficPolicy(service *api.Service) { + // First check the beta annotation and then the first class field. This is so that + // existing Services continue to work till the user decides to transition to the + // first class field. + if _, ok := service.Annotations[BetaAnnotationExternalTraffic]; ok { + delete(service.Annotations, BetaAnnotationExternalTraffic) + return + } + service.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyType("") +} + +// SetServiceHealthCheckNodePort sets the given health check node port on service. +// It does not check whether this service needs healthCheckNodePort. +func SetServiceHealthCheckNodePort(service *api.Service, hcNodePort int32) { + // First check the beta annotation and then the first class field. This is so that + // existing Services continue to work till the user decides to transition to the + // first class field. + if _, ok := service.Annotations[BetaAnnotationExternalTraffic]; ok { + if hcNodePort == 0 { + delete(service.Annotations, BetaAnnotationHealthCheckNodePort) + } else { + service.Annotations[BetaAnnotationHealthCheckNodePort] = fmt.Sprintf("%d", hcNodePort) + } + return + } + service.Spec.HealthCheckNodePort = hcNodePort } diff --git a/pkg/api/v1/service/util.go b/pkg/api/v1/service/util.go index 8ee8a54d6d3..0e48a5844d8 100644 --- a/pkg/api/v1/service/util.go +++ b/pkg/api/v1/service/util.go @@ -76,23 +76,22 @@ func RequestsOnlyLocalTraffic(service *v1.Service) bool { 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) - } + + // First check the beta annotation and then the first class field. This is so that + // existing Services continue to work till the user decides to transition to the + // first class field. + if l, ok := service.Annotations[BetaAnnotationExternalTraffic]; ok { + switch l { + case AnnotationValueExternalTrafficLocal: + return true + case AnnotationValueExternalTrafficGlobal: + return false + default: + glog.Errorf("Invalid value for annotation %v: %v", BetaAnnotationExternalTraffic, l) + return false } } - return false + return service.Spec.ExternalTrafficPolicy == v1.ServiceExternalTrafficPolicyTypeLocal } // NeedsHealthCheck Check if service needs health check. @@ -103,23 +102,62 @@ func NeedsHealthCheck(service *v1.Service) bool { return RequestsOnlyLocalTraffic(service) } -// GetServiceHealthCheckNodePort Return health check node port annotation for service, if one exists +// GetServiceHealthCheckNodePort Return health check node port for service, if one exists func GetServiceHealthCheckNodePort(service *v1.Service) int32 { - // 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) + // First check the beta annotation and then the first class field. This is so that + // existing Services continue to work till the user decides to transition to the + // first class field. + if l, ok := service.Annotations[BetaAnnotationHealthCheckNodePort]; ok { + p, err := strconv.Atoi(l) + if err != nil { + glog.Errorf("Failed to parse annotation %v: %v", BetaAnnotationHealthCheckNodePort, err) + return 0 } + return int32(p) } - return 0 + return service.Spec.HealthCheckNodePort +} + +// SetDefaultExternalTrafficPolicyIfNeeded defaults the ExternalTrafficPolicy field +// for NodePort / LoadBalancer service to Global for consistency. +// TODO: Move this default logic to default.go once beta annotation is deprecated. +func SetDefaultExternalTrafficPolicyIfNeeded(service *v1.Service) { + if _, ok := service.Annotations[BetaAnnotationExternalTraffic]; ok { + // Don't default this field if beta annotation exists. + return + } else if (service.Spec.Type == v1.ServiceTypeNodePort || + service.Spec.Type == v1.ServiceTypeLoadBalancer) && + service.Spec.ExternalTrafficPolicy == "" { + service.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyTypeGlobal + } +} + +// ClearExternalTrafficPolicy resets the ExternalTrafficPolicy field. +func ClearExternalTrafficPolicy(service *v1.Service) { + // First check the beta annotation and then the first class field. This is so existing + // Services continue to work till the user decides to transition to the first class field. + if _, ok := service.Annotations[BetaAnnotationExternalTraffic]; ok { + delete(service.Annotations, BetaAnnotationExternalTraffic) + return + } + service.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyType("") +} + +// SetServiceHealthCheckNodePort sets the given health check node port on service. +// It does not check whether this service needs healthCheckNodePort. +func SetServiceHealthCheckNodePort(service *v1.Service, hcNodePort int32) { + // First check the beta annotation and then the first class field. This is so that + // existing Services continue to work till the user decides to transition to the + // first class field. + if _, ok := service.Annotations[BetaAnnotationExternalTraffic]; ok { + if hcNodePort == 0 { + delete(service.Annotations, BetaAnnotationHealthCheckNodePort) + } else { + service.Annotations[BetaAnnotationHealthCheckNodePort] = fmt.Sprintf("%d", hcNodePort) + } + return + } + service.Spec.HealthCheckNodePort = hcNodePort } // GetServiceHealthCheckPathPort Return the path and nodePort programmed into the Cloud LB Health Check diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index bf69f1fa47e..5da0b5ee804 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -2742,7 +2742,8 @@ func ValidateService(service *api.Service) field.ErrorList { } } - allErrs = append(allErrs, validateServiceExternalTrafficFields(service)...) + allErrs = append(allErrs, validateServiceExternalTrafficFieldsValue(service)...) + allErrs = append(allErrs, validateServiceExternalTrafficAPIVersion(service)...) return allErrs } @@ -2785,61 +2786,68 @@ func validateServicePort(sp *api.ServicePort, requireName, isHeadlessService boo return allErrs } -// validateServiceExternalTrafficFields validates ExternalTraffic related annotations +// validateServiceExternalTrafficFieldsValue validates ExternalTraffic related annotations // have legal value. -func validateServiceExternalTrafficFields(service *api.Service) field.ErrorList { +func validateServiceExternalTrafficFieldsValue(service *api.Service) field.ErrorList { allErrs := field.ErrorList{} - for _, annotation := range []string{apiservice.AlphaAnnotationExternalTraffic, apiservice.BetaAnnotationExternalTraffic} { - if l, ok := service.Annotations[annotation]; ok { - if l != apiservice.AnnotationValueExternalTrafficLocal && - l != apiservice.AnnotationValueExternalTrafficGlobal { - allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "annotations").Key(annotation), l, - fmt.Sprintf("ExternalTraffic must be %v or %v", apiservice.AnnotationValueExternalTrafficLocal, apiservice.AnnotationValueExternalTrafficGlobal))) - } + // Check beta annotations. + if l, ok := service.Annotations[apiservice.BetaAnnotationExternalTraffic]; ok { + if l != apiservice.AnnotationValueExternalTrafficLocal && + l != apiservice.AnnotationValueExternalTrafficGlobal { + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "annotations").Key(apiservice.BetaAnnotationExternalTraffic), l, + fmt.Sprintf("ExternalTraffic must be %v or %v", apiservice.AnnotationValueExternalTrafficLocal, apiservice.AnnotationValueExternalTrafficGlobal))) } } - 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")) - } + if l, ok := service.Annotations[apiservice.BetaAnnotationHealthCheckNodePort]; ok { + p, err := strconv.Atoi(l) + if err != nil { + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "annotations").Key(apiservice.BetaAnnotationHealthCheckNodePort), l, + "HealthCheckNodePort must be a valid port number")) + } else if p <= 0 { + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "annotations").Key(apiservice.BetaAnnotationHealthCheckNodePort), l, + "HealthCheckNodePort must be greater than 0")) } } - allErrs = append(allErrs, validateServiceExternalTrafficAPIVersion(service)...) + // Check first class fields. + if service.Spec.ExternalTrafficPolicy != "" && + service.Spec.ExternalTrafficPolicy != api.ServiceExternalTrafficPolicyTypeGlobal && + service.Spec.ExternalTrafficPolicy != api.ServiceExternalTrafficPolicyTypeLocal { + allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("externalTrafficPolicy"), service.Spec.ExternalTrafficPolicy, + fmt.Sprintf("ExternalTrafficPolicy must be empty, %v or %v", api.ServiceExternalTrafficPolicyTypeGlobal, api.ServiceExternalTrafficPolicyTypeLocal))) + } + if service.Spec.HealthCheckNodePort < 0 { + allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("healthCheckNodePort"), service.Spec.HealthCheckNodePort, + "HealthCheckNodePort must be not less than 0")) + } return allErrs } // serviceExternalTrafficStatus stores flags indicating whether ExternalTraffic -// related beta annotations and alpha annotations are set on service. +// related beta annotations and GA fields are set on service. type serviceExternalTrafficStatus struct { - alphaExternalTrafficIsSet bool - alphaHealthCheckIsSet bool - betaExternalTrafficIsSet bool - betaHealthCheckIsSet bool + betaExternalTrafficIsSet bool + betaHealthCheckIsSet bool + gaExternalTrafficIsSet bool + gaHealthCheckIsSet bool } -func (s *serviceExternalTrafficStatus) useAlphaExternalTrafficWithBeta() bool { - return s.alphaExternalTrafficIsSet && (s.betaExternalTrafficIsSet || s.betaHealthCheckIsSet) +func (s *serviceExternalTrafficStatus) useBetaExternalTrafficWithGA() bool { + return s.betaExternalTrafficIsSet && (s.gaExternalTrafficIsSet || s.gaHealthCheckIsSet) } -func (s *serviceExternalTrafficStatus) useAlphaHealthCheckWithBeta() bool { - return s.alphaHealthCheckIsSet && (s.betaExternalTrafficIsSet || s.betaHealthCheckIsSet) +func (s *serviceExternalTrafficStatus) useBetaHealthCheckWithGA() bool { + return s.betaHealthCheckIsSet && (s.gaExternalTrafficIsSet || s.gaHealthCheckIsSet) } 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] + s.gaExternalTrafficIsSet = service.Spec.ExternalTrafficPolicy != "" + s.gaHealthCheckIsSet = service.Spec.HealthCheckNodePort != 0 return &s } @@ -2850,16 +2858,39 @@ func validateServiceExternalTrafficAPIVersion(service *api.Service) field.ErrorL 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.useBetaExternalTrafficWithGA() { + fieldPath := field.NewPath("metadata", "annotations").Key(apiservice.BetaAnnotationExternalTraffic) + msg := fmt.Sprintf("please replace the beta annotation with 'ExternalTrafficPolicy' field") + allErrs = append(allErrs, field.Invalid(fieldPath, apiservice.BetaAnnotationExternalTraffic, msg)) } - if status.useAlphaHealthCheckWithBeta() { - fieldPath := field.NewPath("metadata", "annotations").Key(apiservice.AlphaAnnotationHealthCheckNodePort) - msg := fmt.Sprintf("please replace the alpha annotation with beta annotation") - allErrs = append(allErrs, field.Invalid(fieldPath, apiservice.AlphaAnnotationHealthCheckNodePort, msg)) + if status.useBetaHealthCheckWithGA() { + fieldPath := field.NewPath("metadata", "annotations").Key(apiservice.BetaAnnotationHealthCheckNodePort) + msg := fmt.Sprintf("please replace the beta annotation with 'HealthCheckNodePort' field") + allErrs = append(allErrs, field.Invalid(fieldPath, apiservice.BetaAnnotationHealthCheckNodePort, msg)) + } + + return allErrs +} + +// ValidateServiceExternalTrafficFieldsCombination validates if ExternalTrafficPolicy, +// HealthCheckNodePort and Type combination are legal. For update, it should be called +// after clearing externalTraffic related fields for the ease of transitioning between +// different service types. +func ValidateServiceExternalTrafficFieldsCombination(service *api.Service) field.ErrorList { + allErrs := field.ErrorList{} + + if service.Spec.Type != api.ServiceTypeLoadBalancer && + service.Spec.Type != api.ServiceTypeNodePort && + service.Spec.ExternalTrafficPolicy != "" { + allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "externalTrafficPolicy"), service.Spec.ExternalTrafficPolicy, + "ExternalTrafficPolicy can only be set on NodePort and LoadBalancer service")) + } + + if !apiservice.NeedsHealthCheck(service) && + service.Spec.HealthCheckNodePort != 0 { + allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "healthCheckNodePort"), service.Spec.HealthCheckNodePort, + "HealthCheckNodePort can only be set on LoadBalancer service with ExternalTrafficPolicy=Local")) } return allErrs diff --git a/pkg/registry/core/service/rest.go b/pkg/registry/core/service/rest.go index c8cdba60b83..66ccf7000cc 100644 --- a/pkg/registry/core/service/rest.go +++ b/pkg/registry/core/service/rest.go @@ -162,32 +162,16 @@ func (rs *REST) Create(ctx genericapirequest.Context, obj runtime.Object) (runti } } - if shouldCheckOrAssignHealthCheckNodePort(service) { - var healthCheckNodePort int - var err error - if l, ok := service.Annotations[apiservice.BetaAnnotationHealthCheckNodePort]; ok { - healthCheckNodePort, err = strconv.Atoi(l) - if err != nil || healthCheckNodePort <= 0 { - return nil, errors.NewInternalError(fmt.Errorf("Failed to parse annotation %v: %v", apiservice.BetaAnnotationHealthCheckNodePort, err)) + // Handle ExternalTraiffc related fields during service creation. + if utilfeature.DefaultFeatureGate.Enabled(features.ExternalTrafficLocalOnly) { + apiservice.SetDefaultExternalTrafficPolicyIfNeeded(service) + if apiservice.NeedsHealthCheck(service) { + if err := rs.allocateHealthCheckNodePort(service); err != nil { + return nil, errors.NewInternalError(err) } } - if healthCheckNodePort > 0 { - // If the request has a health check nodePort in mind, attempt to reserve it - err := nodePortOp.Allocate(int(healthCheckNodePort)) - if err != nil { - return nil, errors.NewInternalError(fmt.Errorf("Failed to allocate requested HealthCheck nodePort %v: %v", healthCheckNodePort, err)) - } - } else { - // If the request has no health check nodePort specified, allocate any - healthCheckNodePort, err = nodePortOp.AllocateNext() - if err != nil { - // TODO: what error should be returned here? It's not a - // field-level validation failure (the field is valid), and it's - // not really an internal error. - return nil, errors.NewInternalError(fmt.Errorf("failed to allocate a nodePort: %v", err)) - } - // Insert the newly allocated health check port as an annotation (plan of record for Alpha) - service.Annotations[apiservice.BetaAnnotationHealthCheckNodePort] = fmt.Sprintf("%d", healthCheckNodePort) + if errs := validation.ValidateServiceExternalTrafficFieldsCombination(service); len(errs) > 0 { + return nil, errors.NewInvalid(api.Kind("Service"), service.Name, errs) } } @@ -239,13 +223,14 @@ func (rs *REST) Delete(ctx genericapirequest.Context, id string) (runtime.Object } } - if shouldCheckOrAssignHealthCheckNodePort(service) { + if utilfeature.DefaultFeatureGate.Enabled(features.ExternalTrafficLocalOnly) && + apiservice.NeedsHealthCheck(service) { nodePort := apiservice.GetServiceHealthCheckNodePort(service) if nodePort > 0 { err := rs.serviceNodePorts.Release(int(nodePort)) if err != nil { // these should be caught by an eventual reconciliation / restart - utilruntime.HandleError(fmt.Errorf("Error releasing service health check %s node port %d: %v", service.Name, nodePort, err)) + utilruntime.HandleError(fmt.Errorf("Error releasing service %s health check node port %d: %v", service.Name, nodePort, err)) } } } @@ -280,92 +265,70 @@ func (*REST) NewList() runtime.Object { return &api.ServiceList{} } +// externalTrafficPolicyUpdate adjusts ExternalTrafficPolicy during service update if needed. +// It is necessary because we default ExternalTrafficPolicy field to different values. +// (NodePort / LoadBalancer: default is Global; Other types: default is empty.) +func externalTrafficPolicyUpdate(oldService, service *api.Service) { + var neededExternalTraffic, needsExternalTraffic bool + if oldService.Spec.Type == api.ServiceTypeNodePort || + oldService.Spec.Type == api.ServiceTypeLoadBalancer { + neededExternalTraffic = true + } + if service.Spec.Type == api.ServiceTypeNodePort || + service.Spec.Type == api.ServiceTypeLoadBalancer { + needsExternalTraffic = true + } + if neededExternalTraffic && !needsExternalTraffic { + // Clear ExternalTrafficPolicy to prevent confusion from ineffective field. + apiservice.ClearExternalTrafficPolicy(service) + } +} + +// healthCheckNodePortUpdate handles HealthCheckNodePort allocation/release +// and adjusts HealthCheckNodePort during service update if needed. func (rs *REST) healthCheckNodePortUpdate(oldService, service *api.Service) (bool, error) { - // Health Check Node Port handling during updates - // - // Case 1. Transition from globalTraffic to OnlyLocal for the ESIPP annotation - // - // Allocate a health check node port or attempt to reserve the user-specified one, if provided. - // Insert health check node port as an annotation into the service's annotations - // - // Case 2. Transition from OnlyLocal to Global for the ESIPP annotation - // - // Free the existing healthCheckNodePort and clear the health check nodePort annotation - // - // Case 3. No change (Global ---stays--> Global) but prevent invalid annotation manipulations - // - // Reject insertion of the "service.alpha.kubernetes.io/healthcheck-nodeport" annotation - // - // Case 4. No change (OnlyLocal ---stays--> OnlyLocal) but prevent invalid annotation manipulations - // - // Reject deletion of the "service.alpha.kubernetes.io/healthcheck-nodeport" annotation - // Reject changing the value of the healthCheckNodePort annotation - // - oldServiceHasHealthCheckNodePort := shouldCheckOrAssignHealthCheckNodePort(oldService) + neededHealthCheckNodePort := apiservice.NeedsHealthCheck(oldService) oldHealthCheckNodePort := apiservice.GetServiceHealthCheckNodePort(oldService) - assignHealthCheckNodePort := shouldCheckOrAssignHealthCheckNodePort(service) - requestedHealthCheckNodePort := apiservice.GetServiceHealthCheckNodePort(service) + needsHealthCheckNodePort := apiservice.NeedsHealthCheck(service) + newHealthCheckNodePort := apiservice.GetServiceHealthCheckNodePort(service) switch { - case !oldServiceHasHealthCheckNodePort && assignHealthCheckNodePort: - glog.Infof("Transition from Global LB service to OnlyLocal service") - if requestedHealthCheckNodePort > 0 { - // If the request has a health check nodePort in mind, attempt to reserve it - err := rs.serviceNodePorts.Allocate(int(requestedHealthCheckNodePort)) - if err != nil { - errmsg := fmt.Sprintf("Failed to allocate requested HealthCheck nodePort %v:%v", - requestedHealthCheckNodePort, err) - el := field.ErrorList{field.Invalid(field.NewPath("metadata", "annotations"), - apiservice.BetaAnnotationHealthCheckNodePort, errmsg)} - return false, errors.NewInvalid(api.Kind("Service"), service.Name, el) - } - glog.Infof("Reserved user requested nodePort: %d", requestedHealthCheckNodePort) - } else { - // If the request has no health check nodePort specified, allocate any - healthCheckNodePort, err := rs.serviceNodePorts.AllocateNext() - if err != nil { - // TODO: what error should be returned here? It's not a - // field-level validation failure (the field is valid), and it's - // not really an internal error. - return false, errors.NewInternalError(fmt.Errorf("failed to allocate a nodePort: %v", err)) - } - // Insert the newly allocated health check port as an annotation (plan of record for Alpha) - service.Annotations[apiservice.BetaAnnotationHealthCheckNodePort] = fmt.Sprintf("%d", healthCheckNodePort) - glog.Infof("Reserved health check nodePort: %d", healthCheckNodePort) + // Case 1: Transition from don't need HealthCheckNodePort to needs HealthCheckNodePort. + // Allocate a health check node port or attempt to reserve the user-specified one if provided. + // Insert health check node port into the service's HealthCheckNodePort field if needed. + case !neededHealthCheckNodePort && needsHealthCheckNodePort: + glog.Infof("Transition to LoadBalancer type service with ExternalTrafficPolicy=Local") + if err := rs.allocateHealthCheckNodePort(service); err != nil { + return false, errors.NewInternalError(err) } - case oldServiceHasHealthCheckNodePort && !assignHealthCheckNodePort: - glog.Infof("Transition from OnlyLocal LB service to Global service") + // Case 2: Transition from needs HealthCheckNodePort to don't need HealthCheckNodePort. + // Free the existing healthCheckNodePort and clear the HealthCheckNodePort field. + case neededHealthCheckNodePort && !needsHealthCheckNodePort: + glog.Infof("Transition to non LoadBalancer type service or LoadBalancer type service with ExternalTrafficPolicy=Global") err := rs.serviceNodePorts.Release(int(oldHealthCheckNodePort)) if err != nil { - glog.Warningf("Error releasing service health check %s node port %d: %v", service.Name, oldHealthCheckNodePort, err) + glog.Warningf("error releasing service health check %s node port %d: %v", service.Name, oldHealthCheckNodePort, err) return false, errors.NewInternalError(fmt.Errorf("failed to free health check nodePort: %v", err)) - } else { - delete(service.Annotations, apiservice.BetaAnnotationHealthCheckNodePort) - delete(service.Annotations, apiservice.AlphaAnnotationHealthCheckNodePort) - glog.Infof("Freed health check nodePort: %d", oldHealthCheckNodePort) } + glog.Infof("Freed health check nodePort: %d", oldHealthCheckNodePort) + // Clear the HealthCheckNodePort field. + apiservice.SetServiceHealthCheckNodePort(service, 0) - case !oldServiceHasHealthCheckNodePort && !assignHealthCheckNodePort: - if _, ok := service.Annotations[apiservice.BetaAnnotationHealthCheckNodePort]; ok { - glog.Warningf("Attempt to insert health check node port annotation DENIED") - el := field.ErrorList{field.Invalid(field.NewPath("metadata", "annotations"), - apiservice.BetaAnnotationHealthCheckNodePort, "Cannot insert healthcheck nodePort annotation")} - return false, errors.NewInvalid(api.Kind("Service"), service.Name, el) - } - - case oldServiceHasHealthCheckNodePort && assignHealthCheckNodePort: - 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") - el := field.ErrorList{field.Invalid(field.NewPath("metadata", "annotations"), - apiservice.BetaAnnotationHealthCheckNodePort, "Cannot change healthcheck nodePort during update")} + // Case 3: Remain in needs HealthCheckNodePort. + // Reject changing the value of the HealthCheckNodePort field. + case neededHealthCheckNodePort && needsHealthCheckNodePort: + if oldHealthCheckNodePort != newHealthCheckNodePort { + glog.Warningf("Attempt to change value of health check node port DENIED") + var fldPath *field.Path + if _, ok := service.Annotations[apiservice.BetaAnnotationHealthCheckNodePort]; ok { + fldPath = field.NewPath("metadata", "annotations").Key(apiservice.BetaAnnotationHealthCheckNodePort) + } else { + fldPath = field.NewPath("spec", "healthCheckNodePort") + } + el := field.ErrorList{field.Invalid(fldPath, newHealthCheckNodePort, + "cannot change healthCheckNodePort on loadBalancer service with externalTraffic=Local during update")} return false, errors.NewInvalid(api.Kind("Service"), service.Name, el) } } @@ -449,9 +412,17 @@ func (rs *REST) Update(ctx genericapirequest.Context, name string, objInfo rest. service.Status.LoadBalancer = api.LoadBalancerStatus{} } - success, err := rs.healthCheckNodePortUpdate(oldService, service) - if !success { - return nil, false, err + // Handle ExternalTraiffc related updates. + if utilfeature.DefaultFeatureGate.Enabled(features.ExternalTrafficLocalOnly) { + apiservice.SetDefaultExternalTrafficPolicyIfNeeded(service) + success, err := rs.healthCheckNodePortUpdate(oldService, service) + if !success || err != nil { + return nil, false, err + } + externalTrafficPolicyUpdate(oldService, service) + if errs := validation.ValidateServiceExternalTrafficFieldsCombination(service); len(errs) > 0 { + return nil, false, errors.NewInvalid(api.Kind("Service"), service.Name, errs) + } } out, err := rs.registry.UpdateService(ctx, service) @@ -566,10 +537,6 @@ func shouldAssignNodePorts(service *api.Service) bool { } } -func shouldCheckOrAssignHealthCheckNodePort(service *api.Service) bool { - return (utilfeature.DefaultFeatureGate.Enabled(features.ExternalTrafficLocalOnly) && apiservice.NeedsHealthCheck(service)) -} - // Loop through the service ports list, find one with the same port number and // NodePort specified, return this NodePort otherwise return 0. func findRequestedNodePort(port int, servicePorts []api.ServicePort) int { @@ -581,3 +548,26 @@ func findRequestedNodePort(port int, servicePorts []api.ServicePort) int { } return 0 } + +// allocateHealthCheckNodePort allocates health check node port to service. +func (rs *REST) allocateHealthCheckNodePort(service *api.Service) error { + healthCheckNodePort := apiservice.GetServiceHealthCheckNodePort(service) + if healthCheckNodePort != 0 { + // If the request has a health check nodePort in mind, attempt to reserve it. + err := rs.serviceNodePorts.Allocate(int(healthCheckNodePort)) + if err != nil { + return fmt.Errorf("failed to allocate requested HealthCheck NodePort %v: %v", + service.Spec.HealthCheckNodePort, err) + } + glog.Infof("Reserved user requested nodePort: %d", service.Spec.HealthCheckNodePort) + } else { + // If the request has no health check nodePort specified, allocate any. + healthCheckNodePort, err := rs.serviceNodePorts.AllocateNext() + if err != nil { + return fmt.Errorf("failed to allocate a HealthCheck NodePort %v: %v", healthCheckNodePort, err) + } + apiservice.SetServiceHealthCheckNodePort(service, int32(healthCheckNodePort)) + glog.Infof("Reserved allocated nodePort: %d", healthCheckNodePort) + } + return nil +}