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

Helm chart has non-default dnsPolicy #4947

Closed
fullykubed opened this issue Oct 27, 2023 · 4 comments · Fixed by #5483
Closed

Helm chart has non-default dnsPolicy #4947

fullykubed opened this issue Oct 27, 2023 · 4 comments · Fixed by #5483
Labels
feature New feature or request

Comments

@fullykubed
Copy link

Description

Observed Behavior:

The Karpenter Helm chart has a default dnsPolicy called Default (link). Interestingly, this is not the default dns policy for Kubernetes which is ClusterFirst (link).

I believe the intent with the chart was to use the default Kubernetes DNS settings as I do not believe there is any benefit in overriding the cluster defaults. I have verified ClusterFirst works as expected.

The current default setup is incompatible with clusters with injected sidecars that need ClusterFirst DNS resolution (e.g., service meshes such as Linkerd).

Expected Behavior:

dnsPolicy in the helm chart should be set to ClusterFirst as the default

Reproduction Steps (Please include YAML):

See the Helm chart.

Versions:

  • Chart Version: v0.31
  • Kubernetes Version (kubectl version): v1.27.4-eks-2d98532
  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment
@fullykubed fullykubed added the bug Something isn't working label Oct 27, 2023
@njtran njtran added the triage/needs-investigation Issues that need to be investigated before triaging label Oct 31, 2023
@jonathan-innis
Copy link
Contributor

jonathan-innis commented Nov 6, 2023

This default policy was changed here: #2199. This was originally recommended from #2186 where a user informed us that there's a bit of an ordering problem if you want to scale up your core-dns pods based on Karpenter-managed capacity (since Karpenter would need DNS to be up first in the ClusterFirst mode).

Are you running into any particular issues with the current default DNS policy? I believe you should be able to change this in the helm values if that default isn't working for your specific cluster.

@jonathan-innis jonathan-innis added question Further information is requested and removed bug Something isn't working triage/needs-investigation Issues that need to be investigated before triaging labels Nov 6, 2023
@fullykubed
Copy link
Author

fullykubed commented Nov 6, 2023

Yes, the current setting can cause issues. The most common use case is anytime you have injected sidecar containers (for example, all popular service meshes). These sidecars usually need access to the cluster dns servers and thus will be unable to launch due to this pod-level DNS setting. I can attest to this being the case with Linkerd and Istio.

I understand the issue presented in #2186 . However, it seems like running karpenter prior to even launching dns is going to be a niche setup that is unlikely to impact most users. I'd propose that the default should stay the kubernetes default in order to follow the principle of least surprise.

If someone has a special case like running karpenter prior to their DNS they have the option to configure it. However, the current setting definitely deviates from normal practice and will impact all service mesh users at minimum.

Copy link
Contributor

This issue has been inactive for 14 days. StaleBot will close this stale issue after 14 more days of inactivity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 5, 2023
@njtran njtran reopened this Dec 11, 2023
@jonathan-innis jonathan-innis added feature New feature or request and removed question Further information is requested lifecycle/stale lifecycle/closed labels Dec 11, 2023
@jonathan-innis
Copy link
Contributor

jonathan-innis commented Jan 4, 2024

I agree with you that we should probably follow the principle of least surprise here. Since using Default is not the default Kubernetes setup, I could see this being surprising when installing the Karpenter deployment.

I'm personally fine with removing the setting in the deployment. I think that we added this to the chart thinking that there were no downsides to doing this. Since changing this would be a breaking change to existing users, we should call this out in the Upgrade docs alongside providing guidance in the "Getting Started" guide around what you should do if you want Karpenter to manage your CoreDNS pods rather than having the CoreDNS pods up before Karpenter starts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants