From 0c9edf9ecfd7a23e012ca3b26409ff5c1ed74f16 Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Fri, 30 Apr 2021 12:03:21 +0530 Subject: [PATCH] Remove leaking log from deploy This commit removing lekaing logs from deploy API in SDK. It also returns typed errors from API resoponse and allows as to check for some custom errors which functions like `IsNotFound` or `IsUnauthorized`. This also updates DeployFuntion API to return a typed response and `http.Response`. Signed-off-by: Vivek Singh --- commands/deploy.go | 21 +++- commands/describe.go | 4 +- commands/errors.go | 13 +++ commands/list.go | 4 +- proxy/client.go | 30 +++++ proxy/deploy.go | 77 ++++++------ proxy/deploy_test.go | 62 +++++----- proxy/errors.go | 270 +++++++++++++++++++++++++++++++++++++++++++ proxy/list.go | 39 ++----- proxy/list_test.go | 8 +- 10 files changed, 421 insertions(+), 107 deletions(-) create mode 100644 proxy/errors.go diff --git a/commands/deploy.go b/commands/deploy.go index c7217a898..8a51399cd 100644 --- a/commands/deploy.go +++ b/commands/deploy.go @@ -276,9 +276,15 @@ Error: %s`, fprocessErr.Error()) if msg := checkTLSInsecure(services.Provider.GatewayURL, deploySpec.TLSInsecure); len(msg) > 0 { fmt.Println(msg) } - statusCode := proxyClient.DeployFunction(ctx, deploySpec) - if badStatusCode(statusCode) { - failedStatusCodes[k] = statusCode + dRes, res, err := proxyClient.DeployFunction(ctx, deploySpec) + + if err == nil { + fmt.Println(dRes.Message) + fmt.Printf("URL: %s\n\n", dRes.URL) + } + + if badStatusCode(res.StatusCode) { + failedStatusCodes[k] = res.StatusCode } } } else { @@ -375,9 +381,14 @@ func deployImage( fmt.Println(msg) } - statusCode = client.DeployFunction(ctx, deploySpec) + dRes, res, err := client.DeployFunction(ctx, deploySpec) + if err != nil { + return res.StatusCode, err + } - return statusCode, nil + fmt.Println(dRes.Message) + fmt.Printf("URL: %s\n\n", dRes.URL) + return res.StatusCode, nil } func mergeSlice(values []string, overlay []string) []string { diff --git a/commands/describe.go b/commands/describe.go index 048763021..01c1b9278 100644 --- a/commands/describe.go +++ b/commands/describe.go @@ -82,9 +82,9 @@ func runDescribe(cmd *cobra.Command, args []string) error { } //To get correct value for invocation count from /system/functions endpoint - functionList, err := cliClient.ListFunctions(ctx, functionNamespace) + functionList, _, err := cliClient.ListFunctions(ctx, functionNamespace) if err != nil { - return err + return actionableErrorMessage(err) } var invocationCount int diff --git a/commands/errors.go b/commands/errors.go index 25ff7f6af..c493626ea 100644 --- a/commands/errors.go +++ b/commands/errors.go @@ -4,7 +4,10 @@ package commands import ( + "fmt" "strings" + + "github.com/openfaas/faas-cli/proxy" ) const ( @@ -24,3 +27,13 @@ func checkTLSInsecure(gateway string, tlsInsecure bool) string { } return "" } + +//actionableErrorMessage print actionable error message based on APIError check +func actionableErrorMessage(err error) error { + if proxy.IsUnknown(err) { + return fmt.Errorf("server returned unexpected status response: %s", err.Error()) + } else if proxy.IsUnauthorized(err) { + return fmt.Errorf("unauthorized access, run \"faas-cli login\" to setup authentication for this server") + } + return err +} diff --git a/commands/list.go b/commands/list.go index 28a52538d..dba7ba116 100644 --- a/commands/list.go +++ b/commands/list.go @@ -73,9 +73,9 @@ func runList(cmd *cobra.Command, args []string) error { return err } - functions, err := proxyClient.ListFunctions(context.Background(), functionNamespace) + functions, _, err := proxyClient.ListFunctions(context.Background(), functionNamespace) if err != nil { - return err + return actionableErrorMessage(err) } if sortOrder == "name" { diff --git a/proxy/client.go b/proxy/client.go index bb32e387c..d1d0d23cf 100644 --- a/proxy/client.go +++ b/proxy/client.go @@ -1,9 +1,12 @@ package proxy import ( + "bytes" "context" + "encoding/json" "fmt" "io" + "io/ioutil" "net/http" "net/http/httputil" "net/url" @@ -129,3 +132,30 @@ func addQueryParams(u string, params map[string]string) (string, error) { func (c *Client) AddCheckRedirect(checkRedirect func(*http.Request, []*http.Request) error) { c.httpClient.CheckRedirect = checkRedirect } + +// parseResponse function parses HTTP response body into a typed struct +// or copies to io.Writer object. If it fails to decode response body +// then it re-construct it so that it can be read later +func parseResponse(res *http.Response, v interface{}) error { + var err error + defer res.Body.Close() + + switch v := v.(type) { + case nil: + case io.Writer: + _, err = io.Copy(v, res.Body) + default: + data, err := ioutil.ReadAll(res.Body) + if err == io.EOF { + err = nil // ignore EOF errors caused by empty response body + } + + decErr := json.Unmarshal(data, v) + if decErr != nil { + err = decErr + // In case of JSON decode error, re-construct response body + res.Body = io.NopCloser(bytes.NewBuffer(data)) + } + } + return err +} diff --git a/proxy/deploy.go b/proxy/deploy.go index b85e4e46d..867376bac 100644 --- a/proxy/deploy.go +++ b/proxy/deploy.go @@ -8,7 +8,6 @@ import ( "context" "encoding/json" "fmt" - "io/ioutil" "net/http" "time" @@ -57,29 +56,48 @@ func generateFuncStr(spec *DeployFunctionSpec) string { return spec.FunctionName } +type DeployResponse struct { + Message string + RollingUpdate bool + URL string +} + // DeployFunction first tries to deploy a function and if it exists will then attempt // a rolling update. Warnings are suppressed for the second API call (if required.) -func (c *Client) DeployFunction(context context.Context, spec *DeployFunctionSpec) int { +func (c *Client) DeployFunction(context context.Context, spec *DeployFunctionSpec) (*DeployResponse, *http.Response, error) { rollingUpdateInfo := fmt.Sprintf("Function %s already exists, attempting rolling-update.", spec.FunctionName) - statusCode, deployOutput := c.deploy(context, spec, spec.Update) + res, err := c.deploy(context, spec, spec.Update) + + if err != nil && IsUnknown(err) { + return nil, res, err + } + + var deployResponse DeployResponse + if err == nil { + deployResponse.Message = fmt.Sprintf("Deployed. %s.", res.Status) + deployResponse.URL = fmt.Sprintf("%s/function/%s", c.GatewayURL.String(), generateFuncStr(spec)) + } - if spec.Update == true && statusCode == http.StatusNotFound { + if spec.Update == true && IsNotFound(err) { // Re-run the function with update=false + res, err = c.deploy(context, spec, false) + if err == nil { + deployResponse.Message = fmt.Sprintf("Deployed. %s.", res.Status) + deployResponse.URL = fmt.Sprintf("%s/function/%s", c.GatewayURL.String(), generateFuncStr(spec)) + } + + } else if res.StatusCode == http.StatusOK { + deployResponse.Message += rollingUpdateInfo + deployResponse.RollingUpdate = true - statusCode, deployOutput = c.deploy(context, spec, false) - } else if statusCode == http.StatusOK { - fmt.Println(rollingUpdateInfo) } - fmt.Println() - fmt.Println(deployOutput) - return statusCode + + return &deployResponse, res, err } // deploy a function to an OpenFaaS gateway over REST -func (c *Client) deploy(context context.Context, spec *DeployFunctionSpec, update bool) (int, string) { - - var deployOutput string +func (c *Client) deploy(context context.Context, spec *DeployFunctionSpec, update bool) (*http.Response, error) { // Need to alter Gateway to allow nil/empty string as fprocess, to avoid this repetition. var fprocessTemplate string if len(spec.FProcess) > 0 { @@ -146,37 +164,20 @@ func (c *Client) deploy(context context.Context, spec *DeployFunctionSpec, updat request, err = c.newRequest(method, "/system/functions", reader) if err != nil { - deployOutput += fmt.Sprintln(err) - return http.StatusInternalServerError, deployOutput + return nil, err } res, err := c.doRequest(context, request) if err != nil { - deployOutput += fmt.Sprintln("Is OpenFaaS deployed? Do you need to specify the --gateway flag?") - deployOutput += fmt.Sprintln(err) - return http.StatusInternalServerError, deployOutput - } - - if res.Body != nil { - defer res.Body.Close() + errMessage := fmt.Sprintln("Is OpenFaaS deployed? Do you need to specify the --gateway flag?") + errMessage += fmt.Sprintln(err) + return res, NewUnknown(errMessage, 0) } - switch res.StatusCode { - case http.StatusOK, http.StatusCreated, http.StatusAccepted: - deployOutput += fmt.Sprintf("Deployed. %s.\n", res.Status) - - deployedURL := fmt.Sprintf("URL: %s/function/%s", c.GatewayURL.String(), generateFuncStr(spec)) - deployOutput += fmt.Sprintln(deployedURL) - case http.StatusUnauthorized: - deployOutput += fmt.Sprintln("unauthorized access, run \"faas-cli login\" to setup authentication for this server") - - default: - bytesOut, err := ioutil.ReadAll(res.Body) - if err == nil { - deployOutput += fmt.Sprintf("Unexpected status: %d, message: %s\n", res.StatusCode, string(bytesOut)) - } + err = checkForAPIError(res) + if err != nil { + return res, err } - - return res.StatusCode, deployOutput + return res, nil } diff --git a/proxy/deploy_test.go b/proxy/deploy_test.go index dfd6bd98b..87418240a 100644 --- a/proxy/deploy_test.go +++ b/proxy/deploy_test.go @@ -21,7 +21,8 @@ type deployProxyTest struct { mockServerResponses []int replace bool update bool - expectedOutput string + expectedMessage string + statusCode int } func runDeployProxyTest(t *testing.T, deployTest deployProxyTest) { @@ -34,32 +35,34 @@ func runDeployProxyTest(t *testing.T, deployTest deployProxyTest) { cliAuth := NewTestAuth(nil) proxyClient, _ := NewClient(cliAuth, s.URL, nil, &defaultCommandTimeout) - stdout := test.CaptureStdout(func() { - proxyClient.DeployFunction(context.TODO(), &DeployFunctionSpec{ - "fprocess", - "function", - "image", - "dXNlcjpwYXNzd29yZA==", - "language", - deployTest.replace, - nil, - "network", - []string{}, - deployTest.update, - []string{}, - map[string]string{}, - map[string]string{}, - FunctionResourceRequest{}, - false, - tlsNoVerify, - "", - "", - }) + dRes, httpRes, _ := proxyClient.DeployFunction(context.TODO(), &DeployFunctionSpec{ + "fprocess", + "function", + "image", + "dXNlcjpwYXNzd29yZA==", + "language", + deployTest.replace, + nil, + "network", + []string{}, + deployTest.update, + []string{}, + map[string]string{}, + map[string]string{}, + FunctionResourceRequest{}, + false, + tlsNoVerify, + "", + "", }) - r := regexp.MustCompile(deployTest.expectedOutput) - if !r.MatchString(stdout) { - t.Fatalf("Output not matched: %s", stdout) + if httpRes.StatusCode != deployTest.statusCode { + t.Fatalf("StatuCode did not match. expected: %d, got: %d", deployTest.statusCode, httpRes.StatusCode) + } + + r := regexp.MustCompile(deployTest.expectedMessage) + if !r.MatchString(dRes.Message) { + t.Fatalf("Output not matched: %s", dRes.Message) } } @@ -70,21 +73,24 @@ func Test_RunDeployProxyTests(t *testing.T) { mockServerResponses: []int{http.StatusOK, http.StatusOK}, replace: true, update: false, - expectedOutput: `(?m:Deployed)`, + statusCode: http.StatusOK, + expectedMessage: `(?m:Deployed)`, }, { title: "404_Deploy", mockServerResponses: []int{http.StatusOK, http.StatusNotFound}, replace: true, update: false, - expectedOutput: `(?m:Unexpected status: 404)`, + statusCode: http.StatusNotFound, + expectedMessage: "", }, { title: "UpdateFailedDeployed", mockServerResponses: []int{http.StatusNotFound, http.StatusOK}, replace: false, update: true, - expectedOutput: `(?m:Deployed)`, + statusCode: http.StatusOK, + expectedMessage: `(?m:Deployed)`, }, } for _, tst := range deployProxyTests { diff --git a/proxy/errors.go b/proxy/errors.go new file mode 100644 index 000000000..47538f4f7 --- /dev/null +++ b/proxy/errors.go @@ -0,0 +1,270 @@ +package proxy + +import ( + "bytes" + "errors" + "io/ioutil" + "net/http" +) + +type APIError struct { + Reason StatusReason + Code int + Status string + Message string +} + +func (e *APIError) Error() string { + return e.Message +} + +const ( + StatusSuccess = "Success" + StatusFailure = "Failure" +) + +type StatusReason string + +const ( + StatusReasonUnknown StatusReason = "Unknown" + + // Status code 500 + StatusReasonInternalError StatusReason = "InternalError" + + // Status code 401 + StatusReasonUnauthorized StatusReason = "Unauthorized" + + // Status code 403 + StatusReasonForbidden StatusReason = "Forbidden" + + // Status code 404 + StatusReasonNotFound StatusReason = "NotFound" + + // Status code 409 + StatusReasonConflict StatusReason = "Conflict" + + // Status code 504 + StatusReasonGatewayTimeout StatusReason = "Timeout" + + // Status code 429 + StatusReasonTooManyRquests StatusReason = "TooManyRequests" + + // Status code 400 + StatusReasonBadRequest StatusReason = "BadRequest" + + // Status code 405 + StatusReasonMethodNotAllowed StatusReason = "MethodNotAllowed" + + // Status code 503 + StatusReasonServiceUnavailable StatusReason = "ServiceUnavailable" +) + +func NewInternalServer(message string) *APIError { + return &APIError{ + Reason: StatusReasonInternalError, + Code: http.StatusInternalServerError, + Status: StatusFailure, + Message: message, + } +} + +func NewUnauthorized(message string) *APIError { + return &APIError{ + Reason: StatusReasonUnauthorized, + Code: http.StatusUnauthorized, + Status: StatusFailure, + Message: message, + } +} + +func NewForbidden(message string) *APIError { + return &APIError{ + Reason: StatusReasonForbidden, + Code: http.StatusForbidden, + Status: StatusFailure, + Message: message, + } +} + +func NewNotFound(message string) *APIError { + return &APIError{ + Reason: StatusReasonNotFound, + Code: http.StatusNotFound, + Status: StatusFailure, + Message: message, + } +} + +func NewConflict(message string) *APIError { + return &APIError{ + Reason: StatusReasonConflict, + Code: http.StatusConflict, + Status: StatusFailure, + Message: message, + } +} + +func NewGatewayTimeout(message string) *APIError { + return &APIError{ + Reason: StatusReasonGatewayTimeout, + Code: http.StatusGatewayTimeout, + Status: StatusFailure, + Message: message, + } +} + +func NewTooManyRequests(message string) *APIError { + return &APIError{ + Reason: StatusReasonTooManyRquests, + Code: http.StatusTooManyRequests, + Status: StatusFailure, + Message: message, + } +} + +func NewBadRequest(message string) *APIError { + return &APIError{ + Reason: StatusReasonBadRequest, + Code: http.StatusBadRequest, + Status: StatusFailure, + Message: message, + } +} + +func NewMethodNotAllowed(message string) *APIError { + return &APIError{ + Reason: StatusReasonMethodNotAllowed, + Code: http.StatusMethodNotAllowed, + Status: StatusFailure, + Message: message, + } +} + +func NewServiceUnavailable(message string) *APIError { + return &APIError{ + Reason: StatusReasonServiceUnavailable, + Code: http.StatusServiceUnavailable, + Status: StatusFailure, + Message: message, + } +} + +func NewUnknown(message string, statusCode int) *APIError { + return &APIError{ + Reason: StatusReasonUnknown, + Code: statusCode, + Message: message, + Status: StatusFailure, + } +} + +func checkForAPIError(resp *http.Response) error { + //HTTP status in 2xx range are success + if c := resp.StatusCode; 200 <= c && c <= 299 { + return nil + } + + if resp.Body != nil { + defer resp.Body.Close() + } + + respBytes, err := ioutil.ReadAll(resp.Body) + + // Re-construct response body in case of error + resp.Body = ioutil.NopCloser(bytes.NewBuffer(respBytes)) + if err != nil { + return err + } + + message := string(respBytes) + + switch resp.StatusCode { + + case http.StatusInternalServerError: + return NewInternalServer(message) + + case http.StatusUnauthorized: + return NewUnauthorized(message) + + case http.StatusForbidden: + return NewForbidden(message) + + case http.StatusNotFound: + return NewNotFound(message) + + case http.StatusConflict: + return NewConflict(message) + + case http.StatusGatewayTimeout: + return NewGatewayTimeout(message) + + case http.StatusTooManyRequests: + return NewTooManyRequests(message) + + case http.StatusMethodNotAllowed: + return NewMethodNotAllowed(message) + + case http.StatusBadRequest: + return NewBadRequest(message) + + case http.StatusServiceUnavailable: + return NewServiceUnavailable(message) + + default: + return NewUnknown(message, resp.StatusCode) + } +} + +func IsUnknown(err error) bool { + return ReasonForError(err) == StatusReasonUnknown +} + +func IsNotFound(err error) bool { + return ReasonForError(err) == StatusReasonNotFound +} + +func IsUnauthorized(err error) bool { + return ReasonForError(err) == StatusReasonUnauthorized +} + +func IsBadRequest(err error) bool { + return ReasonForError(err) == StatusReasonBadRequest +} + +func IsForbidden(err error) bool { + return ReasonForError(err) == StatusReasonForbidden +} + +func IsInternalServerError(err error) bool { + return ReasonForError(err) == StatusReasonInternalError +} + +func IsServiceUnavailable(err error) bool { + return ReasonForError(err) == StatusReasonServiceUnavailable +} + +func IsMethodNotAllowed(err error) bool { + return ReasonForError(err) == StatusReasonMethodNotAllowed +} + +func IsTooManyRequests(err error) bool { + return ReasonForError(err) == StatusReasonTooManyRquests +} + +func IsGatewayTimeout(err error) bool { + return ReasonForError(err) == StatusReasonGatewayTimeout +} + +func IsConflict(err error) bool { + return ReasonForError(err) == StatusReasonConflict +} + +func ReasonForError(err error) StatusReason { + var e *APIError + + if errors.As(err, &e) { + return e.Reason + } + + return StatusReasonUnknown +} diff --git a/proxy/list.go b/proxy/list.go index 5681f9853..5419f4974 100644 --- a/proxy/list.go +++ b/proxy/list.go @@ -5,17 +5,15 @@ package proxy import ( "context" - "encoding/json" "fmt" - "io/ioutil" "net/http" types "github.com/openfaas/faas-provider/types" ) // ListFunctions list deployed functions -func (c *Client) ListFunctions(ctx context.Context, namespace string) ([]types.FunctionStatus, error) { +func (c *Client) ListFunctions(ctx context.Context, namespace string) ([]types.FunctionStatus, *http.Response, error) { var ( results []types.FunctionStatus listEndpoint string @@ -30,42 +28,29 @@ func (c *Client) ListFunctions(ctx context.Context, namespace string) ([]types.F if len(namespace) > 0 { listEndpoint, err = addQueryParams(listEndpoint, map[string]string{namespaceKey: namespace}) if err != nil { - return results, err + return results, nil, err } } getRequest, err := c.newRequest(http.MethodGet, listEndpoint, nil) if err != nil { - return nil, fmt.Errorf("cannot connect to OpenFaaS on URL: %s", c.GatewayURL.String()) + return results, nil, fmt.Errorf("cannot connect to OpenFaaS on URL: %s", c.GatewayURL.String()) } res, err := c.doRequest(ctx, getRequest) if err != nil { - return nil, fmt.Errorf("cannot connect to OpenFaaS on URL: %s", c.GatewayURL.String()) + return results, res, fmt.Errorf("cannot connect to OpenFaaS on URL: %s", c.GatewayURL.String()) } - if res.Body != nil { - defer res.Body.Close() + err = checkForAPIError(res) + if err != nil { + return results, res, err } - switch res.StatusCode { - case http.StatusOK: - - bytesOut, err := ioutil.ReadAll(res.Body) - if err != nil { - return nil, fmt.Errorf("cannot read result from OpenFaaS on URL: %s", c.GatewayURL.String()) - } - jsonErr := json.Unmarshal(bytesOut, &results) - if jsonErr != nil { - return nil, fmt.Errorf("cannot parse result from OpenFaaS on URL: %s\n%s", c.GatewayURL.String(), jsonErr.Error()) - } - case http.StatusUnauthorized: - return nil, fmt.Errorf("unauthorized access, run \"faas-cli login\" to setup authentication for this server") - default: - bytesOut, err := ioutil.ReadAll(res.Body) - if err == nil { - return nil, fmt.Errorf("server returned unexpected status code: %d - %s", res.StatusCode, string(bytesOut)) - } + err = parseResponse(res, &results) + if err != nil { + return results, res, err } - return results, nil + + return results, res, nil } diff --git a/proxy/list_test.go b/proxy/list_test.go index 67990e213..6c720c31b 100644 --- a/proxy/list_test.go +++ b/proxy/list_test.go @@ -7,7 +7,6 @@ import ( "context" "net/http" "reflect" - "regexp" "testing" @@ -44,7 +43,7 @@ func Test_ListFunctions(t *testing.T) { cliAuth := NewTestAuth(nil) client, _ := NewClient(cliAuth, s.URL, nil, &defaultCommandTimeout) - result, err := client.ListFunctions(context.Background(), "") + result, _, err := client.ListFunctions(context.Background(), "") if err != nil { t.Fatalf("Error returned: %s", err) @@ -61,14 +60,13 @@ func Test_ListFunctions_Not200(t *testing.T) { cliAuth := NewTestAuth(nil) client, _ := NewClient(cliAuth, s.URL, nil, &defaultCommandTimeout) - _, err := client.ListFunctions(context.Background(), "") + _, _, err := client.ListFunctions(context.Background(), "") if err == nil { t.Fatalf("Error was not returned") } - r := regexp.MustCompile(`(?m:server returned unexpected status code)`) - if !r.MatchString(err.Error()) { + if !IsBadRequest(err) { t.Fatalf("Error not matched: %s", err) } }