[BUG] Remove Cert-manager as a dependency of redis-operator

[Routhinator]

#129 (comment)

As per my comment here, this chart unexpectedly installed a SECOND cert-manager instance in my cluster without mentioning such in the docs.

A redis operator should not be prescribing certificate management or installing cert-manager into clusters where cert-managers already exist. This caused two instances of cert-manager to war over certificates in my cluster, exhausting API requests to letsencrypt and breaking applications for multiple domains for the next week while I wait for the api limit to cool off.

This needs to be removed. If this chart needs certificate requests, it should add requests, and DOCUMENT the need for cert-manager

[shubham-cmyk]

which chart version are you using because cert-manger is disabled on the default.

[Routhinator]

I had upgraded to 0.15.0 and ended up with the second cert-manager instance. Looking at 0.15.9 now to see if it does the same.

[Routhinator]

Doesn't look like 0.15.9 does it. Which is good.

My point with this issue still stands though. cert-manager is a very core requirement of a cluster - it's needed for ingress and a number of different apps - the scenarios where it will not already be part of a cluster by the time someone is adding redis should be slim. Making this a dependency of this chart is an anti-pattern and I cannot see good practice coming from this choice.

Adding an option to create a certificate request or define the cert-issuer name for cert-requests in the chart makes good sense and is a good pattern. Installing cert-manager - not so much.

[shubham-cmyk]

Doesn't look like 0.15.9 does it. Which is good.

https://github.com/OT-CONTAINER-KIT/helm-charts/blob/main/charts/redis-operator/templates/cert-manager.yaml#L1
The installation of certificates is not enabled in the values.yaml
also cert-manger should not be installed anyway here :
https://github.com/OT-CONTAINER-KIT/helm-charts/blob/main/charts/redis-operator/Chart.yaml#L28

The default is marked false

https://github.com/OT-CONTAINER-KIT/helm-charts/blob/main/charts/redis-operator/values.yaml#L54

[shubham-cmyk]

chart makes good sense and is a good pattern. Installing cert-manager - not so much.

This point seems to be valid. I could consider dropping the dependency here.

[genofire]

could we create an option to just deploy the Certificate (and use an own clusterissuer)?

[egorksv]

Same here - please separate "certmanager.install" and "certmanager.enable", we are running our own cert manager and only want certmanager CRD Resources to be installed

[omniproc]

I added the suggested values to the chart to separate install from usage. However in our case we're using this chart in Flux and Flux seems to ignore the dependency condition and always tries to reach out to look up the cert-manager chart, which fails in an airgapped environment.

My suggestion would be to remove the cert-manager dependency all together and simply have the "enable" flat which will create the cert-manager CRs. In my opinion it's up to the user to make sure cert-manager is available or to provide the path to a certificate created by other means. Having cluster-wide dependencies in individual parts is a broken idea, imho. This obviously can only work in a very hypothetical 1-chart-per-cluster world and will likely break in any other scenario.