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
Add dual-stack support on flannel #3906
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3906 +/- ##
==========================================
- Coverage 11.52% 11.47% -0.05%
==========================================
Files 132 132
Lines 8928 8965 +37
==========================================
Hits 1029 1029
- Misses 7673 7710 +37
Partials 226 226
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
3cdd054
to
b58701f
Compare
f0ae37b
to
1cdc1b7
Compare
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.
First remark - could you please make clear in the PR name that it's only about flannel? I.e. flannel: Add dual-stack support
The current title gives an impression that it's supposed to solve dual-stack in the whole k3s at once, which is not true (there is work in kube-router to be done).
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.
Just two nits lol I'm an idiot, I was commenting code from vendor/
🤦
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.
LGTM, thanks!
pkg/agent/flannel/setup.go
Outdated
confJSON = strings.ReplaceAll(confJSON, "%DUALSTACK%", "true") | ||
for _, cidr := range nodeConfig.AgentConfig.ClusterCIDRs { | ||
if utilsnet.IsIPv6(cidr.IP) { | ||
// Assuming there is going to be only one ipv6 range |
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.
Recent versions of Kubernetes allow you to specify multiple non-overlapping, non-contiguous ipv4/ipv6 ranges. Is it too much work to allow that? We should at least handle it properly.
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.
This is a good point Brad! I wanted to look at this but forgot to do it. I don't think flannel supports multiple ranges but I need to verify this. If it does not, I guess we should fail with an error describing this, right?
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.
After some investigations and questions to sig-network guys, the conclusion is that multiple non-overlapping, non-contiguous ipv4/ipv6 ranges is not supported yet. It's under development kubernetes/enhancements#2593
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.
Interesting, I'll have to dig up the changelog entry I was thinking of. It's possible that one of the cloud providers has added support for this already but the core controller-manager nodeipam does not yet.
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.
I added the link in the comment, so that we don't forget this
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.
Here's what I was thinking of - it appears that this is GKE only and hasn't hit core Kubernetes yet:
https://cloud.google.com/kubernetes-engine/docs/how-to/multi-pod-cidr
1cdc1b7
to
e38a22c
Compare
e38a22c
to
0f1b056
Compare
0f1b056
to
a181746
Compare
I'll add the tests in a different commit but same PR |
72c9f92
to
95ca6cd
Compare
Now that we have tests in, can we pull out the startup check for --flannel-backend=none when using dual-stack? I'm excited to land this! |
Wait, we have unit tests. I'm working on integration tests. Hopefully done tomorrow |
35f6618
to
262b397
Compare
957aba6
to
702addf
Compare
Signed-off-by: Manuel Buil <mbuil@suse.com>
702addf
to
43fe774
Compare
@@ -467,7 +467,7 @@ func validateNetworkConfiguration(serverConfig server.Config) error { | |||
return errors.Wrap(err, "failed to validate cluster-dns") | |||
} | |||
|
|||
if (serverConfig.ControlConfig.FlannelBackend != "none" || serverConfig.ControlConfig.DisableNPC == false) && (dualCluster || dualService) { | |||
if (serverConfig.ControlConfig.DisableNPC == false) && (dualCluster || dualService) { |
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.
Remove the flannel reference from the error message on the next line too.
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.
good catch!
…kend Signed-off-by: Manuel Buil <mbuil@suse.com>
43fe774
to
9fcd79b
Compare
@dereknola could you check the tests please? |
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.
Tests look good
Signed-off-by: Manuel Buil mbuil@suse.com
Proposed Changes
Add the flannel part of the dual-stack support for k3s. First version
Types of Changes
New Feature
Verification
Deploy k3s with:
And having your main interface with an ipv6 address. Of course your infra must allow ipv6 traffic!
Linked Issues
#1405
User-Facing Change
Further Comments