search

openfaas / faas-netes

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

chart: Ingress Operator's `ingress_namespace` env variable should be configurable for different use-cases #661

Closed aleks-fofanov closed 4 years ago

aleks-fofanov commented 4 years ago

Expected Behaviour

The value of Ingress Operator's ingress_namespace environment variable can be configured through chart values.

Current Behaviour

The value of this env variable is not configurable through chart values.

Possible Solution

Allow to configure ingress_namespace through chart values. I've created a PR that implements this.

Context

As discussed in #651, this value should be configurable through chart values for different use-cases. Some users would want to pass traffic from function ingress to the gateway deployed to core services namespace (default behavior) while the others may prefer to pass it directly to the function svc in functions namespace (bypassGateway option in FunctionIngress object).

Your Environment

alexellis commented 4 years ago

Some users would want to pass traffic from function ingress to the gateway deployed to core services namespace (default behavior) while the others may prefer to pass it directly to the function svc in functions namespace (bypassGateway option in FunctionIngress object).

I think there is a broader issue here, that many users want both, not an exclusive option.

A better option would be to spin out its own chart, like we had before, then you are free to install it however you like - one, or two times with either option.

(Just a note - please see our guidelines on opening PRs without approval for the change.)

aleks-fofanov commented 4 years ago

I think there is a broader issue here, that many users want both, not an exclusive option. A better option would be to spin out its own chart, like we had before, then you are free to install it however you like - one, or two times with either option.

Are you suggesting to create a separate chart for ingress operator instead? In what repository, this or the operator's?

(Just a note - please see our guidelines on opening PRs without approval for the change.)

I noticed it, but it contradicts with PR template that clearly states that you are required to create an issue to propose a change.

alexellis commented 4 years ago

Read the contribution guide for the OpenFaaS project in the openfaas/faas repo. There is no contradiction, an issue is required for each PR, but changes also need to be discussed and approved.

The chart would remain in this repo

aleks-fofanov commented 4 years ago

@alexellis And since I opened this issue, I hope you don't mind to discuss related questions here. There are 2 questions:

  1. Why Ingress Operator can't manage ingresses in both core service and function namespaces (ingresses with bypass gateway set to true). Is there any limitation that prevents it from manage them all? I mean we could have a single deployment of the operator which manages both types if ingresses (with bypass gateway set to true and false).
  2. How are you looking at the idea of allowing to run ingress operator in cluster-wide mode, i.e. it runs with cluster role OR a role with binding to multiple namespaces and manages ingresses for multiple deployments of core services?
aleks-fofanov commented 4 years ago

Read the contribution guide for the OpenFaaS project in the openfaas/faas repo. There is no contradiction, an issue is required for each PR, but changes also need to be discussed and approved.

Read it multiple times, but didn't catch what you've mean initially in the note. Now I got it. Thanks. Since the proposed change is very small and doesn't affect existing users I decided to go ahead and create PR and the issue simultaneously :(

alexellis commented 4 years ago

The reason we ask for a proposal is that we have a broader view and most importantly, context of the whole project and roadmap. Sometimes a bandaid can be a suitable solution, at other times it's the wrong solution for the community.

The operator could at some point support multiple namespaces if the cost was deemed worthy, but it will not for the time being. For free, you can get exactly what you want by installing two operators in distinct namespaces and benefit from needed fewer privileges (i.e. no cluster role)

Of course if your company would like to sponsor the development and maintenance of the feature, that's another discussion and an option we could discuss separately.

To recap - yes add the flag, but after spinning out a new chart for the operator in this repo. And would you be willing to take on the work?

aleks-fofanov commented 4 years ago

For free, you can get exactly what you want by installing two operators in distinct namespaces and benefit from needed fewer privileges (i.e. no cluster role)

Well, this couldn't be free because every workload in the cluster needs computing resources to run, even though in this case they could as small as 200m cpu and ~100Mi of memory. But anyways, this comes at additional cost and every deployment of this operator in a multi-tenant cluster adds a small but additional amount to your monthly bill. Got your point, thanks for explanations.

To recap - yes add the flag, but after spinning out a new chart for the operator in this repo. And would you be willing to take on the work?

I will get back to this issue on the next week and see if I can find time to do this.

Thanks again for your input.

alexellis commented 4 years ago

Closing in favour of #664

alexellis commented 4 years ago

I didn't realise you were going to take my words literally, so yes, for what it's worth - you are right on some level. The operator on openfaas-cloud is taking 12Mi after running for several months and 2m of CPU.

If you feel that the R&D costs are better value, then we do provide custom development and I'd be happy to have a separate discussion about that with you. As you know, contributions are also welcome.

aleks-fofanov commented 4 years ago

@alexellis Alex, I didn't mean to offend, ask for custom development or even ask you guys to develop this feature for me. I only wanted to contribute either by implementing a slight change you've mentioned in the comment to #651 or by developing a separate chart for the operator as you suggested. No sure how, but it looks like I hurt your feelings here. I do apologize for this.