Add hubble helm charts to cilium install/kubernetes #10648

soumynathan · 2020-03-20T05:52:14Z

Add hubble helm charts to cilium install/kubernetes

Fixes: #10647
Signed-off-by: Swaminathan Vasudevan svasudevan@suse.com

maintainer-s-little-helper · 2020-03-20T05:52:16Z

Please set the appropriate release note label.

coveralls · 2020-03-20T06:22:24Z

Coverage increased (+0.03%) to 44.651% when pulling ef7360c on soumynathan:add-hubble-helm-charts-to-cilium into 43570d5 on cilium:master.

aanm · 2020-03-20T13:17:38Z

install/kubernetes/cilium/charts/hubble/values.yaml

+resources: {}
+# The priority class system-node-critical marks add-on pods as critical to the node itself.
+# This priority class is only valid under the kube-system namespace.
+priorityClassName: system-node-critical


please add the constraint to specify the priorityClassName based on the k8s version

Can you elaborate on this. I am not sure if I am getting it right.

I think I added similar to the one from the cilium/daemonset.yaml, to add constraints for k8s version.
Please let me know if this would work.

yes, that exactly.

@soumynathan this does not seem to be addressed

I will fix this, I am working on other values to be realigned.

install/kubernetes/cilium/charts/hubble/templates/serviceaccount.yaml

aanm · 2020-03-20T19:26:33Z

test-me-please

soumynathan · 2020-03-20T23:31:29Z

test failure doesn't seem to relate to the patch.

joestringer · 2020-03-23T18:20:01Z

Following the Ginkgo link from the box at the bottom of the issue:

https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/18092/

The tests that fail, fail on both k8s-1.11 and k8s-1.17 which suggests that the failure is reliable, not a flake.

Following one of the test links from the bottom of the page:

https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/18092/testReport/junit/Suite-k8s-1/11/K8sChaosTest_Connectivity_demo_application_Endpoint_can_still_connect_while_Cilium_is_not_running/

Cilium cannot be installed
Expected
    <*helpers.cmdError | 0xc00041f870>: Unable to generate YAML (exit status 1) output: cmd: helm template /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/k8s-1.11-gopath/src/github.com/cilium/cilium/install/kubernetes/cilium --namespace=kube-system  --set global.k8sServicePort=6443  --set preflight.image=cilium-dev  --set global.psp.enabled=true  --set global.etcd.leaseTTL=30s  --set agent.image=cilium-dev  --set managed-etcd.registry=docker.io/cilium  --set global.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].key=cilium.io/ci-node  --set global.debug.enabled=true  --set global.k8s.requireIPv4PodCIDR=true  --set operator.image=operator  --set global.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].operator=NotIn  --set global.nodePort.device=enp0s8  --set global.debug.verbose=flow  --set global.ipv6.enabled=true  --set global.tag=latest  --set global.registry=147.75.194.233/cilium  --set global.ipv4.enabled=true  --set global.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].values[0]=k8s3  --set global.k8sServiceHost=192.168.36.11  --set global.kubeProxyReplacement=strict  --set global.pprof.enabled=true  --set global.logSystemLoad=true  --set global.bpf.preallocateMaps=true  > cilium-15fe1b31abca9b8a.yaml
Exitcode: 1 
Stdout:
 	 
Stderr:
 	 Error: error unpacking hubble in cilium: cannot load values.yaml: error converting YAML to JSON: yaml: line 100: could not find expected ':'

This seems related to the PR.

Sidenote: The Travis failure looks like #10615 which should be fixed on master, so should be resolved after rebase.

aanm · 2020-03-24T10:46:02Z

test-me-please

EDIT: Provisioning failure:
https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/18159/

joestringer · 2020-03-24T16:27:37Z

test-me-please

soumynathan · 2020-03-25T05:48:27Z

This is the change (hubble/values.yaml) that is causing the test to fail. ( NOT sure what is wrong with this) - Any thoughts

  priorityClassName: system-node-critical
{{- end }}

install/kubernetes/cilium/charts/hubble/values.yaml

soumynathan · 2020-03-25T23:29:52Z

test-me-please

aanm · 2020-03-26T11:11:28Z

test-me-please

gandro · 2020-03-26T11:45:04Z

@soumynathan Thanks a lot for this PR!

I am however a bit wary of the implications the duplicated helm chart causes with regards to user experience and code maintenance

Currently all documentation (e.g. Cilium 1.7 stable docs and the Markdown files inside the Hubble repository) refer to the chart over at https://github.com/cilium/hubble/tree/master/install/kubernetes. Should users continue to use the charts at cilium/hubble or should they use the ones in cilium/cilium?
We have not even merged this PR yet, but the Helm charts have already started to diverge between the two repositories (e.g. the if-statement around the priorityClassName was implemented slightly differently in helm: Add priorityClassName for kube-system only hubble#189). This will only get worse over time.

I think we have to make a call which repository (cilium/hubble or cilium/cilium) should contain the Helm charts. If we decide that cilium/cilium is the repository with the charts, then we also need to update the documentation, add the Hubble charts to the CODEOWNERs for the @cilium/hubble team and remove it from the cilium/hubble repository.

I personally have no strong opinion in which repository the charts should live. I can see arguments for both: cilium/cilium has the better CI, but cilium/hubble is closer to the code, allowing atomic changes to e.g. command-line arguments and helm charts at the same time.

tgraf · 2020-03-30T11:46:35Z

Echoing what @gandro already wrote. Given that we have merged Hubble into Cilium. It makes more sense to integrate the Hubble helm charts directly into the Cilium Helm charts to allow enablement of Hubble via a single Helm option as it runs as part of Cilium.

aanm · 2020-03-30T14:02:40Z

@soumynathan Thanks a lot for this PR!

I am however a bit wary of the implications the duplicated helm chart causes with regards to user experience and code maintenance

Currently all documentation (e.g. Cilium 1.7 stable docs and the Markdown files inside the Hubble repository) refer to the chart over at https://github.com/cilium/hubble/tree/master/install/kubernetes. Should users continue to use the charts at cilium/hubble or should they use the ones in cilium/cilium?

We have not even merged this PR yet, but the Helm charts have already started to diverge between the two repositories (e.g. the if-statement around the priorityClassName was implemented slightly differently in cilium/hubble#189). This will only get worse over time.

I think we have to make a call which repository (cilium/hubble or cilium/cilium) should contain the Helm charts. If we decide that cilium/cilium is the repository with the charts, then we also need to update the documentation, add the Hubble charts to the CODEOWNERs for the @cilium/hubble team and remove it from the cilium/hubble repository.

I personally have no strong opinion in which repository the charts should live. I can see arguments for both: cilium/cilium has the better CI, but cilium/hubble is closer to the code, allowing atomic changes to e.g. command-line arguments and helm charts at the same time.

@gandro the helm charts could still live in cilium/hubble for developers. Once the hubble library version is updated in cilium/cilium/go.mod it would also update the helm charts in the same PR.

gandro · 2020-03-30T15:04:24Z

@gandro the helm charts could still live in cilium/hubble for developers. Once the hubble library version is updated in cilium/cilium/go.mod it would also update the helm charts in the same PR.

So a few of the developers working on Hubble discussed this PR offline a bit last week. Our conclusion (as @tgraf already mentioned) was that since we are sunsetting the Hubble "stand-alone mode" in favor of "embedded-mode" (PR #10238), we don't want the Hubble DaemonSet (the "stand-alone" mode) in the Cilium 1.8+ branch. Therefore it doesn't make much sense to me to add the additional maintenance burden of having to sync the Helm charts.

So going forward, the idea is that the helm charts in the cilium/cilium repository will be the ones that users should use to deploy hubble. However, instead of deploying the Hubble DaemonSet, we want the charts to enable the Hubble server running inside cilium-agent.

Stand-alone mode will continue to be available for users of Cilium 1.6 and 1.7, however it doesn't make sense to have these helm charts in the Cilium 1.8+ branch.

aanm · 2020-04-01T07:45:43Z

install/kubernetes/cilium/charts/hubble/values.yaml

+resources: {}
+# The priority class system-node-critical marks add-on pods as critical to the node itself.
+# This priority class is only valid under the kube-system namespace.
+priorityClassName: system-node-critical


@soumynathan this does not seem to be addressed

gandro · 2020-04-01T08:56:06Z

install/kubernetes/cilium/charts/hubble/templates/daemonset.yaml

+apiVersion: apps/v1
+kind: DaemonSet
+metadata:
+  name: hubble
+  namespace: {{ .Release.Namespace }}


As mentioned in #10648 (comment) we are deprecating this DaemonSet as of Cilium 1.8. Could you remove the hubble DaemonSet and all related values instead make sure the chart works with embedded Hubble? The config map for embedded Hubble is here (based on #10794):

https://github.com/cilium/cilium/blob/95f492f25d8ea7a1585503b36683eb690be40b5b/install/kubernetes/cilium/charts/config/templates/configmap.yaml#L363-L387

I think most of the work for the Hubble server itself is already done. The CRI metadata is not implemented in embedded Hubble, so that these values can be removed. I think the biggest change is extending the hubble-listen-addresses in the ConfigMap to add a listener on port 50051 when .Values.ui.enabled is true, similar to how this DaemonSet currently does it.

Please do not hesitate to reach out to either me other other folks in #hubble-devel on Slack if you have questions or need some help or pointers. Thanks!

Sure will address your comments and will contact you for any questions.

What do you think. Should I continue with this PR or drop it?

It's up to you. We do want to have the UI and other components part of the Helm chart eventually, so adapting this PR does make sense. Feel free to ping me privately if you need assistance!

It's up to you. We do want to have the UI and other components part of the Helm chart eventually, so adapting this PR does make sense. Feel free to ping me privately if you need assistance!

Sure will check with you.

It's up to you. We do want to have the UI and other components part of the Helm chart eventually, so adapting this PR does make sense. Feel free to ping me privately if you need assistance!

@gandro So in this case do you want me to completely remove the daemonset.yaml and incorporate the changes in the config/templates/configmap.yaml or want to create a configmap.yaml in hubble/templates/configmap,yaml and configure everything there. Can you confirm.

@gandro So in this case do you want me to completely remove the daemonset.yaml and incorporate the changes in the config/templates/configmap.yaml or want to create a configmap.yaml in hubble/templates/configmap,yaml and configure everything there. Can you confirm.

Kind of. So the way I imagine this is that there is not seperate hubble chart anymore, so hubble/templates/configmap,yaml should not exist. The only new chart should be the one for hubble-ui. hubble itself now lives inside cilium-agent. Therefore, all Hubble related values are set in config/templates/configmap.yaml.

I think the version you pushed now seems to already do that 🎉 I will do a more in depth review soon!

gandro

I just noticed that you also need to update the requirements.yaml to include the hubble-ui chart.

aanm · 2020-04-24T12:08:23Z

install/kubernetes/cilium/charts/agent/templates/daemonset.yaml

+        - containerPort: {{ regexReplaceAll ":([0-9]+)$" .Values.global.hubble.metricsServer "${1}" }}
+          hostPort: {{ regexReplaceAll ":([0-9]+)$" .Values.global.hubble.metricsServer "${1}" }} 


I'm not sure if this should be addressed here or in a follow up. For consistency across hubble and cilium, the user should specify the metrics server in the same way it configures it for Cilium. --set global.hubble.metricsServer is setting IP and port. In Cilium there's a flag dedicated for the address and another dedicated for port. cc @gandro

I agree, but let's do that in a follow-up.

So we will address this in the follow-up

install/kubernetes/cilium/charts/agent/templates/svc.yaml

install/kubernetes/cilium/charts/hubble-ui/templates/deployment.yaml

install/kubernetes/cilium/charts/hubble-ui/templates/clusterrolebinding.yaml

install/kubernetes/cilium/charts/hubble-ui/templates/serviceaccount.yaml

install/kubernetes/cilium/charts/hubble-ui/templates/svc.yaml

install/kubernetes/cilium/values.yaml

install/kubernetes/cilium/charts/hubble-ui/templates/deployment.yaml

gandro · 2020-04-28T11:52:58Z

@soumynathan Can you please add the hubble-ui chart to https://github.com/cilium/cilium/blob/master/install/kubernetes/cilium/requirements.yaml ?

soumynathan · 2020-04-28T14:26:12Z

@soumynathan Can you please add the hubble-ui chart to https://github.com/cilium/cilium/blob/master/install/kubernetes/cilium/requirements.yaml ?

Done.

gandro

Thanks @soumynathan - only two small changes requested.

install/kubernetes/cilium/charts/hubble-ui/Chart.yaml

install/kubernetes/cilium/requirements.yaml

This patch adds hubble related helm charts to cilium install/kubernetes Fixes: cilium#10647 Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>

aanm · 2020-04-29T07:39:05Z

test-me-please

gandro

Looking good! 🎉

rolinh

LGTM. I've added comments for a few points that need to be addressed in a follow-up PR once hubble-relay is included in the charts.

rolinh · 2020-04-29T13:04:34Z

install/kubernetes/cilium/charts/config/templates/configmap.yaml

+{{- end }}
+{{ if and .Values.global.hubble.ui.enabled (not (has "0.0.0.0:50051" .Values.global.hubble.listenAddresses)) }}
+  # A space separated list of additional addresses for Hubble server to listen to.
+  hubble-listen-addresses: {{ append .Values.global.hubble.listenAddresses "0.0.0.0:50051" | join " " | quote }}


Note to self: port 50051 will not work with hubble-relay. This needs to be addressed once hubble-relay is included in the charts.
cc @michi-covalent (as I think you're already working on the charts for hubble-relay).

rolinh · 2020-04-29T13:08:50Z

install/kubernetes/cilium/charts/hubble-ui/templates/svc.yaml

+    k8s-app: hubble-ui
+  ports:
+    - name: http
+      port: 12000


I think we want port 80/443 for hubble-ui but this can be addressed in a follow-up PR (cc @geakstr @michi-covalent)

This PR adds a new helm chart for Hubble Relay deployment/service. A few things to note: - Each Hubble Relay pod must be scheduled on a node with Cilium running. Hubble Relay connects to the hubble unix domain socket to retrive peer information. - For now the readiness/liveness probes simply checks if the gRPC port is open since Hubble Relay doesn't have the status command yet. Closes: #11226 Ref: #11192 #10648 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

soumynathan requested a review from a team March 20, 2020 05:52

maintainer-s-little-helper bot added the dont-merge/needs-release-note label Mar 20, 2020

maintainer-s-little-helper bot added this to In progress in 1.8.0 Mar 20, 2020

aanm requested changes Mar 20, 2020

View reviewed changes

soumynathan force-pushed the add-hubble-helm-charts-to-cilium branch from 1e11bbb to c72fc09 Compare March 20, 2020 17:58

aanm added pending-review release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Mar 20, 2020

maintainer-s-little-helper bot removed the dont-merge/needs-release-note label Mar 20, 2020

joestringer added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 23, 2020

soumynathan force-pushed the add-hubble-helm-charts-to-cilium branch from c72fc09 to 7c2112d Compare March 23, 2020 19:06

joestringer removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 23, 2020

aanm requested changes Mar 25, 2020

View reviewed changes

install/kubernetes/cilium/charts/hubble/values.yaml Outdated Show resolved Hide resolved

soumynathan force-pushed the add-hubble-helm-charts-to-cilium branch from 7c2112d to f105b80 Compare March 25, 2020 17:50

soumynathan force-pushed the add-hubble-helm-charts-to-cilium branch from f105b80 to 51607de Compare March 26, 2020 17:22

aanm requested changes Apr 1, 2020

View reviewed changes

gandro requested changes Apr 1, 2020

View reviewed changes

gandro requested changes Apr 24, 2020

View reviewed changes

aanm requested changes Apr 24, 2020

View reviewed changes

soumynathan force-pushed the add-hubble-helm-charts-to-cilium branch 2 times, most recently from 0f8c877 to 462b610 Compare April 27, 2020 16:40

soumynathan force-pushed the add-hubble-helm-charts-to-cilium branch from 462b610 to 10cbec3 Compare April 28, 2020 14:25

gandro requested changes Apr 28, 2020

View reviewed changes

install/kubernetes/cilium/charts/hubble-ui/Chart.yaml Outdated Show resolved Hide resolved

install/kubernetes/cilium/requirements.yaml Outdated Show resolved Hide resolved

soumynathan force-pushed the add-hubble-helm-charts-to-cilium branch from 10cbec3 to ad879b8 Compare April 28, 2020 14:52

Add hubble helm charts to cilium install/kubernetes

ef7360c

This patch adds hubble related helm charts to cilium install/kubernetes Fixes: cilium#10647 Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>

soumynathan force-pushed the add-hubble-helm-charts-to-cilium branch from ad879b8 to ef7360c Compare April 28, 2020 14:53

aanm requested review from gandro, rolinh and aanm April 29, 2020 07:36

aanm approved these changes Apr 29, 2020

View reviewed changes

gandro approved these changes Apr 29, 2020

View reviewed changes

rolinh approved these changes Apr 29, 2020

View reviewed changes

rolinh mentioned this pull request Apr 29, 2020

hubble-relay: update helm charts to deploy hubble-relay #11226

Closed

michi-covalent mentioned this pull request Apr 29, 2020

helm: Add a chart for hubble-relay #11244

Merged

gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 30, 2020

qmonnet merged commit 0ad14e8 into cilium:master Apr 30, 2020

1.8.0 automation moved this from In progress to Merged Apr 30, 2020

gandro mentioned this pull request Apr 30, 2020

hubble-ui: Follow-up items for the Helm chart #11250

Closed

3 tasks

		- containerPort: {{ regexReplaceAll ":([0-9]+)$" .Values.global.hubble.metricsServer "${1}" }}
		hostPort: {{ regexReplaceAll ":([0-9]+)$" .Values.global.hubble.metricsServer "${1}" }}

Add hubble helm charts to cilium install/kubernetes #10648

Add hubble helm charts to cilium install/kubernetes #10648

Conversation

soumynathan commented Mar 20, 2020

maintainer-s-little-helper bot commented Mar 20, 2020

coveralls commented Mar 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aanm commented Mar 20, 2020

soumynathan commented Mar 20, 2020

joestringer commented Mar 23, 2020

aanm commented Mar 24, 2020 • edited by joestringer

joestringer commented Mar 24, 2020

soumynathan commented Mar 25, 2020

soumynathan commented Mar 25, 2020

aanm commented Mar 26, 2020

gandro commented Mar 26, 2020

tgraf commented Mar 30, 2020

aanm commented Mar 30, 2020

gandro commented Mar 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gandro Apr 8, 2020 • edited

Choose a reason for hiding this comment

gandro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gandro commented Apr 28, 2020

soumynathan commented Apr 28, 2020

gandro left a comment

Choose a reason for hiding this comment

aanm commented Apr 29, 2020

gandro left a comment

Choose a reason for hiding this comment

rolinh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 20, 2020 •

edited

aanm commented Mar 24, 2020 •

edited by joestringer

gandro commented Mar 30, 2020 •

edited

gandro Apr 8, 2020 •

edited