Merge pull request #3856 from smarterclayton/validation_logic_needs_cleanup

Validation of ObjectMeta is inconsistently applied

api/examples/controller-list.json

@@ -3,11 +3,11 @@
     "apiVersion": "v1beta1",
     "items": [
         {
-            "id": "testRun",
+            "id": "test-run",
             "desiredState": {
                 "replicas": 2,
                 "replicaSelector": {
-                    "name": "testRun"
+                    "name": "test-run"
                 },
                 "podTemplate": {
                     "desiredState": {
@@ -23,12 +23,12 @@
                         }
                     },
                     "labels": {
-                        "name": "testRun"
+                        "name": "test-run"
                     }
                 }
             },
             "labels": {
-                "name": "testRun"
+                "name": "test-run"
             }
         }
     ]

api/examples/controller.json

@@ -1,5 +1,5 @@
   {
-    "id": "nginxController",
+    "id": "nginx-controller",
     "apiVersion": "v1beta1",
     "kind": "ReplicationController",
     "desiredState": {
@@ -9,7 +9,7 @@
         "desiredState": {
            "manifest": {
              "version": "v1beta1",
-             "id": "nginxController",
+             "id": "nginx-controller",
              "containers": [{
                "name": "nginx",
                "image": "dockerfile/nginx",

api/examples/pod-list.json

@@ -5,8 +5,8 @@
         {
             "id": "my-pod-1",
             "labels": {
-                "name": "testRun",
+                "name": "test-run",
-                "replicationcontroller": "testRun"
+                "replicationcontroller": "test-run"
             },
             "desiredState": {
               "manifest": {
@@ -29,8 +29,8 @@
         {
             "id": "my-pod-2",
             "labels": {
-                "name": "testRun",
+                "name": "test-run",
-                "replicationcontroller": "testRun"
+                "replicationcontroller": "test-run"
             },
             "desiredState": {
               "manifest": {

cluster/addons/cluster-monitoring/heapster-controller.yaml

@@ -1,5 +1,5 @@
 apiVersion: "v1beta1"
-id: "monitoring-heapsterController"
+id: "monitoring-heapster-controller"
 kind: "ReplicationController"
 desiredState: 
   replicas: 1
@@ -9,7 +9,7 @@ desiredState:
     desiredState:
       manifest:
         version: "v1beta1"
-        id: "monitoring-heapsterController"
+        id: "monitoring-heapster-controller"
         containers:
           - name: "heapster"
             image: "kubernetes/heapster:v0.6"

cluster/addons/cluster-monitoring/influxdb-grafana-controller.yaml

@@ -1,6 +1,6 @@
 apiVersion: "v1beta1"
 kind: "ReplicationController"
-id: "monitoring-influxGrafanaController"
+id: "monitoring-influx-grafana-controller"
 desiredState: 
   replicas: 1
   replicaSelector:
@@ -11,7 +11,7 @@ desiredState:
     desiredState:
       manifest:
         version: "v1beta1"
-        id: "monitoring-influxGrafanaController"
+        id: "monitoring-influx-grafana-controller"
         containers: 
           - name: "influxdb"
             image: "kubernetes/heapster_influxdb:v0.3"

examples/examples_test.go

@@ -27,7 +27,6 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
 	"github.com/golang/glog"
 )
@@ -49,7 +48,7 @@ func validateObject(obj runtime.Object) (errors []error) {
 			t.Namespace = api.NamespaceDefault
 		}
 		api.ValidNamespace(ctx, &t.ObjectMeta)
-		errors = validation.ValidateService(t, registrytest.NewServiceRegistry(), api.NewDefaultContext())
+		errors = validation.ValidateService(t)
 	case *api.ServiceList:
 		for i := range t.Items {
 			errors = append(errors, validateObject(&t.Items[i])...)

examples/guestbook/README.md

@@ -121,7 +121,7 @@ Use the file `examples/guestbook/redis-slave-controller.json`:
 
 ```js
 {
-  "id": "redisSlaveController",
+  "id": "redis-slave-controller",
   "kind": "ReplicationController",
   "apiVersion": "v1beta1",
   "desiredState": {
@@ -131,7 +131,7 @@ Use the file `examples/guestbook/redis-slave-controller.json`:
       "desiredState": {
          "manifest": {
            "version": "v1beta1",
-           "id": "redisSlaveController",
+           "id": "redis-slave-controller",
            "containers": [{
              "name": "slave",
              "image": "brendanburns/redis-slave",
@@ -153,11 +153,11 @@ to create the replication controller by running:
 
 ```shell
 $ cluster/kubectl.sh create -f examples/guestbook/redis-slave-controller.json
-redisSlaveController
+redis-slave-controller
 
 # cluster/kubectl.sh get replicationcontrollers
 NAME                    IMAGE(S)                   SELECTOR            REPLICAS
-redisSlaveController   brendanburns/redis-slave   name=redisslave     2
+redis-slave-controller  brendanburns/redis-slave   name=redisslave     2
 ```
 
 The redis slave configures itself by looking for the Kubernetes service environment variables in the container environment. In particular, the redis slave is started with the following command:
@@ -225,7 +225,7 @@ The pod is described in the file `examples/guestbook/frontend-controller.json`:
 
 ```js
 {
-  "id": "frontendController",
+  "id": "frontend-controller",
   "kind": "ReplicationController",
   "apiVersion": "v1beta1",
   "desiredState": {
@@ -235,7 +235,7 @@ The pod is described in the file `examples/guestbook/frontend-controller.json`:
       "desiredState": {
          "manifest": {
            "version": "v1beta1",
-           "id": "frontendController",
+           "id": "frontend-controller",
            "containers": [{
              "name": "php-redis",
              "image": "kubernetes/example-guestbook-php-redis",
@@ -258,12 +258,12 @@ Using this file, you can turn up your frontend with:
 
 ```shell
 $ cluster/kubectl.sh create -f examples/guestbook/frontend-controller.json
-frontendController
+frontend-controller
 
 $ cluster/kubectl.sh get replicationcontrollers
 NAME                    IMAGE(S)                                   SELECTOR            REPLICAS
-redisSlaveController   brendanburns/redis-slave                   name=redisslave     2
+redis-slave-controller  brendanburns/redis-slave                   name=redisslave     2
-frontendController     kubernetes/example-guestbook-php-redis     name=frontend       3
+frontend-controller     kubernetes/example-guestbook-php-redis     name=frontend       3
 ```
 
 Once that's up (it may take ten to thirty seconds to create the pods) you can list the pods in the cluster, to verify that the master, slaves and frontends are running:

examples/guestbook/frontend-controller.json

@@ -1,5 +1,5 @@
 {
-  "id": "frontendController",
+  "id": "frontend-controller",
   "kind": "ReplicationController",
   "apiVersion": "v1beta1",
   "desiredState": {
@@ -9,7 +9,7 @@
       "desiredState": {
          "manifest": {
            "version": "v1beta1",
-           "id": "frontendController",
+           "id": "frontend-controller",
            "containers": [{
              "name": "php-redis",
              "image": "kubernetes/example-guestbook-php-redis",

examples/guestbook/redis-slave-controller.json

@@ -1,5 +1,5 @@
 {
-  "id": "redisSlaveController",
+  "id": "redis-slave-controller",
   "kind": "ReplicationController",
   "apiVersion": "v1beta1",
   "desiredState": {
@@ -9,7 +9,7 @@
       "desiredState": {
          "manifest": {
            "version": "v1beta1",
-           "id": "redisSlaveController",
+           "id": "redis-slave-controller",
            "containers": [{
              "name": "slave",
              "image": "brendanburns/redis-slave",

examples/walkthrough/k8s201.md

@@ -23,7 +23,7 @@ Replication controllers are the objects to answer these questions.  A replicatio
 An example replica controller that instantiates two pods running nginx looks like:
 
 ```yaml
-id: nginxController
+id: nginx-controller
 apiVersion: v1beta1
 kind: ReplicationController
 desiredState:

examples/walkthrough/replication-controller.yaml

@@ -1,4 +1,4 @@
-id: nginxController
+id: nginx-controller
 apiVersion: v1beta1
 kind: ReplicationController
 desiredState:

hack/e2e-suite/guestbook.sh

@@ -38,10 +38,10 @@ sleep 5
 POD_LIST_1=$($KUBECFG '-template={{range.items}}{{.id}} {{end}}' list pods)
 echo "Pods running: ${POD_LIST_1}"
 
-$KUBECFG stop redisSlaveController
+$KUBECFG stop redis-slave-controller
 # Needed until issue #103 gets fixed
 sleep 25
-$KUBECFG rm redisSlaveController
+$KUBECFG rm redis-slave-controller
 $KUBECFG delete services/redis-master
 $KUBECFG delete pods/redis-master
 

hack/e2e-suite/monitoring.sh

@@ -49,8 +49,8 @@ function setup {
 }
 
 function cleanup {
-  "${KUBECFG}" resize monitoring-influxGrafanaController 0 &> /dev/null || true
+  "${KUBECFG}" resize monitoring-influx-grafana-controller 0 &> /dev/null || true
-  "${KUBECFG}" resize monitoring-heapsterController 0 &> /dev/null || true
+  "${KUBECFG}" resize monitoring-heapster-controller 0 &> /dev/null || true
   while "${KUBECTL}" get pods -l "name=influxGrafana" -o template -t {{range.items}}{{.id}}:{{end}} | grep -c . &> /dev/null \
     || "${KUBECTL}" get pods -l "name=heapster" -o template -t {{range.items}}{{.id}}:{{end}} | grep -c . &> /dev/null; do
     sleep 2

hack/test-cmd.sh

@@ -168,8 +168,8 @@ __EOF__
   kubectl get replicationcontrollers "${kube_flags[@]}"
   kubectl create -f examples/guestbook/frontend-controller.json "${kube_flags[@]}"
   kubectl get replicationcontrollers "${kube_flags[@]}"
-  kubectl describe replicationcontroller frontendController "${kube_flags[@]}" | grep -q 'Replicas:.*3 desired'
+  kubectl describe replicationcontroller frontend-controller "${kube_flags[@]}" | grep -q 'Replicas:.*3 desired'
-  kubectl delete rc frontendController "${kube_flags[@]}"
+  kubectl delete rc frontend-controller "${kube_flags[@]}"
 
   kube::log::status "Testing kubectl(${version}:nodes)"
   kubectl get nodes "${kube_flags[@]}"

pkg/api/errors/errors.go

@@ -157,11 +157,7 @@ func NewBadRequest(reason string) error {
 		Status:  api.StatusFailure,
 		Code:    http.StatusBadRequest,
 		Reason:  api.StatusReasonBadRequest,
-		Details: &api.StatusDetails{
+		Message: reason,
-			Causes: []api.StatusCause{
-				{Message: reason},
-			},
-		},
 	}}
 }
 

pkg/api/errors/validation.go

@@ -84,14 +84,21 @@ type ValidationError struct {
 var _ error = &ValidationError{}
 
 func (v *ValidationError) Error() string {
-	s := spew.Sprintf("%s: %s '%+v'", v.Field, v.Type, v.BadValue)
+	var s string
-	if v.Detail != "" {
+	switch v.Type {
+	case ValidationErrorTypeRequired:
+		s = spew.Sprintf("%s: %s", v.Field, v.Type)
+	default:
+		s = spew.Sprintf("%s: %s '%+v'", v.Field, v.Type, v.BadValue)
+	}
+	if len(v.Detail) != 0 {
 		s += fmt.Sprintf(": %s", v.Detail)
 	}
 	return s
 }
 
 // NewFieldRequired returns a *ValidationError indicating "value required"
+// TODO: remove "value"
 func NewFieldRequired(field string, value interface{}) *ValidationError {
 	return &ValidationError{ValidationErrorTypeRequired, field, value, ""}
 }

pkg/api/errors/validation_test.go

@@ -71,7 +71,7 @@ func TestValidationErrorUsefulMessage(t *testing.T) {
 		Inner interface{}
 		KV    map[string]int
 	}
-	s = NewFieldRequired(
+	s = NewFieldInvalid(
 		"foo",
 		&complicated{
 			Baz:   1,
@@ -79,11 +79,12 @@ func TestValidationErrorUsefulMessage(t *testing.T) {
 			Inner: &complicated{Qux: "asdf"},
 			KV:    map[string]int{"Billy": 2},
 		},
+		"detail",
 	).Error()
 	t.Logf("message: %v", s)
 	for _, part := range []string{
-		"foo", ValidationErrorTypeRequired.String(),
+		"foo", ValidationErrorTypeInvalid.String(),
-		"Baz", "Qux", "Inner", "KV",
+		"Baz", "Qux", "Inner", "KV", "detail",
 		"1", "aoeu", "asdf", "Billy", "2",
 	} {
 		if !strings.Contains(s, part) {

pkg/api/validation/validation.go

@@ -29,9 +29,90 @@ import (
 	"github.com/golang/glog"
 )
 
-// ServiceLister is an abstract interface for testing.
+// ValidateNameFunc validates that the provided name is valid for a given resource type.
-type ServiceLister interface {
+// Not all resources have the same validation rules for names.
-	ListServices(api.Context) (*api.ServiceList, error)
+type ValidateNameFunc func(name string) (bool, string)
+
+// nameIsDNSSubdomain is a ValidateNameFunc for names that must be a DNS subdomain.
+func nameIsDNSSubdomain(name string) (bool, string) {
+	if util.IsDNSSubdomain(name) {
+		return true, ""
+	}
+	return false, "name must be lowercase letters and numbers, with inline dashes or periods"
+}
+
+// nameIsDNS952Label is a ValidateNameFunc for names that must be a DNS 952 label.
+func nameIsDNS952Label(name string) (bool, string) {
+	if util.IsDNS952Label(name) {
+		return true, ""
+	}
+	return false, "name must be lowercase letters, numbers, and dashes"
+}
+
+// ValidateObjectMeta validates an object's metadata.
+func ValidateObjectMeta(meta *api.ObjectMeta, requiresNamespace bool, nameFn ValidateNameFunc) errs.ValidationErrorList {
+	allErrs := errs.ValidationErrorList{}
+	if len(meta.Name) == 0 {
+		allErrs = append(allErrs, errs.NewFieldRequired("name", meta.Name))
+	} else {
+		if ok, qualifier := nameFn(meta.Name); !ok {
+			allErrs = append(allErrs, errs.NewFieldInvalid("name", meta.Name, qualifier))
+		}
+	}
+	if requiresNamespace {
+		if len(meta.Namespace) == 0 {
+			allErrs = append(allErrs, errs.NewFieldRequired("namespace", meta.Namespace))
+		} else if !util.IsDNSSubdomain(meta.Namespace) {
+			allErrs = append(allErrs, errs.NewFieldInvalid("namespace", meta.Namespace, ""))
+		}
+	} else {
+		if len(meta.Namespace) != 0 {
+			allErrs = append(allErrs, errs.NewFieldInvalid("namespace", meta.Namespace, "namespace is not allowed on this type"))
+		}
+	}
+	allErrs = append(allErrs, ValidateLabels(meta.Labels, "labels")...)
+	allErrs = append(allErrs, ValidateLabels(meta.Annotations, "annotations")...)
+
+	// Clear self link internally
+	// TODO: move to its own area
+	meta.SelfLink = ""
+
+	return allErrs
+}
+
+// ValidateObjectMetaUpdate validates an object's metadata when updated
+func ValidateObjectMetaUpdate(old, meta *api.ObjectMeta) errs.ValidationErrorList {
+	allErrs := errs.ValidationErrorList{}
+
+	// in the event it is left empty, set it, to allow clients more flexibility
+	if len(meta.UID) == 0 {
+		meta.UID = old.UID
+	}
+	if meta.CreationTimestamp.IsZero() {
+		meta.CreationTimestamp = old.CreationTimestamp
+	}
+
+	if old.Name != meta.Name {
+		allErrs = append(allErrs, errs.NewFieldInvalid("name", meta.Name, "field is immutable"))
+	}
+	if old.Namespace != meta.Namespace {
+		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", meta.Namespace, "field is immutable"))
+	}
+	if old.UID != meta.UID {
+		allErrs = append(allErrs, errs.NewFieldInvalid("uid", meta.UID, "field is immutable"))
+	}
+	if old.CreationTimestamp != meta.CreationTimestamp {
+		allErrs = append(allErrs, errs.NewFieldInvalid("creationTimestamp", meta.CreationTimestamp, "field is immutable"))
+	}
+
+	allErrs = append(allErrs, ValidateLabels(meta.Labels, "labels")...)
+	allErrs = append(allErrs, ValidateLabels(meta.Annotations, "annotations")...)
+
+	// Clear self link internally
+	// TODO: move to its own area
+	meta.SelfLink = ""
+
+	return allErrs
 }
 
 func validateVolumes(volumes []api.Volume) (util.StringSet, errs.ValidationErrorList) {
@@ -387,19 +468,9 @@ func validateDNSPolicy(dnsPolicy *api.DNSPolicy) errs.ValidationErrorList {
 // ValidatePod tests if required fields in the pod are set.
 func ValidatePod(pod *api.Pod) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
-	if len(pod.Name) == 0 {
+	allErrs = append(allErrs, ValidateObjectMeta(&pod.ObjectMeta, true, nameIsDNSSubdomain).Prefix("metadata")...)
-		allErrs = append(allErrs, errs.NewFieldRequired("name", pod.Name))
-	} else if !util.IsDNSSubdomain(pod.Name) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("name", pod.Name, ""))
-	}
-	if len(pod.Namespace) == 0 {
-		allErrs = append(allErrs, errs.NewFieldRequired("namespace", pod.Namespace))
-	} else if !util.IsDNSSubdomain(pod.Namespace) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", pod.Namespace, ""))
-	}
 	allErrs = append(allErrs, ValidatePodSpec(&pod.Spec).Prefix("spec")...)
-	allErrs = append(allErrs, ValidateLabels(pod.Labels, "labels")...)
+
-	allErrs = append(allErrs, ValidateLabels(pod.Annotations, "annotations")...)
 	return allErrs
 }
 
@@ -434,9 +505,7 @@ func ValidateLabels(labels map[string]string, field string) errs.ValidationError
 func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 
-	if newPod.Name != oldPod.Name {
+	allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldPod.ObjectMeta, &newPod.ObjectMeta).Prefix("metadata")...)
-		allErrs = append(allErrs, errs.NewFieldInvalid("name", newPod.Name, "field is immutable"))
-	}
 
 	if len(newPod.Spec.Containers) != len(oldPod.Spec.Containers) {
 		allErrs = append(allErrs, errs.NewFieldInvalid("spec.containers", newPod.Spec.Containers, "may not add or remove containers"))
@@ -454,22 +523,17 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {
 		// TODO: a better error would include all immutable fields explicitly.
 		allErrs = append(allErrs, errs.NewFieldInvalid("spec.containers", newPod.Spec.Containers, "some fields are immutable"))
 	}
+
 	return allErrs
 }
 
 var supportedSessionAffinityType = util.NewStringSet(string(api.AffinityTypeClientIP), string(api.AffinityTypeNone))
 
 // ValidateService tests if required fields in the service are set.
-func ValidateService(service *api.Service, lister ServiceLister, ctx api.Context) errs.ValidationErrorList {
+func ValidateService(service *api.Service) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
-	if len(service.Name) == 0 {
+	allErrs = append(allErrs, ValidateObjectMeta(&service.ObjectMeta, true, nameIsDNS952Label).Prefix("metadata")...)
-		allErrs = append(allErrs, errs.NewFieldRequired("name", service.Name))
+
-	} else if !util.IsDNS952Label(service.Name) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("name", service.Name, ""))
-	}
-	if !util.IsDNSSubdomain(service.Namespace) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", service.Namespace, ""))
-	}
 	if !util.IsValidPortNum(service.Spec.Port) {
 		allErrs = append(allErrs, errs.NewFieldInvalid("spec.port", service.Spec.Port, ""))
 	}
@@ -482,8 +546,6 @@ func ValidateService(service *api.Service, lister ServiceLister, ctx api.Context
 	if service.Spec.Selector != nil {
 		allErrs = append(allErrs, ValidateLabels(service.Spec.Selector, "spec.selector")...)
 	}
-	allErrs = append(allErrs, ValidateLabels(service.Labels, "labels")...)
-	allErrs = append(allErrs, ValidateLabels(service.Annotations, "annotations")...)
 
 	if service.Spec.SessionAffinity == "" {
 		service.Spec.SessionAffinity = api.AffinityTypeNone
@@ -494,18 +556,35 @@ func ValidateService(service *api.Service, lister ServiceLister, ctx api.Context
 	return allErrs
 }
 
+// ValidateServiceUpdate tests if required fields in the service are set during an update
+func ValidateServiceUpdate(oldService, service *api.Service) errs.ValidationErrorList {
+	allErrs := errs.ValidationErrorList{}
+	allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldService.ObjectMeta, &service.ObjectMeta).Prefix("metadata")...)
+
+	// TODO: PortalIP should be a Status field, since the system can set a value != to the user's value
+	// PortalIP can only be set, not unset.
+	if oldService.Spec.PortalIP != "" && service.Spec.PortalIP != oldService.Spec.PortalIP {
+		allErrs = append(allErrs, errs.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP, "field is immutable"))
+	}
+
+	return allErrs
+}
+
 // ValidateReplicationController tests if required fields in the replication controller are set.
 func ValidateReplicationController(controller *api.ReplicationController) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
-	if len(controller.Name) == 0 {
+	allErrs = append(allErrs, ValidateObjectMeta(&controller.ObjectMeta, true, nameIsDNSSubdomain).Prefix("metadata")...)
-		allErrs = append(allErrs, errs.NewFieldRequired("name", controller.Name))
-	}
-	if !util.IsDNSSubdomain(controller.Namespace) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", controller.Namespace, ""))
-	}
 	allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec).Prefix("spec")...)
-	allErrs = append(allErrs, ValidateLabels(controller.Labels, "labels")...)
+
-	allErrs = append(allErrs, ValidateLabels(controller.Annotations, "annotations")...)
+	return allErrs
+}
+
+// ValidateReplicationControllerUpdate tests if required fields in the replication controller are set.
+func ValidateReplicationControllerUpdate(oldController, controller *api.ReplicationController) errs.ValidationErrorList {
+	allErrs := errs.ValidationErrorList{}
+	allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldController.ObjectMeta, &controller.ObjectMeta).Prefix("metadata")...)
+	allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec).Prefix("spec")...)
+
 	return allErrs
 }
 
@@ -563,12 +642,15 @@ func ValidateReadOnlyPersistentDisks(volumes []api.Volume) errs.ValidationErrorL
 }
 
 // ValidateBoundPod tests if required fields on a bound pod are set.
+// TODO: to be removed.
 func ValidateBoundPod(pod *api.BoundPod) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 	if len(pod.Name) == 0 {
 		allErrs = append(allErrs, errs.NewFieldRequired("name", pod.Name))
-	} else if !util.IsDNSSubdomain(pod.Name) {
+	} else {
-		allErrs = append(allErrs, errs.NewFieldInvalid("name", pod.Name, ""))
+		if ok, qualifier := nameIsDNSSubdomain(pod.Name); !ok {
+			allErrs = append(allErrs, errs.NewFieldInvalid("name", pod.Name, qualifier))
+		}
 	}
 	if len(pod.Namespace) == 0 {
 		allErrs = append(allErrs, errs.NewFieldRequired("namespace", pod.Namespace))
@@ -579,30 +661,26 @@ func ValidateBoundPod(pod *api.BoundPod) errs.ValidationErrorList {
 	return allErrs
 }
 
-// ValidateMinion tests if required fields in the minion are set.
+// ValidateMinion tests if required fields in the node are set.
-func ValidateMinion(minion *api.Node) errs.ValidationErrorList {
+func ValidateMinion(node *api.Node) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
-	if len(minion.Namespace) != 0 {
+	allErrs = append(allErrs, ValidateObjectMeta(&node.ObjectMeta, false, nameIsDNSSubdomain).Prefix("metadata")...)
-		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", minion.Namespace, ""))
-	}
-	if len(minion.Name) == 0 {
-		allErrs = append(allErrs, errs.NewFieldRequired("name", minion.Name))
-	}
-	allErrs = append(allErrs, ValidateLabels(minion.Labels, "labels")...)
-	allErrs = append(allErrs, ValidateLabels(minion.Annotations, "annotations")...)
 	return allErrs
 }
 
 // ValidateMinionUpdate tests to make sure a minion update can be applied.  Modifies oldMinion.
 func ValidateMinionUpdate(oldMinion *api.Node, minion *api.Node) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
+	allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldMinion.ObjectMeta, &minion.ObjectMeta).Prefix("metadata")...)
 
 	if !api.Semantic.DeepEqual(minion.Status, api.NodeStatus{}) {
 		allErrs = append(allErrs, errs.NewFieldInvalid("status", minion.Status, "status must be empty"))
 	}
 
-	// Allow users to update labels and capacity
+	// TODO: move reset function to its own location
-	oldMinion.Labels = minion.Labels
+	// Ignore metadata changes now that they have been tested
+	oldMinion.ObjectMeta = minion.ObjectMeta
+	// Allow users to update capacity
 	oldMinion.Spec.Capacity = minion.Spec.Capacity
 	// Clear status
 	oldMinion.Status = minion.Status
@@ -611,6 +689,8 @@ func ValidateMinionUpdate(oldMinion *api.Node, minion *api.Node) errs.Validation
 		glog.V(4).Infof("Update failed validation %#v vs %#v", oldMinion, minion)
 		allErrs = append(allErrs, fmt.Errorf("update contains more than labels or capacity changes"))
 	}
+
+	// TODO: validate Spec.Capacity
 	return allErrs
 }
 

pkg/api/validation/validation_test.go

@@ -1081,7 +1081,7 @@ func TestValidateService(t *testing.T) {
 	for _, tc := range testCases {
 		registry := registrytest.NewServiceRegistry()
 		registry.List = tc.existing
-		errs := ValidateService(&tc.svc, registry, api.NewDefaultContext())
+		errs := ValidateService(&tc.svc)
 		if len(errs) != tc.numErrs {
 			t.Errorf("Unexpected error list for case %q: %v", tc.name, utilerrors.NewAggregate(errs))
 		}
@@ -1094,7 +1094,7 @@ func TestValidateService(t *testing.T) {
 			Selector: map[string]string{"foo": "bar"},
 		},
 	}
-	errs := ValidateService(&svc, registrytest.NewServiceRegistry(), api.NewDefaultContext())
+	errs := ValidateService(&svc)
 	if len(errs) != 0 {
 		t.Errorf("Unexpected non-zero error list: %#v", errs)
 	}
@@ -1287,15 +1287,15 @@ func TestValidateReplicationController(t *testing.T) {
 		for i := range errs {
 			field := errs[i].(*errors.ValidationError).Field
 			if !strings.HasPrefix(field, "spec.template.") &&
-				field != "name" &&
+				field != "metadata.name" &&
-				field != "namespace" &&
+				field != "metadata.namespace" &&
 				field != "spec.selector" &&
 				field != "spec.template" &&
 				field != "GCEPersistentDisk.ReadOnly" &&
 				field != "spec.replicas" &&
 				field != "spec.template.labels" &&
-				field != "labels" &&
+				field != "metadata.annotations" &&
-				field != "annotations" {
+				field != "metadata.labels" {
 				t.Errorf("%s: missing prefix for: %v", k, errs[i])
 			}
 		}
@@ -1376,9 +1376,10 @@ func TestValidateMinion(t *testing.T) {
 		}
 		for i := range errs {
 			field := errs[i].(*errors.ValidationError).Field
-			if field != "name" &&
+			if field != "metadata.name" &&
-				field != "labels" &&
+				field != "metadata.labels" &&
-				field != "annotations" {
+				field != "metadata.annotations" &&
+				field != "metadata.namespace" {
 				t.Errorf("%s: missing prefix for: %v", k, errs[i])
 			}
 		}
@@ -1504,14 +1505,170 @@ func TestValidateMinionUpdate(t *testing.T) {
 			},
 		}, true},
 	}
-	for _, test := range tests {
+	for i, test := range tests {
 		errs := ValidateMinionUpdate(&test.oldMinion, &test.minion)
 		if test.valid && len(errs) > 0 {
-			t.Errorf("Unexpected error: %v", errs)
+			t.Errorf("%d: Unexpected error: %v", i, errs)
 			t.Logf("%#v vs %#v", test.oldMinion.ObjectMeta, test.minion.ObjectMeta)
 		}
 		if !test.valid && len(errs) == 0 {
-			t.Errorf("Unexpected non-error")
+			t.Errorf("%d: Unexpected non-error", i)
+		}
+	}
+}
+
+func TestValidateServiceUpdate(t *testing.T) {
+	tests := []struct {
+		oldService api.Service
+		service    api.Service
+		valid      bool
+	}{
+		{ // 0
+			api.Service{},
+			api.Service{},
+			true},
+		{ // 1
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name: "foo"}},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name: "bar"},
+			}, false},
+		{ // 2
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"foo": "bar"},
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"foo": "baz"},
+				},
+			}, true},
+		{ // 3
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name: "foo",
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"foo": "baz"},
+				},
+			}, true},
+		{ // 4
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "foo"},
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"foo": "baz"},
+				},
+			}, true},
+		{ // 5
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:        "foo",
+					Annotations: map[string]string{"bar": "foo"},
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:        "foo",
+					Annotations: map[string]string{"foo": "baz"},
+				},
+			}, true},
+		{ // 6
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name: "foo",
+				},
+				Spec: api.ServiceSpec{
+					Selector: map[string]string{"foo": "baz"},
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name: "foo",
+				},
+				Spec: api.ServiceSpec{
+					Selector: map[string]string{"foo": "baz"},
+				},
+			}, true},
+		{ // 7
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "foo"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "127.0.0.1",
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "fooobaz"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "new",
+				},
+			}, false},
+		{ // 8
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "foo"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "127.0.0.1",
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "fooobaz"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "",
+				},
+			}, false},
+		{ // 9
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "foo"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "127.0.0.1",
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "fooobaz"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "127.0.0.2",
+				},
+			}, false},
+	}
+	for i, test := range tests {
+		errs := ValidateServiceUpdate(&test.oldService, &test.service)
+		if test.valid && len(errs) > 0 {
+			t.Errorf("%d: Unexpected error: %v", i, errs)
+			t.Logf("%#v vs %#v", test.oldService.ObjectMeta, test.service.ObjectMeta)
+		}
+		if !test.valid && len(errs) == 0 {
+			t.Errorf("%d: Unexpected non-error", i)
 		}
 	}
 }

pkg/config/config_test.json

@@ -60,8 +60,8 @@
     }
   },
   {
-    "id": "frontendController",
+    "id": "frontend-controller",
-    "name": "frontendController",
+    "name": "frontend-controller",
     "kind": "ReplicationController",
     "apiVersion": "v1beta1",
     "desiredState": {
@@ -97,8 +97,8 @@
       "labels": {"name": "frontend"}
   },
   {
-    "id": "redisSlaveController",
+    "id": "redis-slave-controller",
-    "name": "redisSlaveController",
+    "name": "redis-slave-controller",
     "kind": "ReplicationController",
     "apiVersion": "v1beta1",
     "desiredState": {

pkg/kubectl/cmd/create_test.go

@@ -114,7 +114,7 @@ func TestCreateDirectory(t *testing.T) {
 	cmd.Flags().Set("filename", "../../../examples/guestbook")
 	cmd.Run(cmd, []string{})
 
-	if buf.String() != "frontendController\nfrontend\nredis-master\nredis-master\nredisSlaveController\nredisslave\n" {
+	if buf.String() != "frontend-controller\nfrontend\nredis-master\nredis-master\nredis-slave-controller\nredisslave\n" {
 		t.Errorf("unexpected output: %s", buf.String())
 	}
 }

pkg/kubectl/cmd/delete_test.go

@@ -206,7 +206,7 @@ func TestDeleteDirectory(t *testing.T) {
 	cmd.Flags().Set("filename", "../../../examples/guestbook")
 	cmd.Run(cmd, []string{})
 
-	if buf.String() != "frontendController\nfrontend\nredis-master\nredis-master\nredisSlaveController\nredisslave\n" {
+	if buf.String() != "frontend-controller\nfrontend\nredis-master\nredis-master\nredis-slave-controller\nredisslave\n" {
 		t.Errorf("unexpected output: %s", buf.String())
 	}
 }

pkg/registry/controller/rest_test.go

@@ -143,7 +143,7 @@ func TestControllerDecode(t *testing.T) {
 func TestControllerParsing(t *testing.T) {
 	expectedController := api.ReplicationController{
 		ObjectMeta: api.ObjectMeta{
-			Name: "nginxController",
+			Name: "nginx-controller",
 			Labels: map[string]string{
 				"name": "nginx",
 			},

pkg/registry/minion/rest_test.go

@@ -42,7 +42,7 @@ func TestMinionRegistryREST(t *testing.T) {
 
 	c, err := ms.Create(ctx, &api.Node{ObjectMeta: api.ObjectMeta{Name: "baz"}})
 	if err != nil {
-		t.Errorf("insert failed")
+		t.Fatalf("insert failed: %v", err)
 	}
 	obj := <-c
 	if !api.HasObjectMetaSystemFieldValues(&obj.Object.(*api.Node).ObjectMeta) {
@@ -57,7 +57,7 @@ func TestMinionRegistryREST(t *testing.T) {
 
 	c, err = ms.Delete(ctx, "bar")
 	if err != nil {
-		t.Errorf("delete failed")
+		t.Fatalf("delete failed")
 	}
 	obj = <-c
 	if s, ok := obj.Object.(*api.Status); !ok || s.Status != api.StatusSuccess {
@@ -69,7 +69,7 @@ func TestMinionRegistryREST(t *testing.T) {
 
 	_, err = ms.Delete(ctx, "bar")
 	if err != ErrDoesNotExist {
-		t.Errorf("delete returned wrong error")
+		t.Fatalf("delete returned wrong error")
 	}
 
 	list, err := ms.List(ctx, labels.Everything(), labels.Everything())
@@ -103,7 +103,7 @@ func TestMinionRegistryHealthCheck(t *testing.T) {
 
 	c, err := ms.Create(ctx, &api.Node{ObjectMeta: api.ObjectMeta{Name: "m1"}})
 	if err != nil {
-		t.Errorf("insert failed")
+		t.Fatalf("insert failed: %v", err)
 	}
 	result := <-c
 	if m, ok := result.Object.(*api.Node); !ok || m.Name != "m1" {

pkg/registry/service/rest.go

@@ -84,7 +84,7 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE
 	if !api.ValidNamespace(ctx, &service.ObjectMeta) {
 		return nil, errors.NewConflict("service", service.Namespace, fmt.Errorf("Service.Namespace does not match the provided context"))
 	}
-	if errs := validation.ValidateService(service, rs.registry, ctx); len(errs) > 0 {
+	if errs := validation.ValidateService(service); len(errs) > 0 {
 		return nil, errors.NewInvalid("service", service.Name, errs)
 	}
 
@@ -226,20 +226,21 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE
 	if !api.ValidNamespace(ctx, &service.ObjectMeta) {
 		return nil, errors.NewConflict("service", service.Namespace, fmt.Errorf("Service.Namespace does not match the provided context"))
 	}
-	if errs := validation.ValidateService(service, rs.registry, ctx); len(errs) > 0 {
+
-		return nil, errors.NewInvalid("service", service.Name, errs)
+	oldService, err := rs.registry.GetService(ctx, service.Name)
-	}
-	return apiserver.MakeAsync(func() (runtime.Object, error) {
-		cur, err := rs.registry.GetService(ctx, service.Name)
 	if err != nil {
 		return nil, err
 	}
-		if service.Spec.PortalIP != "" && service.Spec.PortalIP != cur.Spec.PortalIP {
+
-			el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP, "field is immutable")}
+	// Copy over non-user fields
-			return nil, errors.NewInvalid("service", service.Name, el)
+	// TODO: this should be a Status field, since the end user does not set it.
+	// TODO: make this a merge function
+	service.Spec.ProxyPort = oldService.Spec.ProxyPort
+
+	if errs := validation.ValidateServiceUpdate(oldService, service); len(errs) > 0 {
+		return nil, errors.NewInvalid("service", service.Name, errs)
 	}
-		// Copy over non-user fields.
+	return apiserver.MakeAsync(func() (runtime.Object, error) {
-		service.Spec.ProxyPort = cur.Spec.ProxyPort
 		// TODO: check to see if external load balancer status changed
 		err = rs.registry.UpdateService(ctx, service)
 		if err != nil {

pkg/registry/service/rest_test.go

@@ -118,7 +118,7 @@ func TestServiceRegistryUpdate(t *testing.T) {
 	ctx := api.NewDefaultContext()
 	registry := registrytest.NewServiceRegistry()
 	registry.CreateService(ctx, &api.Service{
-		ObjectMeta: api.ObjectMeta{Name: "foo"},
+		ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: api.NamespaceDefault},
 		Spec: api.ServiceSpec{
 			Port:     6502,
 			Selector: map[string]string{"bar": "baz1"},
@@ -132,12 +132,12 @@ func TestServiceRegistryUpdate(t *testing.T) {
 			Selector: map[string]string{"bar": "baz2"},
 		},
 	})
+	if err != nil {
+		t.Fatalf("Expected no error: %v", err)
+	}
 	if c == nil {
 		t.Errorf("Expected non-nil channel")
 	}
-	if err != nil {
-		t.Errorf("Expected no error")
-	}
 	updated_svc := <-c
 	updated_service := updated_svc.Object.(*api.Service)
 	if updated_service.Name != "foo" {
@@ -531,11 +531,9 @@ func TestServiceRegistryIPUpdate(t *testing.T) {
 	update.Spec.Port = 6503
 	update.Spec.PortalIP = "1.2.3.76" // error
 
-	c, _ = rest.Update(ctx, update)
+	_, err := rest.Update(ctx, update)
-	result := <-c
+	if err == nil || !errors.IsInvalid(err) {
-	st := result.Object.(*api.Status)
+		t.Error("Unexpected error type: %v", err)
-	if st.Reason != api.StatusReasonInvalid {
-		t.Errorf("Expected to get an invalid error, got %v", st)
 	}
 }
 

test/integration/auth_test.go

@@ -106,6 +106,7 @@ var aService string = `
   "apiVersion": "v1beta1",
   "id": "a",
   "port": 8000,
+  "portalIP": "10.0.0.1",
   "labels": { "name": "a" },
   "selector": { "name": "a" }
 }