-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Take IPFamily precedence based on order #8460
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #8460 +/- ##
==========================================
+ Coverage 47.16% 49.49% +2.33%
==========================================
Files 143 143
Lines 14855 14817 -38
==========================================
+ Hits 7006 7334 +328
+ Misses 6718 6301 -417
- Partials 1131 1182 +51
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
fdabaa6
to
5123367
Compare
5123367
to
21d9323
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.
LGTM.
@rancher-max we should specifically test upgrades to this commit on dual-stack clusters. Should check the logs to make sure that the deploy controller doesn't get stuck in a loop trying to change the ClusterIPs in a way that is rejected by the apiserver.
81e1e96
to
7b09964
Compare
if (serviceIPv6 != clusterIPv6) || (dualCluster != dualService) || (serviceIPv4 != clusterIPv4) { | ||
return fmt.Errorf("cluster-cidr: %v and service-cidr: %v, must share the same IP version (IPv4, IPv6 or dual-stack)", nodeConfig.AgentConfig.ClusterCIDRs, nodeConfig.AgentConfig.ServiceCIDRs) | ||
} | ||
if (clusterIPv6 && !nodeIPv6) || (dualCluster && !dualNode) || (clusterIPv4 && !nodeIPv4) { | ||
if (clusterIPv6 && !(nodeIPv6 || dualNode)) || (dualCluster && !dualNode) || (clusterIPv4 && !(nodeIPv4 || dualNode)) { |
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 getting comically complicated to follow!
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 point, I'll add a comment
Signed-off-by: Manuel Buil <mbuil@suse.com>
7b09964
to
2677a11
Compare
Signed-off-by: Manuel Buil <mbuil@suse.com>
2677a11
to
f2c7117
Compare
I squashed the utils commit. I will leave the coredns commit separately though |
Proposed Changes
This PR supports networking related config flags to follow the order in dual-stack. In that case, IPv6 address will be prioritized.
Note that flannel-cni is not supporting this and always prioritizes IPv4. I am investigating how to fix this but I wanted to create this PR first to address some of the comments in #8394
This PR includes two commits. The first one 067094a has its own PR and is being reviewed here: #8394
On a high level, the other commit does the following:
Types of Changes
New feature
Verification
Deploy in dual-stack mode with cluster-cidr and service-cidr using order and verify that when listing services or pods, IPv6 addresses appear. Check that all have an IPv4 as well and that connectivity to both addresses is working
Testing
Linked Issues
#8467
User-Facing Change
Further Comments