diff --git a/api/examples/controller-list.json b/api/examples/controller-list.json index 4cadf176865..e883abac57f 100644 --- a/api/examples/controller-list.json +++ b/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" } } ] diff --git a/api/examples/controller.json b/api/examples/controller.json index d385569898b..03910e88c5d 100644 --- a/api/examples/controller.json +++ b/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", diff --git a/api/examples/pod-list.json b/api/examples/pod-list.json index 01e4fd4de75..5d867de44e4 100644 --- a/api/examples/pod-list.json +++ b/api/examples/pod-list.json @@ -5,8 +5,8 @@ { "id": "my-pod-1", "labels": { - "name": "testRun", - "replicationcontroller": "testRun" + "name": "test-run", + "replicationcontroller": "test-run" }, "desiredState": { "manifest": { @@ -29,8 +29,8 @@ { "id": "my-pod-2", "labels": { - "name": "testRun", - "replicationcontroller": "testRun" + "name": "test-run", + "replicationcontroller": "test-run" }, "desiredState": { "manifest": { diff --git a/cluster/addons/cluster-monitoring/heapster-controller.yaml b/cluster/addons/cluster-monitoring/heapster-controller.yaml index d3d311edf97..7f8c9bb506f 100644 --- a/cluster/addons/cluster-monitoring/heapster-controller.yaml +++ b/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" diff --git a/cluster/addons/cluster-monitoring/influxdb-grafana-controller.yaml b/cluster/addons/cluster-monitoring/influxdb-grafana-controller.yaml index 1babc4239e2..b262da15dbb 100644 --- a/cluster/addons/cluster-monitoring/influxdb-grafana-controller.yaml +++ b/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" diff --git a/examples/examples_test.go b/examples/examples_test.go index 1068056fb2a..eafa0049438 100644 --- a/examples/examples_test.go +++ b/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])...) diff --git a/examples/guestbook/README.md b/examples/guestbook/README.md index 3e28f0f8707..61fb160a994 100644 --- a/examples/guestbook/README.md +++ b/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 +NAME IMAGE(S) SELECTOR REPLICAS +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 -frontendController kubernetes/example-guestbook-php-redis name=frontend 3 +NAME IMAGE(S) SELECTOR REPLICAS +redis-slave-controller brendanburns/redis-slave name=redisslave 2 +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: diff --git a/examples/guestbook/frontend-controller.json b/examples/guestbook/frontend-controller.json index 44dfd4d1c8f..06e3f487768 100644 --- a/examples/guestbook/frontend-controller.json +++ b/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", diff --git a/examples/guestbook/redis-slave-controller.json b/examples/guestbook/redis-slave-controller.json index 7830dce44a6..cda7bdcc193 100644 --- a/examples/guestbook/redis-slave-controller.json +++ b/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", diff --git a/examples/walkthrough/k8s201.md b/examples/walkthrough/k8s201.md index 5f123c26ce3..009dc1f53a5 100644 --- a/examples/walkthrough/k8s201.md +++ b/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: diff --git a/examples/walkthrough/replication-controller.yaml b/examples/walkthrough/replication-controller.yaml index bf5106afb79..7273b05e24a 100644 --- a/examples/walkthrough/replication-controller.yaml +++ b/examples/walkthrough/replication-controller.yaml @@ -1,4 +1,4 @@ -id: nginxController +id: nginx-controller apiVersion: v1beta1 kind: ReplicationController desiredState: diff --git a/hack/e2e-suite/guestbook.sh b/hack/e2e-suite/guestbook.sh index 436eb2634a8..bc2956bb763 100755 --- a/hack/e2e-suite/guestbook.sh +++ b/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 diff --git a/hack/e2e-suite/monitoring.sh b/hack/e2e-suite/monitoring.sh index 33d1de435bb..73206842ee7 100755 --- a/hack/e2e-suite/monitoring.sh +++ b/hack/e2e-suite/monitoring.sh @@ -49,8 +49,8 @@ function setup { } function cleanup { - "${KUBECFG}" resize monitoring-influxGrafanaController 0 &> /dev/null || true - "${KUBECFG}" resize monitoring-heapsterController 0 &> /dev/null || true + "${KUBECFG}" resize monitoring-influx-grafana-controller 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 diff --git a/hack/test-cmd.sh b/hack/test-cmd.sh index 49522982876..5d48dcaa10b 100755 --- a/hack/test-cmd.sh +++ b/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 delete rc frontendController "${kube_flags[@]}" + kubectl describe replicationcontroller frontend-controller "${kube_flags[@]}" | grep -q 'Replicas:.*3 desired' + kubectl delete rc frontend-controller "${kube_flags[@]}" kube::log::status "Testing kubectl(${version}:nodes)" kubectl get nodes "${kube_flags[@]}" diff --git a/pkg/api/errors/errors.go b/pkg/api/errors/errors.go index ea1e2fac2a1..930efe7dab2 100644 --- a/pkg/api/errors/errors.go +++ b/pkg/api/errors/errors.go @@ -154,14 +154,10 @@ func NewInvalid(kind, name string, errs ValidationErrorList) error { // NewBadRequest creates an error that indicates that the request is invalid and can not be processed. func NewBadRequest(reason string) error { return &StatusError{api.Status{ - Status: api.StatusFailure, - Code: http.StatusBadRequest, - Reason: api.StatusReasonBadRequest, - Details: &api.StatusDetails{ - Causes: []api.StatusCause{ - {Message: reason}, - }, - }, + Status: api.StatusFailure, + Code: http.StatusBadRequest, + Reason: api.StatusReasonBadRequest, + Message: reason, }} } diff --git a/pkg/api/errors/validation.go b/pkg/api/errors/validation.go index a204e348cec..a3b4eddf8d9 100644 --- a/pkg/api/errors/validation.go +++ b/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) - if v.Detail != "" { + var s string + 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, ""} } diff --git a/pkg/api/errors/validation_test.go b/pkg/api/errors/validation_test.go index cc643d0236a..23fa2c5021d 100644 --- a/pkg/api/errors/validation_test.go +++ b/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(), - "Baz", "Qux", "Inner", "KV", + "foo", ValidationErrorTypeInvalid.String(), + "Baz", "Qux", "Inner", "KV", "detail", "1", "aoeu", "asdf", "Billy", "2", } { if !strings.Contains(s, part) { diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 08c7b2eabe7..24819a87587 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -29,9 +29,90 @@ import ( "github.com/golang/glog" ) -// ServiceLister is an abstract interface for testing. -type ServiceLister interface { - ListServices(api.Context) (*api.ServiceList, error) +// ValidateNameFunc validates that the provided name is valid for a given resource type. +// Not all resources have the same validation rules for names. +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, 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, ValidateObjectMeta(&pod.ObjectMeta, true, nameIsDNSSubdomain).Prefix("metadata")...) 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, errs.NewFieldInvalid("name", newPod.Name, "field is immutable")) - } + allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldPod.ObjectMeta, &newPod.ObjectMeta).Prefix("metadata")...) 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, 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, "")) - } + allErrs = append(allErrs, ValidateObjectMeta(&service.ObjectMeta, true, nameIsDNS952Label).Prefix("metadata")...) + 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, errs.NewFieldRequired("name", controller.Name)) - } - if !util.IsDNSSubdomain(controller.Namespace) { - allErrs = append(allErrs, errs.NewFieldInvalid("namespace", controller.Namespace, "")) - } + allErrs = append(allErrs, ValidateObjectMeta(&controller.ObjectMeta, true, nameIsDNSSubdomain).Prefix("metadata")...) 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) { - allErrs = append(allErrs, errs.NewFieldInvalid("name", pod.Name, "")) + } else { + 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. -func ValidateMinion(minion *api.Node) errs.ValidationErrorList { +// ValidateMinion tests if required fields in the node are set. +func ValidateMinion(node *api.Node) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - if len(minion.Namespace) != 0 { - 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")...) + allErrs = append(allErrs, ValidateObjectMeta(&node.ObjectMeta, false, nameIsDNSSubdomain).Prefix("metadata")...) 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 - oldMinion.Labels = minion.Labels + // TODO: move reset function to its own location + // 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 } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 08c270d28f3..6943cd129a3 100644 --- a/pkg/api/validation/validation_test.go +++ b/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 != "namespace" && + field != "metadata.name" && + field != "metadata.namespace" && field != "spec.selector" && field != "spec.template" && field != "GCEPersistentDisk.ReadOnly" && field != "spec.replicas" && field != "spec.template.labels" && - field != "labels" && - field != "annotations" { + field != "metadata.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" && - field != "labels" && - field != "annotations" { + if field != "metadata.name" && + field != "metadata.labels" && + 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) } } } diff --git a/pkg/config/config_test.json b/pkg/config/config_test.json index df694a0bdad..3696e1dfc3e 100644 --- a/pkg/config/config_test.json +++ b/pkg/config/config_test.json @@ -60,8 +60,8 @@ } }, { - "id": "frontendController", - "name": "frontendController", + "id": "frontend-controller", + "name": "frontend-controller", "kind": "ReplicationController", "apiVersion": "v1beta1", "desiredState": { @@ -97,8 +97,8 @@ "labels": {"name": "frontend"} }, { - "id": "redisSlaveController", - "name": "redisSlaveController", + "id": "redis-slave-controller", + "name": "redis-slave-controller", "kind": "ReplicationController", "apiVersion": "v1beta1", "desiredState": { diff --git a/pkg/kubectl/cmd/create_test.go b/pkg/kubectl/cmd/create_test.go index 2253b9f74b0..c7a79c5c2b4 100644 --- a/pkg/kubectl/cmd/create_test.go +++ b/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()) } } diff --git a/pkg/kubectl/cmd/delete_test.go b/pkg/kubectl/cmd/delete_test.go index 3885d2f9fdd..7fad141e7bf 100644 --- a/pkg/kubectl/cmd/delete_test.go +++ b/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()) } } diff --git a/pkg/registry/controller/rest_test.go b/pkg/registry/controller/rest_test.go index 585a956c0ce..7f58b998f7d 100644 --- a/pkg/registry/controller/rest_test.go +++ b/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", }, diff --git a/pkg/registry/minion/rest_test.go b/pkg/registry/minion/rest_test.go index 8793ff595bd..195fa632598 100644 --- a/pkg/registry/minion/rest_test.go +++ b/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" { diff --git a/pkg/registry/service/rest.go b/pkg/registry/service/rest.go index b62e8c03edc..718b34b798e 100644 --- a/pkg/registry/service/rest.go +++ b/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 { + + oldService, err := rs.registry.GetService(ctx, service.Name) + if err != nil { + return nil, err + } + + // Copy over non-user fields + // 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) } 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")} - return nil, errors.NewInvalid("service", service.Name, el) - } - // Copy over non-user fields. - 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 { diff --git a/pkg/registry/service/rest_test.go b/pkg/registry/service/rest_test.go index ab288b0ae91..eec35f117be 100644 --- a/pkg/registry/service/rest_test.go +++ b/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) - result := <-c - st := result.Object.(*api.Status) - if st.Reason != api.StatusReasonInvalid { - t.Errorf("Expected to get an invalid error, got %v", st) + _, err := rest.Update(ctx, update) + if err == nil || !errors.IsInvalid(err) { + t.Error("Unexpected error type: %v", err) } } diff --git a/test/integration/auth_test.go b/test/integration/auth_test.go index d57dcee7209..6cf9969f82c 100644 --- a/test/integration/auth_test.go +++ b/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" } }