Restructure helm chart into components #16795

dungdm93 · 2021-07-05T17:31:38Z

This is the first part of refactoring helm chart #16792 with the following changes:

Restructure folder: move each component in it own folder
move nodeinit startup & prestop scripts to separated files

dungdm93 · 2021-07-05T17:36:01Z

gandro

Awesome, thanks you so much for splitting this up, makes it much easier to review. Overall looks good to me, I think there is a larger discussion to be had on how to do the etcd.managed removal, as I think there is more to it than just removing the service accounts.

gandro · 2021-07-06T14:06:25Z

install/kubernetes/cilium/templates/etcd-operator-serviceaccount.yaml

-apiVersion: v1
-kind: ServiceAccount
-metadata:
-  name: cilium-etcd-sa


This looks unused, but it's actually not. There are references to it here (different repository, but deployed as part of the cilium-etcd-operator):
https://github.com/cilium/cilium-etcd-operator/blob/fdf1993dded89db77742916a9e55ef898a4e5ad0/etcd-operator/descriptors.go#L62

Cilium also has a a well-known identity for this SA as well:

cilium/pkg/identity/numericidentity.go

Lines 171 to 176 in 4f27865

WellKnown.add(ReservedETCDOperator, []string{

"k8s:io.cilium/app=etcd-operator",

fmt.Sprintf("k8s:%s=%s", api.PodNamespaceLabel, c.CiliumNamespaceName()),

fmt.Sprintf("k8s:%s=cilium-etcd-sa", api.PolicyLabelServiceAccount),

fmt.Sprintf("k8s:%s=%s", api.PolicyLabelCluster, c.LocalClusterName()),

})

I think we want to remove managed-etcd in Cilium 1.11 (see #15464), as it's already deprecated. So this file here will have to go, but there is more that than just deleting this file. We also need to

Remove the cilium-etcd-operator deployment from the helm chart

Remove all references to .Values.etcd.managed

Mention the removal in the upgrade guide

Ensure there are no backwards-compatibility issues

Might be worth doing the etcd.managed removal in a separate PR, so we have separation of concerns and better code reviews.

Thank you for clarify. I changed it back and let etcd.managed removal in another PR 💯

.github/workflows/conformance-k8s-network-policies.yaml

gandro

In terms of correctness, this looks good to me! But I'll let the Helm team members chime in as well, as to if they agree with the approach.

gandro · 2021-07-07T08:46:06Z

test-me-please

sayboras

Small comments as per below, I am still on the fence about moving script to separate files as unlike envoy file, these files are having mix syntax now.

The rest looks good.

sayboras · 2021-07-07T12:57:48Z

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

+          imagePullPolicy: {{ .Values.nodeinit.image.pullPolicy }}
+          securityContext:
+            privileged: true
+{{- if .Values.nodeinit.revertReconfigureKubelet }}


not related to this PR, but I noticed this attribute is not mentioned in values.yaml (even commented). So something for later.

sayboras · 2021-07-07T13:07:28Z

install/kubernetes/cilium/files/nodeinit/prestop.bash

@@ -0,0 +1,49 @@
+#!/bin/bash


I am not sure if this change makes it's easier to read bash script, as this file is now mixed with go template and bash.

I checked this one with below commands and compared the diff, seems like there are some changes with indentation (e.g. tab, space, etc), it might not be issue, but no harm to have second 👀

# From this branch helm template ./cilium --set nodeinit.enabled=true --set nodeinit.revertReconfigureKubelet=true > new.yaml # From the master helm template ./cilium --set nodeinit.enabled=true --set nodeinit.revertReconfigureKubelet=true > original.yaml diff original.yaml new.yaml

Thank @sayboras , I fixed the spaces in nodeinit scripts

And yeah, I know that script is now mixed with go template and bash, but with code highlight from IDE, I think it's much easier to read than previous:
Old:

New:

christarazi · 2021-07-08T18:41:47Z

test-me-please

dungdm93 · 2021-07-09T14:32:49Z

Hello @christarazi, could you please check the CI fails, It seem not the helm error.

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

christarazi · 2021-07-09T17:17:29Z

test-me-please

Each time you push, the previous CI run results are hidden, so I've re-triggered them

sayboras · 2021-07-11T23:15:44Z

FYI. Conformance AWS-CNI (ci-awscni) was failed before on this branch, re-run was successful.

christarazi

LGTM overall. We will need to make changes to the upgrades e2e test to map between the old Helm chart structure and the new, like we did for #13259 (and potentially any followups after that PR).

In terms of CI, it looks like https://jenkins.cilium.io/job/Cilium-PR-K8s-1.21-kernel-4.9/1025 is a case of endpoint-routes mode being enabled in the previous test run and the pods needing to be restarted when disabling endpoint-routes mode[1]. This is what #16835 is supposed to fix. In other words, looks like CI has passed and the only failure is unrelated to this PR.

[1]: See the console output, find the failure, then scroll up to the previous test. You'll notice that it mentions enabling endpoint-routes mode.

christarazi

Actually it seems like the updates e2e test is already passing so no further steps are required. Thanks! 🎉

joestringer · 2021-07-13T17:47:03Z

We will need to make changes to the upgrades e2e test to map between the old Helm chart structure and the new, like we did for #13259 (and potentially any followups after that PR).

@christarazi I take it from your most recent comment this is not necessary? I was under the impression that this was a purely developer-focused change to make it easier to maintain, and shouldn't impact users at all.

christarazi · 2021-07-13T20:22:00Z

@joestringer Yeah don't think it's necessary and the tests are passing anyway. I thought it would have been required because I thought any change to the structure of the chart would cause problems, but apparently not.

sayboras

LGTM 💯

joestringer · 2021-07-13T23:29:24Z

Only failure in e2e is the one fixed by #16835. 👍

joestringer

@cilium/github-sec owner pieces LGTM. The rest should be good based on @sayboras @christarazi reviews.

joestringer · 2021-07-13T23:32:10Z

Since this seems to be not user-facing, I've switched the release-note to misc. If you think this is user facing, please respond and we can switch it back (would also help to describe why it's user-facing or what users might notice).

gandro · 2021-07-19T08:57:07Z

Since this seems to be not user-facing, I've switched the release-note to misc. If you think this is user facing, please respond and we can switch it back (would also help to describe why it's user-facing or what users might notice).

FWIW, I added the minor label. The initial version of the PR removed some deprecated Helm values around managed etcd, but that has since been reverted, so I think misc is absolutely correct here.

[ upstream commit 27a93e8 ] [ Backporter's notes: conflicts due to structure around daemonset manifest having changed in cilium#16795, which was not backported to v1.10. Changes have been manually reapplied where appropriate in `install/kubernetes/cilium/templates/cilium-nodeinit-daemonset.yaml` and `install/kubernetes/cilium/templates/cilium-agent-daemonset.yaml`. Please note in particular the Helm condition in that last file is different than in `master`, even though it should not have any impact. ] Fixes: cilium#14483 Signed-off-by: Raphaël Pinson <raphael@isovalent.com> Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>

[ upstream commit b38d591 ] [ Backporter's notes: conflicts due to structure around daemonset manifest having changed in cilium#16795, which was not backported to v1.10. Changes have been manually reapplied where appropriate in `install/kubernetes/cilium/templates/cilium-nodeinit-daemonset.yaml`. ] Take advantage of the dynamic CHECKPOINT_PATH in the startup-script: https://github.com/cilium/image-tools/blob/396d5a19690229b42398a8be63b5712ea64a2601/images/startup-script/manage-startup-script.sh#L58 Signed-off-by: Raphaël Pinson <raphael@isovalent.com> Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>

[ upstream commit 27a93e8 ] [ Backporter's notes: conflicts due to structure around daemonset manifest having changed in #16795, which was not backported to v1.10. Changes have been manually reapplied where appropriate in `install/kubernetes/cilium/templates/cilium-nodeinit-daemonset.yaml` and `install/kubernetes/cilium/templates/cilium-agent-daemonset.yaml`. Please note in particular the Helm condition in that last file is different than in `master`, even though it should not have any impact. ] Fixes: #14483 Signed-off-by: Raphaël Pinson <raphael@isovalent.com> Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>

[ upstream commit b38d591 ] [ Backporter's notes: conflicts due to structure around daemonset manifest having changed in #16795, which was not backported to v1.10. Changes have been manually reapplied where appropriate in `install/kubernetes/cilium/templates/cilium-nodeinit-daemonset.yaml`. ] Take advantage of the dynamic CHECKPOINT_PATH in the startup-script: https://github.com/cilium/image-tools/blob/396d5a19690229b42398a8be63b5712ea64a2601/images/startup-script/manage-startup-script.sh#L58 Signed-off-by: Raphaël Pinson <raphael@isovalent.com> Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>

dungdm93 requested a review from a team as a code owner July 5, 2021 17:31

dungdm93 requested review from a team and errordeveloper July 5, 2021 17:31

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 5, 2021

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

dungdm93 requested review from a team as code owners July 5, 2021 17:33

dungdm93 requested review from christarazi and joestringer July 5, 2021 17:33

maintainer-s-little-helper bot assigned christarazi and joestringer Jul 5, 2021

rolinh assigned gandro Jul 5, 2021

gandro requested changes Jul 6, 2021

View reviewed changes

dungdm93 mentioned this pull request Jul 7, 2021

refactor and cleanup cilium helm chart #16752

Closed

gandro approved these changes Jul 7, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned gandro Jul 7, 2021

gandro added area/helm Impacts helm charts and user deployment experience release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jul 7, 2021

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

sayboras reviewed Jul 7, 2021

View reviewed changes

dungdm93 added 2 commits July 9, 2021 21:34

restructure helm chart into components

42a7ef0

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

move nodeinit startup & prestop scripts to separated files

68ca76b

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

christarazi reviewed Jul 12, 2021

View reviewed changes

christarazi approved these changes Jul 13, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned christarazi Jul 13, 2021

sayboras approved these changes Jul 13, 2021

View reviewed changes

joestringer approved these changes Jul 13, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned joestringer Jul 13, 2021

joestringer merged commit 2acf253 into cilium:master Jul 13, 2021

joestringer added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jul 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure helm chart into components #16795

Restructure helm chart into components #16795

dungdm93 commented Jul 5, 2021 •

edited

dungdm93 commented Jul 5, 2021

gandro left a comment

gandro Jul 6, 2021

dungdm93 Jul 7, 2021

gandro left a comment

gandro commented Jul 7, 2021

sayboras left a comment

sayboras Jul 7, 2021

sayboras Jul 7, 2021

dungdm93 Jul 7, 2021

christarazi commented Jul 8, 2021

dungdm93 commented Jul 9, 2021

christarazi commented Jul 9, 2021 •

edited

sayboras commented Jul 11, 2021

christarazi left a comment •

edited

christarazi left a comment

joestringer commented Jul 13, 2021

christarazi commented Jul 13, 2021

sayboras left a comment

joestringer commented Jul 13, 2021

joestringer left a comment

joestringer commented Jul 13, 2021

gandro commented Jul 19, 2021

	WellKnown.add(ReservedETCDOperator, []string{
	"k8s:io.cilium/app=etcd-operator",
	fmt.Sprintf("k8s:%s=%s", api.PodNamespaceLabel, c.CiliumNamespaceName()),
	fmt.Sprintf("k8s:%s=cilium-etcd-sa", api.PolicyLabelServiceAccount),
	fmt.Sprintf("k8s:%s=%s", api.PolicyLabelCluster, c.LocalClusterName()),
	})

Restructure helm chart into components #16795

Restructure helm chart into components #16795

Conversation

dungdm93 commented Jul 5, 2021 • edited

dungdm93 commented Jul 5, 2021

gandro left a comment

Choose a reason for hiding this comment

gandro Jul 6, 2021

Choose a reason for hiding this comment

dungdm93 Jul 7, 2021

Choose a reason for hiding this comment

gandro left a comment

Choose a reason for hiding this comment

gandro commented Jul 7, 2021

sayboras left a comment

Choose a reason for hiding this comment

sayboras Jul 7, 2021

Choose a reason for hiding this comment

sayboras Jul 7, 2021

Choose a reason for hiding this comment

dungdm93 Jul 7, 2021

Choose a reason for hiding this comment

christarazi commented Jul 8, 2021

dungdm93 commented Jul 9, 2021

christarazi commented Jul 9, 2021 • edited

sayboras commented Jul 11, 2021

christarazi left a comment • edited

Choose a reason for hiding this comment

christarazi left a comment

Choose a reason for hiding this comment

joestringer commented Jul 13, 2021

christarazi commented Jul 13, 2021

sayboras left a comment

Choose a reason for hiding this comment

joestringer commented Jul 13, 2021

joestringer left a comment

Choose a reason for hiding this comment

joestringer commented Jul 13, 2021

gandro commented Jul 19, 2021

dungdm93 commented Jul 5, 2021 •

edited

christarazi commented Jul 9, 2021 •

edited

christarazi left a comment •

edited