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

Expand the generic registry #4419

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
4 changes: 2 additions & 2 deletions pkg/api/rest/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,12 @@ func CheckGeneratedNameError(strategy RESTCreateStrategy, err error, obj runtime
}

// objectMetaAndKind retrieves kind and ObjectMeta from a runtime object, or returns an error.
func objectMetaAndKind(strategy RESTCreateStrategy, obj runtime.Object) (*api.ObjectMeta, string, error) {
func objectMetaAndKind(typer runtime.ObjectTyper, obj runtime.Object) (*api.ObjectMeta, string, error) {
objectMeta, err := api.ObjectMetaFor(obj)
if err != nil {
return nil, "", errors.NewInternalError(err)
}
_, kind, err := strategy.ObjectVersionAndKind(obj)
_, kind, err := typer.ObjectVersionAndKind(obj)
if err != nil {
return nil, "", errors.NewInternalError(err)
}
Expand Down
37 changes: 35 additions & 2 deletions pkg/api/rest/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,27 @@ import (
"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
)

// ObjectFunc is a function to act on a given object. An error may be returned
// if the hook cannot be completed. An ObjectFunc may transform the provided
// object.
type ObjectFunc func(obj runtime.Object) error

// AllFuncs returns an ObjectFunc that attempts to run all of the provided functions
// in order, returning early if there are any errors.
func AllFuncs(fns ...ObjectFunc) ObjectFunc {
return func(obj runtime.Object) error {
for _, fn := range fns {
if fn == nil {
continue
}
if err := fn(obj); err != nil {
return err
}
}
return nil
}
}

// rcStrategy implements behavior for Replication Controllers.
// TODO: move to a replicationcontroller specific package.
type rcStrategy struct {
Expand Down Expand Up @@ -60,7 +81,7 @@ type podStrategy struct {

// Pods is the default logic that applies when creating and updating Pod
// objects.
var Pods RESTCreateStrategy = podStrategy{api.Scheme, api.SimpleNameGenerator}
var Pods = podStrategy{api.Scheme, api.SimpleNameGenerator}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Strategy still the right name for this? It's an ObjectTyper and NameGenerator? Why are these separate, anyhow?

Could we please adopt a convention for declaring what interfaces receivers implement? #2992

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, aren't these included in RESTCreateStrategy directly? Should RESTCreateStrategy use this strategy? Does RESTUpdateStrategy invoke RESTCreateStrategy for the create-on-put case?

I'm not getting the big picture of the organization we're moving towards.

Don't get me wrong -- this is better than what we have. But, I'm still not seeing the forest through the trees.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my later pull I moved this class to pkg/registry/pod/types.go as "pod.Strategy". So this is the default implementation of the basic pod strategy. I can add the comments here to make that clear.

RESTCreate and RESTUpdate are separate. There would be a different RESTUpdateStrategy for updating the pods as a regular user (PUT /pods/foo) vs updating status (PUT /pods/foo/status -> RESTStatusUpdateStrategy). The separate strategy implementation there is just so we can guarantee type safety (the reflective implementation doesn't buy us a lot).

smarterclayton@dc4c9da#diff-189461deaff2db4ed8975a0bd74f9b4fR43

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add that convention into #4248 since it's moved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to better names - Creater or Updater might confuse people with apiserver.RESTCreater, but there is a parallel.

----- Original Message -----

@@ -60,7 +81,7 @@ type podStrategy struct {

// Pods is the default logic that applies when creating and updating Pod
// objects.
-var Pods RESTCreateStrategy = podStrategy{api.Scheme,
api.SimpleNameGenerator}
+var Pods = podStrategy{api.Scheme, api.SimpleNameGenerator}

Is Strategy still the right name for this? It's an ObjectTyper and
NameGenerator? Why are these separate, anyhow?

Could we please adopt a convention for declaring what interfaces receivers
implement? #2992


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/4419/files#r24680126

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'll look at the later pulls next.


// NamespaceScoped is true for pods.
func (podStrategy) NamespaceScoped() bool {
Expand All @@ -70,7 +91,9 @@ func (podStrategy) NamespaceScoped() bool {
// ResetBeforeCreate clears fields that are not allowed to be set by end users on creation.
func (podStrategy) ResetBeforeCreate(obj runtime.Object) {
pod := obj.(*api.Pod)
pod.Status = api.PodStatus{}
pod.Status = api.PodStatus{
Phase: api.PodPending,
}
}

// Validate validates a new pod.
Expand All @@ -79,6 +102,16 @@ func (podStrategy) Validate(obj runtime.Object) errors.ValidationErrorList {
return validation.ValidatePod(pod)
}

// AllowCreateOnUpdate is false for pods.
func (podStrategy) AllowCreateOnUpdate() bool {
return false
}

// ValidateUpdate is the default update validation for an end user.
func (podStrategy) ValidateUpdate(obj, old runtime.Object) errors.ValidationErrorList {
return validation.ValidatePodUpdate(obj.(*api.Pod), old.(*api.Pod))
}

// svcStrategy implements behavior for Services
// TODO: move to a service specific package.
type svcStrategy struct {
Expand Down
59 changes: 59 additions & 0 deletions pkg/api/rest/update.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
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 rest

import (
"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
)

// RESTUpdateStrategy defines the minimum validation, accepted input, and
// name generation behavior to update an object that follows Kubernetes
// API conventions. A resource may have many UpdateStrategies, depending on
// the call pattern in use.
type RESTUpdateStrategy interface {
runtime.ObjectTyper
// NamespaceScoped returns true if the object must be within a namespace.
NamespaceScoped() bool
// AllowCreateOnUpdate returns true if the object can be created by a PUT.
AllowCreateOnUpdate() bool
// ValidateUpdate is invoked after default fields in the object have been filled in before
// the object is persisted.
ValidateUpdate(obj, old runtime.Object) errors.ValidationErrorList
}

// BeforeUpdate ensures that common operations for all resources are performed on update. It only returns
// errors that can be converted to api.Status. It will invoke update validation with the provided existing
// and updated objects.
func BeforeUpdate(strategy RESTUpdateStrategy, ctx api.Context, obj, old runtime.Object) error {
objectMeta, kind, kerr := objectMetaAndKind(strategy, obj)
if kerr != nil {
return kerr
}
if strategy.NamespaceScoped() {
if !api.ValidNamespace(ctx, objectMeta) {
return errors.NewBadRequest("the namespace of the provided object does not match the namespace sent on the request")
}
} else {
objectMeta.Namespace = api.NamespaceNone
}
if errs := strategy.ValidateUpdate(obj, old); len(errs) > 0 {
return errors.NewInvalid(kind, objectMeta.Name, errs)
}
return nil
}
4 changes: 3 additions & 1 deletion pkg/api/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,8 @@ func ValidatePodSpec(spec *api.PodSpec) errs.ValidationErrorList {
return allErrs
}

// ValidatePodUpdate tests to see if the update is legal
// ValidatePodUpdate tests to see if the update is legal for an end user to make. newPod is updated with fields
// that cannot be changed.
func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {
allErrs := errs.ValidationErrorList{}

Expand All @@ -584,6 +585,7 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {
allErrs = append(allErrs, errs.NewFieldInvalid("spec.containers", newPod.Spec.Containers, "some fields are immutable"))
}

newPod.Status = oldPod.Status
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably could be moved into 4248 - it's still correct, but 4248 is where the reset really needs to happen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind.

return allErrs
}

Expand Down
6 changes: 5 additions & 1 deletion pkg/apiserver/resthandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,11 @@ func CreateResource(r RESTCreater, namespaceFn ResourceNamespaceFunc, linkFn Lin
}

result, err := finishRequest(timeout, func() (runtime.Object, error) {
return r.Create(ctx, obj)
out, err := r.Create(ctx, obj)
if status, ok := out.(*api.Status); ok && err == nil && status.Code == 0 {
status.Code = http.StatusCreated
}
return out, err
})
if err != nil {
errorJSON(err, codec, w)
Expand Down
27 changes: 3 additions & 24 deletions pkg/registry/event/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package event

import (
"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
etcderr "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors/etcd"
"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/generic"
etcdgeneric "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/generic/etcd"
"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
Expand All @@ -28,28 +27,6 @@ import (
// registry implements custom changes to generic.Etcd.
type registry struct {
*etcdgeneric.Etcd
ttl uint64
}

// Create stores the object with a ttl, so that events don't stay in the system forever.
func (r registry) Create(ctx api.Context, id string, obj runtime.Object) error {
key, err := r.Etcd.KeyFunc(ctx, id)
if err != nil {
return err
}
err = r.Etcd.Helper.CreateObj(key, obj, r.ttl)
return etcderr.InterpretCreateError(err, r.Etcd.EndpointName, id)
}

// Update replaces an existing instance of the object, and sets a ttl so that the event
// doesn't stay in the system forever.
func (r registry) Update(ctx api.Context, id string, obj runtime.Object) error {
key, err := r.Etcd.KeyFunc(ctx, id)
if err != nil {
return err
}
err = r.Etcd.Helper.SetObj(key, obj, r.ttl)
return etcderr.InterpretUpdateError(err, r.Etcd.EndpointName, id)
}

// NewEtcdRegistry returns a registry which will store Events in the given
Expand All @@ -66,8 +43,10 @@ func NewEtcdRegistry(h tools.EtcdHelper, ttl uint64) generic.Registry {
KeyFunc: func(ctx api.Context, id string) (string, error) {
return etcdgeneric.NamespaceKeyFunc(ctx, "/registry/events", id)
},
TTLFunc: func(runtime.Object, bool) (uint64, error) {
return ttl, nil
},
Helper: h,
},
ttl: ttl,
}
}
4 changes: 2 additions & 2 deletions pkg/registry/event/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func TestEventCreate(t *testing.T) {
for name, item := range table {
fakeClient, registry := NewTestEventEtcdRegistry(t)
fakeClient.Data[path] = item.existing
err := registry.Create(ctx, key, item.toCreate)
err := registry.CreateWithName(ctx, key, item.toCreate)
if !item.errOK(err) {
t.Errorf("%v: unexpected error: %v", name, err)
}
Expand Down Expand Up @@ -200,7 +200,7 @@ func TestEventUpdate(t *testing.T) {
for name, item := range table {
fakeClient, registry := NewTestEventEtcdRegistry(t)
fakeClient.Data[path] = item.existing
err := registry.Update(ctx, key, item.toUpdate)
err := registry.UpdateWithName(ctx, key, item.toUpdate)
if !item.errOK(err) {
t.Errorf("%v: unexpected error: %v", name, err)
}
Expand Down
10 changes: 5 additions & 5 deletions pkg/registry/event/rest.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (runtime.Object, err
}
api.FillObjectMetaSystemFields(ctx, &event.ObjectMeta)

err := rs.registry.Create(ctx, event.Name, event)
err := rs.registry.CreateWithName(ctx, event.Name, event)
if err != nil {
return nil, err
}
Expand All @@ -79,24 +79,24 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (runtime.Object, boo
}
api.FillObjectMetaSystemFields(ctx, &event.ObjectMeta)

err := rs.registry.Update(ctx, event.Name, event)
err := rs.registry.UpdateWithName(ctx, event.Name, event)
if err != nil {
return nil, false, err
}
out, err := rs.registry.Get(ctx, event.Name)
return out, false, err
}

func (rs *REST) Delete(ctx api.Context, id string) (runtime.Object, error) {
obj, err := rs.registry.Get(ctx, id)
func (rs *REST) Delete(ctx api.Context, name string) (runtime.Object, error) {
obj, err := rs.registry.Get(ctx, name)
if err != nil {
return nil, err
}
_, ok := obj.(*api.Event)
if !ok {
return nil, fmt.Errorf("invalid object type")
}
return &api.Status{Status: api.StatusSuccess}, rs.registry.Delete(ctx, id)
return rs.registry.Delete(ctx, name)
}

func (rs *REST) Get(ctx api.Context, id string) (runtime.Object, error) {
Expand Down