Merge pull request #16717 from liggitt/hpa_ref_validation

Auto commit by PR queue bot

pkg/api/validation/name.go

@@ -27,14 +27,11 @@ var NameMayNotBe = []string{".", ".."}
 // NameMayNotContain specifies substrings that cannot be used in names specified as path segments (like the REST API or etcd store)
 var NameMayNotContain = []string{"/", "%"}
 
-// ValidatePathSegmentName validates the name can be used as a path segment
+// IsValidPathSegmentName validates the name can be safely encoded as a path segment
-func ValidatePathSegmentName(name string, prefix bool) (bool, string) {
+func IsValidPathSegmentName(name string) (bool, string) {
-	// Only check for exact matches if this is the full name (not a prefix)
+	for _, illegalName := range NameMayNotBe {
-	if prefix == false {
+		if name == illegalName {
-		for _, illegalName := range NameMayNotBe {
+			return false, fmt.Sprintf(`name may not be %q`, illegalName)
-			if name == illegalName {
-				return false, fmt.Sprintf(`name may not be %q`, illegalName)
-			}
 		}
 	}
 
@@ -46,3 +43,24 @@ func ValidatePathSegmentName(name string, prefix bool) (bool, string) {
 
 	return true, ""
 }
+
+// IsValidPathSegmentPrefix validates the name can be used as a prefix for a name which will be encoded as a path segment
+// It does not check for exact matches with disallowed names, since an arbitrary suffix might make the name valid
+func IsValidPathSegmentPrefix(name string) (bool, string) {
+	for _, illegalContent := range NameMayNotContain {
+		if strings.Contains(name, illegalContent) {
+			return false, fmt.Sprintf(`name may not contain %q`, illegalContent)
+		}
+	}
+
+	return true, ""
+}
+
+// ValidatePathSegmentName validates the name can be safely encoded as a path segment
+func ValidatePathSegmentName(name string, prefix bool) (bool, string) {
+	if prefix {
+		return IsValidPathSegmentPrefix(name)
+	} else {
+		return IsValidPathSegmentName(name)
+	}
+}

pkg/apis/extensions/validation/validation.go

@@ -55,6 +55,33 @@ func validateHorizontalPodAutoscalerSpec(autoscaler extensions.HorizontalPodAuto
 	if autoscaler.CPUUtilization != nil && autoscaler.CPUUtilization.TargetPercentage < 1 {
 		allErrs = append(allErrs, errs.NewFieldInvalid("cpuUtilization.targetPercentage", autoscaler.CPUUtilization.TargetPercentage, `must be bigger or equal to 1`))
 	}
+	if refErrs := ValidateSubresourceReference(autoscaler.ScaleRef); len(refErrs) > 0 {
+		allErrs = append(allErrs, refErrs.Prefix("scaleRef")...)
+	} else if autoscaler.ScaleRef.Subresource != "scale" {
+		allErrs = append(allErrs, errs.NewFieldValueNotSupported("scaleRef.subresource", autoscaler.ScaleRef.Subresource, []string{"scale"}))
+	}
+	return allErrs
+}
+
+func ValidateSubresourceReference(ref extensions.SubresourceReference) errs.ValidationErrorList {
+	allErrs := errs.ValidationErrorList{}
+	if len(ref.Kind) == 0 {
+		allErrs = append(allErrs, errs.NewFieldRequired("kind"))
+	} else if ok, msg := apivalidation.IsValidPathSegmentName(ref.Kind); !ok {
+		allErrs = append(allErrs, errs.NewFieldInvalid("kind", ref.Kind, msg))
+	}
+
+	if len(ref.Name) == 0 {
+		allErrs = append(allErrs, errs.NewFieldRequired("name"))
+	} else if ok, msg := apivalidation.IsValidPathSegmentName(ref.Name); !ok {
+		allErrs = append(allErrs, errs.NewFieldInvalid("name", ref.Name, msg))
+	}
+
+	if len(ref.Subresource) == 0 {
+		allErrs = append(allErrs, errs.NewFieldRequired("subresource"))
+	} else if ok, msg := apivalidation.IsValidPathSegmentName(ref.Subresource); !ok {
+		allErrs = append(allErrs, errs.NewFieldInvalid("subresource", ref.Subresource, msg))
+	}
 	return allErrs
 }
 

pkg/apis/extensions/validation/validation_test.go

@@ -36,6 +36,8 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) {
 			},
 			Spec: extensions.HorizontalPodAutoscalerSpec{
 				ScaleRef: extensions.SubresourceReference{
+					Kind:        "ReplicationController",
+					Name:        "myrc",
 					Subresource: "scale",
 				},
 				MinReplicas:    newInt(1),
@@ -50,6 +52,8 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) {
 			},
 			Spec: extensions.HorizontalPodAutoscalerSpec{
 				ScaleRef: extensions.SubresourceReference{
+					Kind:        "ReplicationController",
+					Name:        "myrc",
 					Subresource: "scale",
 				},
 				MinReplicas: newInt(1),
@@ -67,6 +71,90 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) {
 		horizontalPodAutoscaler extensions.HorizontalPodAutoscaler
 		msg                     string
 	}{
+		{
+			horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
+				ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
+				Spec: extensions.HorizontalPodAutoscalerSpec{
+					ScaleRef:       extensions.SubresourceReference{Name: "myrc", Subresource: "scale"},
+					MinReplicas:    newInt(1),
+					MaxReplicas:    5,
+					CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
+				},
+			},
+			msg: "scaleRef.kind: required",
+		},
+		{
+			horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
+				ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
+				Spec: extensions.HorizontalPodAutoscalerSpec{
+					ScaleRef:       extensions.SubresourceReference{Kind: "..", Name: "myrc", Subresource: "scale"},
+					MinReplicas:    newInt(1),
+					MaxReplicas:    5,
+					CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
+				},
+			},
+			msg: "scaleRef.kind: invalid",
+		},
+		{
+			horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
+				ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
+				Spec: extensions.HorizontalPodAutoscalerSpec{
+					ScaleRef:       extensions.SubresourceReference{Kind: "ReplicationController", Subresource: "scale"},
+					MinReplicas:    newInt(1),
+					MaxReplicas:    5,
+					CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
+				},
+			},
+			msg: "scaleRef.name: required",
+		},
+		{
+			horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
+				ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
+				Spec: extensions.HorizontalPodAutoscalerSpec{
+					ScaleRef:       extensions.SubresourceReference{Kind: "ReplicationController", Name: "..", Subresource: "scale"},
+					MinReplicas:    newInt(1),
+					MaxReplicas:    5,
+					CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
+				},
+			},
+			msg: "scaleRef.name: invalid",
+		},
+		{
+			horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
+				ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
+				Spec: extensions.HorizontalPodAutoscalerSpec{
+					ScaleRef:       extensions.SubresourceReference{Kind: "ReplicationController", Name: "myrc", Subresource: ""},
+					MinReplicas:    newInt(1),
+					MaxReplicas:    5,
+					CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
+				},
+			},
+			msg: "scaleRef.subresource: required",
+		},
+		{
+			horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
+				ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
+				Spec: extensions.HorizontalPodAutoscalerSpec{
+					ScaleRef:       extensions.SubresourceReference{Kind: "ReplicationController", Name: "myrc", Subresource: ".."},
+					MinReplicas:    newInt(1),
+					MaxReplicas:    5,
+					CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
+				},
+			},
+			msg: "scaleRef.subresource: invalid",
+		},
+		{
+			horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
+				ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
+				Spec: extensions.HorizontalPodAutoscalerSpec{
+					ScaleRef:       extensions.SubresourceReference{Kind: "ReplicationController", Name: "myrc", Subresource: "randomsubresource"},
+					MinReplicas:    newInt(1),
+					MaxReplicas:    5,
+					CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
+				},
+			},
+			msg: "scaleRef.subresource: unsupported",
+		},
 		{
 			horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
 				ObjectMeta: api.ObjectMeta{

pkg/client/unversioned/request.go

@@ -32,6 +32,7 @@ import (
 	"github.com/golang/glog"
 	"k8s.io/kubernetes/pkg/api/errors"
 	"k8s.io/kubernetes/pkg/api/unversioned"
+	"k8s.io/kubernetes/pkg/api/validation"
 	"k8s.io/kubernetes/pkg/client/metrics"
 	"k8s.io/kubernetes/pkg/conversion/queryparams"
 	"k8s.io/kubernetes/pkg/fields"
@@ -149,6 +150,10 @@ func (r *Request) Resource(resource string) *Request {
 		r.err = fmt.Errorf("resource already set to %q, cannot change to %q", r.resource, resource)
 		return r
 	}
+	if ok, msg := validation.IsValidPathSegmentName(resource); !ok {
+		r.err = fmt.Errorf("invalid resource %q: %s", resource, msg)
+		return r
+	}
 	r.resource = resource
 	return r
 }
@@ -164,6 +169,12 @@ func (r *Request) SubResource(subresources ...string) *Request {
 		r.err = fmt.Errorf("subresource already set to %q, cannot change to %q", r.resource, subresource)
 		return r
 	}
+	for _, s := range subresources {
+		if ok, msg := validation.IsValidPathSegmentName(s); !ok {
+			r.err = fmt.Errorf("invalid subresource %q: %s", s, msg)
+			return r
+		}
+	}
 	r.subresource = subresource
 	return r
 }
@@ -181,6 +192,10 @@ func (r *Request) Name(resourceName string) *Request {
 		r.err = fmt.Errorf("resource name already set to %q, cannot change to %q", r.resourceName, resourceName)
 		return r
 	}
+	if ok, msg := validation.IsValidPathSegmentName(resourceName); !ok {
+		r.err = fmt.Errorf("invalid resource name %q: %s", resourceName, msg)
+		return r
+	}
 	r.resourceName = resourceName
 	return r
 }
@@ -194,6 +209,10 @@ func (r *Request) Namespace(namespace string) *Request {
 		r.err = fmt.Errorf("namespace already set to %q, cannot change to %q", r.namespace, namespace)
 		return r
 	}
+	if ok, msg := validation.IsValidPathSegmentName(namespace); !ok {
+		r.err = fmt.Errorf("invalid namespace %q: %s", namespace, msg)
+		return r
+	}
 	r.namespaceSet = true
 	r.namespace = namespace
 	return r

pkg/client/unversioned/request_test.go

@@ -144,6 +144,25 @@ func TestRequestSetTwiceError(t *testing.T) {
 	}
 }
 
+func TestInvalidSegments(t *testing.T) {
+	invalidSegments := []string{".", "..", "test/segment", "test%2bsegment"}
+	setters := map[string]func(string, *Request){
+		"namespace":   func(s string, r *Request) { r.Namespace(s) },
+		"resource":    func(s string, r *Request) { r.Resource(s) },
+		"name":        func(s string, r *Request) { r.Name(s) },
+		"subresource": func(s string, r *Request) { r.SubResource(s) },
+	}
+	for _, invalidSegment := range invalidSegments {
+		for setterName, setter := range setters {
+			r := &Request{}
+			setter(invalidSegment, r)
+			if r.err == nil {
+				t.Errorf("%s: %s: expected error, got none", setterName, invalidSegment)
+			}
+		}
+	}
+}
+
 func TestRequestParam(t *testing.T) {
 	r := (&Request{}).Param("foo", "a")
 	if !reflect.DeepEqual(r.params, url.Values{"foo": []string{"a"}}) {

pkg/registry/generic/etcd/etcd.go

@@ -129,7 +129,7 @@ func NamespaceKeyFunc(ctx api.Context, prefix string, name string) (string, erro
 	if len(name) == 0 {
 		return "", kubeerr.NewBadRequest("Name parameter required.")
 	}
-	if ok, msg := validation.ValidatePathSegmentName(name, false); !ok {
+	if ok, msg := validation.IsValidPathSegmentName(name); !ok {
 		return "", kubeerr.NewBadRequest(fmt.Sprintf("Name parameter invalid: %v.", msg))
 	}
 	key = key + "/" + name
@@ -141,7 +141,7 @@ func NoNamespaceKeyFunc(ctx api.Context, prefix string, name string) (string, er
 	if len(name) == 0 {
 		return "", kubeerr.NewBadRequest("Name parameter required.")
 	}
-	if ok, msg := validation.ValidatePathSegmentName(name, false); !ok {
+	if ok, msg := validation.IsValidPathSegmentName(name); !ok {
 		return "", kubeerr.NewBadRequest(fmt.Sprintf("Name parameter invalid: %v.", msg))
 	}
 	key := prefix + "/" + name

pkg/registry/horizontalpodautoscaler/etcd/etcd_test.go

@@ -44,6 +44,8 @@ func validNewHorizontalPodAutoscaler(name string) *extensions.HorizontalPodAutos
 		},
 		Spec: extensions.HorizontalPodAutoscalerSpec{
 			ScaleRef: extensions.SubresourceReference{
+				Kind:        "ReplicationController",
+				Name:        "myrc",
 				Subresource: "scale",
 			},
 			MaxReplicas:    5,

pkg/storage/util.go

@@ -59,7 +59,7 @@ func NamespaceKeyFunc(prefix string, obj runtime.Object) (string, error) {
 		return "", err
 	}
 	name := meta.Name()
-	if ok, msg := validation.ValidatePathSegmentName(name, false); !ok {
+	if ok, msg := validation.IsValidPathSegmentName(name); !ok {
 		return "", fmt.Errorf("invalid name: %v", msg)
 	}
 	return prefix + "/" + meta.Namespace() + "/" + meta.Name(), nil
@@ -71,7 +71,7 @@ func NoNamespaceKeyFunc(prefix string, obj runtime.Object) (string, error) {
 		return "", err
 	}
 	name := meta.Name()
-	if ok, msg := validation.ValidatePathSegmentName(name, false); !ok {
+	if ok, msg := validation.IsValidPathSegmentName(name); !ok {
 		return "", fmt.Errorf("invalid name: %v", msg)
 	}
 	return prefix + "/" + meta.Name(), nil