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

Allow disabling of Ingress creation #235

Closed
mjnagel opened this issue Jul 21, 2021 · 3 comments · Fixed by #249
Closed

Allow disabling of Ingress creation #235

mjnagel opened this issue Jul 21, 2021 · 3 comments · Fixed by #249
Assignees

Comments

@mjnagel
Copy link
Contributor

mjnagel commented Jul 21, 2021

Summary

Currently the operator/CRD do not allow creation of a Mattermost instance without an ingress. The value spec.ingressName is always required to the best of my knowledge.

Our use case for this is deploying MM without an ingress and using "alternative ingress" via a service mesh and would prefer that this ingress is not created.

Possible fixes

Provide a new value (defaulted to true) that toggles the creation of Ingress. When false, it would also "disable the requirement" to provide a spec.ingressName. There could be numerous ways to implement this, but probably the most backwards compatible would be with a new spec.ingressEnabled flag:

apiVersion: installation.mattermost.com/v1beta1
kind: Mattermost
...
spec:
  ingressEnabled: false # defaults to true
...

If open to changing the spec slightly, it would seem to make more logical sense and better match other operators to do it with a spec.ingress block:

apiVersion: installation.mattermost.com/v1beta1
kind: Mattermost
...
spec:
  ingress:
    enabled: true # defaults to true, just added for example
    name: my-mm-ingress
    annotations:
      kubernetes.io/ingress.class: nginx
...

Totally understand if the second is off the table because it would introduce "breaking" changes. Either of these proposed solutions (or really anything that allows us to disable the Ingress) would work for our needs.

@Szymongib
Copy link
Contributor

Hey @mjnagel, thanks for reporting the issue.

I can see the use case for opting out of Ingress creation and it definitely makes sense.

I also like the proposed solution, especially the second one but as you correctly mentioned we probably do not want to introduce the breaking change immediately.

What we could do tho is to add a new Ingress section and deprecate the old fields using them only if new ones are not provided and dropping them eventually.
So that would be something like:

apiVersion: installation.mattermost.com/v1beta1
kind: Mattermost
...
spec:
  # Deprecated
  ingressName: ""
  # Deprecated
  ingressAnnotations: {}
  ...
  ingress:
    enabled: true 
    name: my-mm-ingress
    annotations:
      kubernetes.io/ingress.class: nginx
...

cc @gabrieljackson

@nicolas-goudry
Copy link

@Szymongib this feature is great, thanks for implementing it! Would you please have an idea about when it will be released?

Thanks!

@Szymongib
Copy link
Contributor

@nicolas-goudry we are planning to do a new release this week if we do not run into any major issues.

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.

3 participants