search

openfaas / faas-netes

Serverless Functions For Kubernetes
https://www.openfaas.com
MIT License
2.12k stars 472 forks source link

POST system/functions 500 instead of 40x #958

Closed francisdb closed 2 years ago

francisdb commented 2 years ago

Expected Behaviour

The api returns 400 for invalid requests

Current Behaviour

Bad requests to system/functions return 500 instead of 400's

Issues we have seen return 500

Are you a GitHub Sponsor (Yes/No?)

No, but waylay.io is a openfaas pro customer

List All Possible Solutions and Workarounds

Openfaas does proper input validation and returns 400

workaround: Do the validation yourself before calling openfaas, eg call system/namespaces to do the namespace check

Which Solution Do You Recommend?

Openfaas does proper input validation and returns 400

Steps to Reproduce (for bugs)

  1. POST system/functions with json that is incomplete / points to the wrong namespace

Context

For the missing namespace this is logged on the gateway before returning 500

  | 2021-11-10 11:16:15 | 2021/11/10 10:16:15 unable create Deployment: namespaces "foobar" not found

Your Environment

ghcr.io/openfaas/gateway:0.20.8 ghcr.io/openfaas/faas-netes:0.12.18

MacOS

await client.post('system/functions', {
      json: deployOptions,
    })
alexellis commented 2 years ago

Hi @francisdb thanks for your suggestions about validation when users do not provide required fields to the API.

We could take a look into this for you, but will need a specific set of operations with expected and actual responses, so I'm asking for more detail.

Here's an example:

Give us first of all, what we must have set up before we run the test.

Then the command you use (curl is best for us both to use as a common method)

curl -i -X POST --data-binary '{
  "serviceName": "francis-fn",
  "image": "...."
}' http://127.0.0.1:8080/system/functions

If you could number your tests, we'll then be able to respond to each independently.

Alex

LucasRoesler commented 2 years ago

@alexellis At Contiamo we have been using (and pretty happy with) ozzo-validation https://github.com/go-ozzo/ozzo-validation for awhile. it should be pretty easy to add this to the request parsing.

If you wanted to go one step further, we even implemented codegen from OpenAPI v3 that will generate models with ozzo-validation based on the spec. https://github.com/contiamo/openapi-generator-go which will probably require cleaning and adding more data to the spec, but this can be seen as a bonus because it makes the spec more accurate and easier for others to then codegen from in other languages. The openapi-geneator-go does both router and model generation, there is a flag for models only (obviously the router is not needed in openfaas)

alexellis commented 2 years ago

@francisdb I know the answer to this, but whoever looks at this issue will definitely not know:

Are you using faas-netes (controller) or the operator? Perhaps give us the flags that you use to install like --set operator.create=true

It matters because faas-netes may have this validation already and if we test with a different configuration to what you're using then we may not be able to repro.

Alex

francisdb commented 2 years ago

@alexellis you probably know better than me then as I am a user of the system, not the manager of it. But maybe @OcamsRazor can help me out here?

francisdb commented 2 years ago

Looks like some of these have already improved And our client implementation does not try to read 500 bodies. In general we expect 500 errors to be generic and non-fixable by the user. We expect details for those not being shown to the user as they might contain sensitive info.

The question is a bit do you suggest to always read the body and use string find clues to know what is wrong or would you suggest something else?

might be a bit nit picking, accepts invalid image reference

curl -i -X POST --data-binary '{
  "service": "francis-fn",
  "image": "...."
}' http://127.0.0.1:8080/system/functions
HTTP/1.1 202 Accepted
Content-Length: 0
Date: Tue, 17 May 2022 11:26:29 GMT
image

500 instead of 409 Conflict

call above a second time

curl -i -X POST --data-binary '{
  "service": "francis-fn",
  "image": "...."
}' http://127.0.0.1:8080/system/functions
HTTP/1.1 500 Internal Server Error
Content-Length: 71
Content-Type: text/plain; charset=utf-8
Date: Tue, 17 May 2022 11:00:49 GMT
X-Content-Type-Options: nosniff

unable create Deployment: deployments.apps "francis-fn" already exists

500 instead of 400

curl -i -X POST --data-binary '{
  "service": "francis-fn-2",
  "image": "",
  "namespace": "missing"
}' http://127.0.0.1:8080/system/functions
HTTP/1.1 500 Internal Server Error
Content-Length: 57
Content-Type: text/plain; charset=utf-8
Date: Tue, 17 May 2022 11:07:29 GMT
X-Content-Type-Options: nosniff

unable create Deployment: namespaces "missing" not found

500 instead of 400

curl -i -X POST --data-binary '{
  "service": "francis-fn-2",
  "image": null
}' http://127.0.0.1:8080/system/functions
HTTP/1.1 500 Internal Server Error
Content-Length: 124
Content-Type: text/plain; charset=utf-8
Date: Tue, 17 May 2022 11:06:04 GMT
X-Content-Type-Options: nosniff

unable create Deployment: Deployment.apps "francis-fn-2" is invalid: spec.template.spec.containers[0].image: Required value

Looks like these are already better validated but the message could be better like the above example

curl -i -X POST --data-binary '{
  "service": null,
  "image": null
}' http://127.0.0.1:8080/system/functions
HTTP/1.1 400 Bad Request
Content-Length: 65
Content-Type: text/plain; charset=utf-8
Date: Tue, 17 May 2022 11:04:59 GMT
X-Content-Type-Options: nosniff

validation failed: () must be a valid DNS entry for service name

400 with no body at all

curl -i -X DELETE --data-binary '{
  "service": "francis-fn2",
  "image": "...."
}' http://127.0.0.1:8080/system/functions
HTTP/1.1 400 Bad Request
Content-Length: 0
Date: Tue, 17 May 2022 11:36:08 GMT

Same for invalid json body, compared the modified POST example above correctly returns 400 failed to unmarshal request: unexpected end of JSON input

francisdb commented 2 years ago

Also these requests expect a json body but return plain text responses, even with -H "Accept: application/json"

OcamsRazor commented 2 years ago

@alexellis you probably know better than me then as I am a user of the system, not the manager of it. But maybe @OcamsRazor can help me out here?

We are running faas-netes, not the operator

alexellis commented 2 years ago

@francisdb thanks for providing the specific test cases you need, along with your configuration. We need this data for any kind of issue you or the team raise in the future.

The errors that you're seeing with a 500 error are from the Kubernetes Go client, not from our code. We can apply additional validation in the HTTP handler before letting it get that far, then you will see a bad request HTTP header and more friendly messages.

I suspect that you're seeing this now because you're going directly to the OpenFaaS API, and most of our users use the CLI.

All OpenFaaS REST inputs are expected to be JSON, however responses are always in text AFAIK.

@welteki when trying out these scenarios, it sounds like we'd want to test with operator.create=false since there are two different sets of HTTP handlers depending on which is being used.

alexellis commented 2 years ago

@francisdb we are starting with required field validation for image and service.

Some of the cases you mention are edge cases, like "no JSON body at all" - do you expect to be sending that kind of request often, or is this issue more about improving the experience when building a new integration with openfaas?

Or are you exposing the OpenFaaS API directly to your customers, who are in response getting confused when they have an invalid request?

Of course we would all love to get to perfection, but from a pragmatic point of view, we'd like to solve the immediate problem you're running into.

Alex

alexellis commented 2 years ago

We've released a new version of faas-netes Pro and CE to cover the changes requested:

We'll await a response on the above from Waylay before doing any more on this.

https://github.com/openfaasltd/faas-netes/pkgs/container/faas-netes https://github.com/openfaas/faas-netes/pkgs/container/faas-netes

Alex

francisdb commented 2 years ago

We don't expect to send an empty body :-) I was just trying all kinds of things.

It was just in the past when creating our client and updating it after issues that we found some things that could be improved.

Thanks for adding the validation, it's a good start, will create more specific issues later if there are more unexpected things.

alexellis commented 2 years ago

We appreciate the feedback 👍