Merge pull request #2329 from derekwaynecarr/validate_minion

Centralize minion validation

pkg/api/validation/validation.go

@@ -492,3 +492,13 @@ func ValidateBoundPod(pod *api.BoundPod) (errors []error) {
 	}
 	return errors
 }
+
+// ValidateMinion tests if required fields in the minion are set.
+func ValidateMinion(minion *api.Minion) errs.ValidationErrorList {
+	allErrs := errs.ValidationErrorList{}
+	if len(minion.Name) == 0 {
+		allErrs = append(allErrs, errs.NewFieldRequired("name", minion.Name))
+	}
+	allErrs = append(allErrs, validateLabels(minion.Labels)...)
+	return allErrs
+}

pkg/api/validation/validation_test.go

@@ -1020,3 +1020,49 @@ func TestValidateBoundPodNoName(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateMinion(t *testing.T) {
+	validSelector := map[string]string{"a": "b"}
+	invalidSelector := map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "b"}
+	successCases := []api.Minion{
+		{
+			ObjectMeta: api.ObjectMeta{Name: "abc"},
+			HostIP:     "something",
+			Labels:     validSelector,
+		},
+		{
+			ObjectMeta: api.ObjectMeta{Name: "abc"},
+			HostIP:     "something",
+		},
+	}
+	for _, successCase := range successCases {
+		if errs := ValidateMinion(&successCase); len(errs) != 0 {
+			t.Errorf("expected success: %v", errs)
+		}
+	}
+
+	errorCases := map[string]api.Minion{
+		"zero-length Name": {
+			ObjectMeta: api.ObjectMeta{Name: ""},
+			HostIP:     "something",
+			Labels:     validSelector,
+		},
+		"invalid-labels": {
+			ObjectMeta: api.ObjectMeta{Name: "abc-123"},
+			Labels:     invalidSelector,
+		},
+	}
+	for k, v := range errorCases {
+		errs := ValidateMinion(&v)
+		if len(errs) == 0 {
+			t.Errorf("expected failure for %s", k)
+		}
+		for i := range errs {
+			field := errs[i].(errors.ValidationError).Field
+			if field != "name" &&
+				field != "label" {
+				t.Errorf("%s: missing prefix for: %v", k, errs[i])
+			}
+		}
+	}
+}

pkg/registry/minion/rest.go

@@ -23,6 +23,8 @@ import (
 	"strconv"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	kerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/master/ports"
@@ -50,8 +52,9 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE
 	if !ok {
 		return nil, fmt.Errorf("not a minion: %#v", obj)
 	}
-	if minion.Name == "" {
-		return nil, fmt.Errorf("ID should not be empty: %#v", minion)
+
+	if errs := validation.ValidateMinion(minion); len(errs) > 0 {
+		return nil, kerrors.NewInvalid("minion", minion.Name, errs)
 	}
 
 	minion.CreationTimestamp = util.Now()

pkg/registry/minion/rest_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest"
 )
@@ -114,3 +115,30 @@ func contains(nodes *api.MinionList, nodeID string) bool {
 	}
 	return false
 }
+
+func TestMinionStorageValidatesCreate(t *testing.T) {
+	storage := NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{}))
+	ctx := api.NewContext()
+	validSelector := map[string]string{"a": "b"}
+	invalidSelector := map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "b"}
+	failureCases := map[string]api.Minion{
+		"zero-length Name": {
+			ObjectMeta: api.ObjectMeta{Name: ""},
+			HostIP:     "something",
+			Labels:     validSelector,
+		},
+		"invalid-labels": {
+			ObjectMeta: api.ObjectMeta{Name: "abc-123"},
+			Labels:     invalidSelector,
+		},
+	}
+	for _, failureCase := range failureCases {
+		c, err := storage.Create(ctx, &failureCase)
+		if c != nil {
+			t.Errorf("Expected nil channel")
+		}
+		if !errors.IsInvalid(err) {
+			t.Errorf("Expected to get an invalid resource error, got %v", err)
+		}
+	}
+}