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

Remove leaking logs from SDK #868

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions commands/deploy.go
Original file line number Diff line number Diff line change
@@ -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 {
4 changes: 2 additions & 2 deletions commands/describe.go
Original file line number Diff line number Diff line change
@@ -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
13 changes: 13 additions & 0 deletions commands/errors.go
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A generic error message functions which can be used in all commands to print error message for generic APIError like 403, 500 etc

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
}
4 changes: 2 additions & 2 deletions commands/list.go
Original file line number Diff line number Diff line change
@@ -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" {
30 changes: 30 additions & 0 deletions proxy/client.go
Original file line number Diff line number Diff line change
@@ -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 = ioutil.NopCloser(bytes.NewBuffer(data))
}
}
return err
}
77 changes: 39 additions & 38 deletions proxy/deploy.go
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of checking types error.

// 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
}
62 changes: 34 additions & 28 deletions proxy/deploy_test.go
Original file line number Diff line number Diff line change
@@ -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 {
270 changes: 270 additions & 0 deletions proxy/errors.go
Original file line number Diff line number Diff line change
@@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

Could it be http.StatusUnauthorized? https://golang.org/src/net/http/status.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can but we will have challenge in solving two problems, if we are not using StatusReason

  1. When we will have same http status code being returned for different errors. I think we are not doing that right now but will be helpful in future.

  2. Since we are only having typed errors for only handful of http status codes, so if API server returns an HTTP response code for which we don't have typed error, we are returning that as UnknownError embedding HTTP Response code and reason. We are checking for UnknownError based on HTTP response code. Please go through this file, https://github.com/openfaas/faas-cli/pull/868/files#diff-25e38af31c2b62e171bb1b4e9a70546d19aa8fbd180d435435814962e2912600

If we remove StatusReason, to figure out UnknownError we will have to check for list of all HTTP status for which we have typed error.


// 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
}
39 changes: 12 additions & 27 deletions proxy/list.go
Original file line number Diff line number Diff line change
@@ -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
}
8 changes: 3 additions & 5 deletions proxy/list_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}