[helm] [self-hosted] correct proxy deployment to use kubernetes.io/tls secrets

[cyrilcros]

I am suggesting this as a fix to #3183
Instead of fetching from an existingSecret various keys, you could use the Kubernetes kubernetes.io/tls secret type. Only fullchain.pem aka tls.crt and privkey.pem aka tls.key seem to be in use in the templates, the current documentations asks for more: https://www.gitpod.io/docs/self-hosted/latest/install/configure-ingress/
See https://github.com/gitpod-io/gitpod/blob/master/chart/config/proxy/lib.ssl.conf . ssl_dhparam also seems to be off?
You would need to edit your current existing secret, this is a breaking change!
Please feel absolutely free to close, this is only intended as a suggestion on how to fix... The documentation for the ingress would need to be changed too.

EDIT: I now just check if fullchain.pem and privkey.pem are in the secret and assume tls.crt and tls.key are there if it isn't the case. That's a non breaking change.

[csweichel]

Thank you for pointing out the kubernetes.io/tls secret type - and for the PR of course :)

As middle ground we could default to tls.crt and tls.key in the values.yaml. This way older setups that use this naming mechanism/are not kubernetes.io/tls compatible would still work, but we'd also nudge folks closer to the Kubernetes way (insert Mandalorian quote here).

It would still be a breaking change for those relying on the defaults, but one for which we could provide an easy upgrade route. WDYT?

[cyrilcros]

I guess it makes the chart a bit more complicated, but you could also just add in the values keySecretRef: privkey.pem and fullcertSecretRef: fullchain.pem and edit it a tiny bit the templates . Ie, you specify what key:value pairs from the secret you want to use as the key and the full certificate. It isn’t a breaking change and new users can just adapt it to tls.key tls.crt.

[aledbf]

Ie, you specify what key:value pairs from the secret you want to use as the key and the full certificate.

Any alternative to TLS Secrets can lead to confusion and additional complexity integrating with tools like cert-manager.
This is a really old issue but still applies kubernetes/kubernetes#27010

but one for which we could provide an easy upgrade route.

Maybe an update to the secret to rename the keys?

@cyrilcros I really like this change :)

[cyrilcros]

Hi, in this case the Helm chart doesn't create a separate secret (see ingress docs). You would still to make a change to the templates (fullchain.pem -> tls.crt and privkey.pem -> tls.key).
The other thing we can do is assume you just have a proper TLS secret if your secret doesn't use the older format. We keep the block starting at

gitpod/chart/templates/proxy-deployment.yaml

Line 159 in 19fb170

{{- if (and $.Values.certificatesSecret.fullChainName $.Values.certificatesSecret.chainName $.Values.certificatesSecret.keyName) }}

and I add after {{- else }} ... whatever is needed for tls.crt / tls. key ... {{- end -}}.
@aledbf @csweichel that way that's no longer a breaking change

[aledbf]

that way that's no longer a breaking change

Can you emit a warning using a NOTES.txt file to suggest the migration to the new format if the old values are used?

[cyrilcros]

@aledbf that's done BUT I also previously altered the docs to document the new method. I have now seen in another issue those are included as a submodule. Should I just remove those doc changes from my pull requests and make them to the right repo?

[cyrilcros]

@aledbf I removed the docs update, I will make something on the docs repository. For me this pull request is done..

[csweichel]

Maybe an update to the secret to rename the keys?

Very good point indeed :)

Should I just remove those doc changes from my pull requests and make them to the right repo?

I think it's fine for now. We'd keep this change in mind and add it to the #3262 issue.

[csweichel]

/werft run

👍 started the job as gitpod-build-kubernetes-tls-fix.0

[cyrilcros]

@aledbf @csweichel any chance this could be merged soon? Thanks!