Add support to connect Clustermesh clusters through Helm Chart

[samueltorres]

This PR aims to add support to connect multiple Clustermesh clusters using the Helm Chart, instead of doing it manually / using the cilium-cli.

Fixes: #17811

Adds support to connect Clustermesh clusters through Helm Chart.

[maintainer-s-little-helper]

Commit 51935ed90fd56afe75023f65c52a314f39aa3352 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[maintainer-s-little-helper]

Commit 7f5b51e8c9f59a11b1050efbf93d0d8ab30e41c8 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[joestringer]

Thanks for the submission. The proposed structure makes sense to me, with the exception of the handling of private keys. I'm not sure the best way to handle that particular piece, but I've raised a dedicated discussion point for it below. (cc @tgraf )

[joestringer]

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?

[samueltorres]

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 :)

[joestringer]

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.

[joestringer]

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.

[samueltorres]

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.

[joestringer]

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.

[joestringer]

LGTM. I would welcome any sort of explanation on how to use these, whether as a docs page in the repo or even as a separate blog somewhere. But I don't think that this is necessary to merge this PR.

[joestringer]

I think that this is in good shape for now, let's just merge this as-is. @samueltorres if you do get a chance to work on docs or a writeup on using this feature, then feel free to submit a separate PR for that. Thanks!

[samueltorres]

Thanks for merging it :)

I started working on it, but didn't want to do a complete re-write on that page. Perhaps we can discuss that on an issue.

[kaworu]

@samueltorres @joestringer created #19057 for documentation follow-up.