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

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

Closed
aleks-fofanov opened this issue Jun 26, 2020 · 10 comments

Comments

@aleks-fofanov
Copy link
Contributor

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

  • FaaS-CLI version ( Full output from: faas-cli version ):
CLI:
 commit:  f7c29ea19b5df9d7aa87e9c70aacf4d9315da2cd
 version: 0.12.4
  • Docker version docker version (e.g. Docker 17.0.05 ): Docker version 19.03.11, build 42e35e61f3

  • What version and distriubtion of Kubernetes are you using? kubectl version

Client Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.3", GitCommit:"2e7996e3e2712684bc73f0dec0200d64eec7fe40", GitTreeState:"clean", BuildDate:"2020-05-21T20:58:41Z", GoVersion:"go1.13.11", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.2", GitCommit:"52c56ce7a8272c798dbc29846288d7cd9fbae032", GitTreeState:"clean", BuildDate:"2020-04-16T11:48:36Z", GoVersion:"go1.13.9", Compiler:"gc", Platform:"linux/amd64"}
  • Operating System and version (e.g. Linux, Windows, MacOS): Ubuntu 18.04.4 LTS

  • Link to your project or a code example to reproduce issue: n/a

  • What network driver are you using and what CIDR? i.e. Weave net / Flannel n/a

@alexellis
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

Closing in favour of #664

@alexellis
Copy link
Member

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
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants