Merge pull request #329 from thockin/valid6

Add validation of Volumes

pkg/api/validation.go

@@ -30,27 +30,51 @@ func isSupportedManifestVersion(value string) bool {
 	return false
 }
 
-func isInvalid(field string, value interface{}) error {
+func errInvalid(field string, value interface{}) error {
 	return fmt.Errorf("%s is invalid: '%v'", field, value)
 }
 
-func isNotSupported(field string, value interface{}) error {
+func errNotSupported(field string, value interface{}) error {
 	return fmt.Errorf("%s is not supported: '%v'", field, value)
 }
 
+func errNotUnique(field string, value interface{}) error {
+	return fmt.Errorf("%s is not unique: '%v'", field, value)
+}
+
+func validateVolumes(volumes []Volume) (util.StringSet, error) {
+	allNames := util.StringSet{}
+	for i := range volumes {
+		vol := &volumes[i] // so we can set default values
+		if len(vol.Name) > 63 || !util.IsDNSLabel(vol.Name) {
+			return util.StringSet{}, errInvalid("Volume.Name", vol.Name)
+		}
+		if allNames.Has(vol.Name) {
+			return util.StringSet{}, errNotUnique("Volume.Name", vol.Name)
+		}
+		allNames.Insert(vol.Name)
+	}
+	return allNames, nil
+}
+
 // ValidateManifest tests that the specified ContainerManifest has valid data.
 // This includes checking formatting and uniqueness.  It also canonicalizes the
 // structure by setting default values and implementing any backwards-compatibility
 // tricks.
+// TODO(thockin): We should probably collect all errors rather than aborting the validation.
 func ValidateManifest(manifest *ContainerManifest) error {
 	if len(manifest.Version) == 0 {
-		return isInvalid("ContainerManifest.Version", manifest.Version)
+		return errInvalid("ContainerManifest.Version", manifest.Version)
 	}
 	if !isSupportedManifestVersion(manifest.Version) {
-		return isNotSupported("ContainerManifest.Version", manifest.Version)
+		return errNotSupported("ContainerManifest.Version", manifest.Version)
 	}
 	if len(manifest.ID) > 255 || !util.IsDNSSubdomain(manifest.ID) {
-		return isInvalid("ContainerManifest.ID", manifest.ID)
+		return errInvalid("ContainerManifest.ID", manifest.ID)
+	}
+	_, err := validateVolumes(manifest.Volumes)
+	if err != nil {
+		return err
 	}
 	// TODO(thockin): finish validation.
 	return nil

pkg/api/validation_test.go

@@ -21,11 +21,44 @@ import (
 	"testing"
 )
 
+func TestValidateVolumes(t *testing.T) {
+	successCase := []Volume{
+		{Name: "abc"},
+		{Name: "123"},
+		{Name: "abc-123"},
+	}
+	names, err := validateVolumes(successCase)
+	if err != nil {
+		t.Errorf("expected success: %v", err)
+	}
+	if len(names) != 3 || !names.Has("abc") || !names.Has("123") || !names.Has("abc-123") {
+		t.Errorf("wrong names result: %v", names)
+	}
+
+	errorCases := map[string][]Volume{
+		"zero-length name":     {{Name: ""}},
+		"name > 63 characters": {{Name: strings.Repeat("a", 64)}},
+		"name not a DNS label": {{Name: "a.b.c"}},
+		"name not unique":      {{Name: "abc"}, {Name: "abc"}},
+	}
+	for k, v := range errorCases {
+		_, err := validateVolumes(v)
+		if err == nil {
+			t.Errorf("expected failure for %s", k)
+		}
+	}
+}
+
 func TestValidateManifest(t *testing.T) {
 	successCases := []ContainerManifest{
 		{Version: "v1beta1", ID: "abc"},
 		{Version: "v1beta1", ID: "123"},
 		{Version: "v1beta1", ID: "abc.123.do-re-mi"},
+		{
+			Version: "v1beta1",
+			ID:      "abc",
+			Volumes: []Volume{{Name: "vol1"}, {Name: "vol2"}},
+		},
 	}
 	for _, manifest := range successCases {
 		err := ValidateManifest(&manifest)
@@ -40,6 +73,11 @@ func TestValidateManifest(t *testing.T) {
 		"zero-length id":         {Version: "v1beta1", ID: ""},
 		"id > 255 characters":    {Version: "v1beta1", ID: strings.Repeat("a", 256)},
 		"id not a DNS subdomain": {Version: "v1beta1", ID: "a.b.c."},
+		"invalid volume name": {
+			Version: "v1beta1",
+			ID:      "abc",
+			Volumes: []Volume{{Name: "vol.1"}},
+		},
 	}
 	for k, v := range errorCases {
 		err := ValidateManifest(&v)