kvaps / kube-linstor

Containerized LINSTOR SDS for Kubernetes, ready for production use.
Apache License 2.0
130 stars 25 forks source link

Etcd DB Conf and Certs #21

Closed jbanety closed 3 years ago

jbanety commented 3 years ago

Hi @kvaps,

I'm looking to add ability to connect to a etcd cluster. At the moment, we can do :

controller:
  db:
    connectionUrl: "etcd://node1:2379,node2:2379"

But etcd is secured with TLS certs so, currently, we can't make a secure connection with your helm chart. Before making a PR, I want to know if you will be OK with my solution.

Add 4 config values, and by the way we can set etcd prefix :

controller:
  db:
    tls: true
    cert: [A base64 encoded PEM format certificate]
    key: [A base64 encoded PEM format private key]
    ca: [A base64 encoded PEM format certificate authority]
    # optional
    etcdPrefix: "/LINSTOR/"

Create kubernetes.io/tls secret db-tls.yaml :

{{- $fullName := include "linstor.fullname" . -}}
{{- if .Values.controller.enabled }}
{{- if .Values.controller.db.tls }}
---
apiVersion: v1
kind: Secret
metadata:
  name: {{ $fullName }}-db-tls
  annotations:
    "helm.sh/resource-policy": "keep"
    "helm.sh/hook": "pre-install"
    "helm.sh/hook-delete-policy": "before-hook-creation"
    "directives.qbec.io/update-policy": "never"
type: kubernetes.io/tls
data:
  tls.crt: {{ .Values.controller.db.cert }}
  tls.key: {{ .Values.controller.db.key }}
  ca.crt: {{ .Values.controller.db.ca }}
{{- end }}
{{- end }}

Modify _helpers.tpl to add reference to certs :

[...]
  connection_url = "{{ .Values.controller.db.connectionUrl }}"
{{- if .Values.controller.db.tls }}
  ca_certificate = "/config/ssl/db/ca.crt"
  client_certificate = "/config/ssl/db/tls.crt"
  client_key_pkcs8_pem = "/config/ssl/db/tls.key"
{{- end }}
{{- if .Values.controller.db.etcdPrefix }}
  [db.etcd]
  prefix = "{{ .Values.controller.db.etcdPrefix }}"
{{- end }}
[...]

Modify controller-deployment.yaml :


{{- $fullName := include "linstor.fullname" . -}}
{{- if .Values.controller.enabled }}
---
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: {{ $fullName }}-controller
  name: {{ $fullName }}-controller
  namespace: {{ .Release.Namespace }}
[...]
      initContainers:
      - name: load-certs
[...]
        command:
[...]
          {{- end }}
          {{- if .Values.controller.db.tls }}
          cp -rf /tls/db /config/ssl/db
          {{- end }}
          rm -f "$tmp"
        volumeMounts:
        {{- if .Values.controller.db.tls }}
        - name: db-tls
          mountPath: /tls/db
        {{- end }}
[...]
      volumes:
      {{- if .Values.controller.db.tls }}
      - name: db-tls
        secret:
          secretName: {{ $fullName }}-db-tls
      {{- end }}
[...]
kvaps commented 3 years ago

Hi, sure, it would be a nice contribution!

The changes are looks ok, except that personaly I don't like the fact puting base64-encoded values into values.yaml. I think cleartext would be better, however I am ready to listen to any objections about this.

Could you please clarify how do you usually deploy the etcd cluster and how do you generate certificates?

I had a plan to add two additional methods for generating TLS-certificates as alternative to Helm builtin functions: cert-manager and kube-webhook-certgen job to get more deterministic output, it would be nice to have these options similar in the future.

Alternative solution can be implementing extraVolumes and extraVolumeMounts for linstor-controller.

jbanety commented 3 years ago

I deploy my K8s cluster with Lokomotive. The Etcd certs are stored in an assets folder after cluster creation.
I'm creating a Lokomotive component to add your kube-linstor chart to the components lib.
So I can refer to the etcd certs from my local machine. Concerning Base64 or not, I have no opinion on that. I'll create a PR asap.