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

Dual-stack with Kube router 2 #3954

Merged
merged 7 commits into from
Jul 5, 2024
Merged

Dual-stack with Kube router 2 #3954

merged 7 commits into from
Jul 5, 2024

Conversation

ncopa
Copy link
Collaborator

@ncopa ncopa commented Jan 18, 2024

Description

Update kube-router to 2.0.1 and enable ipv6

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Copy link
Contributor

The PR is marked as stale since no activity has been recorded in 30 days

Copy link
Contributor

This pull request has merge conflicts that need to be resolved.

Copy link
Contributor

This pull request has merge conflicts that need to be resolved.

Copy link
Contributor

The PR is marked as stale since no activity has been recorded in 30 days

Copy link
Member

@twz123 twz123 left a comment

Choose a reason for hiding this comment

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

NB: This is the error that's reported by containerd in the failed inttest:

failed to setup network for sandbox: plugin type="bridge" name="kubernetes" failed (add): failed to set bridge addr: could not add IP address to "kube-bridge": permission denied

pkg/component/worker/kubelet.go Outdated Show resolved Hide resolved
pkg/component/worker/kubelet.go Outdated Show resolved Hide resolved
pkg/component/worker/kubelet.go Outdated Show resolved Hide resolved
pkg/component/worker/kubelet.go Outdated Show resolved Hide resolved
pkg/component/worker/kubelet.go Outdated Show resolved Hide resolved
Copy link
Contributor

This pull request has merge conflicts that need to be resolved.

@twz123 twz123 added this to the 1.30 milestone Apr 18, 2024
@twz123
Copy link
Member

twz123 commented Apr 18, 2024

/xref #3814

@ncopa ncopa force-pushed the kube-router-2 branch 2 times, most recently from d6e2085 to 490cf6e Compare April 18, 2024 19:33
@ncopa ncopa mentioned this pull request Apr 22, 2024
16 tasks
@ncopa ncopa force-pushed the kube-router-2 branch 2 times, most recently from 7ace5ca to a171a45 Compare April 23, 2024 09:05
@ncopa ncopa force-pushed the kube-router-2 branch 5 times, most recently from b823fe4 to 18cec2f Compare June 7, 2024 15:57
@ncopa ncopa marked this pull request as ready for review June 24, 2024 14:46
@ncopa ncopa requested a review from a team as a code owner June 24, 2024 14:46
@ncopa ncopa enabled auto-merge June 25, 2024 13:35
Copy link
Member

@twz123 twz123 left a comment

Choose a reason for hiding this comment

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

How does this work with dynamic configuration? We'd need to restart kubelet, I guess, since we're actively changing flags based on dual stack?

Hmm, what happens for Calico when toggling dual stack? We're not (yet) reconciling the workers at all... (#1806)

.github/workflows/go.yml Outdated Show resolved Hide resolved
inttest/Makefile.variables Outdated Show resolved Hide resolved
inttest/Makefile.variables Show resolved Hide resolved
inttest/dualstack/dualstack_test.go Outdated Show resolved Hide resolved
var ipv4, ipv6 net.IP
for _, addr := range ipaddrs {
if ipv4 == nil && addr.IP.To4() != nil {
ipv4 = addr.IP
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ipv4 = addr.IP
ipv4 = addr.IP.To4()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would this solve?

Copy link
Member

Choose a reason for hiding this comment

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

It would ensure that ipv4 is actually in its four byte form, so that len(ipv4) == IPv4len. Otherwise, it could be in its 16 byte form, i.e. len(ipv4) == IPv6len. This is a nit, but it feels a bit saner to have a v4 IP returned as 4 bytes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we only care what the address family is. How the buffers are allocated under the hood is completely irrelevant.

pkg/component/worker/kubelet.go Outdated Show resolved Hide resolved
pkg/component/worker/kubelet.go Outdated Show resolved Hide resolved
pkg/component/worker/kubelet.go Outdated Show resolved Hide resolved
pkg/component/worker/kubelet.go Outdated Show resolved Hide resolved
pkg/component/worker/kubelet.go Outdated Show resolved Hide resolved
@twz123
Copy link
Member

twz123 commented Jul 2, 2024

This needs attention from kubectl side, as well. I'm a bit on the fence with the IP auto-detection, because this makes it so much harder for k0sctl to do the right thing, but maybe I'm overstating the problem. WDYT @kke?

@ncopa
Copy link
Collaborator Author

ncopa commented Jul 3, 2024

This needs attention from kubectl side, as well. I'm a bit on the fence with the IP auto-detection, because this makes it so much harder for k0sctl to do the right thing, but maybe I'm overstating the problem. WDYT @kke?

Would you prefer that we required to specify the ipv4 and ipv6 node address in k0s config? How would that work for dynamic addresses? There is no guarantee that the node gets same ipv4 address after a reboot.

@twz123
Copy link
Member

twz123 commented Jul 3, 2024

Would you prefer that we required to specify the ipv4 and ipv6 node address in k0s config?

I think there's two separate things which bother me:

First: There's this schism between single-stack and dual-stack. There is IP auto-detection in kubelet for v4 and v6, but not for both at the same time (which would be reasonable in dual-stack mode, IMO). So k0s has to do it, and is responsible for getting it right, even it doesn't really care about the detected IPs itself. Although I would rather see someone else (i.e. upstream) maintain this 🙃, there is still the option to set the IPs via --node-ip as kubelet extra args if users want to set the IPs (or need to set them since the auto-detection doesn't do the right thing for them). I think that's a fair compromise.

Second, and this is something that needs clarification and probably a fix on the k0sctl or on both k0sctl and k0s side: k0sctl sets --node-ip based on privateInterface. This also needs to get dual-stack support. So people will need to specify exactly zero or two IPs in privateInterface if they are using a dual-stack cluster. I don't think k0sctl can accept more than one IP in this setting, so this is a blocker for use cases like k0sproject/k0sctl#736. One way to fix this on k0s's end might be to look at the user-provided --node-ip value and, if it doesn't have IP addresses for v4 and v6, add the missing ones via auto-detection.

How would that work for dynamic addresses? There is no guarantee that the node gets same ipv4 address after a reboot.

Is that even a thing, workers with dynamic addresses? Maybe. Would require a k0s restart on IP address changes, though. Not sure about the running containers. They probably need to be restarted, as well.

ncopa added 7 commits July 4, 2024 14:34
Signed-off-by: Natanael Copa <ncopa@mirantis.com>
kuber-router supports ipv6. Enable this and add dualstack test.

Unlike calico, kube-router requires that the node has a valid ipv6
InternalIP. This is only set with kublet when --node-ip is specified,
and this option requires that we also set the ipv4. Mimic the logic from
kubelet to find the ipv4 same way as kubelet.

https://github.com/cloudnativelabs/kube-router/blob/master/docs/ipv6.md#how-to-enable-dual-stack-functionality

Signed-off-by: Natanael Copa <ncopa@mirantis.com>
Signed-off-by: Natanael Copa <ncopa@mirantis.com>
This is useful for debugging.

Signed-off-by: Natanael Copa <ncopa@mirantis.com>
Use our kube-router 2.1.3 image which has a backport fix for ipv6 in
github runners.

ref: cloudnativelabs/kube-router#1684
Signed-off-by: Natanael Copa <ncopa@mirantis.com>
Clarify test names for dualstack and add dualstack with kuberouter with
dynamic confg.

Signed-off-by: Natanael Copa <ncopa@mirantis.com>
Improve naming, error message and add comment.

Signed-off-by: Natanael Copa <ncopa@mirantis.com>
@ncopa
Copy link
Collaborator Author

ncopa commented Jul 4, 2024

I would rather see someone else (i.e. upstream) maintain this 🙃

I agree. This is something that should be fixed in kubelet. But for now we will have to work around it in k0s.

k0sctl sets --node-ip based on privateInterface. This also needs to get dual-stack support. So people will need to specify exactly zero or two IPs in privateInterface if they are using a dual-stack cluster.

Yes, and this is due to upstream kubelet behavior.

I don't think k0sctl can accept more than one IP in this setting, so this is a blocker for use cases like k0sproject/k0sctl#736. One way to fix this on k0s's end might be to look at the user-provided --node-ip value and, if it doesn't have IP addresses for v4 and v6, add the missing ones via auto-detection.

I'd rather fix kubelet

Is that even a thing, workers with dynamic addresses? Maybe. Would require a k0s restart on IP address changes, though. Not sure about the running containers. They probably need to be restarted, as well.

I would expect most workers use DHCP for ipv4. Not sure if we care about address changes on dhcp lease expiry runtime. But I think that address change on reboot is something to be expected. (which probably means we shouldn't set the address from k0sctl?)

@ncopa ncopa requested a review from twz123 July 4, 2024 12:53
@ncopa ncopa merged commit dbfcc3a into k0sproject:main Jul 5, 2024
82 checks passed
@ncopa ncopa deleted the kube-router-2 branch July 5, 2024 12:49
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.

3 participants