weaviate / weaviate-helm

Helm charts to deploy Weaviate to k8s
https://weaviate.io/developers/weaviate/current/
BSD 3-Clause "New" or "Revised" License
48 stars 62 forks source link

HTTPS with Helm values (GKE) #153

Open eostis opened 1 year ago

eostis commented 1 year ago

Is there a way to add GKE ManagedCertificates to the Helm values?

etiennedi commented 1 year ago

Let me loop in @iamcaleberic here to comment on best practices, as I'm not sure what the way to go would be here.

Since ManagedCertificates are specific to GKE, I'm not sure if they should be part of the general Helm chart. But maybe there would be a simple way to wrap our Helm chart and make it GKE-specific. Then you could easily add managed certs there? Or maybe there is a better way.

eostis commented 1 year ago

There are already GCP values :)

Strating vithout HTTPS is very unsettling.

iamcaleberic commented 1 year ago

We need to add the ManagedCertificate object to the helm chart

apiVersion: networking.gke.io/v1
kind: ManagedCertificate
metadata:
  name: managed-cert
spec:
  domains:
    - DOMAIN_NAME1
    - DOMAIN_NAME2

And feature gate them in the values to apply to gke specific setups in the values, would also require use of an ingress or gateway to make use of it.

stale[bot] commented 1 year ago

Thank you for your contribution to Weaviate. This issue has not received any activity in a while and has therefore been marked as stale. Stale issues will eventually be autoclosed. This does not mean that we are ruling out to work on this issue, but it most likely has not been prioritized high enough in the last months. If you believe that this issue should remain open, please leave a short reply. This lets us know that the issue is not abandoned and acts as a reminder for our team to consider prioritizing this again. Please also consider if you can make a contribution to help with the solution of this issue. If you are willing to contribute, but don't know where to start, please leave a quick message and we'll try to help you. Thank you, The Weaviate Team

eostis commented 1 year ago

Reopening. This is an important subject.

The documentation now shows several methods of authentication, but all useless without SSL.

jschwanz commented 1 year ago

I just started looking into this, but I'm on Azure and typically using cert-manager + Let's Encrypt. I'm definitely interested to see progress on HTTPS/ingress capabilities.

StefanBogdan commented 1 year ago

Hi @eostis @jschwanz, the reason we do not have ManagedCertificate resource in the Helm Chart is because it is GCP specific. The current helm chart is meant to deploy the Weaviate database in any K8s cluster, no Cloud vendor specific. Also we do not provision an Ingress because some users want to have a private Weaviate instance and they have an application in the same cluster that talks to Weaviate. Our concept of the Helm chart is to have a single repo that deploys Weaviate and any additional stuff that is vendor specific to leave it to the user.

For example in GKE if you want HTTPS protocol for your instance it means that you need an Ingress resource, a ManagedCertificate and a global IP to associate with the Ingress AND to have a domain name. The last 2 are not possible (as far as I know) to provision using k8s resources and this could lead to users having bad experience because they do not understand why it is not working.

In the case of Azure, it means we have to have a dependency on the cert-manager which means we have to make sure it is always up to date, but this could because very difficult to maintain once we add more vendors.

On AWS you do not even need an Ingress you can add a TLS cert on a LoadBalancer by adding the correct annotations to the k8s LoadBalancer Service.

It does not mean we are not willing to add this, it is more that we never had a need for this so far. We assumed that the users who want to have HTTPS will be the ones that understand what it is and why it is needed and how to do this. That is why we never added it.

My question would be: Why not to create the ManagedCertificate and the Ingress outside the weaviate helm chart?

Please let me know if you disagree and the reasons you want it to be part of the Helm chart. After this we will evaluate it and we might add it to the official helm release.

eostis commented 1 year ago

Hi @StefanBogdan,

The main reason is to lower the tech barrier to using Weaviate, especially for small businesses. SSL is difficult to configure, and most/all businesses will refuse to start without.

There are already cloud-specific files, like gcsSecret.yaml for backups. Some more for SSL would not be inconceivable.

A mix of templates and "gcloud" scripts could be a very strong help at setting the SLL for the cluster namespaces. As far as I know, everything can be scripted with K8S: https://cert-manager.io/docs/tutorials/getting-started-with-cert-manager-on-google-kubernetes-engine-using-lets-encrypt-for-ingress-ssl/

jschwanz commented 1 year ago

For me I don't think I'm looking for anything vendor specific. What I would need would be the ability to enable a standard kind: Ingress with metadata.annotations, spec.ingressClassName, spec.rules, and spec.tls. Yeah I recognize I can do all of this outside of Helm, but it'd be more convenient to have it all defined and versioned in one configuration.

StefanBogdan commented 1 year ago

We are going to explore the option of adding the Ingress AND the cert-manager to the helm chart. Thanks for the feedback.

bryantbiggs commented 1 year ago

I would recommend not adding cert-manager to the chart - if you expose Ingress and/or Gateway resources, this should be enough for users to utilize with other resources such as cert-manager

heresandyboy commented 1 year ago

I had also had trouble when deploying to a KOPS managed cluster in AWS. Without an ingress record provided in the templates I was unable to access Weaviate in our cluster.

To workaround it I had cloned the weaviate-helm repo and copied in an ingress template into the templates folder, and added values for ingress. This is all working nicely for me, but having the ingress built into the chart will help with maintaining this.

In case it offers any assistance to this issue, here is my working ingress template and example values:

ingress.yml

{{- if .Values.ingress.enabled -}}
{{- $fullName := .Values.service.name -}}
{{- $svcPort := "" -}}
{{- range .Values.service.ports }}
  {{- if eq .name "http" }}
    {{- $svcPort = .port -}}
  {{- end }}
{{- end }}
{{- if and .Values.ingress.className (not (semverCompare ">=1.18-0" .Capabilities.KubeVersion.GitVersion)) }}
  {{- if not (hasKey .Values.ingress.annotations "kubernetes.io/ingress.class") }}
  {{- $_ := set .Values.ingress.annotations "kubernetes.io/ingress.class" .Values.ingress.className}}
  {{- end }}
{{- end }}
{{- if semverCompare ">=1.19-0" .Capabilities.KubeVersion.GitVersion -}}
apiVersion: networking.k8s.io/v1
{{- else if semverCompare ">=1.14-0" .Capabilities.KubeVersion.GitVersion -}}
apiVersion: networking.k8s.io/v1beta1
{{- else -}}
apiVersion: extensions/v1beta1
{{- end }}
kind: Ingress
metadata:
  name: {{ $fullName }}
  labels:
    app.kubernetes.io/name: weaviate
    app.kubernetes.io/managed-by: helm
  {{- with .Values.ingress.annotations }}
  annotations:
    {{- toYaml . | nindent 4 }}
  {{- end }}
spec:
  {{- if and .Values.ingress.className (semverCompare ">=1.18-0" .Capabilities.KubeVersion.GitVersion) }}
  ingressClassName: {{ .Values.ingress.className }}
  {{- end }}
  {{- if .Values.ingress.tls }}
  tls:
    {{- range .Values.ingress.tls }}
    - hosts:
        {{- range .hosts }}
        - {{ . | quote }}
        {{- end }}
      secretName: {{ .secretName }}
    {{- end }}
  {{- end }}
  rules:
    {{- range .Values.ingress.hosts }}
    - host: {{ .host | quote }}
      http:
        paths:
          {{- range .paths }}
          - path: {{ .path }}
            {{- if and .pathType (semverCompare ">=1.18-0" $.Capabilities.KubeVersion.GitVersion) }}
            pathType: {{ .pathType }}
            {{- end }}
            backend:
              {{- if semverCompare ">=1.19-0" $.Capabilities.KubeVersion.GitVersion }}
              service:
                name: {{ $fullName }}
                port:
                  number: {{ $svcPort }}
              {{- else }}
              serviceName: {{ $fullName }}
              servicePort: {{ $svcPort }}
              {{- end }}
          {{- end }}
    {{- end }}
{{- end }}

values passed into helm

            image:
              tag: 1.20.0
            service:
              name: weaviate
              ports:
                - name: http
                  protocol: TCP
                  port: 80
              type: NodePort

            ingress:
              enabled: true
              tls: []
              hosts:
                - host: weaviate.systems.kubernetes.testing.aws.zen.co.uk
                  paths:
                    - path: '/*'
                      pathType: ImplementationSpecific
              annotations:
                kubernetes.io/ingress.class: alb
                alb.ingress.kubernetes.io/listen-ports: '[{"HTTPS":443}]'
                alb.ingress.kubernetes.io/scheme: internal
                alb.ingress.kubernetes.io/backend-protocol: HTTP
                alb.ingress.kubernetes.io/healthcheck-path: /ping
                alb.ingress.kubernetes.io/group.name: weaviate
                external-dns.alpha.kubernetes.io/hostname: weaviate.systems.kubernetes.testing.aws.zen.co.uk