feat(#23388): Add a preferred address family option for network-interface

[guifran001]

As some services might not support IPv6, nomad
needs a way to expose service on IPv4 instead of IPv6

see #23388 for more details

This pr adds a ignore-ipv6 option to nomad to ignore IPv6 addresses.

If preferred, I could change to code to add a -preferred-ip-family instead.

[hashicorp-cla-app]

All committers have signed the CLA.

[gulducat]

Hi @guifran001, thank you for the PR! We have a near-term priority on evaluating and improving IPv6 in Nomad, so your contribution is timely. :)

If preferred, I could change to code to add a -preferred-ip-family instead.

I do think this is preferable, because some users/environments may actually want to use IPv6, which in some cases is hard to accomplish at the moment.

I'd suggest validating that the user input is one of these two constants: https://github.com/hashicorp/nomad/blob/v1.8.1/nomad/structs/structs.go#L2728-L2729 to be able to error early on bad input before the client agent gets up and running. It looks like you have already found the constant(s), since you check for one here. If the value is an empty string, we should default to the current behavior. Aside from that, I think most of your approach would only need minor tweaking.

Let me know how it goes, and if you'd like any assistance with it!

[guifran001]

Hi @gulducat,
Happy to know that my timing is good.

I have refactor to add a preferred-address-family instead of the ignore-ipv6 option and validated the entry with the constants you mentioned.

I have sorted both NetworkResource and NodeNetworkResource, is that OK or it should only apply to NodeNetworkResource ?

Tell me if it makes sense or if you want me to refactor / improve my code.

[gulducat]

Hey thanks! I have this on my list to review more thoroughly soon. So far I've only kicked the tires a bit to see the scope of what's covered here, found a couple little discrepancies. I'll get back to you sometime this week with more details!

[gulducat]

Hey this is great, thanks! I have just a few comments for ya.

I'm going to be out most of next week, so I might not be able to get back to you until after that. If you make changes before then, I'll try to pass to a teammate, or if you're ok waiting then I will check in after I get back!

[guifran001]

Thank you for the review. I was in an hurry when I did it. Sorry for all the relics that were still there

[gulducat]

Huge improvement! And almost done, I think. Just need a little documentation, a concern with the test, and one of the relics is still hanging around.

[gulducat]

Wouldn't this be an error if the CIDRs are not equal? also the error output shows net.IP which confused me for a sec.

Suggested change

	if strings.ToUpper(net.CIDR) == strings.ToUpper(tc.expectedCIDR) {
	t.Errorf("Expected CIDR %s; got %s", tc.expectedCIDR, net.IP)
	if strings.ToUpper(net.CIDR) != strings.ToUpper(tc.expectedCIDR) {
	t.Errorf("Expected CIDR %s; got %s", tc.expectedCIDR, net.CIDR)

that causes the test to fail:

=== RUN   TestNetworkFingerPrint_default_device/Loopback_IPv6
    network_test.go:343: Expected CIDR 2001:DB8::/48; got 2001:db8::/128

=== RUN   TestNetworkFingerPrint_default_device/Loopback_IPv4
    network_test.go:343: Expected CIDR 127.0.0.1/8; got 127.0.0.0/32

=== RUN   TestNetworkFingerPrint_default_device/Loopback_No_preferred_address_family
    network_test.go:343: Expected CIDR 2001:DB8::/48; got 2001:db8::/128

[guifran001]

Nice catch!
I'm a bit confused about how CIDR 127.0.0.1/8 result in IP address 127.0.0.0 instead of 127.0.0.1

I'm going to compare the IP instead of the CIDR. In the end, we want to make sure they are on the expected family.

[gulducat]

looks like 127.0.0.0 is coming from the test NetworkInterfaceDetectorOnlyLo's usage of net.ParseCIDR

It returns the IP address and the network implied by the IP and prefix length. For example, ParseCIDR("192.0.2.1/24") returns the IP address 192.0.2.1 and the network 192.0.2.0/24.

the mockish test interface implementation is only using the IPNet and discarding IP.

I agree using the IP for this test is fine, since it does confirm the by-family sorting behavior we're expecting to see. 👍

[gulducat]

This looks great @guifran001! Just one more bit of bookkeeping that I forgot to mention:

In the repo root, run make cl to create a changelog file, an improvement with a Note like

client: add a preferred_address_family config to prefer ipv4 or ipv6 for service discovery

(or something like that)

Then I'll merge this in, so it can go out with the 1.8.2 release soon!

[gulducat]

Thanks for the contribution @guifran001! 🎉