-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: Add support to configure dnsPolicy on the Helm chart deployment #2902
Conversation
Welcome @michelzanini! |
/assign @stevehipwell |
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 for the PR @michelzanini, I've added a couple of suggested changes. Out of interest did you open an issue to discuss the issues you were having which led to this PR?
Code changes have being implemented. |
@michelzanini it's not a hard and fast rule but I'd recommend opening an issue before opening a PR, the maintainers may have a wider view and be able to validate an approach before anyone writes any code or has to review it. From the Helm chart perspective I'm fine with this change but I can't tell you if there is an edge case which relies on using the cluster DNS. Have you checked that this change works at least for your use case? |
Yes it does. All this does is allow the pod to go directly to AWS for DNS resolution skipping CoreDNS. CoreDNS is useful if you need to reference a service within the cluster by using the service name. External DNS does not call other pods and works independently so it will only do DNS resolution to external domains such as AWS APIs it needs to call etc. Also recommended in some other places like https://aws.github.io/aws-eks-best-practices/security/docs/pods/#disable-service-discovery. This setting is available to be configured on other Helm charts that use Cloud/AWS APIs such as Cluster Autoscaler and AWS load balancer controller etc.
I can create one if you think its required. Let me know. |
@Raffo @seanmalloy @njuettner could you approve the workflow for first time contribution? |
GitHub Actions CI check are running now. |
/lgtm |
/assign @seanmalloy |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michelzanini, Raffo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
Add support to configure dnsPolicy on the Helm chart deployment.
This allows us to set the
dnsPolicy
toDefault
and skip in-cluster DNS to improve performance.