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

feat: support multiple clusterDNS #6117

Closed
wants to merge 1 commit into from
Closed

Conversation

xahhy
Copy link

@xahhy xahhy commented Apr 29, 2024

Fixes #4934

Description
This change will support us bypass multiple clusterDNS with comma separated IP list and not being treated as IPv6

How was this change tested?

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@xahhy xahhy requested a review from a team as a code owner April 29, 2024 09:50
@xahhy xahhy requested a review from ljosyula April 29, 2024 09:50
Copy link

netlify bot commented Apr 29, 2024

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 16d385c
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/662f6d7048301d000886c40b

@@ -119,6 +119,14 @@ func (e EKS) isIPv6() bool {
if e.KubeletConfig == nil || len(e.KubeletConfig.ClusterDNS) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bottlerocket and AL2023 also have functionality for this. Can we also add support for this to those AMIFamily as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean other AMIs also support we bypass a comma separated DNS IPs in the bootstrap script? I can try add them

@@ -119,6 +119,14 @@ func (e EKS) isIPv6() bool {
if e.KubeletConfig == nil || len(e.KubeletConfig.ClusterDNS) == 0 {
return false
}

ips := strings.Split(e.KubeletConfig.ClusterDNS[0], ",")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClusterDNS is already a string slice, can we just use the slice directly? All of these AMIFamilies should have support now for passing all of the DNS entries through

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. you mean we just respect all the clusterDNS strings and convert as a string comma-separated string with all IPs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Ideally we don't let you do a comma-delimited list in any of the strings for ClusterDNS and you just specify them as separate strings in the structued object.

@jonathan-innis jonathan-innis self-assigned this Apr 30, 2024
Copy link
Contributor

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

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

Successfully merging this pull request may close these issues.

Support inject multiple cluster-dns-ips to kubelet-config.json
2 participants