Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validation of ObjectMeta is inconsistently applied #3856

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions api/examples/controller-list.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
"apiVersion": "v1beta1",
"items": [
{
"id": "testRun",
"id": "test-run",
"desiredState": {
"replicas": 2,
"replicaSelector": {
"name": "testRun"
"name": "test-run"
},
"podTemplate": {
"desiredState": {
Expand All @@ -23,12 +23,12 @@
}
},
"labels": {
"name": "testRun"
"name": "test-run"
}
}
},
"labels": {
"name": "testRun"
"name": "test-run"
}
}
]
Expand Down
4 changes: 2 additions & 2 deletions api/examples/controller.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"id": "nginxController",
"id": "nginx-controller",
"apiVersion": "v1beta1",
"kind": "ReplicationController",
"desiredState": {
Expand All @@ -9,7 +9,7 @@
"desiredState": {
"manifest": {
"version": "v1beta1",
"id": "nginxController",
"id": "nginx-controller",
"containers": [{
"name": "nginx",
"image": "dockerfile/nginx",
Expand Down
8 changes: 4 additions & 4 deletions api/examples/pod-list.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
{
"id": "my-pod-1",
"labels": {
"name": "testRun",
"replicationcontroller": "testRun"
"name": "test-run",
"replicationcontroller": "test-run"
},
"desiredState": {
"manifest": {
Expand All @@ -29,8 +29,8 @@
{
"id": "my-pod-2",
"labels": {
"name": "testRun",
"replicationcontroller": "testRun"
"name": "test-run",
"replicationcontroller": "test-run"
},
"desiredState": {
"manifest": {
Expand Down
4 changes: 2 additions & 2 deletions cluster/addons/cluster-monitoring/heapster-controller.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
apiVersion: "v1beta1"
id: "monitoring-heapsterController"
id: "monitoring-heapster-controller"
kind: "ReplicationController"
desiredState:
replicas: 1
Expand All @@ -9,7 +9,7 @@ desiredState:
desiredState:
manifest:
version: "v1beta1"
id: "monitoring-heapsterController"
id: "monitoring-heapster-controller"
containers:
- name: "heapster"
image: "kubernetes/heapster:v0.6"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
apiVersion: "v1beta1"
kind: "ReplicationController"
id: "monitoring-influxGrafanaController"
id: "monitoring-influx-grafana-controller"
desiredState:
replicas: 1
replicaSelector:
Expand All @@ -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"
Expand Down
3 changes: 1 addition & 2 deletions examples/examples_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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])...)
Expand Down
22 changes: 11 additions & 11 deletions examples/guestbook/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand All @@ -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",
Expand All @@ -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:
Expand Down Expand Up @@ -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": {
Expand All @@ -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",
Expand All @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions examples/guestbook/frontend-controller.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"id": "frontendController",
"id": "frontend-controller",
"kind": "ReplicationController",
"apiVersion": "v1beta1",
"desiredState": {
Expand All @@ -9,7 +9,7 @@
"desiredState": {
"manifest": {
"version": "v1beta1",
"id": "frontendController",
"id": "frontend-controller",
"containers": [{
"name": "php-redis",
"image": "kubernetes/example-guestbook-php-redis",
Expand Down
4 changes: 2 additions & 2 deletions examples/guestbook/redis-slave-controller.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"id": "redisSlaveController",
"id": "redis-slave-controller",
"kind": "ReplicationController",
"apiVersion": "v1beta1",
"desiredState": {
Expand All @@ -9,7 +9,7 @@
"desiredState": {
"manifest": {
"version": "v1beta1",
"id": "redisSlaveController",
"id": "redis-slave-controller",
"containers": [{
"name": "slave",
"image": "brendanburns/redis-slave",
Expand Down
2 changes: 1 addition & 1 deletion examples/walkthrough/k8s201.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion examples/walkthrough/replication-controller.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
id: nginxController
id: nginx-controller
apiVersion: v1beta1
kind: ReplicationController
desiredState:
Expand Down
4 changes: 2 additions & 2 deletions hack/e2e-suite/guestbook.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions hack/e2e-suite/monitoring.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions hack/test-cmd.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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[@]}"
Expand Down
12 changes: 4 additions & 8 deletions pkg/api/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,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,
}}
}

Expand Down
11 changes: 9 additions & 2 deletions pkg/api/errors/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, ""}
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/api/errors/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,19 +71,20 @@ func TestValidationErrorUsefulMessage(t *testing.T) {
Inner interface{}
KV map[string]int
}
s = NewFieldRequired(
s = NewFieldInvalid(
"foo",
&complicated{
Baz: 1,
Qux: "aoeu",
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) {
Expand Down