Validation

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"`
 }

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"`
 }

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

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

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

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
 }