Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refine ESIPP validation logic in validation.go #44741

Merged
merged 1 commit into from Apr 25, 2017
Merged

Refine ESIPP validation logic in validation.go #44741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
135 changes: 77 additions & 58 deletions pkg/api/validation/validation.go
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@ import (
"path"
"reflect"
"regexp"
"strconv"
"strings"

"github.com/golang/glog"
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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) {
// 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))
// validateServiceExternalTrafficFields validates ExternalTraffic related annotations
// have legal value.
func validateServiceExternalTrafficFields(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)))
}
}
}
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"))
}
}
}

onlyLocalValueChanged := oldOnlyLocalAlphaOk && onlyLocalAlphaOk && oldOnlyLocalAlpha != onlyLocalAlpha
onlyLocalValueNew := !oldOnlyLocalAlphaOk && onlyLocalAlphaOk
onlyLocalValueGone := !onlyLocalAlphaOk && !onlyLocalBetaOk && oldOnlyLocalAlphaOk
hcOnlyLocalMismatch := onlyLocalAlphaOk && healthCheckBetaOk
allErrs = append(allErrs, validateServiceExternalTrafficAPIVersion(service)...)

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
}

if onlyLocalValueNew || onlyLocalValueChanged || onlyLocalValueGone || hcOnlyLocalMismatch {
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 the beta version %v",
apiservice.BetaAnnotationExternalTraffic)
msg := fmt.Sprintf("please replace the alpha annotation with beta annotation")
allErrs = append(allErrs, field.Invalid(fieldPath, apiservice.AlphaAnnotationExternalTraffic, msg))
}
return

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))
}

return allErrs
}

// ValidateServiceUpdate tests if required fields in the service are set during an update
Expand All @@ -2829,8 +2849,7 @@ func ValidateServiceUpdate(service, oldService *api.Service) field.ErrorList {
}
}

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

Expand Down
36 changes: 31 additions & 5 deletions pkg/api/validation/validation_test.go
View file
Open in desktop
Expand Up @@ -5630,10 +5630,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,
},
{
Expand Down Expand Up @@ -7036,22 +7062,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",
Expand Down