refactor and cleanup cilium helm chart #16752

dungdm93 · 2021-07-02T04:03:59Z

Signed-off-by: Đặng Minh Dũng dungdm93@live.com

Cilium helm charts is quite large (>70 files) with lots of components but all that file are in flatten structure. Some how, it make hard to organized and hard to add new feature. So this PR aim to refactor and cleanup cilium helm chart with following notable changes:

Restructure folder: move each component in it own folder
Remove deprecated & unused resources:

Delete hubble-ca-configmap.yaml, it has bean marked as deprecated and will be removed in v1.11
Delete cilium-etcd-sa Service Account, etcd-operator ClusterRole/ClusterRoleBinding which are use in nowhere.

Refactor tls logic: provided, helm and cronjob.
Currently, this chart support 3 different kind of TLS: provided, helm-gen, cronjob-gen. But the logic between those 3 types is not the same, for example: helm-gen method always generate both CA and certs (ignore user provided), but cronjob-gen reuse and disable generate cert when user provide CA/cert (this mean no cert auto-rotate).

cilium/install/kubernetes/cilium/templates/_hubble-generate-certs-job-spec.tpl

Lines 47 to 53 in 703b38f

    
           {{- if and .Values.hubble.relay.tls.client.cert .Values.hubble.relay.tls.client.key $hubbleCAProvided }} 
        
           - "--hubble-relay-client-cert-generate=false" 
        
           {{- else }} 
        
           - "--hubble-relay-client-cert-generate=true" 
        
           - "--hubble-relay-client-cert-validity-duration={{ $certValiditySecondsStr }}" 
        
           - "--hubble-relay-client-cert-secret-name=hubble-relay-client-certs" 
        
           {{- end }}

So I simplify and unify this logic as following:

Split each tls method to its own folder:
When tls provided (not auto.enabled), both cert and key are required
When tls auto (both helm and cronjob methods), if user provide CA, use it; else generate new one. Other client/server/remote TLS are generated.

cleanup code
- Move nodeinit prestop and startup scripts to external file (inside files/nodeinit folder) aim to better highlight and easier to update
- Indent go template and use nindent (instead of indent) for better look and easier to collapse/expand when using IDE
- Using include "cilium.image" named template for generate docker image name
- Support priorityClassName. priorityClassName is defined in values.yaml but used in nowhere. Now it's supported.

aanm · 2021-07-02T04:29:16Z

test-me-please

gandro · 2021-07-05T14:24:30Z

This is amazing work, and I think I agree with all improvements listed in the PR description. Thank you very much!

However, due to the large size of the PR and the fact that we have little static safety checks on Helm, this will take a while to be reviewed. I therefore a have a request:

Could you split you this PR into a few smaller commits? For example have one commit per newly added folder (i.e. one commit for all cilium-agent/ related changes, one commit for all hubble/ related changes etc)? I think this will make the review such much easier.

gandro · 2021-07-05T14:29:34Z

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

-{{- /* Workaround so that we can set the minimal k8s version that we support */ -}}
-{{- $k8sVersion := .Capabilities.KubeVersion.Version -}}
-{{- $k8sMajor := .Capabilities.KubeVersion.Major -}}
-{{- $k8sMinor := .Capabilities.KubeVersion.Minor -}}
-
-{{- if .Values.Capabilities -}}
-{{- if .Values.Capabilities.KubeVersion -}}
-{{- if .Values.Capabilities.KubeVersion.Version -}}
-{{- $k8sVersion = .Values.Capabilities.KubeVersion.Version -}}
-{{- if .Values.Capabilities.KubeVersion.Major -}}
-{{- $k8sMajor = toString (.Values.Capabilities.KubeVersion.Major) -}}
-{{- if .Values.Capabilities.KubeVersion.Minor -}}
-{{- $k8sMinor = toString (.Values.Capabilities.KubeVersion.Minor) -}}
-{{- end -}}
-{{- end -}}
-{{- end -}}
-{{- end -}}
-{{- end -}}


I haven't a real review yet, but this caught my eye - please do not remove these. They are here on purpose.

All .Capabilities.KubeVersion checks must be overwritable via .Values.Capabilities.KubeVersion, see #14778

Hello @gandro

.Values.Capabilities.KubeVersion was used to generate quick-install.yaml, but since cilium 1.10 MIN_K8S_VERSION is not using in install/kubernetes/Makefile anymore.

Furthermore, since 3.6, helm template support set Kubernetes version used for Capabilities.KubeVersion by using --kube-version string. Do you think it's safe to remove this workaround?

/cc @aanm

That's a fair point. I think we have a few users using helm template, so it's not just the quick-install.yaml - but I wasn't aware of the feature in Helm 3.6, that will be a good replacement going forward.

But I'll let @aanm chime in here as well. Removing Helm values is usually a breaking change, so we if we remove this, we want to communicate this clearly.

@dungdm93 @gandro For reference, the commit that introduced this change was 4cc699d. So yeah it looks that it is fine to remove these lines as long we update the requirement of helm to >= 3.6.

Thank @aanm, I will make this change right after #16795 is merged

gandro · 2021-07-05T14:30:29Z

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

+        {{- with .Values.extraArgs }}
+        {{- toYaml . | trim | nindent 8 }}
+        {{- end }}
+        {{- if semverCompare ">=1.20-0" .Capabilities.KubeVersion.Version }}


As mentioned above, we do want to use $k8sVersion here, not .Capabilities.KubeVersion.Version

* Restructure folder: move each component in it own folder * Remove deprecated & unused resources * Refactor tls logic: provided, helm and cronjob * cleanup code Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

dungdm93 · 2021-07-05T17:02:54Z

Could you split you this PR into a few smaller commits? For example have one commit per newly added folder (i.e. one commit for all cilium-agent/ related changes, one commit for all hubble/ related changes etc)? I think this will make the review such much easier.

@gandro Thanks for your suggestion. I going to close this one and split it into few smaller PR

gandro · 2021-07-05T17:12:35Z

Could you split you this PR into a few smaller commits? For example have one commit per newly added folder (i.e. one commit for all cilium-agent/ related changes, one commit for all hubble/ related changes etc)? I think this will make the review such much easier.

@gandro Thanks for your suggestion. I going to close this one and split it into few smaller PR

Ah! So I was actually suggesting to have a single PR with multiple commits. But multiple PRs also work, and might be even better.

dungdm93 requested a review from a team as a code owner July 2, 2021 04:04

dungdm93 requested review from a team and errordeveloper July 2, 2021 04:04

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 2, 2021

maintainer-s-little-helper bot assigned errordeveloper Jul 2, 2021

aanm requested review from gandro and seanmwinn July 2, 2021 04:27

maintainer-s-little-helper bot assigned gandro and seanmwinn Jul 2, 2021

aanm added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jul 2, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 2, 2021

aanm self-requested a review July 2, 2021 04:28

maintainer-s-little-helper bot assigned aanm Jul 2, 2021

dungdm93 requested review from a team as code owners July 2, 2021 05:02

dungdm93 requested a review from nebril July 2, 2021 05:02

maintainer-s-little-helper bot assigned nebril Jul 2, 2021

aanm added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 5, 2021

gandro requested changes Jul 5, 2021

View reviewed changes

refactor and cleanup cilium helm chart:

57cca18

* Restructure folder: move each component in it own folder * Remove deprecated & unused resources * Refactor tls logic: provided, helm and cronjob * cleanup code Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

dungdm93 closed this Jul 5, 2021

dungdm93 mentioned this pull request Jul 5, 2021

refactor and cleanup cilium helm chart #16792

Closed

dungdm93 mentioned this pull request Jul 15, 2021

cleanup helm chart #16896

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor and cleanup cilium helm chart #16752

refactor and cleanup cilium helm chart #16752

dungdm93 commented Jul 2, 2021 •

edited

aanm commented Jul 2, 2021

gandro commented Jul 5, 2021 •

edited

gandro Jul 5, 2021

dungdm93 Jul 5, 2021 •

edited

gandro Jul 5, 2021

aanm Jul 6, 2021

dungdm93 Jul 7, 2021

gandro Jul 5, 2021

dungdm93 commented Jul 5, 2021 •

edited

gandro commented Jul 5, 2021

	{{- if and .Values.hubble.relay.tls.client.cert .Values.hubble.relay.tls.client.key $hubbleCAProvided }}
	- "--hubble-relay-client-cert-generate=false"
	{{- else }}
	- "--hubble-relay-client-cert-generate=true"
	- "--hubble-relay-client-cert-validity-duration={{ $certValiditySecondsStr }}"
	- "--hubble-relay-client-cert-secret-name=hubble-relay-client-certs"
	{{- end }}

refactor and cleanup cilium helm chart #16752

refactor and cleanup cilium helm chart #16752

Conversation

dungdm93 commented Jul 2, 2021 • edited

aanm commented Jul 2, 2021

gandro commented Jul 5, 2021 • edited

gandro Jul 5, 2021

Choose a reason for hiding this comment

dungdm93 Jul 5, 2021 • edited

Choose a reason for hiding this comment

gandro Jul 5, 2021

Choose a reason for hiding this comment

aanm Jul 6, 2021

Choose a reason for hiding this comment

dungdm93 Jul 7, 2021

Choose a reason for hiding this comment

gandro Jul 5, 2021

Choose a reason for hiding this comment

dungdm93 commented Jul 5, 2021 • edited

gandro commented Jul 5, 2021

dungdm93 commented Jul 2, 2021 •

edited

gandro commented Jul 5, 2021 •

edited

dungdm93 Jul 5, 2021 •

edited

dungdm93 commented Jul 5, 2021 •

edited