diff --git a/pkg/apis/experimental/types.go b/pkg/apis/experimental/types.go index b379857a8f3..17f326f5b10 100644 --- a/pkg/apis/experimental/types.go +++ b/pkg/apis/experimental/types.go @@ -558,7 +558,12 @@ type HTTPIngressRuleValue struct { // IngressPath associates a path regex with an IngressBackend. // Incoming urls matching the Path are forwarded to the Backend. type HTTPIngressPath struct { - // Path is a regex matched against the url of an incoming request. + // Path is a extended POSIX regex as defined by IEEE Std 1003.1, + // (i.e this follows the egrep/unix syntax, not the perl syntax) + // matched against the path of an incoming request. Currently it can + // contain characters disallowed from the conventional "path" + // part of a URL as defined by RFC 3986. Paths must begin with + // a '/'. Path string `json:"path,omitempty"` // Define the referenced service endpoint which the traffic will be @@ -572,5 +577,5 @@ type IngressBackend struct { ServiceName string `json:"serviceName"` // Specifies the port of the referenced service. - ServicePort util.IntOrString `json:"servicePort,omitempty"` + ServicePort util.IntOrString `json:"servicePort"` } diff --git a/pkg/apis/experimental/v1alpha1/types.go b/pkg/apis/experimental/v1alpha1/types.go index 4c6a429229f..4ef0b44fac1 100644 --- a/pkg/apis/experimental/v1alpha1/types.go +++ b/pkg/apis/experimental/v1alpha1/types.go @@ -579,5 +579,5 @@ type IngressBackend struct { ServiceName string `json:"serviceName"` // Specifies the port of the referenced service. - ServicePort util.IntOrString `json:"servicePort,omitempty"` + ServicePort util.IntOrString `json:"servicePort"` } diff --git a/pkg/apis/experimental/validation/validation.go b/pkg/apis/experimental/validation/validation.go index 0d369597616..e72577d9cd1 100644 --- a/pkg/apis/experimental/validation/validation.go +++ b/pkg/apis/experimental/validation/validation.go @@ -17,7 +17,11 @@ limitations under the License. package validation import ( + "fmt" + "net" + "regexp" "strconv" + "strings" "k8s.io/kubernetes/pkg/api" apivalidation "k8s.io/kubernetes/pkg/api/validation" @@ -27,10 +31,19 @@ import ( errs "k8s.io/kubernetes/pkg/util/fielderrors" "k8s.io/kubernetes/pkg/util/sets" "k8s.io/kubernetes/pkg/util/validation" + utilvalidation "k8s.io/kubernetes/pkg/util/validation" ) const isNegativeErrorMsg string = `must be non-negative` +// TODO: Expose from apivalidation instead of duplicating. +func intervalErrorMsg(lo, hi int) string { + return fmt.Sprintf(`must be greater than %d and less than %d`, lo, hi) +} + +var portRangeErrorMsg string = intervalErrorMsg(0, 65536) +var portNameErrorMsg string = fmt.Sprintf(`must be an IANA_SVC_NAME (at most 15 characters, matching regex %s, it must contain at least one letter [a-z], and hyphens cannot be adjacent to other hyphens): e.g. "http"`, validation.IdentifierNoHyphensBeginEndFmt) + // ValidateHorizontalPodAutoscaler can be used to check whether the given autoscaler name is valid. // Prefix indicates this name will be used as part of generation, in which case trailing dashes are allowed. func ValidateHorizontalPodAutoscalerName(name string, prefix bool) (bool, string) { @@ -372,18 +385,6 @@ func ValidateJobStatusUpdate(oldStatus, status experimental.JobStatus) errs.Vali return allErrs } -// ValidateIngressName validates that the given name can be used as an Ingress name. -func ValidateIngressName(name string, prefix bool) (bool, string) { - return apivalidation.NameIsDNSSubdomain(name, prefix) -} - -// ValidateIngressSpec tests if required fields in the IngressSpec are set. -func ValidateIngressSpec(spec *experimental.IngressSpec) errs.ValidationErrorList { - allErrs := errs.ValidationErrorList{} - // TODO: Validate IngressHosts/Rules etc. - return allErrs -} - // ValidateIngress tests if required fields in the Ingress are set. func ValidateIngress(ingress *experimental.Ingress) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} @@ -392,10 +393,111 @@ func ValidateIngress(ingress *experimental.Ingress) errs.ValidationErrorList { return allErrs } -// ValidateIngressUpdate tests if required fields in the Ingress are set. -func ValidateIngressUpdate(oldController, ingress *experimental.Ingress) errs.ValidationErrorList { +// ValidateIngressName validates that the given name can be used as an Ingress name. +func ValidateIngressName(name string, prefix bool) (bool, string) { + return apivalidation.NameIsDNSSubdomain(name, prefix) +} + +// ValidateIngressSpec tests if required fields in the IngressSpec are set. +func ValidateIngressSpec(spec *experimental.IngressSpec) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&ingress.ObjectMeta, &oldController.ObjectMeta).Prefix("metadata")...) + // TODO: Is a default backend mandatory? + if spec.Backend != nil { + allErrs = append(allErrs, validateIngressBackend(spec.Backend).Prefix("backend")...) + } else if len(spec.Rules) == 0 { + allErrs = append(allErrs, errs.NewFieldInvalid("rules", spec.Rules, "Either a default backend or a set of host rules are required for ingress.")) + } + if len(spec.Rules) > 0 { + allErrs = append(allErrs, validateIngressRules(spec.Rules).Prefix("rules")...) + } + return allErrs +} + +// ValidateIngressUpdate tests if required fields in the Ingress are set. +func ValidateIngressUpdate(oldIngress, ingress *experimental.Ingress) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} + allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&ingress.ObjectMeta, &oldIngress.ObjectMeta).Prefix("metadata")...) allErrs = append(allErrs, ValidateIngressSpec(&ingress.Spec).Prefix("spec")...) return allErrs } + +func validateIngressRules(IngressRules []experimental.IngressRule) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} + if len(IngressRules) == 0 { + return append(allErrs, errs.NewFieldRequired("IngressRules")) + } + for _, ih := range IngressRules { + if len(ih.Host) > 0 { + // TODO: Ports and ips are allowed in the host part of a url + // according to RFC 3986, consider allowing them. + if valid, errMsg := apivalidation.NameIsDNSSubdomain(ih.Host, false); !valid { + allErrs = append(allErrs, errs.NewFieldInvalid("host", ih.Host, errMsg)) + } + if isIP := (net.ParseIP(ih.Host) != nil); isIP { + allErrs = append(allErrs, errs.NewFieldInvalid("host", ih.Host, "Host must be a DNS name, not ip address")) + } + } + allErrs = append(allErrs, validateIngressRuleValue(&ih.IngressRuleValue).Prefix("ingressRule")...) + } + return allErrs +} + +func validateIngressRuleValue(ingressRule *experimental.IngressRuleValue) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} + if ingressRule.HTTP != nil { + allErrs = append(allErrs, validateHTTPIngressRuleValue(ingressRule.HTTP).Prefix("http")...) + } + return allErrs +} + +func validateHTTPIngressRuleValue(httpIngressRuleValue *experimental.HTTPIngressRuleValue) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} + if len(httpIngressRuleValue.Paths) == 0 { + allErrs = append(allErrs, errs.NewFieldRequired("paths")) + } + for _, rule := range httpIngressRuleValue.Paths { + if len(rule.Path) > 0 { + if !strings.HasPrefix(rule.Path, "/") { + allErrs = append(allErrs, errs.NewFieldInvalid("path", rule.Path, "path must begin with /")) + } + // TODO: More draconian path regex validation. + // Path must be a valid regex. This is the basic requirement. + // In addition to this any characters not allowed in a path per + // RFC 3986 section-3.3 cannot appear as a literal in the regex. + // Consider the example: http://host/valid?#bar, everything after + // the last '/' is a valid regex that matches valid#bar, which + // isn't a valid path, because the path terminates at the first ? + // or #. A more sophisticated form of validation would detect that + // the user is confusing url regexes with path regexes. + _, err := regexp.CompilePOSIX(rule.Path) + if err != nil { + allErrs = append(allErrs, errs.NewFieldInvalid("path", rule.Path, "httpIngressRuleValue.path must be a valid regex.")) + } + } + allErrs = append(allErrs, validateIngressBackend(&rule.Backend).Prefix("backend")...) + } + return allErrs +} + +// validateIngressBackend tests if a given backend is valid. +func validateIngressBackend(backend *experimental.IngressBackend) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} + + // All backends must reference a single local service by name, and a single service port by name or number. + if len(backend.ServiceName) == 0 { + return append(allErrs, errs.NewFieldRequired("serviceName")) + } else if ok, errMsg := apivalidation.ValidateServiceName(backend.ServiceName, false); !ok { + allErrs = append(allErrs, errs.NewFieldInvalid("serviceName", backend.ServiceName, errMsg)) + } + if backend.ServicePort.Kind == util.IntstrString { + if !utilvalidation.IsDNS1123Label(backend.ServicePort.StrVal) { + allErrs = append(allErrs, errs.NewFieldInvalid("servicePort", backend.ServicePort.StrVal, apivalidation.DNS1123LabelErrorMsg)) + } + if !utilvalidation.IsValidPortName(backend.ServicePort.StrVal) { + allErrs = append(allErrs, errs.NewFieldInvalid("servicePort", backend.ServicePort.StrVal, portNameErrorMsg)) + } + } else if !utilvalidation.IsValidPortNum(backend.ServicePort.IntVal) { + allErrs = append(allErrs, errs.NewFieldInvalid("servicePort", backend.ServicePort, portRangeErrorMsg)) + } + return allErrs +} diff --git a/pkg/apis/experimental/validation/validation_test.go b/pkg/apis/experimental/validation/validation_test.go index 32d25f48762..dcc65e97ad3 100644 --- a/pkg/apis/experimental/validation/validation_test.go +++ b/pkg/apis/experimental/validation/validation_test.go @@ -17,6 +17,7 @@ limitations under the License. package validation import ( + "fmt" "strings" "testing" @@ -848,3 +849,104 @@ func TestValidateJob(t *testing.T) { } } } + +type ingressRules map[string]string + +func TestValidateIngress(t *testing.T) { + defaultBackend := experimental.IngressBackend{ + ServiceName: "default-backend", + ServicePort: util.IntOrString{Kind: util.IntstrInt, IntVal: 80}, + } + + newValid := func() experimental.Ingress { + return experimental.Ingress{ + ObjectMeta: api.ObjectMeta{ + Name: "foo", + Namespace: api.NamespaceDefault, + }, + Spec: experimental.IngressSpec{ + Backend: &experimental.IngressBackend{ + ServiceName: "default-backend", + ServicePort: util.IntOrString{Kind: util.IntstrInt, IntVal: 80}, + }, + Rules: []experimental.IngressRule{ + { + Host: "foo.bar.com", + IngressRuleValue: experimental.IngressRuleValue{ + HTTP: &experimental.HTTPIngressRuleValue{ + Paths: []experimental.HTTPIngressPath{ + { + Path: "/foo", + Backend: defaultBackend, + }, + }, + }, + }, + }, + }, + }, + Status: experimental.IngressStatus{ + LoadBalancer: api.LoadBalancerStatus{ + Ingress: []api.LoadBalancerIngress{ + {IP: "127.0.0.1"}, + }, + }, + }, + } + } + servicelessBackend := newValid() + servicelessBackend.Spec.Backend.ServiceName = "" + invalidNameBackend := newValid() + invalidNameBackend.Spec.Backend.ServiceName = "defaultBackend" + noPortBackend := newValid() + noPortBackend.Spec.Backend = &experimental.IngressBackend{ServiceName: defaultBackend.ServiceName} + noForwardSlashPath := newValid() + noForwardSlashPath.Spec.Rules[0].IngressRuleValue.HTTP.Paths = []experimental.HTTPIngressPath{ + { + Path: "invalid", + Backend: defaultBackend, + }, + } + noPaths := newValid() + noPaths.Spec.Rules[0].IngressRuleValue.HTTP.Paths = []experimental.HTTPIngressPath{} + badHost := newValid() + badHost.Spec.Rules[0].Host = "foobar:80" + badRegexPath := newValid() + badPathExpr := "/invalid[" + badRegexPath.Spec.Rules[0].IngressRuleValue.HTTP.Paths = []experimental.HTTPIngressPath{ + { + Path: badPathExpr, + Backend: defaultBackend, + }, + } + badPathErr := fmt.Sprintf("spec.rules.ingressRule.http.path: invalid value '%v'", + badPathExpr) + hostIP := "127.0.0.1" + badHostIP := newValid() + badHostIP.Spec.Rules[0].Host = hostIP + badHostIPErr := fmt.Sprintf("spec.rules.host: invalid value '%v'", hostIP) + + errorCases := map[string]experimental.Ingress{ + "spec.backend.serviceName: required value": servicelessBackend, + "spec.backend.serviceName: invalid value": invalidNameBackend, + "spec.backend.servicePort: invalid value": noPortBackend, + "spec.rules.host: invalid value": badHost, + "spec.rules.ingressRule.http.paths: required value": noPaths, + "spec.rules.ingressRule.http.path: invalid value": noForwardSlashPath, + } + errorCases[badPathErr] = badRegexPath + errorCases[badHostIPErr] = badHostIP + + for k, v := range errorCases { + errs := ValidateIngress(&v) + if len(errs) == 0 { + t.Errorf("expected failure for %s", k) + } else { + s := strings.Split(k, ":") + err := errs[0].(*errors.ValidationError) + if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) { + t.Errorf("unexpected error: %v, expected: %s", errs[0], k) + } + } + } +} diff --git a/pkg/registry/ingress/etcd/etcd_test.go b/pkg/registry/ingress/etcd/etcd_test.go index 52625b9892d..6efaf0481ab 100755 --- a/pkg/registry/ingress/etcd/etcd_test.go +++ b/pkg/registry/ingress/etcd/etcd_test.go @@ -36,7 +36,7 @@ func newStorage(t *testing.T) (*REST, *tools.FakeEtcdClient) { } var ( - namespace = "foo-namespace" + namespace = api.NamespaceNone name = "foo-ingress" defaultHostname = "foo.bar.com" defaultBackendName = "default-backend" @@ -102,17 +102,15 @@ func newIngress(pathMap map[string]string) *experimental.Ingress { } } -func validNewIngress() *experimental.Ingress { +func validIngress() *experimental.Ingress { return newIngress(defaultPathMap) } -var validIngress = *validNewIngress() - func TestCreate(t *testing.T) { storage, fakeClient := newStorage(t) test := registrytest.New(t, fakeClient, storage.Etcd) - ingress := validNewIngress() - noDefaultBackendAndRules := validNewIngress() + ingress := validIngress() + noDefaultBackendAndRules := validIngress() noDefaultBackendAndRules.Spec.Backend = &experimental.IngressBackend{} noDefaultBackendAndRules.Spec.Rules = []experimental.IngressRule{} badPath := validIngress() @@ -121,9 +119,8 @@ func TestCreate(t *testing.T) { test.TestCreate( // valid ingress, - // TODO: Add invalid Ingress tests once we have validation. noDefaultBackendAndRules, - noIngressHosts, + badPath, ) } @@ -132,7 +129,7 @@ func TestUpdate(t *testing.T) { test := registrytest.New(t, fakeClient, storage.Etcd) test.TestUpdate( // valid - validNewIngress(), + validIngress(), // updateFunc func(obj runtime.Object) runtime.Object { object := obj.(*experimental.Ingress) @@ -166,26 +163,26 @@ func TestUpdate(t *testing.T) { func TestDelete(t *testing.T) { storage, fakeClient := newStorage(t) test := registrytest.New(t, fakeClient, storage.Etcd) - test.TestDelete(validNewIngress()) + test.TestDelete(validIngress()) } func TestGet(t *testing.T) { storage, fakeClient := newStorage(t) test := registrytest.New(t, fakeClient, storage.Etcd) - test.TestGet(validNewIngress()) + test.TestGet(validIngress()) } func TestList(t *testing.T) { storage, fakeClient := newStorage(t) test := registrytest.New(t, fakeClient, storage.Etcd) - test.TestList(validNewIngress()) + test.TestList(validIngress()) } func TestWatch(t *testing.T) { storage, fakeClient := newStorage(t) test := registrytest.New(t, fakeClient, storage.Etcd) test.TestWatch( - validNewIngress(), + validIngress(), // matching labels []labels.Set{}, // not matching labels diff --git a/pkg/registry/ingress/strategy.go b/pkg/registry/ingress/strategy.go index 76e5ff09df7..d547d08ba65 100644 --- a/pkg/registry/ingress/strategy.go +++ b/pkg/registry/ingress/strategy.go @@ -44,12 +44,19 @@ func (ingressStrategy) NamespaceScoped() bool { return true } +// PrepareForCreate clears the status of an Ingress before creation. func (ingressStrategy) PrepareForCreate(obj runtime.Object) { + ingress := obj.(*experimental.Ingress) + ingress.Status = experimental.IngressStatus{} + + ingress.Generation = 1 } +// PrepareForUpdate clears fields that are not allowed to be set by end users on update. func (ingressStrategy) PrepareForUpdate(obj, old runtime.Object) { newIngress := obj.(*experimental.Ingress) oldIngress := old.(*experimental.Ingress) + //TODO: Clear Ingress status once we have a sub-resource. // Any changes to the spec increment the generation number, any changes to the // status should reflect the generation number of the corresponding object. @@ -60,12 +67,14 @@ func (ingressStrategy) PrepareForUpdate(obj, old runtime.Object) { } +// Validate validates a new Ingress. func (ingressStrategy) Validate(ctx api.Context, obj runtime.Object) fielderrors.ValidationErrorList { ingress := obj.(*experimental.Ingress) err := validation.ValidateIngress(ingress) return err } +// AllowCreateOnUpdate is false for Ingress; this means POST is needed to create one. func (ingressStrategy) AllowCreateOnUpdate() bool { return false } @@ -77,6 +86,7 @@ func (ingressStrategy) ValidateUpdate(ctx api.Context, obj, old runtime.Object) return append(validationErrorList, updateErrorList...) } +// AllowUnconditionalUpdate is the default update policy for Ingress objects. func (ingressStrategy) AllowUnconditionalUpdate() bool { return true }