diff --git a/pkg/api/rest/create_test.go b/pkg/api/rest/create_test.go index 88414b0dcaf6f..bb2be35245ca2 100644 --- a/pkg/api/rest/create_test.go +++ b/pkg/api/rest/create_test.go @@ -17,12 +17,10 @@ limitations under the License. package rest import ( - "reflect" "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" - "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" ) func TestCheckGeneratedNameError(t *testing.T) { @@ -41,75 +39,3 @@ func TestCheckGeneratedNameError(t *testing.T) { t.Errorf("expected try again later error: %v", err) } } - -func TestBeforeCreate(t *testing.T) { - failures := []runtime.Object{ - &api.Service{}, - &api.Service{ - ObjectMeta: api.ObjectMeta{ - Name: "foo", - Namespace: "#$%%invalid", - }, - }, - &api.Service{ - ObjectMeta: api.ObjectMeta{ - Name: "##&*(&invalid", - Namespace: api.NamespaceDefault, - }, - }, - } - for _, test := range failures { - ctx := api.NewDefaultContext() - err := BeforeCreate(Services, ctx, test) - if err == nil { - t.Errorf("unexpected non-error for %v", test) - } - } - - obj := &api.ReplicationController{ - ObjectMeta: api.ObjectMeta{ - Name: "foo", - Namespace: api.NamespaceDefault, - }, - Spec: api.ReplicationControllerSpec{ - Selector: map[string]string{"name": "foo"}, - Template: &api.PodTemplateSpec{ - ObjectMeta: api.ObjectMeta{ - Labels: map[string]string{ - "name": "foo", - }, - }, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Name: "foo", - Image: "foo", - ImagePullPolicy: api.PullAlways, - }, - }, - RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}}, - DNSPolicy: api.DNSDefault, - }, - }, - }, - Status: api.ReplicationControllerStatus{ - Replicas: 3, - }, - } - ctx := api.NewDefaultContext() - err := BeforeCreate(ReplicationControllers, ctx, obj) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if !reflect.DeepEqual(obj.Status, api.ReplicationControllerStatus{}) { - t.Errorf("status was not cleared as expected.") - } - if obj.Name != "foo" || obj.Namespace != api.NamespaceDefault { - t.Errorf("unexpected object metadata: %v", obj.ObjectMeta) - } - - obj.Spec.Replicas = -1 - if err := BeforeCreate(ReplicationControllers, ctx, obj); err == nil { - t.Errorf("unexpected non-error for invalid replication controller.") - } -} diff --git a/pkg/api/rest/types.go b/pkg/api/rest/types.go index 025282b99854e..3373b3ebb5b98 100644 --- a/pkg/api/rest/types.go +++ b/pkg/api/rest/types.go @@ -44,34 +44,6 @@ func AllFuncs(fns ...ObjectFunc) ObjectFunc { } } -// rcStrategy implements behavior for Replication Controllers. -// TODO: move to a replicationcontroller specific package. -type rcStrategy struct { - runtime.ObjectTyper - api.NameGenerator -} - -// ReplicationControllers is the default logic that applies when creating and updating Replication Controller -// objects. -var ReplicationControllers RESTCreateStrategy = rcStrategy{api.Scheme, api.SimpleNameGenerator} - -// NamespaceScoped is true for replication controllers. -func (rcStrategy) NamespaceScoped() bool { - return true -} - -// ResetBeforeCreate clears fields that are not allowed to be set by end users on creation. -func (rcStrategy) ResetBeforeCreate(obj runtime.Object) { - controller := obj.(*api.ReplicationController) - controller.Status = api.ReplicationControllerStatus{} -} - -// Validate validates a new replication controller. -func (rcStrategy) Validate(obj runtime.Object) errors.ValidationErrorList { - controller := obj.(*api.ReplicationController) - return validation.ValidateReplicationController(controller) -} - // svcStrategy implements behavior for Services // TODO: move to a service specific package. type svcStrategy struct { diff --git a/pkg/registry/controller/rest.go b/pkg/registry/controller/rest.go index ec986fda76ff6..d75f2fdca7368 100644 --- a/pkg/registry/controller/rest.go +++ b/pkg/registry/controller/rest.go @@ -29,6 +29,43 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/watch" ) +// rcStrategy implements verification logic for Replication Controllers. +type rcStrategy struct { + runtime.ObjectTyper + api.NameGenerator +} + +// Strategy is the default logic that applies when creating and updating Replication Controller objects. +var Strategy = rcStrategy{api.Scheme, api.SimpleNameGenerator} + +// NamespaceScoped returns true because all Replication Controllers need to be within a namespace. +func (rcStrategy) NamespaceScoped() bool { + return true +} + +// ResetBeforeCreate clears the status of a replication controller before creation. +func (rcStrategy) ResetBeforeCreate(obj runtime.Object) { + controller := obj.(*api.ReplicationController) + controller.Status = api.ReplicationControllerStatus{} +} + +// Validate validates a new replication controller. +func (rcStrategy) Validate(obj runtime.Object) errors.ValidationErrorList { + controller := obj.(*api.ReplicationController) + return validation.ValidateReplicationController(controller) +} + +// AllowCreateOnUpdate is false for replication controllers; this means a POST is +// needed to create one. +func (rcStrategy) AllowCreateOnUpdate() bool { + return false +} + +// ValidateUpdate is the default update validation for an end user. +func (rcStrategy) ValidateUpdate(obj, old runtime.Object) errors.ValidationErrorList { + return validation.ValidateReplicationControllerUpdate(old.(*api.ReplicationController), obj.(*api.ReplicationController)) +} + // PodLister is anything that knows how to list pods. type PodLister interface { ListPods(ctx api.Context, labels labels.Selector) (*api.PodList, error) @@ -38,6 +75,7 @@ type PodLister interface { type REST struct { registry Registry podLister PodLister + strategy rcStrategy } // NewREST returns a new apiserver.RESTStorage for the given registry and PodLister. @@ -45,23 +83,25 @@ func NewREST(registry Registry, podLister PodLister) *REST { return &REST{ registry: registry, podLister: podLister, + strategy: Strategy, } } -// Create registers the given ReplicationController. +// Create registers the given ReplicationController with the system, +// which eventually leads to the controller manager acting on its behalf. func (rs *REST) Create(ctx api.Context, obj runtime.Object) (runtime.Object, error) { controller, ok := obj.(*api.ReplicationController) if !ok { return nil, fmt.Errorf("not a replication controller: %#v", obj) } - if err := rest.BeforeCreate(rest.ReplicationControllers, ctx, obj); err != nil { + if err := rest.BeforeCreate(rs.strategy, ctx, obj); err != nil { return nil, err } out, err := rs.registry.CreateController(ctx, controller) if err != nil { - err = rest.CheckGeneratedNameError(rest.ReplicationControllers, err, controller) + err = rest.CheckGeneratedNameError(rs.strategy, err, controller) } return out, err } @@ -115,11 +155,12 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (runtime.Object, boo if !ok { return nil, false, fmt.Errorf("not a replication controller: %#v", obj) } - if !api.ValidNamespace(ctx, &controller.ObjectMeta) { - return nil, false, errors.NewConflict("controller", controller.Namespace, fmt.Errorf("Controller.Namespace does not match the provided context")) + existingController, err := rs.registry.GetController(ctx, controller.Name) + if err != nil { + return nil, false, err } - if errs := validation.ValidateReplicationController(controller); len(errs) > 0 { - return nil, false, errors.NewInvalid("replicationController", controller.Name, errs) + if err := rest.BeforeUpdate(rs.strategy, ctx, controller, existingController); err != nil { + return nil, false, err } out, err := rs.registry.UpdateController(ctx, controller) return out, false, err diff --git a/pkg/registry/controller/rest_test.go b/pkg/registry/controller/rest_test.go index 48c672379b8a7..70782d22b3703 100644 --- a/pkg/registry/controller/rest_test.go +++ b/pkg/registry/controller/rest_test.go @@ -20,17 +20,20 @@ import ( "encoding/json" "fmt" "io/ioutil" + "reflect" "strings" "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/rest" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/rest/resttest" _ "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1" "github.com/GoogleCloudPlatform/kubernetes/pkg/fields" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest" + "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" ) func TestListControllersError(t *testing.T) { @@ -258,6 +261,7 @@ func TestCreateController(t *testing.T) { storage := REST{ registry: &mockRegistry, podLister: &mockPodRegistry, + strategy: Strategy, } controller := &api.ReplicationController{ ObjectMeta: api.ObjectMeta{Name: "test"}, @@ -287,6 +291,7 @@ func TestControllerStorageValidatesCreate(t *testing.T) { storage := REST{ registry: &mockRegistry, podLister: nil, + strategy: Strategy, } failureCases := map[string]api.ReplicationController{ "empty ID": { @@ -312,34 +317,71 @@ func TestControllerStorageValidatesCreate(t *testing.T) { } } -func TestControllerStorageValidatesUpdate(t *testing.T) { - mockRegistry := registrytest.ControllerRegistry{} +func TestControllerValidatesUpdate(t *testing.T) { + mockRegistry := registrytest.ControllerRegistry{ + Controllers: &api.ReplicationControllerList{}, + } storage := REST{ registry: &mockRegistry, podLister: nil, + strategy: Strategy, } - failureCases := map[string]api.ReplicationController{ - "empty ID": { - ObjectMeta: api.ObjectMeta{Name: ""}, - Spec: api.ReplicationControllerSpec{ - Selector: map[string]string{"bar": "baz"}, - }, + + var validControllerSpec = api.ReplicationControllerSpec{ + Selector: validPodTemplate.Spec.Labels, + Template: &validPodTemplate.Spec, + } + + var validController = api.ReplicationController{ + ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "default"}, + Spec: validControllerSpec, + } + + ctx := api.NewDefaultContext() + storage.Create(ctx, &validController) + ns := "newNamespace" + + updaters := []func(rc api.ReplicationController) (runtime.Object, bool, error){ + func(rc api.ReplicationController) (runtime.Object, bool, error) { + rc.UID = "newUID" + return storage.Update(ctx, &rc) }, - "empty selector": { - ObjectMeta: api.ObjectMeta{Name: "abc"}, - Spec: api.ReplicationControllerSpec{}, + func(rc api.ReplicationController) (runtime.Object, bool, error) { + rc.Name = "" + return storage.Update(ctx, &rc) + }, + func(rc api.ReplicationController) (runtime.Object, bool, error) { + rc.Namespace = ns + return storage.Update(api.WithNamespace(ctx, ns), &rc) + }, + func(rc api.ReplicationController) (runtime.Object, bool, error) { + rc.Spec.Selector = map[string]string{} + return storage.Update(ctx, &rc) }, } - ctx := api.NewDefaultContext() - for _, failureCase := range failureCases { - c, created, err := storage.Update(ctx, &failureCase) - if c != nil || created { + for _, u := range updaters { + c, updated, err := u(validController) + if c != nil || updated { t.Errorf("Expected nil object and not created") } if !errors.IsInvalid(err) { t.Errorf("Expected to get an invalid resource error, got %v", err) } } + // The update should fail if the namespace on the controller is set to something + // other than the namespace on the given context, even if the namespace on the + // controller is valid. + c, updated, err := storage.Update(api.WithNamespace(ctx, ns), &validController) + if c != nil || updated { + t.Errorf("Expected nil object and not created") + } + errSubString := "namespace" + if err == nil { + t.Errorf("Expected an error, but we didn't get one") + } else if !errors.IsBadRequest(err) || + strings.Index(err.Error(), errSubString) == -1 { + t.Errorf("Expected a Bad Request error with the sub string '%s', got %v", errSubString, err) + } } type fakePodLister struct { @@ -380,7 +422,7 @@ func TestCreateControllerWithGeneratedName(t *testing.T) { // TODO: remove, covered by TestCreate func TestCreateControllerWithConflictingNamespace(t *testing.T) { - storage := REST{} + storage := REST{strategy: Strategy} controller := &api.ReplicationController{ ObjectMeta: api.ObjectMeta{Name: "test", Namespace: "not-default"}, } @@ -390,28 +432,12 @@ func TestCreateControllerWithConflictingNamespace(t *testing.T) { if channel != nil { t.Error("Expected a nil channel, but we got a value") } + errSubString := "namespace" if err == nil { t.Errorf("Expected an error, but we didn't get one") - } else if strings.Contains(err.Error(), "Controller.Namespace does not match the provided context") { - t.Errorf("Expected 'Controller.Namespace does not match the provided context' error, got '%v'", err.Error()) - } -} - -func TestUpdateControllerWithConflictingNamespace(t *testing.T) { - storage := REST{} - controller := &api.ReplicationController{ - ObjectMeta: api.ObjectMeta{Name: "test", Namespace: "not-default"}, - } - - ctx := api.NewDefaultContext() - obj, created, err := storage.Update(ctx, controller) - if obj != nil || created { - t.Error("Expected a nil object, but we got a value or created was true") - } - if err == nil { - t.Errorf("Expected an error, but we didn't get one") - } else if strings.Index(err.Error(), "Controller.Namespace does not match the provided context") == -1 { - t.Errorf("Expected 'Controller.Namespace does not match the provided context' error, got '%v'", err.Error()) + } else if !errors.IsBadRequest(err) || + strings.Index(err.Error(), errSubString) == -1 { + t.Errorf("Expected a Bad Request error with the sub string '%s', got %v", errSubString, err) } } @@ -437,3 +463,75 @@ func TestCreate(t *testing.T) { }, ) } + +func TestBeforeCreate(t *testing.T) { + failures := []runtime.Object{ + &api.Service{}, + &api.Service{ + ObjectMeta: api.ObjectMeta{ + Name: "foo", + Namespace: "#$%%invalid", + }, + }, + &api.Service{ + ObjectMeta: api.ObjectMeta{ + Name: "##&*(&invalid", + Namespace: api.NamespaceDefault, + }, + }, + } + for _, test := range failures { + ctx := api.NewDefaultContext() + err := rest.BeforeCreate(rest.Services, ctx, test) + if err == nil { + t.Errorf("unexpected non-error for %v", test) + } + } + + obj := &api.ReplicationController{ + ObjectMeta: api.ObjectMeta{ + Name: "foo", + Namespace: api.NamespaceDefault, + }, + Spec: api.ReplicationControllerSpec{ + Selector: map[string]string{"name": "foo"}, + Template: &api.PodTemplateSpec{ + ObjectMeta: api.ObjectMeta{ + Labels: map[string]string{ + "name": "foo", + }, + }, + Spec: api.PodSpec{ + Containers: []api.Container{ + { + Name: "foo", + Image: "foo", + ImagePullPolicy: api.PullAlways, + }, + }, + RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}}, + DNSPolicy: api.DNSDefault, + }, + }, + }, + Status: api.ReplicationControllerStatus{ + Replicas: 3, + }, + } + ctx := api.NewDefaultContext() + err := rest.BeforeCreate(Strategy, ctx, obj) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if !reflect.DeepEqual(obj.Status, api.ReplicationControllerStatus{}) { + t.Errorf("status was not cleared as expected.") + } + if obj.Name != "foo" || obj.Namespace != api.NamespaceDefault { + t.Errorf("unexpected object metadata: %v", obj.ObjectMeta) + } + + obj.Spec.Replicas = -1 + if err := rest.BeforeCreate(Strategy, ctx, obj); err == nil { + t.Errorf("unexpected non-error for invalid replication controller.") + } +} diff --git a/pkg/registry/registrytest/controller.go b/pkg/registry/registrytest/controller.go index 6fe8e5cb32fa5..8c824502ff2ed 100644 --- a/pkg/registry/registrytest/controller.go +++ b/pkg/registry/registrytest/controller.go @@ -47,12 +47,22 @@ func (r *ControllerRegistry) ListControllers(ctx api.Context) (*api.ReplicationC func (r *ControllerRegistry) GetController(ctx api.Context, ID string) (*api.ReplicationController, error) { r.Lock() defer r.Unlock() + if r.Controllers != nil { + for _, rc := range r.Controllers.Items { + if ID == rc.Name { + return &r.Controllers.Items[0], r.Err + } + } + } return &api.ReplicationController{}, r.Err } func (r *ControllerRegistry) CreateController(ctx api.Context, controller *api.ReplicationController) (*api.ReplicationController, error) { r.Lock() defer r.Unlock() + if r.Controllers != nil { + r.Controllers.Items = append(r.Controllers.Items, *controller) + } return controller, r.Err }