Skip to content

Commit

Permalink
Merge pull request kubernetes#16717 from liggitt/hpa_ref_validation
Browse files Browse the repository at this point in the history 
Auto commit by PR queue bot
  • Loading branch information
k8s-merge-robot committed Nov 3, 2015
2 parents 34cbe48 + b3157d1 commit bad838f
Show file tree
Hide file tree
Showing 8 changed files with 185 additions and 12 deletions.
34 changes: 26 additions & 8 deletions pkg/api/validation/name.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,26 @@ 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
func ValidatePathSegmentName(name string, prefix bool) (bool, string) {
// Only check for exact matches if this is the full name (not a prefix)
if prefix == false {
for _, illegalName := range NameMayNotBe {
if name == illegalName {
return false, fmt.Sprintf(`name may not be %q`, illegalName)
}
// IsValidPathSegmentName validates the name can be safely encoded as a path segment
func IsValidPathSegmentName(name string) (bool, string) {
for _, illegalName := range NameMayNotBe {
if name == illegalName {
return false, fmt.Sprintf(`name may not be %q`, illegalName)
}
}

for _, illegalContent := range NameMayNotContain {
if strings.Contains(name, illegalContent) {
return false, fmt.Sprintf(`name may not contain %q`, illegalContent)
}
}

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)
Expand All @@ -46,3 +55,12 @@ func ValidatePathSegmentName(name string, prefix bool) (bool, string) {

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)
}
}
27 changes: 27 additions & 0 deletions pkg/apis/extensions/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
88 changes: 88 additions & 0 deletions pkg/apis/extensions/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) {
},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{
Kind: "ReplicationController",
Name: "myrc",
Subresource: "scale",
},
MinReplicas: newInt(1),
Expand All @@ -50,6 +52,8 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) {
},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{
Kind: "ReplicationController",
Name: "myrc",
Subresource: "scale",
},
MinReplicas: newInt(1),
Expand All @@ -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{
Expand Down
19 changes: 19 additions & 0 deletions pkg/client/unversioned/request.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
Expand Down
19 changes: 19 additions & 0 deletions pkg/client/unversioned/request_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"}}) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/registry/generic/etcd/etcd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
2 changes: 2 additions & 0 deletions pkg/registry/horizontalpodautoscaler/etcd/etcd_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ func validNewHorizontalPodAutoscaler(name string) *extensions.HorizontalPodAutos
},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{
Kind: "ReplicationController",
Name: "myrc",
Subresource: "scale",
},
MaxReplicas: 5,
Expand Down
4 changes: 2 additions & 2 deletions pkg/storage/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit bad838f

Please sign in to comment.