Better convey duplicate name errors to client (#116)

Motivation: Creating duplicate resources currently results in a 500
error being displayed by the client, with no information about the true
nature of the failure.

Modifications:
* ResourceStore now converts any errors from the etcd client into
  fission errors, capturing the reason for the error in the case of a
  duplicate key (as ErrorNameExists)
* ErrorNameExists errors are signaled to the client with a
  409 (Conflict) HTTP status
* MakeErrorFromHTTP() now reads the body of the error response to
  retrieve the actual error message instead of using the HTTP status
  message
* the controller client now uses MakeErrorFromHTTP() to centralize
  status code -> error code mapping
* tests for all of the resources now check that duplicate resources are
  reported properly

Result: The client can now provide more context when a duplicate name is
given

related to #112

controller/api_test.go

@@ -36,6 +36,13 @@ var g struct {
 	client *client.Client
 }
 
+func assertNameReuseFails(err error, name string) {
+	assert(err != nil, "recreating "+name+" with same name must fail")
+	fe, ok := err.(fission.Error)
+	assert(ok, "error must be a fission Error")
+	assert(fe.Code == fission.ErrorNameExists, "error must be a name exists error")
+}
+
 func TestFunctionApi(t *testing.T) {
 	log.SetFormatter(&log.TextFormatter{DisableColors: true})
 
@@ -56,6 +63,9 @@ func TestFunctionApi(t *testing.T) {
 	uid1 := m.Uid
 	//log.Printf("Created function %v: %v", m.Name, m.Uid)
 
+	_, err = g.client.FunctionCreate(testFunc)
+	assertNameReuseFails(err, "function")
+
 	code, err := g.client.FunctionGetRaw(m)
 	panicIf(err)
 	assert(string(code) == testFunc.Code, "code from FunctionGetRaw must match created function")
@@ -123,6 +133,9 @@ func TestHTTPTriggerApi(t *testing.T) {
 	panicIf(err)
 	defer g.client.HTTPTriggerDelete(m)
 
+	_, err = g.client.HTTPTriggerCreate(testTrigger)
+	assertNameReuseFails(err, "trigger")
+
 	tr, err := g.client.HTTPTriggerGet(m)
 	panicIf(err)
 	testTrigger.Metadata.Uid = m.Uid
@@ -160,6 +173,9 @@ func TestEnvironmentApi(t *testing.T) {
 	panicIf(err)
 	defer g.client.EnvironmentDelete(m)
 
+	_, err = g.client.EnvironmentCreate(testEnv)
+	assertNameReuseFails(err, "environment")
+
 	tr, err := g.client.EnvironmentGet(m)
 	panicIf(err)
 	testEnv.Metadata.Uid = m.Uid
@@ -205,6 +221,9 @@ func TestWatchApi(t *testing.T) {
 	panicIf(err)
 	defer g.client.WatchDelete(m)
 
+	_, err = g.client.WatchCreate(testWatch)
+	assertNameReuseFails(err, "watch")
+
 	w, err := g.client.WatchGet(m)
 	panicIf(err)
 	testWatch.Metadata.Uid = m.Uid

controller/client/client.go

@@ -80,19 +80,7 @@ func (c *Client) url(relativeUrl string) string {
 
 func (c *Client) handleResponse(resp *http.Response) ([]byte, error) {
 	if resp.StatusCode != 200 {
-		var errCode int
+		return nil, fission.MakeErrorFromHTTP(resp)
-		switch resp.StatusCode {
-		case 403:
-			errCode = fission.ErrorNotAuthorized
-		case 404:
-			errCode = fission.ErrorNotFound
-		case 400:
-			errCode = fission.ErrorInvalidArgument
-		default:
-			errCode = fission.ErrorInternal
-		}
-		return nil, fission.MakeError(errCode,
-			fmt.Sprintf("HTTP error %v", resp.StatusCode))
 	}
 	body, err := ioutil.ReadAll(resp.Body)
 	return body, err

controller/resourceStore.go

@@ -25,6 +25,9 @@ import (
 	"github.com/coreos/etcd/client"
 	"github.com/satori/go.uuid"
 	"golang.org/x/net/context"
+
+	"fmt"
+	"github.com/fission/fission"
 )
 
 type (
@@ -93,7 +96,7 @@ func (rs *ResourceStore) create(r resource) error {
 
 	_, err = rs.KeysAPI.Set(context.Background(), key, string(serialized),
 		&client.SetOptions{PrevExist: client.PrevNoExist})
-	return err
+	return handleEtcdError(err)
 }
 
 func (rs *ResourceStore) read(rkey string, res resource) error {
@@ -105,7 +108,7 @@ func (rs *ResourceStore) read(rkey string, res resource) error {
 
 	resp, err := rs.KeysAPI.Get(context.Background(), key, nil)
 	if err != nil {
-		return err
+		return handleEtcdError(err)
 	}
 	return rs.serializer.deserialize([]byte(resp.Node.Value), res)
 }
@@ -123,13 +126,13 @@ func (rs *ResourceStore) update(r resource) error {
 
 	_, err = rs.KeysAPI.Set(context.Background(), key, string(serialized),
 		&client.SetOptions{PrevExist: client.PrevExist})
-	return err
+	return handleEtcdError(err)
 }
 
 func (rs *ResourceStore) delete(typename, rkey string) error {
 	key := typename + "/" + rkey
 	_, err := rs.KeysAPI.Delete(context.Background(), key, nil) // ignore response
-	return err
+	return handleEtcdError(err)
 }
 
 // getAll finds all entries under key.  If none or found or key
@@ -140,7 +143,7 @@ func (rs *ResourceStore) getAll(key string) ([]string, error) {
 		if client.IsKeyNotFound(err) {
 			return []string{}, nil
 		}
-		return nil, err
+		return nil, handleEtcdError(err)
 	}
 
 	res := make([]string, 0, len(resp.Node.Nodes))
@@ -162,7 +165,7 @@ func (rs *ResourceStore) writeFile(parentKey string, contents []byte) (string, s
 	resp, err := rs.KeysAPI.CreateInOrder(context.Background(), parentKey, uid, nil)
 	if err != nil {
 		_ = rs.FileStore.delete(uid)
-		return "", "", err
+		return "", "", handleEtcdError(err)
 	}
 
 	return resp.Node.Key, uid, nil
@@ -172,7 +175,7 @@ func (rs *ResourceStore) readFile(key string, uid *string) ([]byte, error) {
 	key = "file/" + key
 	resp, err := rs.KeysAPI.Get(context.Background(), key, &client.GetOptions{Sort: true})
 	if err != nil {
-		return nil, err
+		return nil, handleEtcdError(err)
 	}
 
 	if uid == nil {
@@ -194,14 +197,14 @@ func (rs *ResourceStore) readFile(key string, uid *string) ([]byte, error) {
 	}
 
 	contents, err := rs.FileStore.read(*uid)
-	return contents, err
+	return contents, handleEtcdError(err)
 }
 
 func (rs *ResourceStore) deleteFile(key string, uid string) error {
 	key = "file/" + key
 	resp, err := rs.KeysAPI.Get(context.Background(), key, &client.GetOptions{Sort: true})
 	if err != nil {
-		return err
+		return handleEtcdError(err)
 	}
 
 	var node *client.Node
@@ -222,12 +225,12 @@ func (rs *ResourceStore) deleteFile(key string, uid string) error {
 
 	_, err = rs.KeysAPI.Delete(context.Background(), node.Key, nil)
 	if err != nil {
-		return err
+		return handleEtcdError(err)
 	}
 
 	if len(resp.Node.Nodes) == 1 {
 		_, err = rs.KeysAPI.Delete(context.Background(), key, &client.DeleteOptions{Dir: true})
-		return err
+		return handleEtcdError(err)
 	}
 	return nil
 }
@@ -236,7 +239,7 @@ func (rs *ResourceStore) deleteAllFiles(key string) error {
 	key = "file/" + key
 	resp, err := rs.KeysAPI.Get(context.Background(), key, &client.GetOptions{Sort: true})
 	if err != nil {
-		return err
+		return handleEtcdError(err)
 	}
 	for _, u := range resp.Node.Nodes {
 		err = rs.FileStore.delete(u.Value)
@@ -246,9 +249,26 @@ func (rs *ResourceStore) deleteAllFiles(key string) error {
 
 		_, err = rs.KeysAPI.Delete(context.Background(), u.Key, nil)
 		if err != nil {
-			return err
+			return handleEtcdError(err)
 		}
 	}
 	_, err = rs.KeysAPI.Delete(context.Background(), key, &client.DeleteOptions{Dir: true})
-	return err
+	return handleEtcdError(err)
+}
+
+func handleEtcdError(e error) error {
+	ee, ok := e.(client.Error)
+	if !ok {
+		return e
+	}
+	code := fission.ErrorInternal
+	msg := ee.Error()
+
+	//TODO: handle any other etcd error codes we care about
+	switch ee.Code {
+	case client.ErrorCodeNodeExist:
+		code = fission.ErrorNameExists
+		msg = fmt.Sprintf("%v (%v)", ee.Message, ee.Cause)
+	}
+	return fission.MakeError(code, msg)
 }

error.go

@@ -18,7 +18,9 @@ package fission
 
 import (
 	"fmt"
+	"io/ioutil"
 	"net/http"
+	"strings"
 )
 
 func (e Error) Error() string {
@@ -36,29 +38,39 @@ func MakeErrorFromHTTP(resp *http.Response) error {
 
 	var errCode int
 	switch resp.StatusCode {
+	case 400:
+		errCode = ErrorInvalidArgument
 	case 403:
 		errCode = ErrorNotAuthorized
 	case 404:
 		errCode = ErrorNotFound
-	case 400:
+	case 409:
-		errCode = ErrorInvalidArgument
+		errCode = ErrorNameExists
 	default:
 		errCode = ErrorInternal
 	}
-	return MakeError(errCode, resp.Status)
+
+	msg := resp.Status
+	defer resp.Body.Close()
+	body, err := ioutil.ReadAll(resp.Body)
+	if err == nil && len(body) > 0 {
+		msg = strings.TrimSpace(string(body))
+	}
+
+	return MakeError(errCode, msg)
 }
 
 func (err Error) HTTPStatus() int {
 	var code int
 	switch err.Code {
-	case ErrorNotFound:
-		code = 404
 	case ErrorInvalidArgument:
 		code = 400
-	case ErrorNoSpace:
-		code = 500
 	case ErrorNotAuthorized:
 		code = 403
+	case ErrorNotFound:
+		code = 404
+	case ErrorNameExists:
+		code = 409
 	default:
 		code = 500
 	}
@@ -70,8 +82,8 @@ func GetHTTPError(err error) (int, string) {
 	var code int
 	fe, ok := err.(Error)
 	if ok {
-		msg = fe.Message
 		code = fe.HTTPStatus()
+		msg = fe.Message
 	} else {
 		code = 500
 		msg = err.Error()