-
Notifications
You must be signed in to change notification settings - Fork 2k
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
networking: option to enable ipv6 on bridge network #23882
Conversation
ipRanges = append(ipRanges, []Range{{Subnet: conf.IPv6Subnet}}) | ||
ipRoutes = append(ipRoutes, Route{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.
these are the meat and potatoes; everything else revolves around this.
bb411c4
to
6f8a98d
Compare
Co-authored-by: Martina Santangelo <[email protected]>
6f8a98d
to
014865a
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 👍
"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.
It's a little weird that our tests have invalid values here (I think these aren't valid?)
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.
It's a good point - they're not validated anywhere until they actually get used at allocation-time.
I always hated finding out about bad client config that way, when an alloc fails through no fault of the jobspec. I would prefer to have the client fail to start instead of be sitting around quietly poisoned. Would this be a good place to add a check?
I can also add validation for custom ipv4 subnet while I'm in here, too. A major version bump is probably a good time to go for that, eh?
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.
Yeah, that looks like as good a place as any... there's a few other bits of client config validation we do there.
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 went down the road of requiring that these test configs be valid, but the test cases are really about parsing the HCL, not also validating them, which happens a little later.
So in a2f436b I just added unit tests around the one function that I added the validation to, and left these values as they are.
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
Users may optionally enable IPv6 on the Nomad
bridge
network mode by settingbridge_network_subnet_ipv6
in the client config.Aside from that, there is a small functional change regarding our attempt to force-cleanup iptables if CNI fails to do so. Now if the postrouting rule is not found, it only logs that instead of returning an error. This way the new best-effort attempt to cleanup
ip6tables
can be less frightening in the logs if ipv6 isn't even enabled on the bridge.Resolves #14101