Merge pull request kubernetes#3404 from smarterclayton/method_not_all…

…owed

Allow RESTStorage objects to not implement methods

pkg/api/errors/errors.go

@@ -159,6 +159,19 @@ func NewBadRequest(reason string) error {
  }}
 }
 
+// NewMethodNotSupported returns an error indicating the requested action is not supported on this kind.
+func NewMethodNotSupported(kind, action string) error {
+ return &StatusError{api.Status{
+ Status: api.StatusFailure,
+ Code: http.StatusMethodNotAllowed,
+ Reason: api.StatusReasonMethodNotAllowed,
+ Details: &api.StatusDetails{
+ Kind: kind,
+ },
+ Message: fmt.Sprintf("%s is not supported on resources of kind %q", action, kind),
+ }}
+}
+
 // NewInternalError returns an error indicating the item is invalid and cannot be processed.
 func NewInternalError(err error) error {
  return &StatusError{api.Status{
@@ -192,6 +205,12 @@ func IsInvalid(err error) bool {
  return reasonForError(err) == api.StatusReasonInvalid
 }
 
+// IsMethodNotSupported determines if the err is an error which indicates the provided action could not
+// be performed because it is not supported by the server.
+func IsMethodNotSupported(err error) bool {
+ return reasonForError(err) == api.StatusReasonMethodNotAllowed
+}
+
 // IsBadRequest determines if err is an error which indicates that the request is invalid.
 func IsBadRequest(err error) bool {
  return reasonForError(err) == api.StatusReasonBadRequest

pkg/api/errors/errors_test.go

@@ -40,6 +40,12 @@ func TestErrorNew(t *testing.T) {
  if IsInvalid(err) {
  t.Errorf("expected to not be %s", api.StatusReasonInvalid)
  }
+ if IsBadRequest(err) {
+ t.Errorf("expected to not be %s", api.StatusReasonBadRequest)
+ }
+ if IsMethodNotSupported(err) {
+ t.Errorf("expected to not be %s", api.StatusReasonMethodNotAllowed)
+ }
 
  if !IsConflict(NewConflict("test", "2", errors.New("message"))) {
  t.Errorf("expected to be conflict")
@@ -50,6 +56,12 @@ func TestErrorNew(t *testing.T) {
  if !IsInvalid(NewInvalid("test", "2", nil)) {
  t.Errorf("expected to be %s", api.StatusReasonInvalid)
  }
+ if !IsBadRequest(NewBadRequest("reason")) {
+ t.Errorf("expected to be %s", api.StatusReasonBadRequest)
+ }
+ if !IsMethodNotSupported(NewMethodNotSupported("foo", "delete")) {
+ t.Errorf("expected to be %s", api.StatusReasonMethodNotAllowed)
+ }
 }
 
 func TestNewInvalid(t *testing.T) {

pkg/api/types.go

@@ -931,6 +931,11 @@ const (
  // data was invalid. API calls that return BadRequest can never succeed.
  StatusReasonBadRequest StatusReason = "BadRequest"
 
+ // StatusReasonMethodNotAllowed means that the action the client attempted to perform on the
+ // resource was not supported by the code - for instance, attempting to delete a resource that
+ // can only be created. API calls that return MethodNotAllowed can never succeed.
+ StatusReasonMethodNotAllowed StatusReason = "MethodNotAllowed"
+
  // StatusReasonInternalError indicates that an internal error occurred, it is unexpected
  // and the outcome of the call is unknown.
  // Details (optional):

pkg/apiserver/apiserver.go

@@ -129,62 +129,70 @@ func registerResourceHandlers(ws *restful.WebService, version string, path strin
  nameParam := ws.PathParameter("name", "name of the "+kind).DataType("string")
  namespaceParam := ws.PathParameter("namespace", "object name and auth scope, such as for teams and projects").DataType("string")
 
- ws.Route(
- addParamIf(
- ws.POST(path).To(h).
- Doc("create a "+kind).
- Operation("create"+kind).
- Reads(versionedObject), // from the request
- namespaceParam, namespaceScope))
-
- // TODO: This seems like a hack. Add NewList() to storage?
- listKind := kind + "List"
- if _, ok := kinds[listKind]; !ok {
- glog.V(1).Infof("no list type: %v\n", listKind)
+ createRoute := ws.POST(path).To(h).
+ Doc("create a " + kind).
+ Operation("create" + kind).
+ Reads(versionedObject) // from the request
+ addParamIf(createRoute, namespaceParam, namespaceScope)
+ if _, ok := storage.(RESTCreater); ok {
+ ws.Route(createRoute.Reads(versionedObject)) // from the request
  } else {
+ ws.Route(createRoute.Returns(http.StatusMethodNotAllowed, "creating objects is not supported", nil))
+ }
+
+ listRoute := ws.GET(path).To(h).
+ Doc("list objects of kind " + kind).
+ Operation("list" + kind)
+ addParamIf(listRoute, namespaceParam, namespaceScope)
+ if lister, ok := storage.(RESTLister); ok {
+ list := lister.NewList()
+ _, listKind, err := api.Scheme.ObjectVersionAndKind(list)
  versionedListPtr, err := api.Scheme.New(version, listKind)
  if err != nil {
- glog.Errorf("error making list: %v\n", err)
- } else {
- versionedList := indirectArbitraryPointer(versionedListPtr)
- glog.V(3).Infoln("type: ", reflect.TypeOf(versionedList))
- ws.Route(
- addParamIf(
- ws.GET(path).To(h).
- Doc("list objects of kind "+kind).
- Operation("list"+kind).
- Returns(http.StatusOK, "OK", versionedList),
- namespaceParam, namespaceScope))
+ glog.Errorf("error making list object: %v\n", err)
+ return
  }
+ versionedList := indirectArbitraryPointer(versionedListPtr)
+ glog.V(3).Infoln("type: ", reflect.TypeOf(versionedList))
+ ws.Route(listRoute.Returns(http.StatusOK, "OK", versionedList))
+ } else {
+ ws.Route(listRoute.Returns(http.StatusMethodNotAllowed, "listing objects is not supported", nil))
  }
 
- ws.Route(
- addParamIf(
- ws.GET(path+"/{name}").To(h).
- Doc("read the specified "+kind).
- Operation("read"+kind).
- Param(nameParam).
- Writes(versionedObject), // on the response
- namespaceParam, namespaceScope))
+ getRoute := ws.GET(path + "/{name}").To(h).
+ Doc("read the specified " + kind).
+ Operation("read" + kind).
+ Param(nameParam)
+ addParamIf(getRoute, namespaceParam, namespaceScope)
+ if _, ok := storage.(RESTGetter); ok {
+ ws.Route(getRoute.Writes(versionedObject)) // on the response
+ } else {
+ ws.Route(ws.GET(path+"/{name}").To(h).
+ Returns(http.StatusMethodNotAllowed, "reading individual objects is not supported", nil))
+ }
 
- ws.Route(
- addParamIf(
- ws.PUT(path+"/{name}").To(h).
- Doc("update the specified "+kind).
- Operation("update"+kind).
- Param(nameParam).
- Reads(versionedObject), // from the request
- namespaceParam, namespaceScope))
+ updateRoute := ws.PUT(path + "/{name}").To(h).
+ Doc("update the specified " + kind).
+ Operation("update" + kind).
+ Param(nameParam)
+ addParamIf(updateRoute, namespaceParam, namespaceScope)
+ if _, ok := storage.(RESTUpdater); ok {
+ ws.Route(updateRoute.Reads(versionedObject)) // from the request
+ } else {
+ ws.Route(updateRoute.Returns(http.StatusMethodNotAllowed, "updating objects is not supported", nil))
+ }
 
  // TODO: Support PATCH
-
- ws.Route(
- addParamIf(
- ws.DELETE(path+"/{name}").To(h).
- Doc("delete the specified "+kind).
- Operation("delete"+kind).
- Param(nameParam),
- namespaceParam, namespaceScope))
+ deleteRoute := ws.DELETE(path + "/{name}").To(h).
+ Doc("delete the specified " + kind).
+ Operation("delete" + kind).
+ Param(nameParam)
+ addParamIf(deleteRoute, namespaceParam, namespaceScope)
+ if _, ok := storage.(RESTDeleter); ok {
+ ws.Route(deleteRoute)
+ } else {
+ ws.Route(deleteRoute.Returns(http.StatusMethodNotAllowed, "deleting objects is not supported", nil))
+ }
 }
 
 // Adds the given param to the given route builder if shouldAdd is true. Does nothing if shouldAdd is false.

pkg/apiserver/apiserver_test.go

@@ -183,6 +183,10 @@ func (storage *SimpleRESTStorage) New() runtime.Object {
  return &Simple{}
 }
 
+func (storage *SimpleRESTStorage) NewList() runtime.Object {
+ return &SimpleList{}
+}
+
 func (storage *SimpleRESTStorage) Create(ctx api.Context, obj runtime.Object) (<-chan RESTResult, error) {
  storage.created = obj.(*Simple)
  if err := storage.errors["create"]; err != nil {
@@ -288,6 +292,68 @@ func TestNotFound(t *testing.T) {
  }
 }
 
+type UnimplementedRESTStorage struct{}
+
+func (UnimplementedRESTStorage) New() runtime.Object {
+ return &Simple{}
+}
+
+func TestMethodNotAllowed(t *testing.T) {
+ type T struct {
+ Method string
+ Path string
+ }
+ cases := map[string]T{
+ "GET object": {"GET", "/prefix/version/foo/bar"},
+ "GET list": {"GET", "/prefix/version/foo"},
+ "POST list": {"POST", "/prefix/version/foo"},
+ "PUT object": {"PUT", "/prefix/version/foo/bar"},
+ "DELETE object": {"DELETE", "/prefix/version/foo/bar"},
+ //"watch list": {"GET", "/prefix/version/watch/foo"},
+ //"watch object": {"GET", "/prefix/version/watch/foo/bar"},
+ "proxy object": {"GET", "/prefix/version/proxy/foo/bar"},
+ "redirect object": {"GET", "/prefix/version/redirect/foo/bar"},
+ }
+ handler := Handle(map[string]RESTStorage{
+ "foo": UnimplementedRESTStorage{},
+ }, codec, "/prefix", testVersion, selfLinker, admissionControl)
+ server := httptest.NewServer(handler)
+ defer server.Close()
+ client := http.Client{}
+ for k, v := range cases {
+ request, err := http.NewRequest(v.Method, server.URL+v.Path, bytes.NewReader([]byte(`{"kind":"Simple","apiVersion":"version"}`)))
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+
+ response, err := client.Do(request)
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ continue
+ }
+ defer response.Body.Close()
+ data, _ := ioutil.ReadAll(response.Body)
+ t.Logf("resp: %s", string(data))
+ if response.StatusCode != http.StatusMethodNotAllowed {
+ t.Errorf("%s: expected %d for %s, Got %s", k, http.StatusMethodNotAllowed, v.Method, string(data))
+ continue
+ }
+ obj, err := codec.Decode(data)
+ if err != nil {
+ t.Errorf("%s: unexpected decode error: %v", k, err)
+ continue
+ }
+ status, ok := obj.(*api.Status)
+ if !ok {
+ t.Errorf("%s: unexpected object: %#v", k, obj)
+ continue
+ }
+ if status.Reason != api.StatusReasonMethodNotAllowed {
+ t.Errorf("%s: unexpected status: %#v", k, status)
+ }
+ }
+}
+
 func TestVersion(t *testing.T) {
  handler := Handle(map[string]RESTStorage{}, codec, "/prefix", testVersion, selfLinker, admissionControl)
  server := httptest.NewServer(handler)

pkg/apiserver/interfaces.go

@@ -24,28 +24,43 @@ import (
 )
 
 // RESTStorage is a generic interface for RESTful storage services.
-// Resources which are exported to the RESTful API of apiserver need to implement this interface.
+// Resources which are exported to the RESTful API of apiserver need to implement this interface. It is expected
+// that objects may implement any of the REST* interfaces.
+// TODO: implement dynamic introspection (so GenericREST objects can indicate what they implement)
 type RESTStorage interface {
  // New returns an empty object that can be used with Create and Update after request data has been put into it.
  // This object must be a pointer type for use with Codec.DecodeInto([]byte, runtime.Object)
  New() runtime.Object
+}
 
+type RESTLister interface {
+ // NewList returns an empty object that can be used with the List call.
+ // This object must be a pointer type for use with Codec.DecodeInto([]byte, runtime.Object)
+ NewList() runtime.Object
  // List selects resources in the storage which match to the selector.
  List(ctx api.Context, label, field labels.Selector) (runtime.Object, error)
+}
 
+type RESTGetter interface {
  // Get finds a resource in the storage by id and returns it.
  // Although it can return an arbitrary error value, IsNotFound(err) is true for the
  // returned error value err when the specified resource is not found.
  Get(ctx api.Context, id string) (runtime.Object, error)
+}
 
+type RESTDeleter interface {
  // Delete finds a resource in the storage and deletes it.
  // Although it can return an arbitrary error value, IsNotFound(err) is true for the
  // returned error value err when the specified resource is not found.
  Delete(ctx api.Context, id string) (<-chan RESTResult, error)
+}
 
+type RESTCreater interface {
  // Create creates a new version of a resource.
  Create(ctx api.Context, obj runtime.Object) (<-chan RESTResult, error)
+}
 
+type RESTUpdater interface {
  // Update finds a resource in the storage and updates it. Some implementations
  // may allow updates creates the object - they should set the Created flag of
  // the returned RESTResultto true. In the event of an asynchronous error returned

pkg/apiserver/proxy.go

@@ -30,6 +30,7 @@ import (
  "time"
 
  "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+ "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
  "github.com/GoogleCloudPlatform/kubernetes/pkg/httplog"
  "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
  "github.com/GoogleCloudPlatform/kubernetes/pkg/util"
@@ -106,7 +107,7 @@ func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
  redirector, ok := storage.(Redirector)
  if !ok {
  httplog.LogOf(req, w).Addf("'%v' is not a redirector", kind)
- notFound(w, req)
+ errorJSON(errors.NewMethodNotSupported(kind, "proxy"), r.codec, w)
  return
  }
 

pkg/apiserver/redirect.go

@@ -20,6 +20,7 @@ import (
  "net/http"
 
  "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+ "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
  "github.com/GoogleCloudPlatform/kubernetes/pkg/httplog"
  "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
 )
@@ -53,7 +54,7 @@ func (r *RedirectHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
  redirector, ok := storage.(Redirector)
  if !ok {
  httplog.LogOf(req, w).Addf("'%v' is not a redirector", kind)
- notFound(w, req)
+ errorJSON(errors.NewMethodNotSupported(kind, "redirect"), r.codec, w)
  return
  }