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
helm: Add SA to nodeinit ds #24836
helm: Add SA to nodeinit ds #24836
Conversation
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.
One small comment as per below, the rest looks good to me. Thanks.
install/kubernetes/cilium/templates/cilium-nodeinit/serviceaccount.yaml
Outdated
Show resolved
Hide resolved
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.
Thanks! Could you elaborate why this is needed/what the intended use-case is? Especially since we seem to be enabling this by default with this commit.
Sure. A EE customer is asking for it. As it was already added for the other deployments / daemonsets in this commit 0685982. We can also disable it by default. So that the nodeinit mounts the default SA. |
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.
Thanks! I did a quick check at the used startup scripts and if they need any non-standard RBAC permissions, but does not seem to be the case.
Travis failiure is legitimate:
HINT: to fix this, run 'make -C Documentation update-helm-values' |
Helm smoke test still red: https://github.com/cilium/cilium/actions/runs/4678821697/jobs/8288020398?pr=24836 It doesn't say what exactly the issue is though. Try running 'make -C install/kubernetes' locally and see if there are any changes to the generated files maybe. |
I'm getting the following error when running
|
Apaprently it's an issue on Mac OS. Could do the same thing on Ubuntu. |
Everything is green🎉 |
/test |
@darox The test failures in the extended test suite on AKS, EKS and GKE (all using nodeinit) look legitimate. All cilium-agents seem to be stuck in the
Meanwhile, it seems that nodeinit itself was not deployed due to a service account error:
|
I will have a look at it. Thx |
@gandro How can I find out more about the tests? Did some on EKS and kind. Just made some manual tests and can't confirm the behavior:
|
The cluster you shared does not look healthy, but it seems to have created the service account at least. Can you share the instructions you've used? Generally speaking, I would expect this to be reproducible if you use the Helm chart from this branch and the necessary ENI Helm fags. The workflow file shows the commands we're using in CI: https://github.com/cilium/cilium/actions/runs/4690762173/workflow It's going through the Cilium CLI (which pulls in the Helm chart), that could be another potential cause. But I would validate purely the Helm chart first. |
I still couldn't reproduce it:
Values.yaml looks like this, i.e. adapted to what is configured in CI:
|
the following command also worked:
|
Thanks! Yeah the Helm chart itself looks good. I think the issue is with the Cilium CLI. I was able to reproduce the issue on Kind: $ git rev-parse --abbrev-ref HEAD
add-sa-options-nodeinit
$ cilium install --chart-directory=install/kubernetes/cilium --helm-set nodeinit.enabled=true
cilium install --chart-directory=install/kubernetes/cilium --helm-set nodeinit.enabled=true
🔮 Auto-detected Kubernetes kind: kind
✨ Running "kind" validation checks
✅ Detected kind version "0.17.0"
ℹ️ Using Cilium version 1.13.90
🔮 Auto-detected cluster name: kind-kind
🔮 Auto-detected datapath mode: tunnel
🔮 Auto-detected kube-proxy has been installed
ℹ️ helm template --namespace kube-system cilium "install/kubernetes/cilium" --version 1.13.90 --set cluster.id=0,cluster.name=kind-kind,encryption.nodeEncryption=false,ipam.mode=kubernetes,kubeProxyReplacement=disabled,nodeinit.enabled=true,operator.replicas=1,serviceAccounts.cilium.name=cilium,serviceAccounts.operator.name=cilium-operator,tunnel=vxlan
ℹ️ Storing helm values file in kube-system/cilium-cli-helm-values Secret
...
# Eventually times out, leaving behind a cluster without `cilium-node-init-*` pods
$ For some reason the node init daemonset is not deployed that way. Looks like a bug in Cilium CLI, I can even reproduce this with the |
OK, I haven't tried with cilium CLI and |
This commit is to make sure that users can enable/disable SA token auto
mount, which is recommended in NSA security hardening guide. This PR is for the cilium-nodeinit daemonset.
https://media.defense.gov/2022/Aug/29/2003066362/-1/-1/0/CTR_KUBERNETES_HARDENING_GUIDANCE_1.2_20220829.PDF