search

vmware-archive / kubeless

Kubernetes Native Serverless Framework
https://kubeless.io
Apache License 2.0
6.86k stars 755 forks source link

Add CORS support to python and golang runtimes #934

Closed vallard closed 5 years ago

vallard commented 5 years ago

similar to issue #250 it would be nice to have cors support for python or golang. For python, if I could at least have my function return a bottle.Response that would work but the runtime doesn't seem to allow it. Here is my code: 

def list(event, context):
  return bottle.Response("foo", 200, {"Access-Control-Allow-Origin" : "*"})
andresmgot commented 5 years ago

Hi @vallard, that'd be great. Right now we don't have the resources to work on that, would you mind giving it a try? We are open to pull requests :)

imishravmw commented 5 years ago

@andresmgot This fix does not seems to best approach for solving CORS. As sometime a user may not want to enable CORS or one may wants to enable CORS only for a specific domain.

imishravmw commented 5 years ago

Better way to fix CORS support to enable or disable CORS in Ingress resource when Kubeless creates Ingress from HttpTrigger. @andresmgot , do you think this is proper approach. As while creating http trigger, we can give options for enabling cors and domain like below. kubeless trigger http create get-python --function-name get-python --path hello --cors true --cors-domain "*.subdomain.com" We will like to work on this, which will add CORS support for all runtimes.

vallard commented 5 years ago

We would require those flags be something we could put in a serverless config file as well. The default node implementation just enables it by default. The ingress approach though seems pretty good.

There are also issues I have as the default golang only returns a string when we’d like to also return JSON or different status codes other than 200. That is probably for another issue though.

On Oct 29, 2018, at 7:10 AM, imishravmw notifications@github.com wrote:

Better way to fix CORS support to enable or disable CORS in Ingress resource when Kubeless creates Ingress from HttpTrigger. @andresmgot , do you think this is proper approach. As while creating http trigger, we can give options for enabling cors and domain like below. kubeless trigger http create get-python --function-name get-python --path hello --cors true --cors-domain "*.subdomain.com" We will like to work on this, which will add CORS support for all runtimes.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

andresmgot commented 5 years ago

I am not sure how that would work at the ingress level (I am not sure if you can configure that in an ingress object). In the function, if that is a feature you need, I would just define an env var for that purpose. For example ACCESS_CONTROL_ALLOW_ORIGIN, ACCESS_CONTROL_ALLOW_METHODS... In the function, if those variables are not set, we can set the default to "allow all". That way it wouldn't be necessary to add a new flag.

imishravmw commented 5 years ago

We would require those flags be something we could put in a serverless config file as well. The default node implementation just enables it by default. The ingress approach though seems pretty good. There are also issues I have as the default golang only returns a string when we’d like to also return JSON or different status codes other than 200. That is probably for another issue though. …

yes certainly, we need to make changes to serverless to provide that, one draw back in server less is it only provides cors "true" or "false", it should also support cors domain when cors is true.

imishravmw commented 5 years ago

I am not sure how that would work at the ingress level (I am not sure if you can configure that in an ingress object). In the function, if that is a feature you need, I would just define an env var for that purpose. For example ACCESS_CONTROL_ALLOW_ORIGIN, ACCESS_CONTROL_ALLOW_METHODS... In the function, if those variables are not set, we can set the default to "allow all". That way it wouldn't be necessary to add a new flag.

Kubernetes nginx Ingress does support CORS annotations, please refer nginx-ingress-doc. Adding env to function has a drawback, that I might create two HTTP trigger, with one I want CORS, but with another I don't want. That will not be solved by doing only at function level. as CORS is handled by Ingress controller. But certainly we could something like, if cors is defined at function level, that gets applied to all triggers by default and if cors is not set at function level, then one can define at a individual trigger level.

vallard commented 5 years ago

Do you have any target for when you are going to put this PR in? We are getting along fine with our custom images at present but I could submit the changes as suggested by Andres tomorrow for golang and python.

On Tue, Oct 30, 2018 at 2:52 AM imishravmw notifications@github.com wrote:

I am not sure how that would work at the ingress level (I am not sure if you can configure that in an ingress object). In the function, if that is a feature you need, I would just define an env var for that purpose. For example ACCESS_CONTROL_ALLOW_ORIGIN, ACCESS_CONTROL_ALLOW_METHODS... In the function, if those variables are not set, we can set the default to "allow all". That way it wouldn't be necessary to add a new flag.

Kubbernetes nginx Ingress does support CORS annotations, please refer nginx-ingress-doc https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/ . Adding env to function has a drawback, that I might create two HTTP trigger, with one I want CORS, but with another I don't want. That will not be solved by doing only at function level. as CORS is handled by Ingress controller. But certainly we could something like, if cors is defined at function level, that gets applied to all triggers by default and if cors is not set at function level, then one can define at a individual trigger level.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubeless/kubeless/issues/934#issuecomment-434236911, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKVTWhWLebm4FVIaJDK9nfxygo1RzfNks5uqCFCgaJpZM4X7DTQ .

andresmgot commented 5 years ago

@vallard you can go ahead with your changes since it would be useful to handle CORS in the function anyway. As far as I understood we can do it in both places, the function and the Ingress controller. @imishravmw can follow up with further PRs if needed. I would not include the env vars for the moment, just the accept-all policy that we have right now.

imishravmw commented 5 years ago

@andresmgot I will be raising PR for CORS at ingress level , so it will be available for all runtimes by same solution. I am just waiting for some approvals at my org before raising PR.

imishravmw commented 5 years ago

I have got two PR ,one in http-trigger repo https://github.com/kubeless/http-trigger/pull/10 , which needs to get first done to pass another PR which is in kubeless repo https://github.com/kubeless/kubeless/pull/1008 ,

second PR is failing CI checks because it depends upon first PR. Even first PR https://github.com/kubeless/http-trigger/pull/10 is failing with circleci errors, but not sure why ,can someone help for this. @andresmgot

andresmgot commented 5 years ago

@imishravmw this repo is now using the latest release of the http-triggger with your changes. You can update the PR in this repository.

imishravmw commented 5 years ago

@imishravmw this repo is now using the latest release of the http-triggger with your changes. You can update the PR in this repository.

I could not understand, what you want me to do by saying "update the PR" There is related issue https://github.com/kubeless/kubeless/issues/819 , both of these could be closed.

andresmgot commented 5 years ago

I meant that at that point you could update #1008 (that is now closed). Thanks for your contribution!