Remove leaking logs from CLI proxy package

viveksyngh commented 3 years ago

As proxy package is used as SDK it should remove all logging and should return those outputs as results from those API

Expected Behaviour

It should not leak logs when SDK are called.

Current Behaviour

It is having some print/log statements like

https://github.com/openfaas/faas-cli/blob/693a50d60c01974c2b5a7c6081f254f385e5b682/proxy/deploy.go#L75

Possible Solution

Remove these print statements

Standardised API results to provide more information to the callers
TODO - Share API format results

Steps to Reproduce (for bugs)

1. 2. 3. 4.

Context

Your Environment

FaaS-CLI version ( Full output from: faas-cli version ):
Docker version ( Full output from: docker version ):
Are you using Docker Swarm (FaaS-swarm ) or Kubernetes (FaaS-netes)?
Operating System and version (e.g. Linux, Windows, MacOS):
Link to your project or a code example to reproduce issue:

alexellis commented 3 years ago

@viveksyngh could you pick this up next please?

viveksyngh commented 3 years ago

Yes

alexellis commented 3 years ago

Today we have:

func (c *Client) DeployFunction(context context.Context, spec *DeployFunctionSpec) int {

But the example on the call (GitHub) looked like:

func (c *Client) DeployFunction(context context.Context, spec *DeployFunctionSpec) (DeployResponse, http.Response, error) {

viveksyngh commented 3 years ago

I was looking into making CLI response consistent and I was thinking of following approach. Every API from SDK can return three output which are as following

Structured Output for that API for e.g. For list Functions it be a ListFunctionOutput
A Response struct which wraps HTTP response object as well as additional information from SDK for advance features like pagination, rate limiting
An error which will be basically error message

Response struct looks something as below

type Response struct {
    *http.Response
   // Additional information required
}

For errors I looked at few of the SDK and looks like if there in any response which is not in the range of 200 to 299, then they consider it as an error and respond with error message for example

https://github.com/google/go-github/blob/bf3472827069b02a1a19bab191b2b19471bf3edb/github/github.go#L807

One challenge I found is some of our API's like DeployFunction or DeleteFunction do not return anything in response body. We need to return something for these interfaces to make them consistent with other interfaces. I am not sure what we information we can return, may be we can return empty struct for now and update it later if required.

LucasRoesler commented 3 years ago

@viveksyngh a couple thoughts

a signature like func Operation(req StructureReqType) (StructureResponse, *http.Response, error) seems good to me, I have seen it in other places before.
your comment about error range (ie not in 200--299) seems valid, i have also seen the same elsewhere
I don't agree that all methods need a structured response, for example func Delete(req DeleteRequest) (*http.Response, error) seems fine to me. In context of the entire SDK, it should be clear that this has an empty response.

Overall, I would support/approve your proposal.

For the sake of historical completeness. There are two other options I have seen.

A. I have also seen is having both the structured Response types and a structured Error type that implement


type Responser interface { 
    GetResponse() *http.Response
}

so that you can embed the raw http.Response pointer inside the structured types (as a private field). This allows the SDK to only return structured types while still making the http.Response available.

The advantage is that the SDK is less noisy for the happy path use cases. Most of the time you don't need the http.Response object at all, the response or the error already provide enough information. In the faas-cli, I dont think we would use the http.Response object very much so that most usages would look like

function, _, err := sdk.Operation(req)

Embedding the http.Response into the structured types means you get back to

function, err := sdk.Operation(req)

// oh no i need the raw response to inspect an header
resp := function.Respone()

There are two downside. First, of course, is that accessing the http.Response is not obvious. But this can be resolved by documentation. The second is that the http.Response pointer is being passed around a lot more than you probably want.

This leads to the other option I have send

B. To alleviate both downsides from option (A), we can create *Response objects that contain two fields the structured data and the raw response object

type FunctionResponse struct {
    Function types.FunctionDeployment
    Response *http.Response
}

It is very clear that this is wrapping a structured payload and that it provides access to the raw Response object. It is also not going to be a valid input for any other sdk method.

This also does something that you might like, it gives you something to always return, even for Delete requests, for example

type DeleteResponse struct {
    Response *http.Response
}

alexellis commented 3 years ago

Could we get a couple of brief scenarios showing happy path / sad path / error and the populated values for:

res, httpRes, err := sdk.Deploy()

happy = 200 deploys OK sad = 401 - not allowed to access the namespace error = can't open HTTP connection or timed out

From @LucasRoesler - consider using a "typed error" such as FunctionNotFound or NamespaceNotAllowed

Can we take any inspiration from the K8s API? https://pkg.go.dev/k8s.io/apimachinery/pkg/api/errors Such as func IsNotFound(err error) bool

viveksyngh commented 3 years ago

Response format for different scenarios.

res, httpRes, err := sdk.Deploy()

res -> will be an instance of DeployResponse or nil

type DeployResponse struct {
…
}

httpResponse -> instance of Response

type Response struct {
 *http.Response
}

err -> Error is an instance error or an type which implement error interface. For type error we can use an struct which wraps error message and also implements error interface.

For example

type struct StatusError {
    Reason string  // Reasons are constants which will be used with different Status codes or Errors
    Message string // Message to be returned
    …
}

func (e *StatusError) Error() string {
    return e.Message
}

Then we can also define some method as below which unwrap an error and check if the reason is matching with corresponding Reason message

func IsUnauthorized(err error) {
   return ResonForError(err) == StatusReasonUauthorized
}

Happy Path with 200

res -> is type of DeployResponse with relevant data httpRes -> struct with HTTP response object wrapped err -> nil

401 with Unauthorized

res -> nil httpRes -> struct with HTTP response object wrapped err -> err

IsUnauthorized(err) -- will return true in this case

3.can't open HTTP connection or timed out

res -> nil httpRes -> struct with HTTP response object wrapped err -> err

Is this case err will be StatusError with Reason as StatusReasonUnknown

alexellis commented 3 years ago

Discussed on the call with the contributors group. Please can you go ahead and do a PR for the one method you were working on - deploy/update?

alexellis commented 3 years ago

Let's make sure the middle parameter httpRes isn't wrapped. We don't need that today because we have nothing additional to add, so stdlib is less surprising for consumers.

alexellis commented 3 years ago

@Waterdrips do you want to pick up the secrets changes?

Waterdrips commented 3 years ago

@Waterdrips do you want to pick up the secrets changes?

PR Raised #885

LucasRoesler commented 3 years ago

@alexellis @Waterdrips and @utsavanand2 continuing from the zoom discussion yesterday

I had a thought this morning about the 3 legged response from the sdk, I actually think it introduces an awkwardness around when to read the res.Body:

if the response is a success, then we need to read the payload into a struct and then close the body. So the user need to be careful to not touch the res.Body again
if there is an non-success (but not a http client error), then we need to make sure we do not read the body, otherwise when the user goes to debug the response body, they reader will be at EOF, so no more content to read and if we have read it we will close it, so actually they will get an error if they try to read on a closed Reader.
if we do not read the body on non-success, then the caller needs to be responsible for closing the Body but there is one more case, a marshalling error, in this case we are reading the body and will close it. In this case, the user can not read the body to see why it failed. We would have to create a custom error that wraps the bytes to make this accessible to them

There are of course ways to handle all of this, but we probably need to be very clear in the documentation about how to actually work with the response object: sometimes you read it, sometimes you don't, and sometimes you need to check the error for the body

We can say this is ok, but I think it is worth considering if we like this DX

The alternative that we discussed, a typed error OpenFaaSError that contains the body, headers, and status code means that there are exactly two flows

success, use the returned struct
error, check the error for the content

in both cases, the body is already handled correctly.

alexellis commented 3 years ago

@viveksyngh what are your thoughts on what Lucas said about the body, closing it and making it available to the end user?

alexellis commented 3 years ago

@LucasRoesler this is something that I hadn't considered. Can someone look into the way the github-go library does this? Since we've been looking at it as a sort of example of how to do things.

alexellis commented 3 years ago

The example for creating a repo ignores the response:

https://github.com/google/go-github/blob/master/example/newrepo/main.go#L44

Here's the create repo function:

https://github.com/google/go-github/blob/edaee9aa26ea0b2fedd961398433b8940b2afef1/github/repos.go#L337

Which calls "Do" and it closes the body, then caches a copy for reading at a later time if needed:

https://github.com/google/go-github/blob/edaee9aa26ea0b2fedd961398433b8940b2afef1/github/github.go#L608

Either an io.Writer can be passed in, or a typed JSON object for decoding the response.

viveksyngh commented 3 years ago

I looked closely at the example and implementation of the go-github library.

To fix issue issues mentioned by @LucasRoesler

we can read the response body, check for the errors and try to parse the response body. After that , we can rewrite the response body, so that It can be read by the consumer of the SDK again. This is how it has been also, handled by the go-github library as well.

This https://github.com/google/go-github/blob/edaee9aa26ea0b2fedd961398433b8940b2afef1/github/github.go#L911

But looks like in case of success it does not rewrite the response body, rather takes input (io.Writer or an interface) from the consumer and parses the response body.

https://github.com/google/go-github/blob/edaee9aa26ea0b2fedd961398433b8940b2afef1/github/github.go#L609

However, I would like community members to decide and have consensus on which way we want to implement this, have 3 responses or embed these details in errors.

In my opinion, we would also need to provide additional documentation if we embed http related information in the error object that, what informations we are embedding and how can someone consume it. Providing an http response object is more intuitive.

LucasRoesler commented 3 years ago

Most people seem happy with the 3 return values instead of 2. Go with that.

openfaas / faas-cli