-
Notifications
You must be signed in to change notification settings - Fork 208
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 helm-based installation for hubble integration #778
Conversation
b15ae23
to
04427b2
Compare
putting in draft since the CI is failing a lot |
04427b2
to
04d400d
Compare
04d400d
to
bbd80de
Compare
These changes are needed for the upcoming changes that adds helm support to the remaining CLI options. Signed-off-by: André Martins <[email protected]>
Since in YAML dots are used to represent a path, when converting helm values into a string representation YAML keys that had dots were wrongly represented. This commit escapes the dots to correctly represent the helm keys. Signed-off-by: André Martins <[email protected]>
bbd80de
to
c358bde
Compare
tested with gke. enabling hubble is working, but i'm seeing issues with ✅ install cilium
✅ enable hubble with ui
|
c358bde
to
0620c5f
Compare
0620c5f
to
2c565e4
Compare
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.
tested again with the updated branch, and everything worked.
✅ cilium install
✅ cilium hubble enable
✅ cilium hubble port-forward
and then hubble observe
.
✅ cilium hubble ui
and checked kube-system
namespace shows flows.
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 tested the changes and it is functional. However, I have some concerns, a low one, a medium one and a high one :)
- (low) Why is
helm template ...
printed out when runningcilium hubble port-forward
? I wouldn't expect this command to be printed out for commands that I don't expect to change anytning in the cluster. - (medium) We now print the b64 encoded TLS keypair when installing. Could we somehow hide or truncate it before printing?
% cilium hubble enable --ui
🔑 Found CA in secret cilium-ca
ℹ️ helm template --namespace kube-system cilium cilium/cilium --version 1.11.3 --set cluster.id=0,cluster.name=kind-kind,encryption.nodeEncryption=false,hubble.enabled=true,hubble.listenAddress=:4244,hubble.relay.enabled=true,hubble.relay.tls.server.enabled=true,hubble.socketPath=/var/run/cilium/hubble.sock,hubble.tls.ca.cert=LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUNGRENDQWJxZ0F3SUJBZ0lVZkM0MldQRzZOdFBmdXJWMS9IZGljUEJ6dnRvd0NnWUlLb1pJemowRUF3SXcKYURFTE1Ba0dBMVVFQmhNQ1ZWTXhGakFVQmdOVkJBZ1REVk5oYmlCR2NtRnVZMmx6WTI4eEN6QUpCZ05WQkFjVApBa05CTVE4d0RRWURWUVFLRXdaRGFXeHBkVzB4RHpBTkJnTlZCQXNUQmtOcGJHbDFiVEVTTUJBR0ExVUVBeE1KClEybHNhWFZ0SUVOQk1CNFhEVEl5TURRd056RTRNRE13TUZvWERUSTNNRFF3TmpFNE1ETXdNRm93YURFTE1Ba0cKQTFVRUJoTUNWVk14RmpBVUJnTlZCQWdURFZOaGJpQkdjbUZ1WTJselkyOHhDekFKQmdOVkJBY1RBa05CTVE4dwpEUVlEVlFRS0V3WkRhV3hwZFcweER6QU5CZ05WQkFzVEJrTnBiR2wxYlRFU01CQUdBMVVFQXhNSlEybHNhWFZ0CklFTkJNRmt3RXdZSEtvWkl6ajBDQVFZSUtvWkl6ajBEQVFjRFFnQUVPOFMyUGxNNHEwcVhSUGJrN01wOURuVk8KTnhVcWx4L3M3VVdUVERlTzBQQnNqdjZDa25JUmNkK2RMZW1oUjhsSzZkZGhDQUd6eHYvMXRJSm5YRXNmQjZOQwpNRUF3RGdZRFZSMFBBUUgvQkFRREFnRUdNQThHQTFVZEV3RUIvd1FGTUFNQkFmOHdIUVlEVlIwT0JCWUVGTGtuCiszTnlKdDJVRHN4Z3NDY01RR0ZjaXdoL01Bb0dDQ3FHU000OUJBTUNBMGdBTUVVQ0lRQ1JDcjZHcC9heThBS1IKNXJXWklabm54dXBmalZ0ZktuVTcrVGxQVE5vVFp3SWdUT3JIcno4UmE1dUwzbXBGNk9DdFd0MkIrVVI1TjVlSApJeDJjTGJOcU1NMD0KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo=,hubble.tls.ca.key=LS0tLS1CRUdJTiBFQyBQUklWQVRFIEtFWS0tLS0tCk1IY0NBUUVFSUxVRmFreStMNmttcW14S3ZVc25jYzhtdXRxRmVaNmlrZ1ExQWwxdlZXbEFvQW9HQ0NxR1NNNDkKQXdFSG9VUURRZ0FFTzhTMlBsTTRxMHFYUlBiazdNcDlEblZPTnhVcWx4L3M3VVdUVERlTzBQQnNqdjZDa25JUgpjZCtkTGVtaFI4bEs2ZGRoQ0FHenh2LzF0SUpuWEVzZkJ3PT0KLS0tLS1FTkQgRUMgUFJJVkFURSBLRVktLS0tLQo=,hubble.tls.enabled=true,hubble.ui.enabled=true,hubble.ui.securityContext.enabled=false,ipam.mode=kubernetes,kubeProxyReplacement=disabled,operator.replicas=1,serviceAccounts.cilium.name=cilium,serviceAccounts.operator.name=cilium-operator,tunnel=vxlan
✨ Patching ConfigMap cilium-config to enable Hubble...
🚀 Creating ConfigMap for Cilium version 1.11.3...
♻️ Restarted Cilium pods
⌛ Waiting for Cilium to become ready before deploying other Hubble component(s)...
✨ Generating certificates...
🔑 Generating certificates for Relay...
✨ Deploying Relay...
✨ Generating certificates...
🔑 Generating certificates for UI...
✨ Deploying Hubble UI and Hubble UI Backend...
⌛ Waiting for Hubble to be installed...
✅ Hubble was successfully enabled!
- (high) I think that enabling TLS for the Relay service should be done as part of another PR as it affects the user experience. I want to make it clear that overall I think we should enable TLS (or even mTLS) by default for the Relay service. However, doing so without further consideration breaks current user's workflows and would require us to also update Cilium documentation. Indeed, when a user now runs
cilium hubble port-forward
and then run a Hubble CLI command such ashubble status
orhubble observe
, they are greeted with the following message (after some time elapsed):
% hubble status
failed to connect to 'localhost:4245': context deadline exceeded: connection closed before server preface received
This is because the Hubble CLI needs to be told to use TLS, e.g. by providing the --tls
flag. However, in itself, this is insufficient as users also need to either pass the scary --tls-allow-insecure
OR import the CA and validate via server name (which is rather painful to do.
I think that ultimately, we should update the Hubble CLI so that it reads the kubeconfig and does the right thing for things to work out of the box (without even requiring cilium hubble port-forward
). Could we then create issues for relevant work to enable TLS by default and drop this change from this PR?
2c565e4
to
1e1eac3
Compare
By storing helm-based config values into Cilium's ConfigMap we are able to generate hubble deployment files by reading those values. Once the manifests are generated, the new helm flags used to enable hubble and/or hubble-ui and/or hubble-relay will also be store in Cilium's ConfigMap. Similarly to 'hubble enable', 'hubble disable' command also stores the helm-based config values into Cilium's ConfigMap. Signed-off-by: André Martins <[email protected]>
1e1eac3
to
ebd12e3
Compare
andre claims that robin gave a +1 offline. merging. |
@aanm Have you tested this change with Kind, by any chance? Trying with the latest release, I'm getting the following error:
I bisected to commit 269a984, but can't tell for sure whether this is a bug or if I'm doing something wrong. [EDIT] Sorted this out offline. This is because I installed Cilium with Helm, instead of using |
Besides switched to a helm-based installation. This PR also enables TLS for hubble. Previously, cilium-cli had the TLS disabled for hubble-ui