From a87570f805f9b1f22e4c26834706c4670f179c8e 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 +++- proxy/deploy.go | 76 +++++++------- proxy/deploy_test.go | 62 ++++++------ proxy/errors.go | 229 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 318 insertions(+), 70 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/proxy/deploy.go b/proxy/deploy.go index b85e4e46d..326ab60cc 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 spec.Update == true && statusCode == http.StatusNotFound { + 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 && 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,21 @@ 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 + errMessage := fmt.Sprintln("Is OpenFaaS deployed? Do you need to specify the --gateway flag?") + errMessage += fmt.Sprintln(err) + return res, NewUnknown(errMessage, 0) } - if res.Body != nil { - defer res.Body.Close() - } - - 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..de1190edf --- /dev/null +++ b/proxy/errors.go @@ -0,0 +1,229 @@ +package proxy + +import ( + "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 { + if resp.Body != nil { + defer resp.Body.Close() + } + + respBytes, err := ioutil.ReadAll(resp.Body) + if err != nil { + return err + } + + message := string(respBytes) + + switch resp.StatusCode { + + case http.StatusInternalServerError: + return NewInternalServer(message) + + case http.StatusUnauthorized: + return NewInternalServer(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: + if c := resp.StatusCode; 200 <= c && c <= 299 { + return nil + } + + return NewUnknown(message, resp.StatusCode) + } +} + +func IsUnknown(err error) bool { + return ReasonForError(err) == StatusReasonUnknown +} + +func IsNotFound(err error) bool { + return ReasonForError(err) == StatusReasonNotFound +} + +func ReasonForError(err error) StatusReason { + var e *APIError + + if errors.As(err, &e) { + return e.Reason + } + + return StatusReasonUnknown +}