-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
Add support to connect Clustermesh clusters through Helm Chart #17851
Merged
joestringer
merged 1 commit into
cilium:master
from
samueltorres:add-clustermesh-config-on-helm-chart
Feb 23, 2022
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
14 changes: 14 additions & 0 deletions
14
install/kubernetes/cilium/templates/clustermesh-config/_helpers.tpl
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
15
install/kubernetes/cilium/templates/clustermesh-config/clustermesh-secret.yaml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 }} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 evidently I was not aware that there were already other places in the codebase where we're doing this.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.