Skip to content
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

Fix pullSecrets, fix assignment of NET_BIND_SERVICE capability #93

Merged
merged 7 commits into from
Apr 13, 2023

Conversation

firefrei
Copy link
Contributor

@firefrei firefrei commented Feb 6, 2023

I tried to use the helm chart in v1.19.7 on our RKE2-based Kubernetes cluster which has special hardening according to CIS-1.6 implemented. However, the images could not be pulled from our private repo as well as the pods terminated with "operation not permitted".

The reason for these two errors were:

  • imagePullSecrets were not used (mentioned in values.yaml as image.pullSecrets, but not evaluated in template)
  • the NET_BIND_SERVICE capability was not set in securityContext of pod
    • securityContext was not mentioned in values.yaml, however it was evaluated in template.
  • in the PSP element, the capability was named incorrectly. It was CAP_NET_BIND_SERVICE, however, due to official K8s docs, there should be no CAP_ prefix.

This MR fixes all these issues. It tested the helm chart with these fixes, the pods are now able to start!

@firefrei
Copy link
Contributor Author

firefrei commented Feb 6, 2023

Hmmm... the linter complains that I did not bump the version number. Should I do this?

 ✖︎ coredns => (version: "1.19.7", path: "charts/coredns") > chart version not ok. Needs a version bump! 

@mrueg
Copy link
Collaborator

mrueg commented Feb 7, 2023

The image pull secrets are linked with the serviceaccount see: https://github.com/coredns/helm/blob/master/charts/coredns/templates/serviceaccount-autoscaler.yaml#L26

@firefrei
Copy link
Contributor Author

firefrei commented Feb 7, 2023

@mrueg Okay, so when I have serviceAccount.create set to false, which is the default, then I cannot use an imagePullSecret? What's the idea behind this?

{{- if and .Values.deployment.enabled .Values.serviceAccount.create }}

@firefrei
Copy link
Contributor Author

@mrueg how should we proceed with this PR?
What is the desired way of using imagePullSecrets? I would suggest to use it with the Deployment, to not rely on the service account.
Should I extend the PR and remove it from the ServiceAccount? Should I remove the imagePullSecret again from the Deployment and limit this PR to the NET_BIND_SERVIC fix?

@hagaibarel
Copy link
Collaborator

Hi @firefrei I agree with you that specifying a pullSecret is much more common on the deployment than on the service account, because the service account is conditional. I think we can proceed with the PR as-is. Please rebase and bump the chart version by a minor version

@firefrei
Copy link
Contributor Author

firefrei commented Apr 3, 2023

rebased and version bumped

@hagaibarel
Copy link
Collaborator

Thanks, looking good, one last thing is to update the changelog, see https://github.com/coredns/helm/blob/master/CONTRIBUTING.md#changelog for instructions on what and how

@firefrei
Copy link
Contributor Author

firefrei commented Apr 4, 2023

Done! I hope I inserted the item at the correct position in in the list :-)

charts/coredns/Chart.yaml Outdated Show resolved Hide resolved
charts/coredns/Chart.yaml Outdated Show resolved Hide resolved
Signed-off-by: Matthias Frei <mf@frei.media>
…ability.

Signed-off-by: Matthias Frei <mf@frei.media>
Signed-off-by: Matthias Frei <mf@frei.media>
Signed-off-by: Matthias Frei <mf@frei.media>
Signed-off-by: Matthias Frei <mf@frei.media>
Signed-off-by: Matthias Frei <mf@frei.media>
Signed-off-by: Matthias Frei <mf@frei.media>
@firefrei
Copy link
Contributor Author

Sorry for the delay,
next try :-)

@hagaibarel hagaibarel merged commit 5781216 into coredns:master Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants