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.

Sign up for GitHub

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

Jump to bottom

bottlerocket with mulitple cluster-dns #4836

Open
mzupan opened this issue Oct 15, 2023 · 7 comments · May be fixed by #7546
Open

bottlerocket with mulitple cluster-dns #4836

mzupan opened this issue Oct 15, 2023 · 7 comments · May be fixed by #7546
Assignees
@jmdeal
Labels
feature New feature or request

Comments

@mzupan
Copy link

mzupan commented Oct 15, 2023

Description

Observed Behavior:

Not able to set mulitple ips for cluster-dns-ip

bottlerocket allows you to set it via

# Valid, single IP
[settings.kubernetes]
"cluster-dns-ip" = "10.0.0.1"

# Also valid, multiple nameserver IPs
[settings.kubernetes]
"cluster-dns-ip" = ["10.0.0.1", "10.0.0.2"]

Trying to set it like

[settings.kubernetes]
"cluster-dns-ip" = ["10.0.0.1", "10.0.0.2"]

Gives the following log

karpenter-f7cdc49f4-25kbx controller 2023-10-15T02:10:53.850Z	ERROR	controller	Reconciler error	{"commit": "322822a", "controller": "machine.lifecycle", "controllerGroup": "karpenter.sh", "controllerKind": "Machine", "Machine": {"name":"workers-gmsx5"}, "namespace": "", "name": "workers-gmsx5", "reconcileID": "8ace2c6e-45f7-4bb4-84d8-79388dde83b0", "error": "launching machine, creating instance, getting launch template configs, getting launch templates, creating launch template, invalid UserData toml: cannot decode TOML array into struct field bootstrap.BottlerocketKubernetes.ClusterDNSIP of type *string"}

Looking at the code it takes a string but even if you do it with a comma it always just has one which is the default kube-dns

https://github.com/aws/karpenter/blob/main/pkg/providers/amifamily/bootstrap/bottlerocket.go#L59

Expected Behavior:

be able to have a /etc/resolv.conf in pods like

node-local helps to stop against loading a lot of coredns servers so we aren't rate limited by the vpc resolver.

$ cat /etc/resolv.conf
search testing.svc.cluster.local svc.cluster.local cluster.local ec2.internal
nameserver 169.254.20.10
nameserver 172.20.0.10

Reproduction Steps (Please include YAML):

Tried both of the following

     userData: |
        [settings.kubernetes]
        "cluster-dns-ip" = ["169.254.20.10","172.20.0.10"]

and

     userData: |
        [settings.kubernetes]
        "cluster-dns-ip" = "169.254.20.10,172.20.0.10"

Versions:

  • Chart Version:
  • Kubernetes Version (kubectl version):
  • 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
@mzupan mzupan added the bug Something isn't working label Oct 15, 2023
@jonathan-innis
Copy link
Contributor

jonathan-innis commented Oct 15, 2023 •
edited
Loading

This one is a little painful since our support for Bottlerocket merging is driven by our own struct, meaning that if we were to just do something simple like change this *string value to []string, we would be breaking users on the newer version. I'd like to be able to just update our type for ClusterDNS in the bottlerocketsettings.go file to be a []string but I think we're going to have to support both *string and []string like Bottlerocket does.

I haven't dug in enough to understand exactly how to achieve this through a TOML parse, since I doubt that the default toml library in go supports this functionality.

@jonathan-innis jonathan-innis added feature New feature or request and removed bug Something isn't working labels Oct 15, 2023
@jonathan-innis
Copy link
Contributor

I'm also going to change this to a feature request since we have had support for the ClusterDNSIP feature with Bottlerocket, but it looks like they added the support for this to be a string or an array in this PR.

@njtran njtran mentioned this issue May 20, 2024
Closed
@njtran
Copy link
Contributor

njtran commented May 20, 2024

Looks like this can be closed out as part of Bottlerocket v1.8.0. bottlerocket-os/bottlerocket#2132 (comment)

Can you confirm @mzupan @z0rc if this works on this version?

@njtran njtran self-assigned this May 20, 2024
@z0rc
Copy link

z0rc commented May 20, 2024 •
edited
Loading

@njtran incorrect. Bottlerocket 1.8.0 introduced support of setting cluster-dns as array. Kaprenter doesn't support this, previous comment still stands.

OP mentions setting cluster-dns-ip via user data, which is another case and isn't supported, see #5584 (comment).

@njtran njtran assigned jmdeal and unassigned njtran May 20, 2024
@jseiser
Copy link

jseiser commented Sep 17, 2024

Just wanted to add that we would really need this feature. We deal with Conntrack issues, so node-local DNS would be a easy fix, but without being able to pass 2 IP's we would be creating a new problem where anytime that local cache pod went down, the host would lose DNS access.

@mzupan
Copy link
Author

mzupan commented Oct 4, 2024

this also fails under karpenter for the nodepool CR

      kubelet:
        clusterDNS:
          - "169.254.20.10"
          - "172.20.0.10"

It just has the first IP in dns

@woehrl01
Copy link
Contributor

woehrl01 commented Dec 16, 2024 •
edited
Loading

Hi there, just want to follow up here as I just stumbled over the same problem.

(As you already have mention) I can see that there is already string[] used here: https://github.com/aws/karpenter-provider-aws/blob/e0cd8a470ab533c1f6643a763c0c65575a0fd631/pkg/providers/amifamily/bootstrap/bottlerocketsettings.go#L60C3-L60C118

can't we "just" change ClusterDNSIP here

karpenter-provider-aws/pkg/providers/amifamily/bootstrap/bottlerocketsettings.go

Line 51 in e0cd8a4

ClusterDNSIP *string `toml:"cluster-dns-ip,omitempty"`
to just be of type string[] as well and assign the full array here

karpenter-provider-aws/pkg/providers/amifamily/bootstrap/bottlerocket.go

Line 55 in e0cd8a4

s.Settings.Kubernetes.ClusterDNSIP = &b.KubeletConfig.ClusterDNS[0]

I haven't seen where the merging you have mentioned is happening, maybe you could give me a hint @jonathan-innis

I'm happy to take a look and create a PR to adress this.

@woehrl01 woehrl01 linked a pull request Dec 20, 2024 that will close this issue
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jmdeal jmdeal

Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: support providing multiple dns ips woehrl01/karpenter
7 participants
@mzupan @z0rc @woehrl01 @jseiser @njtran @jmdeal @jonathan-innis