Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support to connect Clustermesh clusters through Helm Chart #17851

Merged
merged 1 commit into from
Feb 23, 2022
Merged

Add support to connect Clustermesh clusters through Helm Chart #17851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions Documentation/helm-values.rst
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions install/kubernetes/cilium/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ contributors across the globe, there is almost always someone available to help.
| clustermesh.apiserver.tls.server.extraIpAddresses | list | `[]` | Extra IP addresses added to certificate when it's auto generated |
| clustermesh.apiserver.tolerations | list | `[]` | Node tolerations for pod assignment on nodes with taints ref: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/ |
| clustermesh.apiserver.updateStrategy | object | `{"rollingUpdate":{"maxUnavailable":1},"type":"RollingUpdate"}` | clustermesh-apiserver update strategy |
| clustermesh.config | object | `{"clusters":[],"domain":"mesh.cilium.io","enabled":false}` | Clustermesh explicit configuration. |
| clustermesh.config.clusters | list | `[]` | List of clusters to be peered in the mesh. |
| clustermesh.config.domain | string | `"mesh.cilium.io"` | Default dns domain for the Clustermesh API servers This is used in the case cluster addresses are not provided and IPs are used. |
| clustermesh.config.enabled | bool | `false` | Enable the Clustermesh explicit configuration. |
| clustermesh.useAPIServer | bool | `false` | Deploy clustermesh-apiserver for clustermesh |
| cni.binPath | string | `"/opt/cni/bin"` | Configure the path to the CNI binary directory on the host. |
| cni.chainingMode | string | `"none"` | Configure chaining on top of other CNI plugins. Possible values: - none - generic-veth - aws-cni - portmap |
Expand Down
9 changes: 9 additions & 0 deletions install/kubernetes/cilium/templates/cilium-agent/daemonset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,15 @@ spec:
tolerations:
{{- toYaml . | trim | nindent 8 }}
{{- end }}
{{- if and .Values.clustermesh.useAPIServer .Values.clustermesh.config.enabled }}
hostAliases:
{{- range $cluster := .Values.clustermesh.config.clusters }}
{{- range $ip := $cluster.ips }}
- ip: {{ $ip }}
hostnames: [ "{{ $cluster.name }}.{{ $.Values.clustermesh.config.domain }}" ]
{{- end }}
{{- end }}
{{- end }}
volumes:
# To keep state between restarts / upgrades
- name: cilium-run
Expand Down
14 changes: 14 additions & 0 deletions install/kubernetes/cilium/templates/clustermesh-config/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{{- define "clustermesh-config-generate-etcd-cfg" }}
{{- $cluster := index . 0 -}}
{{- $domain := index . 1 -}}

endpoints:
{{- if $cluster.ips }}
- https://{{ $cluster.name }}.{{ $domain }}:{{ $cluster.port }}
{{ else }}
- https://{{ $cluster.address | required "missing clustermesh.apiserver.config.clusters.address" }}:{{ $cluster.port }}
{{- end }}
trusted-ca-file: /var/lib/cilium/clustermesh/{{ $cluster.name }}.etcd-client-ca.crt
key-file: /var/lib/cilium/clustermesh/{{ $cluster.name }}.etcd-client.key
cert-file: /var/lib/cilium/clustermesh/{{ $cluster.name }}.etcd-client.crt
{{- end }}
15 changes: 15 additions & 0 deletions install/kubernetes/cilium/templates/clustermesh-config/clustermesh-secret.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{{- if and .Values.clustermesh.useAPIServer .Values.clustermesh.config.enabled }}
---
apiVersion: v1
kind: Secret
metadata:
name: cilium-clustermesh
namespace: {{ .Release.Namespace }}
data:
{{- range .Values.clustermesh.config.clusters }}
{{ .name }}: {{ include "clustermesh-config-generate-etcd-cfg" (list . $.Values.clustermesh.config.domain) | b64enc }}
{{ .name }}.etcd-client-ca.crt: {{ $.Values.clustermesh.apiserver.tls.ca.cert }}
{{ .name }}.etcd-client.key: {{ .tls.key }}
{{ .name }}.etcd-client.crt: {{ .tls.cert }}
{{- end }}
{{- end }}
27 changes: 27 additions & 0 deletions install/kubernetes/cilium/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1617,6 +1617,33 @@ clustermesh:
# -- Deploy clustermesh-apiserver for clustermesh
useAPIServer: false

# -- Clustermesh explicit configuration.
config:
# -- Enable the Clustermesh explicit configuration.
enabled: false
# -- Default dns domain for the Clustermesh API servers
# This is used in the case cluster addresses are not provided
# and IPs are used.
domain: mesh.cilium.io
# -- List of clusters to be peered in the mesh.
clusters: []
# clusters:
# # -- Name of the cluster
# - name: cluster1
# # -- Address of the cluster, use this if you created DNS records for
# # the cluster Clustermesh API server.
# address: cluster1.mesh.cilium.io
# # -- Port of the cluster Clustermesh API server.
# port: 2379
# # -- IPs of the cluster Clustermesh API server, use multiple ones when
# # you have multiple IPs to access the Clustermesh API server.
# ips:
# - 172.18.255.201
# # -- base64 encoded PEM values for the cluster client certificate, private key and certificate authority.
# tls:
# cert: ""
# key: ""
Comment on lines +1642 to +1645
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the intended model for the handling of private keys here? As far as I follow the older instructions that used to provide the outline of setting up clustermesh with helm (link), these were typically generated separately on the administrator's machine and injected directly into the clusters. However in a GitOps world, I tend to think of helm values files / configurations getting checked in somewhere. Putting those two together, it seems like the idea is to put private keys into git which seems like a "bad thing":tm: .

Have you thought about the risks of this or a reasonable way to handle these secrets safely?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree with you. This is not really the securest way of storing this kind of material.

As you said, this would ideally be checked out in a git repositoy. This change was proposed be to be used alongside the clustermesh.tls.client.cert / key setting in helm values. I think I could consider this solution as secure as setting up the certs / keys on that helm value.

In my specific case I would be applying this via terraform using the helm provider at cluster bootstrap, hiding away those keys, instead of directly storing them on git in plain text.

For those who use flux / argocd there are some features to pull values from existing secrets, see here as reference, which would make this solution a little more secure.

If anyone finds these solutions to not comply with their security needs, one possible solution would be to make use of pre-installed secrets and making use of secretRefs + volumemounts with projections to correctly set the right file paths. I'm not sure if that is feasible without any further testing.

Thanks for the feedback Joe :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you said, this would ideally be checked out in a git repositoy. This change was proposed be to be used alongside the clustermesh.tls.client.cert / key setting in helm values. I think I could consider this solution as secure as setting up the certs / keys on that helm value.

👍 evidently I was not aware that there were already other places in the codebase where we're doing this.

If anyone finds these solutions to not comply with their security needs, one possible solution would be to make use of pre-installed secrets and making use of secretRefs + volumemounts with projections to correctly set the right file paths. I'm not sure if that is feasible without any further testing.

I'm not so concerned about the users with security needs, as they will (hopefully) already be studying closely each step and understanding the movement of such secrets. They're more likely to do the right thing. It's more whether there's a default posture that could cause a well-meaning but otherwise blissfully unaware user to follow the wrong path.

I wonder whether it'd even be better to remove the install/kubernetes/cilium/templates/clustermesh-config/clustermesh-secret.yaml piece (and the corresponding existing key configuration fields), and just write down explicitly somewhere that someone must generate those (eg, using the existing tooling from https://github.com/cilium/clustermesh-tools). The usability would be a little more difficult, but it seems harder to shoot yourself in the foot.

cc @jrajahalme @seanmwinn also for perspectives on including private keys in helm values.

Copy link
Member

@joestringer joestringer Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more whether there's a default posture that could cause a well-meaning but otherwise blissfully unaware user to follow the wrong path.

Maybe I'm already solving my own question here; the simple path for clustermesh is to use https://github.com/cilium/cilium-cli, so maybe I don't need to be so worried about this for the Helm side. One other thought I had is just to add a nice big warning into the helm values descriptions for each of the private keys to call out that users should be careful about how they distribute those settings and follow security best practices.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm already solving my own question here; the simple path for clustermesh is to use https://github.com/cilium/cilium-cli, so maybe I don't need to be so worried about this for the Helm side. One other thought I had is just to add a nice big warning into the helm values descriptions for each of the private keys to call out that users should be careful about how they distribute those settings and follow security best practices.

I think that is a nice way to put this problem, and I find that suggestion really nice.

As Clustermesh API PKI material was already configurable in the Helm Chart, this PR is mostly an extension of those already existing configurations.

But I leave this up to you :) I feel great to be able to contribute this amazing project.

Copy link
Member

@joestringer joestringer Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah actually it looks somewhat consistent with thes examples as well:

https://docs.cilium.io/en/stable/concepts/observability/hubble-configuration/#use-custom-tls-certificates

I think that the answer is to "use --set-file" rather than directly checking the private keys into the values files. Then as part of the process for installing, presumably the user would generate them and pass them in via --set-file.

This is probably fine as-is then.


apiserver:
# -- Clustermesh API server image.
image:
Expand Down