Skip to content

Conversation

@chris-dot-exe
Copy link

Noticed that only one TLS secret is created when TLS is enabled in the global settings and a secretName is set because the name is set for all modules without any suffix/prefix.

In this change it checks if secretName is not empty and creates individual secretNames for each module (suffixes the module name) if it is empty it won't add the secretName at all since nginx Ingress controller seams to support this and then loads default certs.

@Tim-herbie
Copy link
Owner

/review

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

YAML Indentation

The conditional secretName block may be misaligned under the list item, causing invalid YAML. Ensure that secretName is indented at the same level as hosts within the - mapping in all ingress templates.

{{- if not (empty .Values.global.tls.secretName)}}
secretName: {{ .Values.global.tls.secretName }}-collabora
{{- end }}
Template Nil Handling

The empty function might not safely handle an unset global.tls.secretName (nil) versus an empty string. Verify that omitting this value doesn’t produce rendering errors and that the conditional behaves as expected across all modules.

{{- if not (empty .Values.global.tls.secretName)}}
secretName: {{ .Values.global.tls.secretName }}-collabora
{{- end }}

@Tim-herbie
Copy link
Owner

Hi @chris-dot-exe,
thank you for contributing to this repository!

I understand your intention to use different secrets for the ingress resources. However, I don’t fully agree with your statement that “only one TLS secret is created when TLS is enabled.” Currently, no TLS secret is automatically created by this Helm chart; it has to be created manually by the user.

Noticed that only one TLS secret is created when TLS is enabled in the global settings and a secretName is set because the name is set for all modules without any suffix/prefix.

Merging your changes right now would require each user to create individual secrets:
Secret name: "$(Values.global.tls.secretName) + prefix of the application".

Do you agree with this, or am I misunderstanding something?


I tested it to confirm that my understanding is correct:

helm values:

 helm -n opencloud get values  opencloud

USER-SUPPLIED VALUES:
global:
  tls:
    enabled: true
    secretName: mytest
ingress:
  enabled: true

example keycloak ingress:

k -n opencloud get ingress opencloud-keycloak -o yaml                                                                         kube minikube
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    meta.helm.sh/release-name: opencloud
    meta.helm.sh/release-namespace: opencloud
  creationTimestamp: "2026-01-22T12:35:42Z"
  generation: 3
  labels:
    app.kubernetes.io/managed-by: Helm
  name: opencloud-keycloak
  namespace: opencloud
  resourceVersion: "2129"
  uid: 8900744e-7a8a-4e99-882e-e7f80abf2de5
spec:
  rules:
  - host: keycloak.opencloud.test
    http:
      paths:
      - backend:
          service:
            name: opencloud-keycloak
            port:
              number: 8080
        path: /
        pathType: Prefix
  tls:
  - hosts:
    - keycloak.opencloud.test
    secretName: mytest
status:
  loadBalancer: {}

secrets:

 k -n opencloud get secrets
NAME                              TYPE                 DATA   AGE
opencloud-keycloak                Opaque               2      30m
opencloud-minio                   Opaque               2      30m
opencloud-onlyoffice              Opaque               1      4m23s
opencloud-opencloud               Opaque               1      30m
opencloud-postgres                Opaque               2      30m
opencloud-rabbitmq                Opaque               1      4m23s
sh.helm.release.v1.opencloud.v1   helm.sh/release.v1   1      30m
sh.helm.release.v1.opencloud.v2   helm.sh/release.v1   1      21m
sh.helm.release.v1.opencloud.v3   helm.sh/release.v1   1      12m
sh.helm.release.v1.opencloud.v4   helm.sh/release.v1   1      9m39s
sh.helm.release.v1.opencloud.v5   helm.sh/release.v1   1      4m23s

=> secretname mytest was not created!

@chris-dot-exe
Copy link
Author

chris-dot-exe commented Jan 22, 2026

@Tim-herbie I can't tell from your examples if you use an cert-manager? In my experience, cert-manager is responsible for creating and managing the TLS secrets, and it typically stores one certificate per secret.

Wildcard domain would be a option with only one secret I guess but then the domains can't be defined individually in each modules ingress template.

But maybe I'm also overlooking something here and it would break something.

In my setup, I’m using Traefik as the ingress controller together with cert-manager to issue certificates per domain.
I don’t have experience with other ingress controllers or alternative certificate management approaches so possible that it doesn't work in any case.

I just now that other Charts use a similar approach but if this isn't a universal solution here then feel free to decline the PR ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants