helm chart: add helm option to customize nodeinit scripts #24375

mblaschke · 2023-03-15T00:45:55Z

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing! 🎉

implement a way to customize cilium nodeinit script to be able to adapt it to different environments before cilium is started. might be a possible way for workaround in #18706

add helm option to customize nodeinit scripts

brandshaide · 2023-03-15T07:55:57Z

AFAIK this is already being addressed with #24288 - but would be nice as a feature not in particular related to the linked issue 👍

gandro

Overall looks good to me! Thanks

gandro

Seems like there is an issue with the generated artifacts:

From CI:

+++ /home/runner/work/cilium/cilium/install/kubernetes/cilium/values.yaml	2023-03-15 12:38:31.571414468 +0000
@@ -2168,7 +2168,7 @@
   # -- bootstrapFile is the location of the file where the bootstrap timestamp is
   # written by the node-init DaemonSet
   bootstrapFile: "/tmp/cilium-bootstrap.d/cilium-bootstrap-time"
-  
+
   # -- startup offers way to customize startup nodeinit script (pre and post position)
   startup:
    preScript: ""

error: cilium/values.yaml has been modified
Make sure to apply your changes to cilium/values.yaml.tmpl and run 'make -C install/kubernetes cilium/values.yaml' to regenerate.
make: *** [Makefile:49: check-values-yaml] Error 1
make: Leaving directory '/home/runner/work/cilium/cilium/install/kubernetes'

mblaschke · 2023-03-16T01:06:49Z

@gandro done

gandro · 2023-03-16T09:13:50Z

Thank you. There still seems to be an issue with some generated artifacts, should be fixed if you run make -C install/kubernetes.

Could you also please squash the commits into on? Thanks!

diff --git a/install/kubernetes/cilium/README.md b/install/kubernetes/cilium/README.md
index 162173fcaf83..9581ca3f6a86 100644
--- a/install/kubernetes/cilium/README.md
+++ b/install/kubernetes/cilium/README.md
@@ -464,9 +464,11 @@ contributors across the globe, there is almost always someone available to help.
 | nodeinit.nodeSelector | object | `{"kubernetes.io/os":"linux"}` | Node labels for nodeinit pod assignment ref: https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#nodeselector |
 | nodeinit.podAnnotations | object | `{}` | Annotations to be added to node-init pods. |
 | nodeinit.podLabels | object | `{}` | Labels to be added to node-init pods. |
+| nodeinit.prestop | object | `{"postScript":"","preScript":""}` | prestop offers way to customize prestop nodeinit script (pre and post position) |
 | nodeinit.priorityClassName | string | `""` | The priority class to use for the nodeinit pod. |
 | nodeinit.resources | object | `{"requests":{"cpu":"100m","memory":"100Mi"}}` | nodeinit resource limits & requests ref: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ |
 | nodeinit.securityContext | object | `{"capabilities":{"add":["SYS_MODULE","NET_ADMIN","SYS_ADMIN","SYS_CHROOT","SYS_PTRACE"]},"privileged":false,"seLinuxOptions":{"level":"s0","type":"spc_t"}}` | Security context to be added to nodeinit pods. |
+| nodeinit.startup | object | `{"postScript":"","preScript":""}` | startup offers way to customize startup nodeinit script (pre and post position) |
 | nodeinit.tolerations | list | `[{"operator":"Exists"}]` | Node tolerations for nodeinit scheduling to nodes with taints ref: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/ |
 | nodeinit.updateStrategy | object | `{"type":"RollingUpdate"}` | node-init update strategy |
 | operator.affinity | object | `{"podAntiAffinity":{"requiredDuringSchedulingIgnoredDuringExecution":[{"labelSelector":{"matchLabels":{"io.cilium/app":"operator"}},"topologyKey":"kubernetes.io/hostname"}]}}` | Affinity for cilium-operator |

gandro · 2023-03-16T13:11:11Z

Hopefully the last issue: Documentation lint is failing because of prestop missing in the wordlist. To fix it, run Documentation/update-spelling_wordlist.sh prestop . Thank you.

 Please fix the following spelling mistakes:
* Documentation/helm-values.rst:1656: (prestop)  nodeinit.prestop
* Documentation/helm-values.rst:1657: (prestop)  prestop offers way to customize prestop nodeinit script (pre and post position)
* Documentation/helm-values.rst:1657: (prestop)  prestop offers way to customize prestop nodeinit script (pre and post position)
* Documentation/helm-values.rst:1656: (prestop)  nodeinit.prestop
* Documentation/helm-values.rst:1657: (prestop)  prestop offers way to customize prestop nodeinit script (pre and post position)
* Documentation/helm-values.rst:1657: (prestop)  prestop offers way to customize prestop nodeinit script (pre and post position)

If the words are not misspelled, run:
Documentation/update-spelling_wordlist.sh prestop

gandro

Looks good to me, assuming CI also agrees

gandro · 2023-03-16T17:45:32Z

/test

ravilr · 2023-03-17T07:01:59Z

this ability to run custom script in node-init startup daemonset container is indeed very useful. Thanks @mblaschke

On AWS EKS clusters, which come online with aws-vpc-cni addon installed automatically (EKS cluster create API still don't provide any way to disable the addon during cluster create, only allows disabling after the addon is installed : aws/containers-roadmap#923 open for a long time without resolution), running cilium in overlay routing mode (tunnel=enabled and not eni enabled) requires cleaning up some leftover residual iptables rules created by aws-vpc-cni (deleting and disabling the aws-vpc-cni addon doesn't cleanup some iptables rules) as AWS-SNAT-CHAIN-* rules intercepts all cilium's vxlan overlay traffic out of the node.
Right now, this cleanup is only being done for cilium installed in aws eni enabled mode: https://github.com/cilium/cilium/blob/v1.12.7/install/kubernetes/cilium/files/nodeinit/poststart-eni.bash#L19

This PR allows the cilium chart users to customize the startup script in node-init container by including the above cleanup bits as chart value override, when installing Cilium on EKS in overlay routing mode (tunnel=enabled) and solves a real problem that exists today.

@gandro due to above reason, any chances of this PR getting cherry-picked after this merge to cilium-v1.12 and v1.13 release branches also ? Thanks.

qmonnet

Change looks good but I would love to have a bit more context in the commit description, ideally with an example use case.

ravilr · 2023-03-18T01:21:44Z

this is how we intend to use this for AWS EKS setups (with Cilium tunnel=enabled overlay routing mode):

cilium helm chart's values_override.yaml snippet:

nodeinit:
  enabled: true
  startup:
    preScript: |-
      if [[ "$(iptables-save | egrep -c 'AWS-SNAT-CHAIN|AWS-CONNMARK')" != "0" ]];
      then
        echo 'Deleting iptables rules created by the AWS CNI VPC plugin'
        iptables-save | grep -v AWS-SNAT-CHAIN |grep -v AWS-CONNMARK | iptables-restore
      fi

mblaschke · 2023-03-18T09:54:41Z

add systemd settings before cilium is starting to avoid that systemd is cleaning up stuff

in managed k8s clusters there is no way to proper provision is right before cilium is starting, so that's the use case.

also according to the comment here https://github.com/cilium/cilium/blob/master/install/kubernetes/cilium/templates/cilium-nodeinit/daemonset.yaml#L66 it gave me the feeling that i could customize the startup script but that's not possible as it's hardcoded in the chart./

gandro · 2023-03-20T09:22:10Z

in managed k8s clusters there is no way to proper provision is right before cilium is starting, so that's the use case.

I think there are ways around this, for example you can create the nodes with a custom node taint and have a custom DaemonSet configure the nodes for you and then remove the taint once the nodes are ready for Cilium. Cilium's getting started docs itself use a similar trick to avoid cloud CNIs for being installed on the nodes too. Or you can use a custom EKS bootstrap command (overrideBootstrapCommand in eksctl) to achieve similar things too.

But I do think it makes sense to allow some customization in the Helm charts, so I'm fine with the change as is.

@gandro due to above reason, any chances of this PR getting cherry-picked after this merge to cilium-v1.12 and v1.13 release branches also ? Thanks.

According to your back-porting criteria, we only backport bug fixes, for which I don't think this PR qualifies. Sorry.

gandro · 2023-03-20T09:25:49Z

this is how we intend to use this for AWS EKS setups (with Cilium tunnel=enabled overlay routing mode):

cilium helm chart's values_override.yaml snippet:
nodeinit:
  enabled: true
  startup:
    preScript: |-
      if [[ "$(iptables-save | egrep -c 'AWS-SNAT-CHAIN|AWS-CONNMARK')" != "0" ]];
      then
        echo 'Deleting iptables rules created by the AWS CNI VPC plugin'
        iptables-save | grep -v AWS-SNAT-CHAIN |grep -v AWS-CONNMARK | iptables-restore
      fi

These iptables rules are added by the AWS VPC CNI, right? If I recall correctly, the AWS VPC DaemonSet does respect taints. For that use-case, you might be able to make use of the node.cilium.io/agent-not-ready taint: https://docs.cilium.io/en/v1.13/installation/taints/#taint-effects - I think should prevent the AWS CNI from being deployed.

EKS node groups can be configured with that taint: https://docs.cilium.io/en/v1.13/installation/k8s-install-helm/#install-cilium (check the EKS tab)

qmonnet · 2023-03-20T11:28:25Z

this is how we intend to use this for AWS EKS setups

Thanks for the details! What I meant is that I would like to also have that info in the commit description, not just in the PR discussion, because that's the first place we're looking at when trying to understand the motivation for a code change through the Git history.

ravilr · 2023-03-21T03:55:18Z

These iptables rules are added by the AWS VPC CNI, right? If I recall correctly, the AWS VPC DaemonSet does respect taints. For that use-case, you might be able to make use of the node.cilium.io/agent-not-ready taint

No, that's not the case. aws-vpc-cni daemonset auto-installed by EKS on cluster create, tolerates everything.

Also, the Cilium official docs mentions about these rules cleanup to be done manually for Cilium overlay routing mode on EKS: https://github.com/cilium/cilium/blob/3f9d44ecddb74e37641d779e3944f64abbb65cf4/Documentation/installation/k8s-install-helm.rst#install-cilium (search for AWS-SNAT-CHAIN-0 there).
So, the change in this PR allows taking care of this cleanup as part of the cilium helm installation itself.

gandro · 2023-03-21T09:57:09Z

No, that's not the case. aws-vpc-cni daemonset auto-installed by EKS on cluster create, tolerates everything.

I see, thanks for checking. I guess that's why in our CI, we always manually patch the aws-vpc-cni DaemonSet to not be scheduled on any new nodes.

mblaschke · 2023-03-21T22:03:51Z

In Azure you cannot touch anything which is maintained by Azure because they revert all changes.
Also for future issues this change would be very helpful to be able to apply fixes.

gandro · 2023-03-22T09:43:20Z

Sounds good, I think the approach here is reasonably flexible.

@mblaschke could you add the motivation to the git commit messages as requested by @qmonnet here?
@tommyp1ckles ping for review

mblaschke · 2023-03-22T11:48:52Z

@gandro is this enough?

gandro · 2023-03-22T16:41:27Z

@gandro is this enough?

Looks good to me! Thanks.

Let's wait for the review from sig-k8s and then this should be ready to merge. Previous CI was green.

Allows customization of nodeinit scripts to adapt node configuration before cilium is starting to allow eg. systemd configuration changes and reload of networkd. This customization also allows a possibility to add hotfixes if node VMs needs fixed due to updates. Signed-off-by: Markus Blaschke <mblaschke82@gmail.com>

mblaschke · 2023-03-24T19:21:42Z

did a rebase for ci run

gandro · 2023-03-27T08:43:15Z

/test

gandro · 2023-03-27T11:16:29Z

Runtime hit #24580 - restarting.

gandro · 2023-03-27T11:16:41Z

/test-runtime

mblaschke requested review from a team as code owners March 15, 2023 00:45

mblaschke requested review from tommyp1ckles and gandro March 15, 2023 00:45

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 Mar 15, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 15, 2023

gandro added release-note/misc This PR makes changes that have no direct user impact. area/helm Impacts helm charts and user deployment experience labels Mar 15, 2023

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

gandro approved these changes Mar 15, 2023

View reviewed changes

gandro requested changes Mar 15, 2023

View reviewed changes

mblaschke force-pushed the pre-post-startup branch from b6534d9 to 88a0934 Compare March 15, 2023 22:16

mblaschke force-pushed the pre-post-startup branch 2 times, most recently from 48cbc77 to 6bc09b2 Compare March 16, 2023 11:26

mblaschke force-pushed the pre-post-startup branch from 6bc09b2 to f3a5649 Compare March 16, 2023 13:46

mblaschke requested a review from a team as a code owner March 16, 2023 13:46

mblaschke requested a review from qmonnet March 16, 2023 13:46

gandro approved these changes Mar 16, 2023

View reviewed changes

qmonnet approved these changes Mar 17, 2023

View reviewed changes

qmonnet mentioned this pull request Mar 17, 2023

CNI: move CNI configuration management to the agent, convert to Hive #24389

Merged

mblaschke force-pushed the pre-post-startup branch 2 times, most recently from 67a0c23 to 197da79 Compare March 22, 2023 11:48

mblaschke force-pushed the pre-post-startup branch from 197da79 to fc8e951 Compare March 22, 2023 13:48

tommyp1ckles approved these changes Mar 22, 2023

View reviewed changes

youngnick force-pushed the pre-post-startup branch from fc8e951 to fdc5589 Compare March 22, 2023 23:52

mblaschke force-pushed the pre-post-startup branch from fdc5589 to 8f0a1d1 Compare March 24, 2023 19:21

gandro mentioned this pull request Mar 27, 2023

CI (runtime): route_linux_test.go:173: RouteSuitePrivileged.TestRule_String #24580

Closed

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 27, 2023

julianwiedmann merged commit 07191bf into cilium:master Mar 27, 2023
42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

helm chart: add helm option to customize nodeinit scripts #24375

helm chart: add helm option to customize nodeinit scripts #24375

mblaschke commented Mar 15, 2023

brandshaide commented Mar 15, 2023 •

edited

gandro left a comment

gandro left a comment

mblaschke commented Mar 16, 2023

gandro commented Mar 16, 2023

gandro commented Mar 16, 2023

gandro left a comment •

edited

gandro commented Mar 16, 2023

ravilr commented Mar 17, 2023 •

edited

qmonnet left a comment

ravilr commented Mar 18, 2023

mblaschke commented Mar 18, 2023 •

edited

gandro commented Mar 20, 2023

gandro commented Mar 20, 2023 •

edited

qmonnet commented Mar 20, 2023

ravilr commented Mar 21, 2023 •

edited

gandro commented Mar 21, 2023 •

edited

mblaschke commented Mar 21, 2023

gandro commented Mar 22, 2023 •

edited

mblaschke commented Mar 22, 2023

gandro commented Mar 22, 2023

mblaschke commented Mar 24, 2023

gandro commented Mar 27, 2023

gandro commented Mar 27, 2023

gandro commented Mar 27, 2023

helm chart: add helm option to customize nodeinit scripts #24375

helm chart: add helm option to customize nodeinit scripts #24375

Conversation

mblaschke commented Mar 15, 2023

brandshaide commented Mar 15, 2023 • edited

gandro left a comment

Choose a reason for hiding this comment

gandro left a comment

Choose a reason for hiding this comment

mblaschke commented Mar 16, 2023

gandro commented Mar 16, 2023

gandro commented Mar 16, 2023

gandro left a comment • edited

Choose a reason for hiding this comment

gandro commented Mar 16, 2023

ravilr commented Mar 17, 2023 • edited

qmonnet left a comment

Choose a reason for hiding this comment

ravilr commented Mar 18, 2023

mblaschke commented Mar 18, 2023 • edited

gandro commented Mar 20, 2023

gandro commented Mar 20, 2023 • edited

qmonnet commented Mar 20, 2023

ravilr commented Mar 21, 2023 • edited

gandro commented Mar 21, 2023 • edited

mblaschke commented Mar 21, 2023

gandro commented Mar 22, 2023 • edited

mblaschke commented Mar 22, 2023

gandro commented Mar 22, 2023

mblaschke commented Mar 24, 2023

gandro commented Mar 27, 2023

gandro commented Mar 27, 2023

gandro commented Mar 27, 2023

brandshaide commented Mar 15, 2023 •

edited

gandro left a comment •

edited

ravilr commented Mar 17, 2023 •

edited

mblaschke commented Mar 18, 2023 •

edited

gandro commented Mar 20, 2023 •

edited

ravilr commented Mar 21, 2023 •

edited

gandro commented Mar 21, 2023 •

edited

gandro commented Mar 22, 2023 •

edited