From e4ec49ee6bbfe8adc9e4869325e1be7907482275 Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Mon, 29 Sep 2014 17:17:42 -0400 Subject: [PATCH 1/6] Require namespace on controller, pod, service objects --- pkg/api/context.go | 44 ++++++++++++++++++++++++++- pkg/api/context_test.go | 51 ++++++++++++++++++++++++++++++++ pkg/api/types.go | 8 +++++ pkg/api/v1beta1/types.go | 1 + pkg/api/v1beta2/types.go | 1 + pkg/api/validation/validation.go | 14 +++++++++ pkg/apiserver/resthandler.go | 3 +- pkg/registry/controller/rest.go | 8 +++++ pkg/registry/pod/rest.go | 8 ++++- pkg/registry/service/rest.go | 7 +++++ 10 files changed, 142 insertions(+), 3 deletions(-) create mode 100644 pkg/api/context_test.go diff --git a/pkg/api/context.go b/pkg/api/context.go index e5f4e240d5f..c3f33ba85d3 100644 --- a/pkg/api/context.go +++ b/pkg/api/context.go @@ -17,6 +17,8 @@ limitations under the License. package api import ( + stderrs "errors" + "code.google.com/p/go.net/context" ) @@ -25,7 +27,47 @@ type Context interface { Value(key interface{}) interface{} } -// NewContext instantiates a base context object for request flows +// The key type is unexported to prevent collisions +type key int + +// namespaceKey is the context key for the request namespace. +const namespaceKey key = 0 + +// NewContext instantiates a base context object for request flows. func NewContext() Context { return context.TODO() } + +// NewDefaultContext instantiates a base context object for request flows in the default namespace +func NewDefaultContext() Context { + return WithNamespace(NewContext(), NamespaceDefault) +} + +// WithValue returns a copy of parent in which the value associated with key is val. +func WithValue(parent Context, key interface{}, val interface{}) Context { + internalCtx, ok := parent.(context.Context) + if !ok { + panic(stderrs.New("Invalid context type")) + } + return context.WithValue(internalCtx, key, val) +} + +// WithNamespace returns a copy of parent in which the namespace value is set +func WithNamespace(parent Context, namespace string) Context { + return WithValue(parent, namespaceKey, namespace) +} + +// NamespaceFrom returns the value of the namespace key on the ctx +func NamespaceFrom(ctx Context) (string, bool) { + namespace, ok := ctx.Value(namespaceKey).(string) + return namespace, ok +} + +// ValidNamespaceOnCreateOrUpdate returns false if the namespace on the context differs from the resource. If the resource has no namespace, it is set to the value in the context. +func ValidNamespaceOnCreateOrUpdate(ctx Context, resource *JSONBase) bool { + ns, ok := NamespaceFrom(ctx) + if len(resource.Namespace) == 0 { + resource.Namespace = ns + } + return ns == resource.Namespace && ok +} diff --git a/pkg/api/context_test.go b/pkg/api/context_test.go new file mode 100644 index 00000000000..f6d9d4ccdb9 --- /dev/null +++ b/pkg/api/context_test.go @@ -0,0 +1,51 @@ +/* +Copyright 2014 Google Inc. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package api_test + +import ( + "testing" + + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" +) + +// TestNamespaceContext validates that a namespace can be get/set on a context object +func TestNamespaceContext(t *testing.T) { + ctx := api.NewDefaultContext() + result, ok := api.NamespaceFrom(ctx) + if !ok { + t.Errorf("Error getting namespace") + } + if api.NamespaceDefault != result { + t.Errorf("Expected: %v, Actual: %v", api.NamespaceDefault, result) + } +} + +func TestValidNamespaceOnCreateOrUpdate(t *testing.T) { + ctx := api.NewDefaultContext() + namespace, _ := api.NamespaceFrom(ctx) + resource := api.ReplicationController{} + if !api.ValidNamespaceOnCreateOrUpdate(ctx, &resource.JSONBase) { + t.Errorf("expected success") + } + if namespace != resource.Namespace { + t.Errorf("expected resource to have the default namespace assigned during validation") + } + resource = api.ReplicationController{JSONBase: api.JSONBase{Namespace: "other"}} + if api.ValidNamespaceOnCreateOrUpdate(ctx, &resource.JSONBase) { + t.Errorf("Expected error that resource and context errors do not match") + } +} diff --git a/pkg/api/types.go b/pkg/api/types.go index a081f0a7e12..fc3637f1c31 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -237,8 +237,16 @@ type JSONBase struct { SelfLink string `json:"selfLink,omitempty" yaml:"selfLink,omitempty"` ResourceVersion uint64 `json:"resourceVersion,omitempty" yaml:"resourceVersion,omitempty"` APIVersion string `json:"apiVersion,omitempty" yaml:"apiVersion,omitempty"` + Namespace string `json:"namespace",omitempty" yaml:"namespace,omitempty"` } +const ( + // NamespaceDefault means the object is in the default namespace which is applied when not specified by clients + NamespaceDefault string = "default" + // NamespaceAll is the default argument to specify on a context when you want to list or filter resources across all namespaces + NamespaceAll string = "" +) + // PodStatus represents a status of a pod. type PodStatus string diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index 12131d35ccf..cc6addfdc49 100644 --- a/pkg/api/v1beta1/types.go +++ b/pkg/api/v1beta1/types.go @@ -248,6 +248,7 @@ type JSONBase struct { SelfLink string `json:"selfLink,omitempty" yaml:"selfLink,omitempty"` ResourceVersion uint64 `json:"resourceVersion,omitempty" yaml:"resourceVersion,omitempty"` APIVersion string `json:"apiVersion,omitempty" yaml:"apiVersion,omitempty"` + Namespace string `json:"namespace",omitempty" yaml:"namespace,omitempty"` } func (*JSONBase) IsAnAPIObject() {} diff --git a/pkg/api/v1beta2/types.go b/pkg/api/v1beta2/types.go index 7f320f7c807..359921f1ed0 100644 --- a/pkg/api/v1beta2/types.go +++ b/pkg/api/v1beta2/types.go @@ -246,6 +246,7 @@ type JSONBase struct { SelfLink string `json:"selfLink,omitempty" yaml:"selfLink,omitempty"` ResourceVersion uint64 `json:"resourceVersion,omitempty" yaml:"resourceVersion,omitempty"` APIVersion string `json:"apiVersion,omitempty" yaml:"apiVersion,omitempty"` + Namespace string `json:"namespace",omitempty" yaml:"namespace,omitempty"` } // PodStatus represents a status of a pod. diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index d209e3c8790..61151150402 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -313,6 +313,7 @@ func ValidatePod(pod *api.Pod) errs.ErrorList { if len(pod.ID) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("id", pod.ID)) } + allErrs = append(allErrs, validateNotEmptyDNSSubdomain(pod.Namespace, "pod.Namespace")...) allErrs = append(allErrs, ValidatePodState(&pod.DesiredState).Prefix("desiredState")...) return allErrs } @@ -325,6 +326,7 @@ func ValidateService(service *api.Service) errs.ErrorList { } else if !util.IsDNS952Label(service.ID) { allErrs = append(allErrs, errs.NewFieldInvalid("id", service.ID)) } + allErrs = append(allErrs, validateNotEmptyDNSSubdomain(service.Namespace, "service.Namespace")...) if !util.IsValidPortNum(service.Port) { allErrs = append(allErrs, errs.NewFieldInvalid("Service.Port", service.Port)) } @@ -345,6 +347,7 @@ func ValidateReplicationController(controller *api.ReplicationController) errs.E if len(controller.ID) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("id", controller.ID)) } + allErrs = append(allErrs, validateNotEmptyDNSSubdomain(controller.Namespace, "controller.Namespace")...) allErrs = append(allErrs, ValidateReplicationControllerState(&controller.DesiredState).Prefix("desiredState")...) return allErrs } @@ -366,3 +369,14 @@ func ValidateReplicationControllerState(state *api.ReplicationControllerState) e allErrs = append(allErrs, ValidateManifest(&state.PodTemplate.DesiredState.Manifest).Prefix("podTemplate.desiredState.manifest")...) return allErrs } + +// validateNotEmptyDNSSubdomain validates the provided value is not empty and is a dns subdomain. +func validateNotEmptyDNSSubdomain(value string, label string) errs.ErrorList { + allErrs := errs.ErrorList{} + if value == "" { + allErrs = append(allErrs, errs.NewFieldInvalid(label, value)) + } else if !util.IsDNSSubdomain(value) { + allErrs = append(allErrs, errs.NewFieldInvalid(label, value)) + } + return allErrs +} diff --git a/pkg/apiserver/resthandler.go b/pkg/apiserver/resthandler.go index 50ec2cd97bc..e2c7457d3c7 100644 --- a/pkg/apiserver/resthandler.go +++ b/pkg/apiserver/resthandler.go @@ -100,7 +100,8 @@ func curry(f func(runtime.Object, *http.Request) error, req *http.Request) func( // timeout= Timeout for synchronous requests, only applies if sync=true // labels= Used for filtering list operations func (h *RESTHandler) handleRESTStorage(parts []string, req *http.Request, w http.ResponseWriter, storage RESTStorage) { - ctx := api.NewContext() + // TODO for now, we perform all operations in the default namespace + ctx := api.NewDefaultContext() sync := req.URL.Query().Get("sync") == "true" timeout := parseTimeout(req.URL.Query().Get("timeout")) switch req.Method { diff --git a/pkg/registry/controller/rest.go b/pkg/registry/controller/rest.go index 2665be976d6..6a3aa7194e1 100644 --- a/pkg/registry/controller/rest.go +++ b/pkg/registry/controller/rest.go @@ -17,6 +17,7 @@ limitations under the License. package controller import ( + stderrs "errors" "fmt" "time" @@ -59,6 +60,10 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan runtime.Obje if !ok { return nil, fmt.Errorf("not a replication controller: %#v", obj) } + if !api.ValidNamespaceOnCreateOrUpdate(ctx, &controller.JSONBase) { + return nil, errors.NewConflict("controller", controller.Namespace, stderrs.New("Controller.Namespace does not match the provided context")) + } + if len(controller.ID) == 0 { controller.ID = uuid.NewUUID().String() } @@ -128,6 +133,9 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan runtime.Obje if !ok { return nil, fmt.Errorf("not a replication controller: %#v", obj) } + if !api.ValidNamespaceOnCreateOrUpdate(ctx, &controller.JSONBase) { + return nil, errors.NewConflict("controller", controller.Namespace, stderrs.New("Controller.Namespace does not match the provided context")) + } if errs := validation.ValidateReplicationController(controller); len(errs) > 0 { return nil, errors.NewInvalid("replicationController", controller.ID, errs) } diff --git a/pkg/registry/pod/rest.go b/pkg/registry/pod/rest.go index 8f3919aaf4c..89416130f04 100644 --- a/pkg/registry/pod/rest.go +++ b/pkg/registry/pod/rest.go @@ -17,6 +17,7 @@ limitations under the License. package pod import ( + stderrs "errors" "fmt" "sync" "time" @@ -69,6 +70,9 @@ func NewREST(config *RESTConfig) *REST { func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan runtime.Object, error) { pod := obj.(*api.Pod) + if !api.ValidNamespaceOnCreateOrUpdate(ctx, &pod.JSONBase) { + return nil, errors.NewConflict("pod", pod.Namespace, stderrs.New("Pod.Namespace does not match the provided context")) + } pod.DesiredState.Manifest.UUID = uuid.NewUUID().String() if len(pod.ID) == 0 { pod.ID = pod.DesiredState.Manifest.UUID @@ -77,7 +81,6 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan runtime.Obje if errs := validation.ValidatePod(pod); len(errs) > 0 { return nil, errors.NewInvalid("pod", pod.ID, errs) } - pod.CreationTimestamp = util.Now() return apiserver.MakeAsync(func() (runtime.Object, error) { @@ -159,6 +162,9 @@ func (*REST) New() runtime.Object { func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan runtime.Object, error) { pod := obj.(*api.Pod) + if !api.ValidNamespaceOnCreateOrUpdate(ctx, &pod.JSONBase) { + return nil, errors.NewConflict("pod", pod.Namespace, stderrs.New("Pod.Namespace does not match the provided context")) + } if errs := validation.ValidatePod(pod); len(errs) > 0 { return nil, errors.NewInvalid("pod", pod.ID, errs) } diff --git a/pkg/registry/service/rest.go b/pkg/registry/service/rest.go index 0394ece8cc5..2f430435f39 100644 --- a/pkg/registry/service/rest.go +++ b/pkg/registry/service/rest.go @@ -17,6 +17,7 @@ limitations under the License. package service import ( + stderrs "errors" "fmt" "math/rand" "strconv" @@ -52,6 +53,9 @@ func NewREST(registry Registry, cloud cloudprovider.Interface, machines minion.R func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan runtime.Object, error) { srv := obj.(*api.Service) + if !api.ValidNamespaceOnCreateOrUpdate(ctx, &srv.JSONBase) { + return nil, errors.NewConflict("service", srv.Namespace, stderrs.New("Service.Namespace does not match the provided context")) + } if errs := validation.ValidateService(srv); len(errs) > 0 { return nil, errors.NewInvalid("service", srv.ID, errs) } @@ -165,6 +169,9 @@ func GetServiceEnvironmentVariables(ctx api.Context, registry Registry, machine func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan runtime.Object, error) { srv := obj.(*api.Service) + if !api.ValidNamespaceOnCreateOrUpdate(ctx, &srv.JSONBase) { + return nil, errors.NewConflict("service", srv.Namespace, stderrs.New("Service.Namespace does not match the provided context")) + } if errs := validation.ValidateService(srv); len(errs) > 0 { return nil, errors.NewInvalid("service", srv.ID, errs) } From 0312d80ffa9faafc93172f6a68d430c5e84a67c1 Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Mon, 29 Sep 2014 17:18:18 -0400 Subject: [PATCH 2/6] Update test cases to use default context --- pkg/api/context_test.go | 6 ++- pkg/api/validation/validation_test.go | 57 ++++++++++++++++++--------- pkg/registry/controller/rest_test.go | 6 +-- pkg/registry/pod/rest_test.go | 12 +++--- pkg/registry/service/rest_test.go | 24 +++++------ 5 files changed, 64 insertions(+), 41 deletions(-) diff --git a/pkg/api/context_test.go b/pkg/api/context_test.go index f6d9d4ccdb9..1cd9c2b5373 100644 --- a/pkg/api/context_test.go +++ b/pkg/api/context_test.go @@ -46,6 +46,10 @@ func TestValidNamespaceOnCreateOrUpdate(t *testing.T) { } resource = api.ReplicationController{JSONBase: api.JSONBase{Namespace: "other"}} if api.ValidNamespaceOnCreateOrUpdate(ctx, &resource.JSONBase) { - t.Errorf("Expected error that resource and context errors do not match") + t.Errorf("Expected error that resource and context errors do not match because resource has different namespace") + } + ctx = api.NewContext() + if api.ValidNamespaceOnCreateOrUpdate(ctx, &resource.JSONBase) { + t.Errorf("Expected error that resource and context errors do not match since context has no namespace") } } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index e5db3369964..d7bd751dbdd 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -366,7 +366,7 @@ func TestValidateManifest(t *testing.T) { func TestValidatePod(t *testing.T) { errs := ValidatePod(&api.Pod{ - JSONBase: api.JSONBase{ID: "foo"}, + JSONBase: api.JSONBase{ID: "foo", Namespace: api.NamespaceDefault}, Labels: map[string]string{ "foo": "bar", }, @@ -384,7 +384,7 @@ func TestValidatePod(t *testing.T) { t.Errorf("Unexpected non-zero error list: %#v", errs) } errs = ValidatePod(&api.Pod{ - JSONBase: api.JSONBase{ID: "foo"}, + JSONBase: api.JSONBase{ID: "foo", Namespace: api.NamespaceDefault}, Labels: map[string]string{ "foo": "bar", }, @@ -397,7 +397,7 @@ func TestValidatePod(t *testing.T) { } errs = ValidatePod(&api.Pod{ - JSONBase: api.JSONBase{ID: "foo"}, + JSONBase: api.JSONBase{ID: "foo", Namespace: api.NamespaceDefault}, Labels: map[string]string{ "foo": "bar", }, @@ -424,16 +424,27 @@ func TestValidateService(t *testing.T) { { name: "missing id", svc: api.Service{ + JSONBase: api.JSONBase{Namespace: api.NamespaceDefault}, Port: 8675, Selector: map[string]string{"foo": "bar"}, }, // Should fail because the ID is missing. numErrs: 1, }, + { + name: "missing namespace", + svc: api.Service{ + JSONBase: api.JSONBase{ID: "foo"}, + Port: 8675, + Selector: map[string]string{"foo": "bar"}, + }, + // Should fail because the Namespace is missing. + numErrs: 1, + }, { name: "invalid id", svc: api.Service{ - JSONBase: api.JSONBase{ID: "123abc"}, + JSONBase: api.JSONBase{ID: "123abc", Namespace: api.NamespaceDefault}, Port: 8675, Selector: map[string]string{"foo": "bar"}, }, @@ -443,7 +454,7 @@ func TestValidateService(t *testing.T) { { name: "missing port", svc: api.Service{ - JSONBase: api.JSONBase{ID: "abc123"}, + JSONBase: api.JSONBase{ID: "abc123", Namespace: api.NamespaceDefault}, Selector: map[string]string{"foo": "bar"}, }, // Should fail because the port number is missing/invalid. @@ -452,7 +463,7 @@ func TestValidateService(t *testing.T) { { name: "invalid port", svc: api.Service{ - JSONBase: api.JSONBase{ID: "abc123"}, + JSONBase: api.JSONBase{ID: "abc123", Namespace: api.NamespaceDefault}, Port: 65536, Selector: map[string]string{"foo": "bar"}, }, @@ -462,7 +473,7 @@ func TestValidateService(t *testing.T) { { name: "invalid protocol", svc: api.Service{ - JSONBase: api.JSONBase{ID: "abc123"}, + JSONBase: api.JSONBase{ID: "abc123", Namespace: api.NamespaceDefault}, Port: 8675, Protocol: "INVALID", Selector: map[string]string{"foo": "bar"}, @@ -473,7 +484,7 @@ func TestValidateService(t *testing.T) { { name: "missing selector", svc: api.Service{ - JSONBase: api.JSONBase{ID: "foo"}, + JSONBase: api.JSONBase{ID: "foo", Namespace: api.NamespaceDefault}, Port: 8675, }, // Should fail because the selector is missing. @@ -482,7 +493,7 @@ func TestValidateService(t *testing.T) { { name: "valid 1", svc: api.Service{ - JSONBase: api.JSONBase{ID: "abc123"}, + JSONBase: api.JSONBase{ID: "abc123", Namespace: api.NamespaceDefault}, Port: 1, Protocol: "TCP", Selector: map[string]string{"foo": "bar"}, @@ -492,7 +503,7 @@ func TestValidateService(t *testing.T) { { name: "valid 2", svc: api.Service{ - JSONBase: api.JSONBase{ID: "abc123"}, + JSONBase: api.JSONBase{ID: "abc123", Namespace: api.NamespaceDefault}, Port: 65535, Protocol: "UDP", Selector: map[string]string{"foo": "bar"}, @@ -502,7 +513,7 @@ func TestValidateService(t *testing.T) { { name: "valid 3", svc: api.Service{ - JSONBase: api.JSONBase{ID: "abc123"}, + JSONBase: api.JSONBase{ID: "abc123", Namespace: api.NamespaceDefault}, Port: 80, Selector: map[string]string{"foo": "bar"}, }, @@ -519,7 +530,7 @@ func TestValidateService(t *testing.T) { svc := api.Service{ Port: 6502, - JSONBase: api.JSONBase{ID: "foo"}, + JSONBase: api.JSONBase{ID: "foo", Namespace: api.NamespaceDefault}, Selector: map[string]string{"foo": "bar"}, } errs := ValidateService(&svc) @@ -544,14 +555,14 @@ func TestValidateReplicationController(t *testing.T) { successCases := []api.ReplicationController{ { - JSONBase: api.JSONBase{ID: "abc"}, + JSONBase: api.JSONBase{ID: "abc", Namespace: api.NamespaceDefault}, DesiredState: api.ReplicationControllerState{ ReplicaSelector: validSelector, PodTemplate: validPodTemplate, }, }, { - JSONBase: api.JSONBase{ID: "abc-123"}, + JSONBase: api.JSONBase{ID: "abc-123", Namespace: api.NamespaceDefault}, DesiredState: api.ReplicationControllerState{ ReplicaSelector: validSelector, PodTemplate: validPodTemplate, @@ -566,33 +577,40 @@ func TestValidateReplicationController(t *testing.T) { errorCases := map[string]api.ReplicationController{ "zero-length ID": { - JSONBase: api.JSONBase{ID: ""}, + JSONBase: api.JSONBase{ID: "", Namespace: api.NamespaceDefault}, + DesiredState: api.ReplicationControllerState{ + ReplicaSelector: validSelector, + PodTemplate: validPodTemplate, + }, + }, + "missing-namespace": { + JSONBase: api.JSONBase{ID: "abc-123"}, DesiredState: api.ReplicationControllerState{ ReplicaSelector: validSelector, PodTemplate: validPodTemplate, }, }, "empty selector": { - JSONBase: api.JSONBase{ID: "abc"}, + JSONBase: api.JSONBase{ID: "abc", Namespace: api.NamespaceDefault}, DesiredState: api.ReplicationControllerState{ PodTemplate: validPodTemplate, }, }, "selector_doesnt_match": { - JSONBase: api.JSONBase{ID: "abc"}, + JSONBase: api.JSONBase{ID: "abc", Namespace: api.NamespaceDefault}, DesiredState: api.ReplicationControllerState{ ReplicaSelector: map[string]string{"foo": "bar"}, PodTemplate: validPodTemplate, }, }, "invalid manifest": { - JSONBase: api.JSONBase{ID: "abc"}, + JSONBase: api.JSONBase{ID: "abc", Namespace: api.NamespaceDefault}, DesiredState: api.ReplicationControllerState{ ReplicaSelector: validSelector, }, }, "negative_replicas": { - JSONBase: api.JSONBase{ID: "abc"}, + JSONBase: api.JSONBase{ID: "abc", Namespace: api.NamespaceDefault}, DesiredState: api.ReplicationControllerState{ Replicas: -1, ReplicaSelector: validSelector, @@ -608,6 +626,7 @@ func TestValidateReplicationController(t *testing.T) { field := errs[i].(errors.ValidationError).Field if !strings.HasPrefix(field, "desiredState.podTemplate.") && field != "id" && + field != "controller.Namespace" && field != "desiredState.replicaSelector" && field != "desiredState.replicas" { t.Errorf("%s: missing prefix for: %v", k, errs[i]) diff --git a/pkg/registry/controller/rest_test.go b/pkg/registry/controller/rest_test.go index 5a5e4e728fd..81ed77790cb 100644 --- a/pkg/registry/controller/rest_test.go +++ b/pkg/registry/controller/rest_test.go @@ -243,7 +243,7 @@ func TestCreateController(t *testing.T) { PodTemplate: validPodTemplate, }, } - ctx := api.NewContext() + ctx := api.NewDefaultContext() channel, err := storage.Create(ctx, controller) if err != nil { t.Fatalf("Unexpected error: %v", err) @@ -279,8 +279,8 @@ func TestControllerStorageValidatesCreate(t *testing.T) { DesiredState: api.ReplicationControllerState{}, }, } + ctx := api.NewDefaultContext() for _, failureCase := range failureCases { - ctx := api.NewContext() c, err := storage.Create(ctx, &failureCase) if c != nil { t.Errorf("Expected nil channel") @@ -310,8 +310,8 @@ func TestControllerStorageValidatesUpdate(t *testing.T) { DesiredState: api.ReplicationControllerState{}, }, } + ctx := api.NewDefaultContext() for _, failureCase := range failureCases { - ctx := api.NewContext() c, err := storage.Update(ctx, &failureCase) if c != nil { t.Errorf("Expected nil channel") diff --git a/pkg/registry/pod/rest_test.go b/pkg/registry/pod/rest_test.go index 884dfcc3f7d..d6e6f0d7e19 100644 --- a/pkg/registry/pod/rest_test.go +++ b/pkg/registry/pod/rest_test.go @@ -69,7 +69,7 @@ func TestCreatePodRegistryError(t *testing.T) { }, } pod := &api.Pod{DesiredState: desiredState} - ctx := api.NewContext() + ctx := api.NewDefaultContext() ch, err := storage.Create(ctx, pod) if err != nil { t.Errorf("Expected %#v, Got %#v", nil, err) @@ -89,7 +89,7 @@ func TestCreatePodSetsIds(t *testing.T) { }, } pod := &api.Pod{DesiredState: desiredState} - ctx := api.NewContext() + ctx := api.NewDefaultContext() ch, err := storage.Create(ctx, pod) if err != nil { t.Errorf("Expected %#v, Got %#v", nil, err) @@ -116,7 +116,7 @@ func TestCreatePodSetsUUIDs(t *testing.T) { }, } pod := &api.Pod{DesiredState: desiredState} - ctx := api.NewContext() + ctx := api.NewDefaultContext() ch, err := storage.Create(ctx, pod) if err != nil { t.Errorf("Expected %#v, Got %#v", nil, err) @@ -496,7 +496,7 @@ func TestPodStorageValidatesCreate(t *testing.T) { storage := REST{ registry: podRegistry, } - ctx := api.NewContext() + ctx := api.NewDefaultContext() pod := &api.Pod{} c, err := storage.Create(ctx, pod) if c != nil { @@ -513,7 +513,7 @@ func TestPodStorageValidatesUpdate(t *testing.T) { storage := REST{ registry: podRegistry, } - ctx := api.NewContext() + ctx := api.NewDefaultContext() pod := &api.Pod{} c, err := storage.Update(ctx, pod) if c != nil { @@ -545,7 +545,7 @@ func TestCreatePod(t *testing.T) { JSONBase: api.JSONBase{ID: "foo"}, DesiredState: desiredState, } - ctx := api.NewContext() + ctx := api.NewDefaultContext() channel, err := storage.Create(ctx, pod) if err != nil { t.Errorf("unexpected error: %v", err) diff --git a/pkg/registry/service/rest_test.go b/pkg/registry/service/rest_test.go index ddadda9850f..33f3706151f 100644 --- a/pkg/registry/service/rest_test.go +++ b/pkg/registry/service/rest_test.go @@ -40,7 +40,7 @@ func TestServiceRegistryCreate(t *testing.T) { JSONBase: api.JSONBase{ID: "foo"}, Selector: map[string]string{"bar": "baz"}, } - ctx := api.NewContext() + ctx := api.NewDefaultContext() c, _ := storage.Create(ctx, svc) created_svc := <-c created_service := created_svc.(*api.Service) @@ -76,7 +76,7 @@ func TestServiceStorageValidatesCreate(t *testing.T) { Selector: map[string]string{}, }, } - ctx := api.NewContext() + ctx := api.NewDefaultContext() for _, failureCase := range failureCases { c, err := storage.Create(ctx, &failureCase) if c != nil { @@ -90,7 +90,7 @@ func TestServiceStorageValidatesCreate(t *testing.T) { } func TestServiceRegistryUpdate(t *testing.T) { - ctx := api.NewContext() + ctx := api.NewDefaultContext() registry := registrytest.NewServiceRegistry() registry.CreateService(ctx, &api.Service{ Port: 6502, @@ -120,7 +120,7 @@ func TestServiceRegistryUpdate(t *testing.T) { } func TestServiceStorageValidatesUpdate(t *testing.T) { - ctx := api.NewContext() + ctx := api.NewDefaultContext() registry := registrytest.NewServiceRegistry() registry.CreateService(ctx, &api.Service{ Port: 6502, @@ -152,7 +152,7 @@ func TestServiceStorageValidatesUpdate(t *testing.T) { } func TestServiceRegistryExternalService(t *testing.T) { - ctx := api.NewContext() + ctx := api.NewDefaultContext() registry := registrytest.NewServiceRegistry() fakeCloud := &cloud.FakeCloud{} machines := []string{"foo", "bar", "baz"} @@ -190,7 +190,7 @@ func TestServiceRegistryExternalServiceError(t *testing.T) { Selector: map[string]string{"bar": "baz"}, CreateExternalLoadBalancer: true, } - ctx := api.NewContext() + ctx := api.NewDefaultContext() c, _ := storage.Create(ctx, svc) <-c if len(fakeCloud.Calls) != 1 || fakeCloud.Calls[0] != "get-zone" { @@ -202,7 +202,7 @@ func TestServiceRegistryExternalServiceError(t *testing.T) { } func TestServiceRegistryDelete(t *testing.T) { - ctx := api.NewContext() + ctx := api.NewDefaultContext() registry := registrytest.NewServiceRegistry() fakeCloud := &cloud.FakeCloud{} machines := []string{"foo", "bar", "baz"} @@ -223,7 +223,7 @@ func TestServiceRegistryDelete(t *testing.T) { } func TestServiceRegistryDeleteExternal(t *testing.T) { - ctx := api.NewContext() + ctx := api.NewDefaultContext() registry := registrytest.NewServiceRegistry() fakeCloud := &cloud.FakeCloud{} machines := []string{"foo", "bar", "baz"} @@ -245,7 +245,7 @@ func TestServiceRegistryDeleteExternal(t *testing.T) { } func TestServiceRegistryMakeLinkVariables(t *testing.T) { - ctx := api.NewContext() + ctx := api.NewDefaultContext() registry := registrytest.NewServiceRegistry() registry.List = api.ServiceList{ Items: []api.Service{ @@ -310,7 +310,7 @@ func TestServiceRegistryMakeLinkVariables(t *testing.T) { } func TestServiceRegistryGet(t *testing.T) { - ctx := api.NewContext() + ctx := api.NewDefaultContext() registry := registrytest.NewServiceRegistry() fakeCloud := &cloud.FakeCloud{} machines := []string{"foo", "bar", "baz"} @@ -329,7 +329,7 @@ func TestServiceRegistryGet(t *testing.T) { } func TestServiceRegistryResourceLocation(t *testing.T) { - ctx := api.NewContext() + ctx := api.NewDefaultContext() registry := registrytest.NewServiceRegistry() registry.Endpoints = api.Endpoints{Endpoints: []string{"foo:80"}} fakeCloud := &cloud.FakeCloud{} @@ -359,7 +359,7 @@ func TestServiceRegistryResourceLocation(t *testing.T) { } func TestServiceRegistryList(t *testing.T) { - ctx := api.NewContext() + ctx := api.NewDefaultContext() registry := registrytest.NewServiceRegistry() fakeCloud := &cloud.FakeCloud{} machines := []string{"foo", "bar", "baz"} From e032c3075c728911eb540b53099af0dc3005cf22 Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Mon, 29 Sep 2014 17:38:25 -0400 Subject: [PATCH 3/6] Fix examples test by setting a context prior to validation --- examples/examples_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/examples/examples_test.go b/examples/examples_test.go index 7e326da08ef..259c8a78833 100644 --- a/examples/examples_test.go +++ b/examples/examples_test.go @@ -32,6 +32,7 @@ import ( ) func validateObject(obj runtime.Object) (errors []error) { + ctx := api.NewDefaultContext() switch t := obj.(type) { case *api.ReplicationController: errors = validation.ValidateManifest(&t.DesiredState.PodTemplate.DesiredState.Manifest) @@ -40,12 +41,14 @@ func validateObject(obj runtime.Object) (errors []error) { errors = append(errors, validateObject(&t.Items[i])...) } case *api.Service: + api.ValidNamespaceOnCreateOrUpdate(ctx, &t.JSONBase) errors = validation.ValidateService(t) case *api.ServiceList: for i := range t.Items { errors = append(errors, validateObject(&t.Items[i])...) } case *api.Pod: + api.ValidNamespaceOnCreateOrUpdate(ctx, &t.JSONBase) errors = validation.ValidateManifest(&t.DesiredState.Manifest) case *api.PodList: for i := range t.Items { From f3859394ab16dc193cea27e95f8c6d7298f6cb80 Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Mon, 29 Sep 2014 17:44:25 -0400 Subject: [PATCH 4/6] Update schemas for api docs --- api/doc/controller-schema.json | 4 ++++ api/doc/pod-schema.json | 4 ++++ api/doc/service-schema.json | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/api/doc/controller-schema.json b/api/doc/controller-schema.json index bec1d9ac1cb..a62b3360e6e 100644 --- a/api/doc/controller-schema.json +++ b/api/doc/controller-schema.json @@ -12,6 +12,10 @@ "type": "string", "required": false }, + "namespace": { + "type": "string", + "required": false + }, "creationTimestamp": { "type": "string", "required": false diff --git a/api/doc/pod-schema.json b/api/doc/pod-schema.json index 8d4279a07bd..3bd8f66caad 100644 --- a/api/doc/pod-schema.json +++ b/api/doc/pod-schema.json @@ -12,6 +12,10 @@ "type": "string", "required": false }, + "namespace": { + "type": "string", + "required": false + }, "creationTimestamp": { "type": "string", "required": false diff --git a/api/doc/service-schema.json b/api/doc/service-schema.json index 750bca469c4..78e781f71fc 100644 --- a/api/doc/service-schema.json +++ b/api/doc/service-schema.json @@ -12,6 +12,10 @@ "type": "string", "required": false }, + "namespace": { + "type": "string", + "required": false + }, "creationTimestamp": { "type": "string", "required": false From 6625e58a2034e11c1df50cf53a35e1ec4ffde27d Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Wed, 1 Oct 2014 10:42:07 -0400 Subject: [PATCH 5/6] Updates to review comments --- examples/examples_test.go | 4 ++-- pkg/api/context.go | 4 ++-- pkg/api/context_test.go | 15 +++++++++++---- pkg/registry/controller/rest.go | 9 ++++----- pkg/registry/pod/rest.go | 9 ++++----- pkg/registry/service/rest.go | 9 ++++----- 6 files changed, 27 insertions(+), 23 deletions(-) diff --git a/examples/examples_test.go b/examples/examples_test.go index 259c8a78833..feb1afdf54d 100644 --- a/examples/examples_test.go +++ b/examples/examples_test.go @@ -41,14 +41,14 @@ func validateObject(obj runtime.Object) (errors []error) { errors = append(errors, validateObject(&t.Items[i])...) } case *api.Service: - api.ValidNamespaceOnCreateOrUpdate(ctx, &t.JSONBase) + api.ValidNamespace(ctx, &t.JSONBase) errors = validation.ValidateService(t) case *api.ServiceList: for i := range t.Items { errors = append(errors, validateObject(&t.Items[i])...) } case *api.Pod: - api.ValidNamespaceOnCreateOrUpdate(ctx, &t.JSONBase) + api.ValidNamespace(ctx, &t.JSONBase) errors = validation.ValidateManifest(&t.DesiredState.Manifest) case *api.PodList: for i := range t.Items { diff --git a/pkg/api/context.go b/pkg/api/context.go index c3f33ba85d3..5fe851bb12a 100644 --- a/pkg/api/context.go +++ b/pkg/api/context.go @@ -63,8 +63,8 @@ func NamespaceFrom(ctx Context) (string, bool) { return namespace, ok } -// ValidNamespaceOnCreateOrUpdate returns false if the namespace on the context differs from the resource. If the resource has no namespace, it is set to the value in the context. -func ValidNamespaceOnCreateOrUpdate(ctx Context, resource *JSONBase) bool { +// ValidNamespace returns false if the namespace on the context differs from the resource. If the resource has no namespace, it is set to the value in the context. +func ValidNamespace(ctx Context, resource *JSONBase) bool { ns, ok := NamespaceFrom(ctx) if len(resource.Namespace) == 0 { resource.Namespace = ns diff --git a/pkg/api/context_test.go b/pkg/api/context_test.go index 1cd9c2b5373..146963cb196 100644 --- a/pkg/api/context_test.go +++ b/pkg/api/context_test.go @@ -32,24 +32,31 @@ func TestNamespaceContext(t *testing.T) { if api.NamespaceDefault != result { t.Errorf("Expected: %v, Actual: %v", api.NamespaceDefault, result) } + + ctx = api.NewContext() + result, ok = api.NamespaceFrom(ctx) + if ok { + t.Errorf("Should not be ok because there is no namespace on the context") + } } -func TestValidNamespaceOnCreateOrUpdate(t *testing.T) { +// TestValidNamespace validates that namespace rules are enforced on a resource prior to create or update +func TestValidNamespace(t *testing.T) { ctx := api.NewDefaultContext() namespace, _ := api.NamespaceFrom(ctx) resource := api.ReplicationController{} - if !api.ValidNamespaceOnCreateOrUpdate(ctx, &resource.JSONBase) { + if !api.ValidNamespace(ctx, &resource.JSONBase) { t.Errorf("expected success") } if namespace != resource.Namespace { t.Errorf("expected resource to have the default namespace assigned during validation") } resource = api.ReplicationController{JSONBase: api.JSONBase{Namespace: "other"}} - if api.ValidNamespaceOnCreateOrUpdate(ctx, &resource.JSONBase) { + if api.ValidNamespace(ctx, &resource.JSONBase) { t.Errorf("Expected error that resource and context errors do not match because resource has different namespace") } ctx = api.NewContext() - if api.ValidNamespaceOnCreateOrUpdate(ctx, &resource.JSONBase) { + if api.ValidNamespace(ctx, &resource.JSONBase) { t.Errorf("Expected error that resource and context errors do not match since context has no namespace") } } diff --git a/pkg/registry/controller/rest.go b/pkg/registry/controller/rest.go index 6a3aa7194e1..326144c1f16 100644 --- a/pkg/registry/controller/rest.go +++ b/pkg/registry/controller/rest.go @@ -17,7 +17,6 @@ limitations under the License. package controller import ( - stderrs "errors" "fmt" "time" @@ -60,8 +59,8 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan runtime.Obje if !ok { return nil, fmt.Errorf("not a replication controller: %#v", obj) } - if !api.ValidNamespaceOnCreateOrUpdate(ctx, &controller.JSONBase) { - return nil, errors.NewConflict("controller", controller.Namespace, stderrs.New("Controller.Namespace does not match the provided context")) + if !api.ValidNamespace(ctx, &controller.JSONBase) { + return nil, errors.NewConflict("controller", controller.Namespace, fmt.Errorf("Controller.Namespace does not match the provided context")) } if len(controller.ID) == 0 { @@ -133,8 +132,8 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan runtime.Obje if !ok { return nil, fmt.Errorf("not a replication controller: %#v", obj) } - if !api.ValidNamespaceOnCreateOrUpdate(ctx, &controller.JSONBase) { - return nil, errors.NewConflict("controller", controller.Namespace, stderrs.New("Controller.Namespace does not match the provided context")) + if !api.ValidNamespace(ctx, &controller.JSONBase) { + return nil, errors.NewConflict("controller", controller.Namespace, fmt.Errorf("Controller.Namespace does not match the provided context")) } if errs := validation.ValidateReplicationController(controller); len(errs) > 0 { return nil, errors.NewInvalid("replicationController", controller.ID, errs) diff --git a/pkg/registry/pod/rest.go b/pkg/registry/pod/rest.go index 89416130f04..4b45b096db2 100644 --- a/pkg/registry/pod/rest.go +++ b/pkg/registry/pod/rest.go @@ -17,7 +17,6 @@ limitations under the License. package pod import ( - stderrs "errors" "fmt" "sync" "time" @@ -70,8 +69,8 @@ func NewREST(config *RESTConfig) *REST { func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan runtime.Object, error) { pod := obj.(*api.Pod) - if !api.ValidNamespaceOnCreateOrUpdate(ctx, &pod.JSONBase) { - return nil, errors.NewConflict("pod", pod.Namespace, stderrs.New("Pod.Namespace does not match the provided context")) + if !api.ValidNamespace(ctx, &pod.JSONBase) { + return nil, errors.NewConflict("pod", pod.Namespace, fmt.Errorf("Pod.Namespace does not match the provided context")) } pod.DesiredState.Manifest.UUID = uuid.NewUUID().String() if len(pod.ID) == 0 { @@ -162,8 +161,8 @@ func (*REST) New() runtime.Object { func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan runtime.Object, error) { pod := obj.(*api.Pod) - if !api.ValidNamespaceOnCreateOrUpdate(ctx, &pod.JSONBase) { - return nil, errors.NewConflict("pod", pod.Namespace, stderrs.New("Pod.Namespace does not match the provided context")) + if !api.ValidNamespace(ctx, &pod.JSONBase) { + return nil, errors.NewConflict("pod", pod.Namespace, fmt.Errorf("Pod.Namespace does not match the provided context")) } if errs := validation.ValidatePod(pod); len(errs) > 0 { return nil, errors.NewInvalid("pod", pod.ID, errs) diff --git a/pkg/registry/service/rest.go b/pkg/registry/service/rest.go index 2f430435f39..702c3c48db0 100644 --- a/pkg/registry/service/rest.go +++ b/pkg/registry/service/rest.go @@ -17,7 +17,6 @@ limitations under the License. package service import ( - stderrs "errors" "fmt" "math/rand" "strconv" @@ -53,8 +52,8 @@ func NewREST(registry Registry, cloud cloudprovider.Interface, machines minion.R func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan runtime.Object, error) { srv := obj.(*api.Service) - if !api.ValidNamespaceOnCreateOrUpdate(ctx, &srv.JSONBase) { - return nil, errors.NewConflict("service", srv.Namespace, stderrs.New("Service.Namespace does not match the provided context")) + if !api.ValidNamespace(ctx, &srv.JSONBase) { + return nil, errors.NewConflict("service", srv.Namespace, fmt.Errorf("Service.Namespace does not match the provided context")) } if errs := validation.ValidateService(srv); len(errs) > 0 { return nil, errors.NewInvalid("service", srv.ID, errs) @@ -169,8 +168,8 @@ func GetServiceEnvironmentVariables(ctx api.Context, registry Registry, machine func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan runtime.Object, error) { srv := obj.(*api.Service) - if !api.ValidNamespaceOnCreateOrUpdate(ctx, &srv.JSONBase) { - return nil, errors.NewConflict("service", srv.Namespace, stderrs.New("Service.Namespace does not match the provided context")) + if !api.ValidNamespace(ctx, &srv.JSONBase) { + return nil, errors.NewConflict("service", srv.Namespace, fmt.Errorf("Service.Namespace does not match the provided context")) } if errs := validation.ValidateService(srv); len(errs) > 0 { return nil, errors.NewInvalid("service", srv.ID, errs) From 016d39470576ed4db628ec5050d529d978a8b298 Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Wed, 1 Oct 2014 14:29:47 -0400 Subject: [PATCH 6/6] Remove redundant code in validation --- pkg/api/validation/validation.go | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 61151150402..940892b4047 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -313,7 +313,9 @@ func ValidatePod(pod *api.Pod) errs.ErrorList { if len(pod.ID) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("id", pod.ID)) } - allErrs = append(allErrs, validateNotEmptyDNSSubdomain(pod.Namespace, "pod.Namespace")...) + if !util.IsDNSSubdomain(pod.Namespace) { + allErrs = append(allErrs, errs.NewFieldInvalid("pod.Namespace", pod.Namespace)) + } allErrs = append(allErrs, ValidatePodState(&pod.DesiredState).Prefix("desiredState")...) return allErrs } @@ -326,7 +328,9 @@ func ValidateService(service *api.Service) errs.ErrorList { } else if !util.IsDNS952Label(service.ID) { allErrs = append(allErrs, errs.NewFieldInvalid("id", service.ID)) } - allErrs = append(allErrs, validateNotEmptyDNSSubdomain(service.Namespace, "service.Namespace")...) + if !util.IsDNSSubdomain(service.Namespace) { + allErrs = append(allErrs, errs.NewFieldInvalid("service.Namespace", service.Namespace)) + } if !util.IsValidPortNum(service.Port) { allErrs = append(allErrs, errs.NewFieldInvalid("Service.Port", service.Port)) } @@ -347,7 +351,9 @@ func ValidateReplicationController(controller *api.ReplicationController) errs.E if len(controller.ID) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("id", controller.ID)) } - allErrs = append(allErrs, validateNotEmptyDNSSubdomain(controller.Namespace, "controller.Namespace")...) + if !util.IsDNSSubdomain(controller.Namespace) { + allErrs = append(allErrs, errs.NewFieldInvalid("controller.Namespace", controller.Namespace)) + } allErrs = append(allErrs, ValidateReplicationControllerState(&controller.DesiredState).Prefix("desiredState")...) return allErrs } @@ -369,14 +375,3 @@ func ValidateReplicationControllerState(state *api.ReplicationControllerState) e allErrs = append(allErrs, ValidateManifest(&state.PodTemplate.DesiredState.Manifest).Prefix("podTemplate.desiredState.manifest")...) return allErrs } - -// validateNotEmptyDNSSubdomain validates the provided value is not empty and is a dns subdomain. -func validateNotEmptyDNSSubdomain(value string, label string) errs.ErrorList { - allErrs := errs.ErrorList{} - if value == "" { - allErrs = append(allErrs, errs.NewFieldInvalid(label, value)) - } else if !util.IsDNSSubdomain(value) { - allErrs = append(allErrs, errs.NewFieldInvalid(label, value)) - } - return allErrs -}