-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
ipv6: allow ipv6 connectivity for bridge network #23747
Conversation
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 looks really good!
I have a few comments, but mainly we'll need to:
- select a much narrower range for the default ipv6 subnet
- verify whether this works on a host with ipv6 disabled
@@ -26,6 +26,10 @@ const ( | |||
// allocation when not specified by the client | |||
defaultNomadAllocSubnet = "172.26.64.0/20" // end 172.26.79.255 | |||
|
|||
// defaultNomadAllocSubnetIPv6 is the subnet to use for host local ipv6 address | |||
// allocation when not specified by the client | |||
defaultNomadAllocSubnetIPv6 = "fd00::/8" // Unique local address |
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.
we'll want to pick a much smaller subset of this huge range.
|
||
logger hclog.Logger | ||
} | ||
|
||
func newBridgeNetworkConfigurator(log hclog.Logger, alloc *structs.Allocation, bridgeName, ipRange string, hairpinMode bool, cniPath string, ignorePortMappingHostIP bool, node *structs.Node) (*bridgeNetworkConfigurator, error) { | ||
func newBridgeNetworkConfigurator(log hclog.Logger, alloc *structs.Allocation, bridgeName, ip4Range string, ipv6Range string, hairpinMode bool, cniPath string, ignorePortMappingHostIP bool, node *structs.Node) (*bridgeNetworkConfigurator, error) { |
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.
since bridgeName, ip4Range, ipv6Range
are all strings, I think they can be lumped together type-wise:
func newBridgeNetworkConfigurator(log hclog.Logger, alloc *structs.Allocation, bridgeName, ip4Range string, ipv6Range string, hairpinMode bool, cniPath string, ignorePortMappingHostIP bool, node *structs.Node) (*bridgeNetworkConfigurator, error) { | |
func newBridgeNetworkConfigurator(log hclog.Logger, alloc *structs.Allocation, bridgeName, ip4Range, ipv6Range string, hairpinMode bool, cniPath string, ignorePortMappingHostIP bool, node *structs.Node) (*bridgeNetworkConfigurator, error) { |
once we get this many arguments, I do start to wonder if we could set it up differently, but that can be a wondering for another time.
if err != nil { | ||
return err | ||
} | ||
if b.allocSubnetIPv6 != "" { |
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 will never be empty if the caller used the newBridge...
constructor, so maybe limited utility checking here? I suppose they don't do any harm, and could feasibly avoid damage if a future caller doesn't hold it 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.
Yea you're right, it's pretty useless here😅! I was thinking more specifically in the case that IPv6 is not enabled in the machine, I think this check will make more sense once I push some changes I just made.
ip6t, err := iptables.New(iptables.IPFamily(iptables.ProtocolIPv6), iptables.Timeout(5)) | ||
if err != nil { | ||
return err | ||
} | ||
if err = ensureChain(ip6t, "filter", cniAdminChainName); err != nil { | ||
return err |
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 do wonder how the ip6tables
stuff might behave on non-ipv6-enabled systems, like if it could error in cases where ipv4 has been working without issues. I think I'd like to merely log on ip6 errors, to ensure that current behavior can continue uninterrupted.
or, maybe if the subnet is set to something non-default, then we could return error? like the user had clear specific ipv6 intentions, then bail, otherwise just log?
], | ||
[ | ||
{ | ||
"subnet": %q |
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.
again I'm wondering if there's a way this could introduce problems on systems that don't ipv6. have you tried disabling ipv6 on a test machine to see whether the bridge plugin errors trying to use this config?
if the bridge plugin does not like it, then we'll need to generate this template differently somehow, so it can conditionally add ipv6 subnet/dst only if the system supports it.
{ "dst": "0.0.0.0/0" }, | ||
{ "dst" : "::/0" } |
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.
nitpick: indentation inconsistency between these
{ "dst": "0.0.0.0/0" }, | |
{ "dst" : "::/0" } | |
{ "dst": "0.0.0.0/0" }, | |
{ "dst" : "::/0" } |
@@ -207,6 +243,11 @@ const nomadCNIConfigTemplate = `{ | |||
"backend": "iptables", | |||
"iptablesAdminChainName": %q | |||
}, | |||
{ | |||
"type": "firewall", | |||
"backend": "ip6tables", |
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 remember adding this, but at the time I think we were kinda flinging spaghetti at the wall to see what stuck. is this actually needed? and again, is it ok on systems without ipv6?
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.
Actually, just checked, there's no need for it! Thanks for the heads up :)
cni_path = "/tmp/cni_path" | ||
bridge_network_name = "custom_bridge_name" | ||
bridge_network_subnet = "custom_bridge_subnet" | ||
bridge_network_subnet_ipv6 = "custom_bridge_subnet_ipv6" |
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.
nit: the =
s should all be in alignment
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.
oh that might be what this test failure is complaining about: https://github.com/hashicorp/nomad/actions/runs/10255235476/job/28371795890?pr=23747#step:6:43
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.
Ohh thank you! I was looking for what part of the linting I was missing 👍
for simplicity, closing this one in favor of #23882 the main difference there, feature-wise, is that we do not supply a default IPv6 subnet, so the user must opt in to it by providing a subnet in the config. that sidesteps many of the concerns I commented on here. we can always choose to set a default later, but for now it's opt-in only. |
This PR integrates IPv6 connectivity within the bridge network by adding IPv6 range configuration for bridge networking and a default IPv6 subnet IP address if no configuration is specified.
Resolves #14101.