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

Added ipv6 only support with flannel #5238

Merged
merged 2 commits into from Mar 11, 2022

Conversation

rbrtbnfgl
Copy link
Contributor

Signed-off-by: Roberto Bonafiglia roberto.bonafiglia@suse.com

Proposed Changes

Types of Changes

Added support for IPv6 only setup using flannel

Verification

Setup multiple nodes with IPv6 and configure the IPv6 addresses on the config file

node-ip: 2001:cafe:42::1c8 # use the node IPv6 address
cluster-cidr: 2001:cafe:43:0::/56
service-cidr: 2001:cafe:43:1::/112
disable-network-policy: true
flannel-ipv6-masq: true

Linked Issues

#5237

User-Facing Change


Further Comments

Signed-off-by: Roberto Bonafiglia <roberto.bonafiglia@suse.com>
@rbrtbnfgl rbrtbnfgl requested a review from a team as a code owner March 9, 2022 09:39
}
} else {
confJSON = strings.ReplaceAll(confJSON, "%CIDR%", "0.0.0.0/0")
confJSON = strings.ReplaceAll(confJSON, "%DUALSTACK%", "true")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the %DUALSTACK% parameter to be %IPV6_ENABLED%? I think it makes more sense

confJSON = strings.ReplaceAll(confJSON, "%DUALSTACK%", "true")
for _, cidr := range nodeConfig.AgentConfig.ClusterCIDRs {
if utilsnet.IsIPv6(cidr.IP) {
// Only one ipv6 range available. This might change in future: https://github.com/kubernetes/enhancements/issues/2593
Copy link
Contributor

Choose a reason for hiding this comment

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

What would happen if there were more than one ipv6 range? I wonder if it would be worth checking and if there is more than one fail with an error

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me like after the first ReplaceAll call the %CIDR_IPV6% template string would be gone from the string, so only the first CIDR block will be used. That seems fine.

Is Flannel OK with the %CIDR_IPV6% string remaining unsubstituted in the JSON if IPv6 is not enabled?

Copy link
Contributor Author

@rbrtbnfgl rbrtbnfgl Mar 10, 2022

Choose a reason for hiding this comment

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

I tested it configuring more then one CIDR for IPv4 and IPv6. K3s gives error and does not even start; the logs say that CIDR len should be at least two on dualstack configuration.
If IPv6 is disabled the CIDR config for IPv6 it's not used.

@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2022

Codecov Report

Merging #5238 (ff85faa) into master (9334690) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5238      +/-   ##
=========================================
- Coverage    9.54%   9.53%   -0.02%     
=========================================
  Files         134     134              
  Lines        9404    9420      +16     
=========================================
  Hits          898     898              
- Misses       8315    8331      +16     
  Partials      191     191              
Flag Coverage Δ
unittests 9.53% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/flannel/flannel.go 0.00% <0.00%> (ø)
pkg/agent/flannel/setup.go 0.00% <0.00%> (ø)
pkg/cli/server/server.go 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9334690...ff85faa. Read the comment docs.

Signed-off-by: Roberto Bonafiglia <roberto.bonafiglia@suse.com>
confJSON = strings.ReplaceAll(confJSON, "%DUALSTACK%", "true")
for _, cidr := range nodeConfig.AgentConfig.ClusterCIDRs {
if utilsnet.IsIPv6(cidr.IP) {
// Only one ipv6 range available. This might change in future: https://github.com/kubernetes/enhancements/issues/2593
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me like after the first ReplaceAll call the %CIDR_IPV6% template string would be gone from the string, so only the first CIDR block will be used. That seems fine.

Is Flannel OK with the %CIDR_IPV6% string remaining unsubstituted in the JSON if IPv6 is not enabled?

@rbrtbnfgl rbrtbnfgl merged commit 29c55f5 into k3s-io:master Mar 11, 2022
@rbrtbnfgl rbrtbnfgl deleted the ipv6-only-with-flannel branch March 11, 2022 14:13
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.

None yet

4 participants